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

Secure signing: Key rotation with CLI #1002

Open
wants to merge 11 commits into
base: l-monninger/key-spec
Choose a base branch
from

Conversation

andygolay
Copy link
Contributor

@andygolay andygolay commented Jan 15, 2025

Summary

  • RFCs: PMIP-7, PMIP-8
  • Categories: any of secure-signing, cli, signing-admin

CLI for signing admin to rotate keys in AWS and Vault apps

Changelog

  • Added util/signing/signing-admin crate with CLI functionality, including KeyManager trait with rotate_key and fetch_public_key methods, implemented for AwsKey and VaultKey structs.
  • Added notify_application function to call public_key/set for the application whose key is being updated.
  • Usage:
  1. Run the HSM demo app if testing locally. (In demo/hsm, run cargo build --release and then in the root of the workspace run for example ./target/release/hsm-demo server ed25519 hashi-corp-vault movement_devNet_fullNode_demoApp_signer_wethTransferSign_replica1 to run a Vault app with that key name)

In a separate terminal:
2. util/signing/signing-admin, run cargo build --release
3. In /target/release run (for example with demo app running locally per instruction 1 above):

AWS:

./signing-admin rotate-key --canonical-string movement/devNet/fullNode/demoApp/signer/wethTransferSign/replica1 --application-url http://0.0.0.0:3000 --backend aws 

Expected output:

Rotating AWS KMS key for alias: alias/movement/devNet/fullNode/demoApp/signer/wethTransferSign/replica1
Successfully rotated key for alias: alias/movement/devNet/fullNode/demoApp/signer/wethTransferSign/replica1
Key rotated. New Key ID: 7a5a5eb3-7e4f-49fc-bbab-ca91e1c5abcb
Fetching public key for AWS Key ID: 7a5a5eb3-7e4f-49fc-bbab-ca91e1c5abcb
Successfully fetched public key: [48, 86, 48, 16, 6, 7, 42, 134, 72, 206, 61, 2, 1, 6, 5, 43, 129, 4, 0, 10, 3, 66, 0, 4, 86, 112, 71, 21, 80, 99, 138, 224, 5, 106, 24, 205, 183, 129, 223, 247, 192, 86, 193, 224, 31, 250, 229, 22, 109, 242, 74, 150, 59, 59, 176, 70, 45, 134, 119, 49, 55, 58, 80, 106, 221, 242, 166, 198, 253, 70, 169, 152, 105, 255, 124, 228, 32, 175, 112, 124, 111, 65, 216, 10, 159, 174, 139, 156]
Retrieved public key: [48, 86, 48, 16, 6, 7, 42, 134, 72, 206, 61, 2, 1, 6, 5, 43, 129, 4, 0, 10, 3, 66, 0, 4, 86, 112, 71, 21, 80, 99, 138, 224, 5, 106, 24, 205, 183, 129, 223, 247, 192, 86, 193, 224, 31, 250, 229, 22, 109, 242, 74, 150, 59, 59, 176, 70, 45, 134, 119, 49, 55, 58, 80, 106, 221, 242, 166, 198, 253, 70, 169, 152, 105, 255, 124, 228, 32, 175, 112, 124, 111, 65, 216, 10, 159, 174, 139, 156]
Notifying application at http://0.0.0.0:3000/public_key/set with public key [48, 86, 48, 16, 6, 7, 42, 134, 72, 206, 61, 2, 1, 6, 5, 43, 129, 4, 0, 10, 3, 66, 0, 4, 86, 112, 71, 21, 80, 99, 138, 224, 5, 106, 24, 205, 183, 129, 223, 247, 192, 86, 193, 224, 31, 250, 229, 22, 109, 242, 74, 150, 59, 59, 176, 70, 45, 134, 119, 49, 55, 58, 80, 106, 221, 242, 166, 198, 253, 70, 169, 152, 105, 255, 124, 228, 32, 175, 112, 124, 111, 65, 216, 10, 159, 174, 139, 156]
Successfully notified the application.
Application updated with new public key: [48, 86, 48, 16, 6, 7, 42, 134, 72, 206, 61, 2, 1, 6, 5, 43, 129, 4, 0, 10, 3, 66, 0, 4, 86, 112, 71, 21, 80, 99, 138, 224, 5, 106, 24, 205, 183, 129, 223, 247, 192, 86, 193, 224, 31, 250, 229, 22, 109, 242, 74, 150, 59, 59, 176, 70, 45, 134, 119, 49, 55, 58, 80, 106, 221, 242, 166, 198, 253, 70, 169, 152, 105, 255, 124, 228, 32, 175, 112, 124, 111, 65, 216, 10, 159, 174, 139, 156]

Vault:

./signing-admin rotate-key --canonical-string movement_devNet_fullNode_demoApp_signer_wethTransferSign_replica1 --application-url http://0.0.0.0:3000 --backend vault  

Expected output:

Rotating key in Vault for 'movement_devNet_fullNode_demoApp_signer_wethTransferSign_replica1'
Successfully rotated key for 'movement_devNet_fullNode_demoApp_signer_wethTransferSign_replica1'
Key rotated. New Key ID: movement_devNet_fullNode_demoApp_signer_wethTransferSign_replica1
Fetching public key from Vault for 'movement_devNet_fullNode_demoApp_signer_wethTransferSign_replica1'
Successfully fetched public key: [144, 155, 105, 32, 51, 122, 170, 248, 180, 208, 201, 1, 227, 77, 113, 248, 34, 121, 214, 17, 39, 13, 196, 62, 240, 51, 235, 153, 109, 83, 81, 191]
Retrieved public key: [144, 155, 105, 32, 51, 122, 170, 248, 180, 208, 201, 1, 227, 77, 113, 248, 34, 121, 214, 17, 39, 13, 196, 62, 240, 51, 235, 153, 109, 83, 81, 191]
Notifying application at http://0.0.0.0:3000/public_key/set with public key [144, 155, 105, 32, 51, 122, 170, 248, 180, 208, 201, 1, 227, 77, 113, 248, 34, 121, 214, 17, 39, 13, 196, 62, 240, 51, 235, 153, 109, 83, 81, 191]
Successfully notified the application.
Application updated with new public key: [144, 155, 105, 32, 51, 122, 170, 248, 180, 208, 201, 1, 227, 77, 113, 248, 34, 121, 214, 17, 39, 13, 196, 62, 240, 51, 235, 153, 109, 83, 81, 191]

Outstanding issues

  • There need to be access controls so that only the signing admin role can call the functions.
  • It's possible to accidentally put in the wrong app.
  • Currently with aws set to --backend the CLI takes a key alias delineated by slashes, rather than underscores. (See usage example above.)

@andygolay andygolay changed the title feat: begin adding key rotation for cli HSM Demo: Key rotation with CLI Jan 16, 2025
@andygolay andygolay marked this pull request as ready for review January 16, 2025 19:39
@andygolay andygolay changed the title HSM Demo: Key rotation with CLI Secure signing: Key rotation with CLI Jan 16, 2025
@andygolay
Copy link
Contributor Author

andygolay commented Jan 16, 2025

Just a note cc @l-monninger, the way I convert the secp256k1 public key (on the AWS side) is from DER public key to the 64-byte public key, i.e. an uncompressed Eth address with the prefix stripped off. Please let me know if that's not the way we want apps to store them for their application public key state?

}

/// Creates a new AWS KMS key
fn create_key(&self) -> Result<String> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please follow a similar organizational pattern to the one used in the HSM demo app. I would prefer that if you extract these implementations out, you do it as a separate crate with traits.

util/signing/signing-admin/src/aws.rs Outdated Show resolved Hide resolved
util/signing/signing-admin/src/aws.rs Outdated Show resolved Hide resolved
util/signing/signing-admin/src/key_manager.rs Outdated Show resolved Hide resolved
util/signing/signing-admin/src/vault.rs Outdated Show resolved Hide resolved
}
}

impl KeyManager for VaultKey {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The way I see it, KeyManager should most likely be generic over a Application and a SigningBackend.

The Application can be expected to implement some phases of the rotation, while the SigningBackend others, so these are also traits ofc.

The thing I do not see a clear argument for is how you are planing to get away without a 2-PC or 3-PC. Both of these operations are fallible.

Please read up on properties of a 2-PC and 3-PC and make your case.

Copy link
Contributor Author

@andygolay andygolay Jan 17, 2025

Choose a reason for hiding this comment

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

Ah, I didn't realize 2PC and 3PC are formally defined. I thought you meant a two-step or three-step process. Okay I'll read up on them.

So the problem is that the transaction isn't atomic, for example the signing admin can update the signing backend that handles signing for an app replica, but the request to public_key/set might fail?

In that case, what if, if the request to public_key/set fails, then the key is simply rotated back to the version it was initially at before calling rotate_key?

I'm assuming different app replicas always use different public keys.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The first option you're describing is essentially a 2-PC, yes. Instead of a committing and then reverting though, you would usually prepare and then commit.

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 if we're doing prepare and then commit, does the "prepare" step happen in Terraform? I'm still a little unclear on what would be optimal for a preparation method? cc @mcmillennick

@andygolay andygolay requested a review from l-monninger January 17, 2025 23:59
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.

2 participants