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

Fix broken sigining of EXT2 rootfs #1456

Merged
merged 1 commit into from
Feb 10, 2023
Merged

Conversation

mikbras
Copy link
Contributor

@mikbras mikbras commented Feb 9, 2023

Signing of EXT2 images (using myst sign) was copying the entire EXT2 rootfs into the enclave image rather than a stub.

Added ./tests/sign to verify signing of EXT4-based images. Added negative test too.

@mikbras mikbras force-pushed the signing.fix branch 2 times, most recently from 713fd51 to 4f81bb6 Compare February 9, 2023 23:38
Copy link
Contributor

@salsal97 salsal97 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 fixing this!

@@ -440,10 +443,28 @@ int _sign(int argc, const char* argv[])
assert(myst_validate_file_path(program_file));
assert(myst_validate_file_path(temp_oeconfig_file));

/* if not a CPIO archive, create a zero-filled file with one page */
if (myst_cpio_test(rootfs) == -ENOTSUP)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check roothashes_opt for a quicker determination?

@mikbras mikbras force-pushed the signing.fix branch 2 times, most recently from a39343f to c744062 Compare February 10, 2023 00:01
@@ -0,0 +1,56 @@
TOP=$(abspath ../..)
Copy link
Contributor

Choose a reason for hiding this comment

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

This test should live in the tests/myst directory with the other signing tests

@mikbras mikbras force-pushed the signing.fix branch 3 times, most recently from 58b9dde to 7ec1638 Compare February 10, 2023 01:18
@@ -28,7 +28,9 @@ package.pem:

package: package.pem ext2rootfs
echo "Generating a signed package"
@myst package-sgx --roothash=roothash appdir package.pem ../config.json
@myst package-sgx --roothash=roothash package.pem ../config.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does it mean when both roothash and appdir are present in args? Should this be prevented(we should not change what we have, just wondering..)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The TEE_aware sample also takes appdir as a command line arg

Copy link
Collaborator

@radhikaj radhikaj left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @mikbras

@paulcallen paulcallen merged commit 9991e3b into deislabs:main Feb 10, 2023
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