Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

[Docs] Ensure that insecure options are clearly marked as such #2454

Merged
merged 2 commits into from
Jun 28, 2021

Conversation

mkow
Copy link
Member

@mkow mkow commented Jun 16, 2021

Description of the changes

I'm still not sure about one option: sgx.debug. Is it actually possible to set it to true and accidentally ship such an enclave? This will be visible in the attestation, but maybe that's not enough?

In the future it would be good to have some kind of warning banner at the start of Graphene if it's insecure, which would be enabled in all debug builds, when using insecure/non-prod manifest settings, etc.

ps. I squashed in a few formatting fixes, reST was broken in a few places.


This change is Reviewable

Copy link

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @mkow)

a discussion (no related file):

I'm still not sure about one option: sgx.debug. Is it actually possible to set it to true and accidentally ship such an enclave? This will be visible in the attestation, but maybe that's not enough?

It is possible. But I don't see a problem -- it is reflected during remote attestation, and a good verification library will throw some huge warning or something like this.



Documentation/manifest-syntax.rst, line 477 at r1 (raw file):

    sgx.trusted_files.[identifier] = "[URI]"

This syntax specifies the files to be cryptographically hashed build-time, and

at build time?

Copy link
Member Author

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)


Documentation/manifest-syntax.rst, line 477 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

at build time?

Done.

boryspoplawski
boryspoplawski previously approved these changes Jun 23, 2021
Copy link
Contributor

@boryspoplawski boryspoplawski left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)

dimakuv
dimakuv previously approved these changes Jun 28, 2021
Copy link

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

@dimakuv dimakuv dismissed stale reviews from boryspoplawski and themself via 8f77a8e June 28, 2021 07:15
@dimakuv dimakuv force-pushed the mkow/secure-defaults branch from e237da4 to 8f77a8e Compare June 28, 2021 07:15
Copy link

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@dimakuv dimakuv merged commit 8f77a8e into master Jun 28, 2021
@dimakuv dimakuv deleted the mkow/secure-defaults branch June 28, 2021 07:44
@mkow mkow mentioned this pull request Sep 9, 2021
11 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants