Sign EIFs separately #652

atanzu · 2025-01-23T14:26:16Z

Issue #, if available:
#204

Description of changes:

This PR allows to sign pre-built EIFs using the new sign-eif command. Users can use either a local private key, or a KMS key.

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

meerd · 2025-01-24T10:52:54Z

SPECS/aws-nitro-enclaves-cli.spec

@@ -195,6 +195,12 @@ fi
 %{ne_include_dir}/*

 %changelog
+* Thu Jan 23 2025 Mark Kirichenko <[email protected]> - 1.4.0-0


I think this change and the version bump in Cargo.toml should be published as a separate PR.

Ack, removed this commit from the PR branch

meerd · 2025-01-24T10:53:12Z

sources

@@ -1 +1 @@
-23d02e54e9571764baa45425e1a873936e6ad338  nitro-cli-dependencies.tar.gz
+4b6d694643b6c4723658d5a7b2797103875c269a  nitro-cli-dependencies.tar.gz


meerd · 2025-01-24T11:12:36Z

src/lib.rs

+                            .help("Local path to developer's X509 signing certificate."),
+                    )
+                    .arg(
+                        Arg::new("private-key")


I have concerns about including this feature in the new command. This approach to handling private keys doesn't follow security best practices. While I understand the desire to maintain consistency with the build-eif command, I recommend removing this feature from the sign-eif command entirely.

If there are compelling reasons to keep it, we should:

Add detailed explanation in the help text explaining the security implications

Implement warning messages to alert users that this is not the recommended approach for private key handling

CC: @eugkoira

My vision is, we might have customers who build EIFs in one (insecure) environment, and then sign those EIFs separately in a secure environment with a local private key, similar to #639.

I don't get it, is the proposal to not have the private-key signing at all?
Why is it a bad practice?

We all know what could happen if a private key is leaked. :) Given the security risks associated with private key exposure, the users should be encouraged to use KMS as the recommended best practice. (Or using PKCS#11 in the future. This was also mentioned by @dwmw2 here.)

I believe there are two options:

Omit local private-key signing for sign-eif command and mark the argument in build-eif as deprecated

Keep private-key as an option for sign-eif but add warnings about security implications.

Customers can still have a secure isolated environment for signing with a private key like in #639. Why should we restrict their possibilities?

We can specify in our documentation the recommended way but I don't understand the urge to remove it completely.

Customers can literally use Nitro Enclaves to receive the signing key from KMS and pass it as parameter here, right? I don't believe the tool should per se assume an insecure environment. To clarify environmental requirements and establish best practices is the task of our documentation.

Signing requires a private key. That's kind of fundamental. The question is where that private key is found.

The real problem here seems to be that we have two completely separate command line arguments to provide the private key. There's the --private-key argument, which makes sense. A user will reasonably expect to be able to pass that a filename, an RFC7512 PKCS#11 URI, and maybe other kinds of things like a TPM URI referencing keys in non-volatile storage (although that's kind of dead now, and mostly a user would expect to provide a PEM file with a TPM-wrapped key and have that Just Work too).

And then we have the --kms-key-id argument. Which probably should have just accepted an ARN for a KMS key, e.g. --private-key arn:aws:kms:us-west-2:111122223333:key/0987dcba-09fe-87dc-65ba-ab0987654321, from the start? If we're talking about cleaning up the user interface, I'd much prefer us to deprecate --kms-key-id and use only --private-key to tell the tooling which private key to use.

I agree that having a single parameter which dispatches internally basing on the provided value is the best option in terms of UX, and good thing that --kms-key-id has only been introduced to mainline, there is no actual release which fixed that in stone.

good thing that --kms-key-id has only been introduced to mainline, there is no actual release which fixed that in stone.

Excellent. So let's fix that to use --private-key arn:aws:kms:… before ever releasing it.

meerd · 2025-01-24T11:14:09Z

src/lib.rs

+                            .help("Path pointing to a prebuilt Eif image")
+                    )
+                    .arg(
+                        Arg::new("signing-certificate")


If we decide to retain the local signing option, this should be --signing-certificate-path for clarity.

How I see it, the parameter names shall be the same as the ones used when signing an image during build. Updating syntax for build-enclave is a breaking change unless we keep both the old parameter name and the new one, plus add a deprecation warning.

I vote for consistency with what we already have

we keep both the old parameter name and the new one, plus add a deprecation warning

Yes, this is what I meant. To clarify, --signing-certificate or --private-key should remain in place until the next major release of nitro-cli.

meerd · 2025-01-24T11:14:30Z

src/lib.rs

+                    )
+                    .arg(
+                        Arg::new("private-key")
+                            .long("private-key")


If we decide to retain the local signing option, this should be --private-key-path for clarity.

it will be inconsistent with build-eif experience and can bring more confusion then benefits

eugkoira · 2025-01-24T14:21:12Z

src/common/commands_parser.rs

+        let kms_key_id = parse_kms_key_id(args);
+        let kms_key_region = parse_kms_key_region(args);
+
+        validate_signing_credentials(&signing_certificate, &private_key, &kms_key_id)?;


a bit weird that kms_key_region is not validated

It is OK to have it empty, AWS client will try to get the region from env or uses the default one.

no, I mean potentially we could have region and private key at the same time

Which is why you could just pass the ARN of the KMS key in as the --private-key argument... :)

eugkoira · 2025-01-24T14:36:20Z

src/lib.rs

+                        Arg::new("kms-key-region")
+                            .long("kms-key-region")
+                            .help("Specify region in which the KMS key resides")
+                            .requires("kms-key-id")


also testing those rules same as for build-eif would be beneficial

This commit allows to add or update signature of already existing EIFs. Signed-off-by: Mark Kirichenko <[email protected]>

This commit adds tests cases to validate the signing functionality. Signed-off-by: Mark Kirichenko <[email protected]>

atanzu force-pushed the feature/sign-eifs-separately branch from 79f9243 to eff4299 Compare January 24, 2025 07:19

atanzu marked this pull request as ready for review January 24, 2025 07:53

meerd reviewed Jan 24, 2025

View reviewed changes

eugkoira reviewed Jan 24, 2025

View reviewed changes

eugkoira previously approved these changes Jan 24, 2025

View reviewed changes

eugkoira reviewed Jan 24, 2025

View reviewed changes

atanzu added 2 commits January 28, 2025 09:16

Add sign-eif command

486ee2d

This commit allows to add or update signature of already existing EIFs. Signed-off-by: Mark Kirichenko <[email protected]>

Add tests for EIF signing

1ebe752

This commit adds tests cases to validate the signing functionality. Signed-off-by: Mark Kirichenko <[email protected]>

atanzu dismissed eugkoira’s stale review via 1ebe752 January 28, 2025 09:31

atanzu force-pushed the feature/sign-eifs-separately branch from eff4299 to 1ebe752 Compare January 28, 2025 09:31

eugkoira approved these changes Jan 28, 2025

View reviewed changes

atanzu requested review from mariusknaust, meerd and dwmw2 January 29, 2025 10:31

meerd approved these changes Jan 29, 2025

View reviewed changes

atanzu merged commit a79f794 into main Jan 29, 2025
14 checks passed

atanzu deleted the feature/sign-eifs-separately branch January 29, 2025 15:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sign EIFs separately #652

Sign EIFs separately #652

atanzu commented Jan 23, 2025

meerd Jan 24, 2025

atanzu Jan 28, 2025

meerd Jan 24, 2025

meerd Jan 24, 2025

atanzu Jan 24, 2025

eugkoira Jan 24, 2025 •

edited

Loading

meerd Jan 24, 2025

eugkoira Jan 24, 2025

agraf Jan 24, 2025

dwmw2 Jan 24, 2025 •

edited

Loading

atanzu Jan 24, 2025

dwmw2 Jan 24, 2025

meerd Jan 24, 2025

atanzu Jan 24, 2025

eugkoira Jan 24, 2025

meerd Jan 24, 2025

meerd Jan 24, 2025

eugkoira Jan 24, 2025

eugkoira Jan 24, 2025

atanzu Jan 24, 2025

eugkoira Jan 24, 2025

dwmw2 Jan 24, 2025

eugkoira Jan 24, 2025

		@@ -1 +1 @@
		23d02e54e9571764baa45425e1a873936e6ad338 nitro-cli-dependencies.tar.gz
		4b6d694643b6c4723658d5a7b2797103875c269a nitro-cli-dependencies.tar.gz

Sign EIFs separately #652

Sign EIFs separately #652

Conversation

atanzu commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugkoira Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwmw2 Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugkoira Jan 24, 2025 •

edited

Loading

dwmw2 Jan 24, 2025 •

edited

Loading