feat: DID schema to support publicKeyJwk & secp256k1 #1510
Conversation
| return nil, fmt.Errorf("unable to read JWK, %w", err) | ||
| } | ||
|
|
||
| if strings.EqualFold(key.Kty, secp256k1Kty) && strings.EqualFold(key.Crv, secp256k1Crv) { |
There was a problem hiding this comment.
some comments on the various conditions in this function would be helpful (linked to the JWK data model).
(this very specific case seems a bit suspicious).
There was a problem hiding this comment.
Are there other cases we aren't covering yet?
There was a problem hiding this comment.
To make it more generic, I would override UnmarshalJSON() of JWK to support secp256k1 elliptic curve (MarshallJSON() for symmetry). And I would create e.g. Bytes() method returning []byte for JWK as well where x509.MarshalPKIXPublicKey() could be used.
There was a problem hiding this comment.
@sudeshrshetty the above is just my personal opinion, I'm OK with your solution in general.
There was a problem hiding this comment.
@troyronda almost all cases are covered by go-jose, my implementation only covers 'EC/secp256k1', rest of the types will be handled by go-jose.
pkg/doc/jose/jwk.go
Outdated
| } | ||
| } | ||
|
|
||
| func ecPublicKey(xBuffer, yBuffer *byteBuffer) ([]byte, error) { |
There was a problem hiding this comment.
rename this function to btcPublicKey
pkg/doc/jose/jwk_test.go
Outdated
| "kid": "sample@sample.id", | ||
| "x": "wQehEGTVCu32yp8IwTaBCqPUIYslyd-WoFRsfDKE9II", | ||
| "y": "rIJO8RmkExUecJ5i15L9OC7rl7pwmYFR8QQgdM1ERWI", | ||
| "alg": "EdDSA" |
There was a problem hiding this comment.
This doesn't look right: "alg": "EdDSA"
with
"crv": "secp256k1",
secp256k1 is an ECDSA curve to my knowledge. We only support Ed25519 as EdDSA curve.
There was a problem hiding this comment.
that's only sample input I generated for tests, you are right EdDSA also works.
pkg/doc/jose/jwk_test.go
Outdated
| "kid": "sample@sample.id", | ||
| "x": "wQehEGTVCu32yp8IwTaBCqPUIYslyd-WoFRsfDKE9II", | ||
| "y": "", | ||
| "alg": "EdDSA" |
There was a problem hiding this comment.
I see, alg is ignored by JWK parser since it uses 'kty=EC' and logic based on curve, I fixed tests by regenerating JWK using ES256(ECDSA using P-256)
pkg/doc/jose/jwk_test.go
Outdated
| "kid": "sample@sample.id", | ||
| "x": "", | ||
| "y": "", | ||
| "alg": "EdDSA" |
There was a problem hiding this comment.
same as above
pkg/doc/jose/jwk_test.go
Outdated
| "kid": "sample@sample.id", | ||
| "x": "x", | ||
| "y": "y", | ||
| "alg": "EdDSA" |
There was a problem hiding this comment.
same as above
pkg/doc/jose/jwk_test.go
Outdated
| "kid": "sample@sample.id", | ||
| "x": "{", | ||
| "y": "y", | ||
| "alg": "EdDSA" |
There was a problem hiding this comment.
same here.. I think we need a mapping validation between alg and crv as they're related
There was a problem hiding this comment.
I would like to see a validation check between crv and alg that we currently support as follows:
ECDSA crv is valid only for P-256, P-384, P-521 and secp256k1 algs
EdDSA crv is valid only for Ed25519 alg
If we wanna support other key algs, we should add them to the list.
There was a problem hiding this comment.
I agree, go-jose library is doing that, I just branched out my logic when I see 'kty=EC,crv=secp256k1'. 'alg' is not mandatory field in jwk if crv/ec are explanatory enough.
Codecov Report
@@ Coverage Diff @@
## master #1510 +/- ##
==========================================
- Coverage 91.32% 91.27% -0.05%
==========================================
Files 150 151 +1
Lines 10219 10285 +66
==========================================
+ Hits 9332 9388 +56
- Misses 511 517 +6
- Partials 376 380 +4
Continue to review full report at Codecov.
|
- PublicKeys to support `publicKeyJwk` - Support for JWK public key decode including `secp256k1` - Context URI updated to validate `https://www.w3.org/ns/did/v1` as well as the older URI `https://w3id.org/did/v1` - Removed public key max number of properties validation, as per spec 'The public key objects MAY include additional properties' - closes hyperledger-archives#1509 Signed-off-by: sudesh.shetty <sudesh.shetty@securekey.com>
publicKeyJwksecp256k1https://www.w3.org/ns/did/v1as wellas the older URI
https://w3id.org/did/v1'The public key objects MAY include additional properties'
Signed-off-by: sudesh.shetty sudesh.shetty@securekey.com