AWS Key Manager #65
AWS Key Manager #65
Conversation
# Conflicts: # crypto/build.gradle.kts # crypto/src/test/kotlin/web5/sdk/crypto/Secp256k1Test.kt
tomdaffurn
requested review from
andresuribe87,
nitro-neal and
phoebe-lew
as code owners
| * - [JWSAlgorithm.ES256K] | ||
| */ | ||
| public class AwsKeyManager( | ||
| private val kmsClient: AWSKMS = AWSKMSClientBuilder.standard().build() |
There was a problem hiding this comment.
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.
It's already possibly for the caller to supply, isn't it?
fun `test a custom KMS client`() {
val kmsClient = AWSKMSClient.builder()
.withCredentials(AWSStaticCredentialsProvider(BasicAWSCredentials("foo", "bar")))
.build()
val customisedKeyManager = AwsKeyManager(kmsClient = kmsClient)
assertThrows<AmazonServiceException> {
customisedKeyManager.generatePrivateKey(JWSAlgorithm.ES256K)
}
}
| ) | ||
|
|
||
| private val algorithmDetails = mapOf( | ||
| JWSAlgorithm.ES256K to AlgorithmDetails( |
There was a problem hiding this comment.
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.
ES256K to AlgorithmDetails(
algorithm = ES256K,
curve = SECP256K1,
keySpec = ECC_SECG_P256K1,
signingAlgorithm = ECDSA_SHA_256,
newDigest = { SHA256Digest() }
)
| * @return The alias belonging to [publicKey] | ||
| */ | ||
| override fun getDefaultAlias(publicKey: JWK): String { | ||
| val jwkThumbprint = publicKey.computeThumbprint() |
There was a problem hiding this comment.
Curious why thumbprint instead of using the kid field?
There was a problem hiding this comment.
This does go into kid up in generatePrivateKey
There was a problem hiding this comment.
@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 match
| * | ||
| * Implementations should ensure that the signing process is secured, utilizing secure cryptographic | ||
| * practices and safeguarding the private key during the operation. The specific signing algorithm | ||
| * used may depend on the type and parameters of the stored key. | ||
| */ | ||
| public fun sign(keyAlias: String, payload: Payload) | ||
| public fun sign(keyAlias: String, signingInput: ByteArray): ByteArray |
There was a problem hiding this comment.
I think this would make it easier to consume for everyone.
| public fun sign(keyAlias: String, signingInput: ByteArray): ByteArray | |
| public fun sign(keyAlias: String, signingInput: ByteArray): JWSObject |
There was a problem hiding this comment.
There was an argument made that all crypto ops should be bytes-in/bytes-out. This makes KeyManager compatible with Crypto.kt
| * @return The alias belonging to [publicKey] | ||
| * @throws IllegalArgumentException if the key is not known to the [KeyManager] | ||
| */ | ||
| public fun getDefaultAlias(publicKey: JWK): String |
There was a problem hiding this comment.
Given that JWK already have a kid 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.
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.
I'm gonna merge this now to unblock #63, but happy to keep the convo going in Slack
Co-authored-by: Andres Uribe <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #65 +/- ##
==========================================
- Coverage 13.09% 0.00% -13.10%
==========================================
Files 15 16 +1
Lines 443 519 +76
Branches 54 57 +3
==========================================
- Hits 58 0 -58
- Misses 380 519 +139
+ Partials 5 0 -5
|
Overview