Skip to content

Commit

Permalink
fix: wrong Ed25519 KID value (hyperledger-archives#2533)
Browse files Browse the repository at this point in the history 
This change fixes go-jose's Ed25519 JWK Thumbprint value used in the KMS KID creation.

It also includes a check for emptiness of the key bytes with calling CreateKID() and
fixed related unit tests in the framework.

Finally the webkms bdd tests are commented out since the remote kms server requires
an udpate. They will be uncommented in a future revision when the server image tag
is updated with a revision of aries-framework-go that contains this change.

closes hyperledger-archives#2530

Signed-off-by: Baha Shaaban <[email protected]>
  • Loading branch information
@baha-ai @sudeshrshetty
baha-ai authored and sudeshrshetty committed Oct 14, 2021
1 parent 8932979 commit 74c6cd9
Show file tree
Hide file tree
Showing 7 changed files with 243 additions and 54 deletions.
43 changes: 41 additions & 2 deletions pkg/client/didexchange/client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ SPDX-License-Identifier: Apache-2.0
package didexchange

import (
"bytes"
"crypto/ed25519"
"crypto/rand"
"encoding/json"
Expand All @@ -17,6 +18,7 @@ import (
"time"

"github.com/btcsuite/btcutil/base58"
"github.com/google/tink/go/keyset"
"github.com/google/uuid"
"github.com/stretchr/testify/require"

Expand All @@ -26,6 +28,8 @@ import (
"github.com/hyperledger/aries-framework-go/pkg/didcomm/protocol/mediator"
"github.com/hyperledger/aries-framework-go/pkg/doc/did"
"github.com/hyperledger/aries-framework-go/pkg/framework/aries"
"github.com/hyperledger/aries-framework-go/pkg/kms"
"github.com/hyperledger/aries-framework-go/pkg/kms/localkms"
mockprotocol "github.com/hyperledger/aries-framework-go/pkg/mock/didcomm/protocol"
mocksvc "github.com/hyperledger/aries-framework-go/pkg/mock/didcomm/protocol/didexchange"
mockroute "github.com/hyperledger/aries-framework-go/pkg/mock/didcomm/protocol/mediator"
Expand Down Expand Up @@ -1100,6 +1104,10 @@ func TestServiceEvents(t *testing.T) {
ed25519KH, err := mockkms.CreateMockED25519KeyHandle()
require.NoError(t, err)

pubKeyBytes := exportPubKeyBytes(t, ed25519KH)
kid, err := localkms.CreateKID(pubKeyBytes, kms.ED25519Type)
require.NoError(t, err)

// create the client
c, err := New(&mockprovider.Provider{
ProtocolStateStorageProviderValue: protocolStateStore,
Expand All @@ -1108,7 +1116,13 @@ func TestServiceEvents(t *testing.T) {
didexchange.DIDExchange: didExSvc,
mediator.Coordination: &mockroute.MockMediatorSvc{},
},
KMSValue: &mockkms.KeyManager{CreateKeyValue: ed25519KH},
KMSValue: &mockkms.KeyManager{
CreateKeyValue: ed25519KH,
CreateKeyID: kid,
CrAndExportPubKeyValue: pubKeyBytes,
CrAndExportPubKeyID: kid,
GetKeyValue: ed25519KH,
},
})
require.NoError(t, err)
require.NotNil(t, c)
Expand Down Expand Up @@ -1195,6 +1209,10 @@ func TestAcceptExchangeRequest(t *testing.T) {
ed25519KH, err := mockkms.CreateMockED25519KeyHandle()
require.NoError(t, err)

pubKeyBytes := exportPubKeyBytes(t, ed25519KH)
kid, err := localkms.CreateKID(pubKeyBytes, kms.ED25519Type)
require.NoError(t, err)

// create the client
c, err := New(&mockprovider.Provider{
ProtocolStateStorageProviderValue: mockstore.NewMockStoreProvider(),
Expand All @@ -1203,7 +1221,13 @@ func TestAcceptExchangeRequest(t *testing.T) {
didexchange.DIDExchange: didExSvc,
mediator.Coordination: &mockroute.MockMediatorSvc{},
},
KMSValue: &mockkms.KeyManager{CreateKeyValue: ed25519KH},
KMSValue: &mockkms.KeyManager{
CreateKeyValue: ed25519KH,
CreateKeyID: kid,
CrAndExportPubKeyValue: pubKeyBytes,
CrAndExportPubKeyID: kid,
GetKeyValue: ed25519KH,
},
},
)
require.NoError(t, err)
Expand Down Expand Up @@ -1279,6 +1303,21 @@ func TestAcceptExchangeRequest(t *testing.T) {
require.Contains(t, err.Error(), "did exchange client - accept exchange request:")
}

func exportPubKeyBytes(t *testing.T, kh *keyset.Handle) []byte {
t.Helper()

pubKH, err := kh.Public()
require.NoError(t, err)

buf := new(bytes.Buffer)
pubKeyWriter := localkms.NewWriter(buf)

err = pubKH.WriteWithNoSecrets(pubKeyWriter)
require.NoError(t, err)

return buf.Bytes()
}

func TestAcceptInvitation(t *testing.T) {
store := mockstore.NewMockStoreProvider()
didExSvc, err := didexchange.New(&mockprotocol.MockProvider{
Expand Down
30 changes: 29 additions & 1 deletion pkg/controller/command/didexchange/command_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"time"

"github.com/btcsuite/btcutil/base58"
"github.com/google/tink/go/keyset"
"github.com/google/uuid"
"github.com/stretchr/testify/require"

Expand All @@ -30,6 +31,8 @@ import (
"github.com/hyperledger/aries-framework-go/pkg/doc/did"
"github.com/hyperledger/aries-framework-go/pkg/framework/aries"
"github.com/hyperledger/aries-framework-go/pkg/framework/aries/api/vdr"
"github.com/hyperledger/aries-framework-go/pkg/kms"
"github.com/hyperledger/aries-framework-go/pkg/kms/localkms"
"github.com/hyperledger/aries-framework-go/pkg/mock/didcomm/protocol"
mockdidexchange "github.com/hyperledger/aries-framework-go/pkg/mock/didcomm/protocol/didexchange"
mockroute "github.com/hyperledger/aries-framework-go/pkg/mock/didcomm/protocol/mediator"
Expand Down Expand Up @@ -744,6 +747,10 @@ func TestCommand_AcceptExchangeRequest(t *testing.T) {
ed25519KH, err := mockkms.CreateMockED25519KeyHandle()
require.NoError(t, err)

pubKeyBytes := exportPubKeyBytes(t, ed25519KH)
kid, err := localkms.CreateKID(pubKeyBytes, kms.ED25519Type)
require.NoError(t, err)

// create the client
cmd, err := New(&mockprovider.Provider{
ProtocolStateStorageProviderValue: protocolStateStore,
Expand All @@ -752,7 +759,13 @@ func TestCommand_AcceptExchangeRequest(t *testing.T) {
didexsvc.DIDExchange: didExSvc,
mediator.Coordination: &mockroute.MockMediatorSvc{},
},
KMSValue: &mockkms.KeyManager{CreateKeyValue: ed25519KH},
KMSValue: &mockkms.KeyManager{
CreateKeyValue: ed25519KH,
CreateKeyID: kid,
CrAndExportPubKeyValue: pubKeyBytes,
CrAndExportPubKeyID: kid,
GetKeyValue: ed25519KH,
},
},
&mockwebhook.Notifier{
NotifyFunc: func(topic string, message []byte) error {
Expand Down Expand Up @@ -835,6 +848,21 @@ func TestCommand_AcceptExchangeRequest(t *testing.T) {
})
}

func exportPubKeyBytes(t *testing.T, kh *keyset.Handle) []byte {
t.Helper()

pubKH, err := kh.Public()
require.NoError(t, err)

buf := new(bytes.Buffer)
pubKeyWriter := localkms.NewWriter(buf)

err = pubKH.WriteWithNoSecrets(pubKeyWriter)
require.NoError(t, err)

return buf.Bytes()
}

func TestCommand_SaveConnection(t *testing.T) {
t.Run("test save connection - success", func(t *testing.T) {
cmd, err := New(mockProvider(), mockwebhook.NewMockWebhookNotifier(), "", false)
Expand Down
13 changes: 7 additions & 6 deletions pkg/didcomm/packer/legacy/authcrypt/authcrypt_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,9 @@ func persistKey(t *testing.T, pub, priv string, km kms.KeyManager) error {
t.Helper()

kid, err := localkms.CreateKID(base58.Decode(pub), kms.ED25519Type)
require.NoError(t, err)
if err != nil {
return err
}

edPriv := ed25519.PrivateKey(base58.Decode(priv))
if len(edPriv) == 0 {
Expand Down Expand Up @@ -324,9 +326,8 @@ func TestEncryptComponents(t *testing.T) {

t.Run("Failure: generate recipient header with bad sender key", func(t *testing.T) {
_, err := packer2.buildRecipient(&[32]byte{}, []byte(""), base58.Decode(rec1Pub))
require.Error(t, err)
require.Contains(t, err.Error(), "getKeySet: failed to read json keyset from reader: cannot read data"+
" for keysetID")
require.EqualError(t, err, "buildRecipient: failed to create KID for public key: createKID: "+
"empty key")
})

t.Run("Failure: generate recipient header with bad recipient key", func(t *testing.T) {
Expand Down Expand Up @@ -472,7 +473,7 @@ func unpackComponentFailureTest(t *testing.T, protectedHeader, msg, recKeyPub, r

err := persistKey(t, recKeyPub, recKeyPriv, w)

if errString == "error converting bad public key" {
if errString == "createKID: empty key" {
require.EqualError(t, err, errString)
return
}
Expand Down Expand Up @@ -647,7 +648,7 @@ func TestUnpackComponents(t *testing.T) {
prot,
`"iv": "oDZpVO648Po3UcoW", "ciphertext": "pLrFQ6dND0aB4saHjSklcNTDAvpFPmIvebCis7S6UupzhhPOHwhp6o97_EphsWbwqqHl0HTiT7W9kUqrvd8jcWgx5EATtkx5o3PSyHfsfm9jl0tmKsqu6VG0RML_OokZiFv76ZUZuGMrHKxkCHGytILhlpSwajg=", "tag": "6GigdWnW59aC9Y8jhy76rA=="}`, // nolint: lll
badKeyPub, badKeyPriv,
"error converting bad public key")
"createKID: empty key")
})
}

Expand Down
55 changes: 44 additions & 11 deletions pkg/doc/util/jwkkid/kid_creator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,25 @@ var errInvalidKeyType = errors.New("key type is not supported")
// - base64 raw (no padding) URL encoded KID
// - error in case of error
func CreateKID(keyBytes []byte, kt kms.KeyType) (string, error) {
// X25519 JWK is not supported by go jose, manually build it and build its resulting KID.
if kt == kms.X25519ECDHKWType {
if len(keyBytes) == 0 {
return "", errors.New("createKID: empty key")
}

switch kt {
case kms.X25519ECDHKWType: // X25519 JWK is not supported by go jose, manually build it and build its resulting KID.
x25519KID, err := createX25519KID(keyBytes)
if err != nil {
return "", fmt.Errorf("createKID: %w", err)
}

return x25519KID, nil
case kms.ED25519Type: // go-jose JWK thumbprint of Ed25519 has a bug, manually build it and build its resulting KID.
ed25519KID, err := createED25519KID(keyBytes)
if err != nil {
return "", fmt.Errorf("createKID: %w", err)
}

return ed25519KID, nil
}

jwk, err := buildJWK(keyBytes, kt)
Expand Down Expand Up @@ -69,11 +80,13 @@ func buildJWK(keyBytes []byte, kt kms.KeyType) (*jose.JWK, error) {
if err != nil {
return nil, fmt.Errorf("buildJWK: failed to build JWK from ecdsa DER key: %w", err)
}
case kms.ED25519Type:
jwk, err = jose.JWKFromPublicKey(ed25519.PublicKey(keyBytes))
if err != nil {
return nil, fmt.Errorf("buildJWK: failed to build JWK from ed25519 key: %w", err)
}
// TODO remove `case kms.ED25519Type` in CreateKID() and uncomment below case when go-jose fixes Ed25519
// JWK thumbprint. Also remove `createED25519KID(keyBytes []byte)` function further below.
// case kms.ED25519Type:
// jwk, err = jose.JWKFromPublicKey(ed25519.PublicKey(keyBytes))
// if err != nil {
// return nil, fmt.Errorf("buildJWK: failed to build JWK from ed25519 key: %w", err)
// }
case kms.ECDSAP256TypeIEEEP1363, kms.ECDSAP384TypeIEEEP1363, kms.ECDSAP521TypeIEEEP1363:
c := getCurveByKMSKeyType(kt)
x, y := elliptic.Unmarshal(c, keyBytes)
Expand Down Expand Up @@ -165,27 +178,47 @@ func createX25519KID(marshalledKey []byte) (string, error) {
return "", fmt.Errorf("createX25519KID: %w", err)
}

thumbprint := thumbprintX25519(jwk)
thumbprint := sha256(jwk)

return base64.RawURLEncoding.EncodeToString(thumbprint), nil
}

func createED25519KID(keyBytes []byte) (string, error) {
const ed25519ThumbprintTemplate = `{"crv":"Ed25519","kty":"OKP","x":"%s"}`

lenKey := len(keyBytes)

if lenKey > ed25519.PublicKeySize {
return "", errors.New("createED25519KID: invalid Ed25519 key")
}

pad := make([]byte, ed25519.PublicKeySize-lenKey)
ed25519RawKey := append(pad, keyBytes...)

jwk := fmt.Sprintf(ed25519ThumbprintTemplate, base64.RawURLEncoding.EncodeToString(ed25519RawKey))

thumbprint := sha256(jwk)

return base64.RawURLEncoding.EncodeToString(thumbprint), nil
}

func buildX25519JWK(keyBytes []byte) (string, error) {
const x25519ThumbprintTemplate = `{"crv":"X25519","kty":"OKP","x":"%s"}`

if len(keyBytes) > cryptoutil.Curve25519KeySize {
lenKey := len(keyBytes)
if lenKey > cryptoutil.Curve25519KeySize {
return "", errors.New("buildX25519JWK: invalid ECDH X25519 key")
}

pad := make([]byte, cryptoutil.Curve25519KeySize-len(keyBytes))
pad := make([]byte, cryptoutil.Curve25519KeySize-lenKey)
x25519RawKey := append(pad, keyBytes...)

jwk := fmt.Sprintf(x25519ThumbprintTemplate, base64.RawURLEncoding.EncodeToString(x25519RawKey))

return jwk, nil
}

func thumbprintX25519(jwk string) []byte {
func sha256(jwk string) []byte {
h := crypto.SHA256.New()
_, _ = h.Write([]byte(jwk)) // nolint: errcheck // SHA256 digest returns empty error on Write()

Expand Down
Loading

0 comments on commit 74c6cd9

Please sign in to comment.