Implement command 'sign-eif' #406
Conversation
src/common/commands_parser.rs
Outdated
| pub private_key: Option<String>, | ||
| /// The region in which the KMS key resides. | ||
| pub region: Option<String>, | ||
| /// The KMS key id. | ||
| pub key_id: Option<String>, |
There was a problem hiding this comment.
This should work as an enum as such { LocalKey(Path), KmsKey(KeyArn, Region) }
src/common/commands_parser.rs
Outdated
| impl SignArgs { | ||
| /// Construct a new `SignArg` instance from the given command-line arguments. | ||
| pub fn new_with(args: &ArgMatches) -> NitroCliResult<Self> { | ||
| let signing_method = parse_signing_method(args) |
There was a problem hiding this comment.
This signing_method argument is really not required. It should be indicated by either the presence of key_id or private_key and no need to have a separate argument for it.
src/common/commands_parser.rs
Outdated
| .map_err(|err| err.add_subaction("Parse signing method".to_string()))?; | ||
| let private_key = parse_private_key(args); | ||
| let region = parse_region(args); | ||
| let key_id = parse_key_id(args); |
There was a problem hiding this comment.
key_id is a bit hard to interpret as "KMS Key ID". Perhaps kms_key_arn make more sense. Also region is not very obvious.
There was a problem hiding this comment.
region should also be optional. kms_client should load it from .aws/config or from AWS_REGION env variable. Perhaps there's some way to check if any of these are set or not.
src/lib.rs
Outdated
| .takes_value(true) | ||
| .help("The KMS key id.") | ||
| .required(false) | ||
| .conflicts_with("config"), |
There was a problem hiding this comment.
Should also conflict with private_key.
src/lib.rs
Outdated
| .takes_value(true) | ||
| .help("The region in which the KMS key resides.") | ||
| .required(false) | ||
| .conflicts_with("config"), |
There was a problem hiding this comment.
Should conflict with private_key.
|
@pro-vlad Thanks for this! Are you still planning to get it merged? We're looking for this feature |
- Adjust existing tests to work with the changes needed for kms-signing - Add test for signing existing eifs
- use default region from config if region argument was not provided - change argument conditions and conflicts - use async calls to init kms key - print measurements after signing an existing image
pro-vlad
force-pushed
the
kms-image-signing
branch
from
d7ba5aa
to
979e106
Compare
| .map(|val| val.to_string()); | ||
|
|
||
| let signing_key = match (kms_key_arn, private_key_path) { |
There was a problem hiding this comment.
This shouldn't just be a key path. It should accept PKCS#11 URIs and conform to david.woodhou.se/draft-woodhouse-cert-best-practice.html
Issue #, if available:
#204
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.