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

Allow signing EIFs with KMS while building #650

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

atanzu
Copy link
Contributor

@atanzu atanzu commented Jan 20, 2025

This commit updates nitro-cli to use aws-nitro-enclaves-image-format v0.4 which allows to sign EIF during build using a key from KMS. In order to do so, user needs to specify KMS key ARN and region instead of the local private key file.

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@atanzu
Copy link
Contributor Author

atanzu commented Jan 20, 2025

License check fails due to changes in ring crate: EmbarkStudios/cargo-about#271

Security check fails due to rustsec/rustsec#1318


match (&signing_certificate, &private_key) {
(Some(_), None) => {
match (&signing_certificate, &private_key, &kms_key_id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

a bit redundant after we setup clap to verify all the rules with group and require

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So shall I just panic then?

Copy link
Contributor

Choose a reason for hiding this comment

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

otherwise if we ignore clap rules, this misses (Some(_), None, None) case, also kms_key_region is not involved here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer to have a more robust error handling in case this function is called from somewhere else in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

may be use assert?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this function is meant to be called on the clap::ArgMatches and the options are just check (without unpacking them), we are just validating a precondition and an assert should be enough. But I don't have a strong opinion on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will remove this check completely because later on we still have another check (when creating a SignKeyInfo.

tests/tests.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
mariusknaust
mariusknaust previously approved these changes Jan 21, 2025

match (&signing_certificate, &private_key) {
(Some(_), None) => {
match (&signing_certificate, &private_key, &kms_key_id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this function is meant to be called on the clap::ArgMatches and the options are just check (without unpacking them), we are just validating a precondition and an assert should be enough. But I don't have a strong opinion on this.

This commit updates `nitro-cli` to use `aws-nitro-enclaves-image-format`
v0.4 which allows to sign EIF during build using a key from KMS. In
order to do so, user needs to specify KMS key ARN and region instead of
the local private key file.

Signed-off-by: Mark Kirichenko <[email protected]>
This commit removes version pinning for `cargo-audit` tool.
This commit enables a specific workaround for the `ring` crate, so
`cargo-about` could properly extract the license information.

Signed-off-by: Mark Kirichenko <[email protected]>
This commit extends CLI test suite to validate new CLI arguments.

Signed-off-by: Mark Kirichenko <[email protected]>
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