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

Run release enclaves without OE DEBUG flag #1083

Merged
merged 23 commits into from
Apr 22, 2020

Conversation

eddyashton
Copy link
Member

Previously all of our non-virtual runs used OE_ENCLAVE_FLAG_DEBUG. OE documentation on this is as follows:

/**
 *  Flag passed into oe_create_enclave to run the enclave in debug mode.
 *  The flag allows the enclave to be created without the enclave binary
 *  being signed. It also gives a developer permission to debug the process
 *  and get access to enclave memory. What this means is ** DO NOT SHIP
 *  CODE WITH THE OE_ENCLAVE_FLAG_DEBUG ** because it is unsecure. What
 *  it does give is the ability to develop your enclave more easily. Before
 *  you ship the code you need to have a proper code signing story for the
 *  enclave shared library.
 */

This PR adds a release option, plumbed through Python and into cchost, which will launch a real OE enclave without this flag.

Impact:

  • Our CI will test both approaches (--enclave-type debug in Debug builds, --enclave-type release otherwise), so we should know if either fails to launch. Of course this doesn't test that we can actually attach oegdb to a debug build, and conversely if only the release build fails we have few tools to work out why...
  • Your non-Debug builds won't be debuggable with the auto-produced test commands. I think if you're editing these lines to pass them through oegdb, its easy to add --enclave-type debug manually when you want it, and anyway there's little to see if you're not in a Debug build

@eddyashton eddyashton requested a review from a team as a code owner April 20, 2020 15:42
@ccf-bot
Copy link
Collaborator

ccf-bot commented Apr 20, 2020

release_enclaves@7524 aka 20200422.5 vs master ewma over 50 builds from 7138 to 7499
images

@achamayou
Copy link
Member

@eddyashton could you check what the Debug flag in oe_sign.conf does and whether it makes sense to keep it? It feels like at least the samples should do away with it if we're defaulting to release, which I agree is a sensible choice.

Given that change, I would suggest adding a "Debugging" entry in the documentation near the sample explaining the various places where Debug info can be turned on/off.

@jumaffre
Copy link
Contributor

I wonder whether the auto-produced test command in python should include --enclave-type=debug anyway, even if node was started in release? We could write a warning message is that's the case.

@achamayou
Copy link
Member

@jumaffre that crossed my mind, but unless the .so is built at least with symbols, and possible signed with Debug=1, I'm not sure if that would help at all, so it's tempting to keep it simple.

@eddyashton
Copy link
Member Author

The Debug flag in oe_sign.conf is needed to enable running with OE_ENCLAVE_FLAG_DEBUG. The combination of Debug=0 in the config file with --enclave-type debug on the command line produces:

2020-04-21T08:39:37.422466Z [(H)ERROR] tid(0x7f1b76bb1780) | Enclave image was signed without debug flag but is being loaded with OE_ENCLAVE_FLAG_DEBUG set in oe_create_enclave call
 (oe_result_t=(null)) [OE_DEBUG_DOWNGRADE:../host/sgx/create.c:5188886]
2020-04-21T08:39:37.422762Z [(H)ERROR] tid(0x7f1b76bb1780) | :OE_DEBUG_DOWNGRADE [../host/sgx/create.c:oe_create_enclave:763]
terminate called after throwing an instance of 'std::logic_error'
  what():  Could not create enclave: OE_DEBUG_DOWNGRADE

So if we want to allow optional debugging, this has to be enabled. Maybe there's no harm in building enclaves where this is an option? If we want to disallow it in CMAKE_BUILD_TYPE=Debug builds, then the sensible approach is to move away from the config files and use the equivalent macros in code.

A naive strip on the enclave files breaks them (OE_UNEXPECTED during oe_create_enclave), so if we want to build smaller release images we'll need to work out what symbols OE requires and retain them.

@jumaffre - I'd rather not add special case modifications to the test commands. Pausing and pretty-printing a command is a convenience, but it should also be possible to paste the commands from a 'real' run and get equivalent results. If we tweak the command slightly and it works one way but not the other, we're just introducing subtle complications.

I'm leaning towards building debuggable enclaves all the time (at least as far as "you can attach gdb to it", and its just as useless without symbols as for any other app), but running most of our real-looking/recommended tests without this OE flag. Lets discuss further at standup.

@@ -54,7 +53,6 @@ To add a new node to an existing opening network, other nodes should be started

$ cchost
Copy link
Member

Choose a reason for hiding this comment

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

Can we have a Debug/Debugging section that says "if you are going to be debugging, pass this flag and make sure the config has X, and be aware of the fact that your security guarantees are null and void", perhaps with a link to doc that exists about oegdb if any (or mention oegdb otherwise).

cmake/ccf_app.cmake Outdated Show resolved Hide resolved
Copy link
Member

@achamayou achamayou left a comment

Choose a reason for hiding this comment

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

Thank you for updating the doc!

@eddyashton eddyashton merged commit f361144 into microsoft:master Apr 22, 2020
@eddyashton eddyashton deleted the release_enclaves branch April 22, 2020 10:51
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.

5 participants