Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SGX 2.10 & docker improvements #414

Merged
merged 4 commits into from
Aug 5, 2020

Conversation

g2flyer
Copy link
Contributor

@g2flyer g2flyer commented Jul 24, 2020

What this PR does / why we need it:

There are two main changes here

  • it upgrades our images to sgx 2.10 and correspondingly latest sgxssl with a number of security fixes.
  • it restructures the docker image hierachy to preduce (considerably) slimmer images for ccenv and peer, should allow for quicker rebuilds. Also docker builds are further delayed so any (non-cached) rebuilds should get less in a way of chasing code related build problems.
    Additionally, there are a few small cleanup fixes i noticed as part of this effort.

What this PR does not yet do (will do it in seperate PR) is to refactor the use of uae_service to get rid of some legacy / epid centered libraries and header file (see a corresponding comment also in sgx psw install section in base-rt.

Which issue(s) this PR fixes:

This should address #411 (for now; once we close this, we can move forward #411 to MVP).

Special notes for your reviewer:

In case you have used DOCKER_DEV_IMAGE_APT_ADD__PKGS in $FPC_TOP/config.override.mk: I replaced the accidental and confusing double _ with a single one, so you will have to change your config override file. However, you also get now more control that you can add packages to all images (via DOCKER_BASE_RT_IMAGE_APT_ADD_PKGS), to all images where fpc/fabric stuff is built (via DOCKER_BASE_DEV_IMAGE_APT_ADD_PKGS) and yet further packages just for the dev image (via DOCKER_DEV_IMAGE_APT_ADD_PKGS). Note also that i did remove packages which i knew would not be required for any default use of containers such as vim`. So you might want to revisit your *_ADD_PKGS vars in light of that.

Otherwise, as far as testing goes, there should be nothing special. It should work in SIM and HW mode and also when immediately runnning demo scripts without having run make first. (at least it did so for me :-)

In case you start a fresh install on a vanilla machine and want to run in SGX HW mode, you also will have to do a apt-get install libsgx-uae-service which is not mentioned in the SGX SDK install doc. Note this package, though, will get obsolete once we merge PR #419 (but having it installed doesn't hurt).

PS: i noticed now that all dockerfile show as completely new. I was pretty sure i always used git mv and checking my history the commands still show up. Not sure why the provenance was lost. Maybe too big changes in conjunction with git mv? Or the fact that i've done actually multiple git mv for dev (though not for base, now base-rt)? Sorry for that. Unfortunately, i don't see really a good way now to change that so you see a diff and the provenance shows that, e.g., base moved to base-rt and dev moved to base-dev ....

PS(2); if you haven't done a make clobber in the old setup, you will have obsolete images. Here the corresponding cleanup ...

docker rmi hyperledger/fabric-private-chaincode-base:latest;
docker rmi hyperledger/fabric-private-chaincode-base:master

Does this PR introduce a user-facing changes and/or breaks backward compatability?:

@g2flyer g2flyer requested a review from a team July 24, 2020 22:20
@g2flyer g2flyer force-pushed the msteiner.sgx-2.10 branch 6 times, most recently from 3c813f0 to b79695c Compare July 25, 2020 01:21
@mbrandenburger mbrandenburger added this to the July/August Milestone milestone Jul 27, 2020
Copy link
Contributor

@mbrandenburger mbrandenburger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works pretty well and I like the new docker output! Some nits see below; works on my Mac.

Also, .travis. still has some references to old sgx/ssl. I suggest to remove all that stuff from travis and just use docker. We could do this cleanup in this PR or create another one later. This is up to you.

utils/docker/base-dev/Dockerfile Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
@g2flyer
Copy link
Contributor Author

g2flyer commented Jul 27, 2020

Also, .travis. still has some references to old sgx/ssl.

Oops, completely forgot (have to update my checklist of what all to look at when updating sgx ...)

I suggest to remove all that stuff from travis and just use docker. We could do this cleanup in this PR or create another one later. This is up to you.

I'll fix it here. I will upgrade to sgx2.10 but as you correctly point out, we do not really need that as we all run in docker so will comment out any unnecessary steps (will save some time, though probably not too much as this is almost certainly cached by travis ..)

@g2flyer g2flyer force-pushed the msteiner.sgx-2.10 branch 8 times, most recently from 20f036a to 06bfb55 Compare July 27, 2020 17:40
@g2flyer
Copy link
Contributor Author

g2flyer commented Jul 27, 2020

I'll fix it here. I will upgrade to sgx2.10 but as you correctly point out, we do not really need that as we all run in docker so will comment out any unnecessary steps (will save some time, though probably not too much as this is almost certainly cached by travis ..)

Ok, i started fixing it but turned out it was a bit more complicated than i thought and i run out of the time i gave me on it (i.e., time while i was attending another meeting ;-) and removed now everything so docker dev build is the only thing which will work. But as long as travis wouldn't have SGX HW support, this already should be as good as it gets in terms of testing ...

@mbrandenburger
Copy link
Contributor

@bvavala any chance to look into this already?

@bvavala
Copy link
Contributor

bvavala commented Aug 3, 2020

@bvavala any chance to look into this already?

It's in progress, we're doing the same for PDO as well.

Copy link
Contributor

@bvavala bvavala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works fine.
Some orthogonal issues encountered:

  • docker_buildkit 1 makes build hang behind a proxy
  • demo fails with epid_linkable attestation type (it seems docker does not set the env variable properly)
  • if you change attestation type, you must run a make clean, otherwise it fails, and there are no good indications why

config.mk Outdated
PROJECT_NAME=fabric-private-chaincode
# Docker related settings
#--------------------------------------------------
export DOCKER_BUILDKIT ?= 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Context: behind a proxy; DOCKER_BUILDKIT=1
Problem: while building base-rt, apt-get update... basez... hangs
Solution: default DOCKER_BUILDKIT=0

Suggested change
export DOCKER_BUILDKIT ?= 1
export DOCKER_BUILDKIT ?= 0

* remove obsolete refernces to FPC_DOCKER_NAME_CMD (but leave corresponding
  source for now)
* based on that, also moved utils in build list just before demo so
  docker stuff (only used stuff in utils right now) is built as late as
  possible as this could potentially be time-consuming
* missing marbles example package in go.mod
* removed obsolete references to sgxconfig

Signed-off-by: Michael Steiner <[email protected]>
@g2flyer
Copy link
Contributor Author

g2flyer commented Aug 4, 2020

  • docker_buildkit 1 makes build hang behind a proxy

Seems the issue is with earlier versions of docker from ubuntu 18.04. doing apt-install upgrade would resolve likely resolve it but changing default to 0 is more robust. Will do ...

  • demo fails with epid_linkable attestation type (it seems docker does not set the env variable properly)

I've created an issue #426 for it, will address it in a separate PR ..

  • if you change attestation type, you must run a make clean, otherwise it fails, and there are no good indications why

Not sure how we easily could fail gracefully. But will update docu as part of fixing #426 and mentioning the required make clean

…tion

* This is mainly driven by a new (optional) DOCKER_QUIET_BUILD env variable
* Also, given that we test in travis only through docker, remove the travis install steps

Signed-off-by: Michael Steiner <[email protected]>
Signed-off-by: Michael Steiner <[email protected]>
@bvavala bvavala merged commit 01e3e9a into hyperledger:master Aug 5, 2020
@g2flyer g2flyer deleted the msteiner.sgx-2.10 branch August 5, 2020 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants