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

adding oesign signing engine test #1428

Merged
merged 1 commit into from
Oct 7, 2022
Merged

Conversation

salsal97
Copy link
Contributor

@salsal97 salsal97 commented Oct 6, 2022

This test tests the mystikos support for signing engine via oesign. Adopted from https://github.com/openenclave/openenclave/pull/2455/files.

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 all the work, @salsal97

argv[0],
"sign",
rootfs_file,
pem_file, // TODO: maybe I need to take this out
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you make this null and see if it works? You should not need a pem file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does oesign need a pem file with the signing engine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not need one, null seems to work. Will remove the comment. The thing here is we use positional arguments, so if we do not have a pem file it would potentially be a breaking change for everyone..

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we let people specify "NULL" as a command line param.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely, they can already do that. Tested it out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added that to this test so we can maintain is as the recommended way to use signing engines

Copy link
Contributor

@mikbras mikbras left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for doing this Saloni!

@vtikoo
Copy link
Collaborator

vtikoo commented Oct 6, 2022

As we are testing myst sign, would recommend moving this test to tests/myst along with the other myst executable tests. In the same spirit consider dropping the oe suffix where possible. myst sign does invoke oesign internally, but thats an implementation detail.

@@ -497,6 +498,7 @@ int _sign(int argc, const char* argv[])
pem_file,
NULL,
NULL,
NULL,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did the oesign() signature change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it did

@salsal97 salsal97 merged commit f43fccb into deislabs:main Oct 7, 2022
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.

4 participants