refactor: Support JwsVerificationKey2020 for DID public key #1538

kdimak · 2020-03-31T08:09:33Z

For linked data signatures with JwsVerificationKey2020 type, it's not enough information to make signature verification based on the public key bytes and a type (JwsVerificationKey2020). We need to know additionally what signature algorithm (and/or curve) was used - e.g. ES256 or EdDSA.
In the case of DID documents, we can read this information from the public key if it's defined as publicKeyJwk.
As a result, we extend PublicKey struct with Curve, Alg and KeyType fields and use them in the signature verifier implementations (e.g. in PublicKeyVerifierEC).

closes #1527, #1513

Signed-off-by: Dmitriy Kinoshenko dkinoshenko@gmail.com

codecov · 2020-03-31T08:14:06Z

Codecov Report

Merging #1538 into master will increase coverage by 0.09%.
The diff coverage is 95.28%.

@@            Coverage Diff             @@
##           master    #1538      +/-   ##
==========================================
+ Coverage   91.32%   91.42%   +0.09%     
==========================================
  Files         156      156              
  Lines       10633    10752     +119     
==========================================
+ Hits         9711     9830     +119     
+ Misses        518      517       -1     
- Partials      404      405       +1

Impacted Files	Coverage Δ
pkg/doc/signature/verifier/verifier.go	`95.65% <ø> (ø)`
pkg/doc/did/doc.go	`91.37% <82.05%> (-0.77%)`	⬇️
pkg/doc/jose/jwk.go	`98.59% <98.33%> (+8.11%)`	⬆️
.../suite/jsonwebsignature2020/public_key_verifier.go	`95.74% <100.00%> (+4.07%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e17df7e...669dc5b. Read the comment docs.

baha-ai · 2020-03-31T14:59:21Z

pkg/doc/signature/suite/jsonwebsignature2020/public_key_verifier.go

 	"math/big"

+	"github.com/decred/dcrd/dcrec/secp256k1/v2"


have you tried github.com/btcsuite ? I believe they have secp256k1 alg as well.

I didn't. github.com/decred was initially added by @sudeshrshetty but I think we can switch to github.com/btcsuite.

baha-ai · 2020-03-31T15:03:32Z

pkg/doc/signature/suite/jsonwebsignature2020/public_key_verifier.go

+	switch pubKey.Curve {
+	case "P-256":
+		curve = elliptic.P256()
+		keySize = 32
+	case "P-384":
+		curve = elliptic.P384()
+		keySize = 48
+	case "P-521":
+		curve = elliptic.P521()
+		keySize = 66
+	case "secp256k1":
+		curve = secp256k1.S256()
+		keySize = 32
+	default:
+		return fmt.Errorf("ecdsa: unsupported elliptic curve '%s'", pubKey.Curve)
+	}


this doesn't belong to Verify()

create a separate function

probably create a struct keyInfo that has a curve and keySize if needed in more than one place
probably expose it to be used for Sign as well ?

OK! I will try to simplify it.

baha-ai · 2020-03-31T15:05:53Z

pkg/doc/jose/jwk.go

+	y   *big.Int
+	Kty string
+	Crv string
+}


there's no kid needed?

It's already there, inside embedded jose.JSONWebKey. x, y, key type, curve are not copied from jose.rawJSONWebKey to jose.JSONWebKey as a result of unmarshalling. That's why I had to add them to JWK struct to support secp256k1 properly.

there seems to be duplication of fields with JSONWebKey then.. it's better to remove duplicate fields

sudeshrshetty · 2020-03-31T20:47:23Z

pkg/doc/did/doc.go

+
+	Value []byte
+
+	Algorithm string `json:"-"`


Why we need to show Alg, Curve & KeyType in exported did.Doc.PublicKey? Why can't we just decode public key internally and stick to old structure?

I think we can at least make it hidden, why not?

Those new fields are made private, thank you @sudeshrshetty

looks good, but what is the use of those private fields?
As I can see we are setting it from jwk instance but they are not being read anywhere afterwards.

usually Curve is exposed (like in ecdsa.PublicKey)

Why clients should care about curve? For key type based logics - they should be able to figure out using public key type

For JsonWebSignature2020, it's not enough to have only type value of the public key. Look at the example:

"publicKey": [ { "id": "did:example:123#_Qq0UL2Fq651Q0Fjd6TvnYE-faHiOpRlPVQcY_-tA4A", "type": "JwsVerificationKey2020", "controller": "did:example:123", "publicKeyJwk": { "crv": "Ed25519", "x": "VCpo2LMLhn6iWku8MKvSLg2ZAoC-nlOyPVQaO3FxVeQ", "kty": "OKP", "kid": "_Qq0UL2Fq651Q0Fjd6TvnYE-faHiOpRlPVQcY_-tA4A" } }, { "id": "did:example:123#4SZ-StXrp5Yd4_4rxHVTCYTHyt4zyPfN1fIuYsm6k3A", "type": "JwsVerificationKey2020", "controller": "did:example:123", "publicKeyJwk": { "crv": "secp256k1", "x": "Z4Y3NNOxv0J6tCgqOBFnHnaZhJF6LdulT7z8A-2D5_8", "y": "i5a2NtJoUKXkLm6q8nOEu9WOkso1Ag6FTUT6k_LMnGk", "kty": "EC", "kid": "4SZ-StXrp5Yd4_4rxHVTCYTHyt4zyPfN1fIuYsm6k3A" } } ]

The type is the same and is JwsVerificationKey2020.
We can't deduce is it Ed25519 or secp256k1 etc. We need to extract extra information from publicKeyJwk.
That's the problem solved in this PR.

baha-ai · 2020-03-31T21:41:25Z

pkg/doc/jose/jwk.go

-		//  if kty="EC" and Crv="secp256k1", then handle differently
-		return btcPublicKey(key.X, key.Y)
+	if jwk.Y == nil {
+		return nil, fmt.Errorf("missed y value in EC key")


I noticed there are many detailed error messages returned like the above line.

If these messages reveal key details, then it's a security vulnerability. Please review all the returned errors in this document and only return generic messages to the user (or probably log detailed error messages in debug mode only).

I agree. I changed the detailed error messages to a single generic invalid JWK.

troyronda · 2020-04-02T12:30:56Z

pkg/doc/did/doc.go

+
+	Value []byte
+
+	algorithm string


this would seem like it could be a publicKeyJWK sub struct?

yes, we can just add publicKeyJwk field of *jose.JWK type.

I will also update verifier.PublicKey struct.

troyronda · 2020-04-02T12:43:09Z

pkg/doc/signature/suite/jsonwebsignature2020/public_key_verifier.go

+			Curve:   elliptic.P521(),
+			KeySize: p521KeySize,
+		}
+	case "secp256k1":


what about X25519 / Ed25519?

Filed #1556.

closes hyperledger-archives#1527, hyperledger-archives#1513 Signed-off-by: Dmitriy Kinoshenko <dkinoshenko@gmail.com>

kdimak force-pushed the issue-1527 branch from 679e019 to ebf55eb Compare March 31, 2020 08:48

kdimak requested review from sudeshrshetty, fqutishat, rolsonquadras, troyronda, sandrask and baha-ai March 31, 2020 08:54

baha-ai reviewed Mar 31, 2020

View reviewed changes

kdimak force-pushed the issue-1527 branch from ebf55eb to 8115136 Compare March 31, 2020 20:26

sudeshrshetty reviewed Mar 31, 2020

View reviewed changes

kdimak force-pushed the issue-1527 branch 2 times, most recently from 604d9ad to ce99590 Compare March 31, 2020 21:11

baha-ai reviewed Mar 31, 2020

View reviewed changes

kdimak force-pushed the issue-1527 branch from ce99590 to c251104 Compare April 2, 2020 07:10

troyronda reviewed Apr 2, 2020

View reviewed changes

refactor: Support JwsVerificationKey2020 for DID public key

669dc5b

closes hyperledger-archives#1527, hyperledger-archives#1513 Signed-off-by: Dmitriy Kinoshenko <dkinoshenko@gmail.com>

kdimak force-pushed the issue-1527 branch from c251104 to 669dc5b Compare April 2, 2020 15:40

kdimak mentioned this pull request Apr 2, 2020

Support Ed25519 / RSA in JsonWebSignature2020 signature verifier #1556

Closed

troyronda approved these changes Apr 2, 2020

View reviewed changes

troyronda merged commit dab0fe3 into hyperledger-archives:master Apr 2, 2020

kdimak deleted the issue-1527 branch April 2, 2020 17:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Support JwsVerificationKey2020 for DID public key #1538

refactor: Support JwsVerificationKey2020 for DID public key #1538

kdimak commented Mar 31, 2020

codecov bot commented Mar 31, 2020 •

edited

baha-ai Mar 31, 2020

kdimak Mar 31, 2020

baha-ai Mar 31, 2020

kdimak Mar 31, 2020

baha-ai Mar 31, 2020

kdimak Mar 31, 2020 •

edited

baha-ai Mar 31, 2020 •

edited

sudeshrshetty Mar 31, 2020

kdimak Mar 31, 2020

kdimak Mar 31, 2020

sudeshrshetty Mar 31, 2020 •

edited

baha-ai Mar 31, 2020

sudeshrshetty Mar 31, 2020

kdimak Apr 2, 2020

baha-ai Mar 31, 2020

kdimak Apr 2, 2020

troyronda Apr 2, 2020

kdimak Apr 2, 2020

kdimak Apr 2, 2020

kdimak Apr 2, 2020

troyronda Apr 2, 2020

kdimak Apr 2, 2020

refactor: Support JwsVerificationKey2020 for DID public key #1538

refactor: Support JwsVerificationKey2020 for DID public key #1538

Conversation

kdimak commented Mar 31, 2020

codecov bot commented Mar 31, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdimak Mar 31, 2020 • edited

Choose a reason for hiding this comment

baha-ai Mar 31, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudeshrshetty Mar 31, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 31, 2020 •

edited

kdimak Mar 31, 2020 •

edited

baha-ai Mar 31, 2020 •

edited

sudeshrshetty Mar 31, 2020 •

edited