Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
AWS Key Manager #65
AWS Key Manager #65
Changes from 13 commits
71f2a43
f7048e5
004ccda
3041656
ba60577
1fa1124
52863e8
b90987a
e507699
39091ae
f898622
e6180cc
7a39930
ac95d4d
2b395c3
bbef724
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider allowing the kmsClient to be provided by the caller (e.g. to allow configurable behaviour of the client like timeouts)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already possibly for the caller to supply, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a note, it is possible to drop the enum type for the assignment, which might be useful given the use of named params. e.g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why thumbprint instead of using the
kid
field?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does go into
kid
up ingeneratePrivateKey
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andresuribe87 because
kid
isn't a required property of JWKs (reference).IMO, Using JWK thumbprints makes it so there's a deterministic mapping between a DID's verification method (presuming that the verification method has
publicKeyJwk
, a requirement we intend to surface as part of an upcoming interop profile) and a key manager's key alias without having to rely on a Verification Method ID or JWK kid. Especially because a verification method id and a kid (even if both are present) don't have to matchThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would make it easier to consume for everyone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was an argument made that all crypto ops should be bytes-in/bytes-out. This makes KeyManager compatible with Crypto.kt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that
JWK
already have akid
field, is it possible to use that field to identify keys?IMO, it's easier to put the alias within that field, rather than expanding the interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this a good point, and kicked off a big discussion. Both AwsKeyManager and InMemoryKeyManager are setting the alias into
kid
, so it would work with the code we have here.It's not clear that the publicKeyJwk will always have a
kid
though. It's optional. I think it's up to the DID method how that publicKeyJwk is put together?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm gonna merge this now to unblock #63, but happy to keep the convo going in Slack