fix(vcwallet): convert key to kid before issuing vc

[jceb]

Title:

Fix VC wallet: Unable to issue credential

Description:

Fixes #3029

Summary:

Converts the verification method / public key to the key ID that's required by keyManager to retrieve the private key for signing. The conversion steps were taken from the standard VC issuing endpoint.

[jceb]

@Baha-sk Unfortunately, the PR isn't ready yet. A bunch of tests are failing due to the changes that I made. -> This brings me to a larger question:

I noticed that the ImportPrivateKey function accepts a custom key id which serves as the handle to the key store for future operations like issuing a credential. At least for the REST interface it's currently not possible to specify a custom key id that produces a signed VC. Instead, the key id is deterministically derived from the public key that's referenced in the DID document. As a developer I don't have to do anything to manage the key id. This behavior makes a lot of sense to me, because operations like issuing a VC require an identifier - if I can pass in the identifier/controller and the verification method that points to a public key in the DID document I see lots of consistency and easy to follow references.

With custom key ids, I have a much harder time grasping what's going on because the custom key ids are not part of any DID document. However, the current tests are using custom key ids a lot to store the same key multiple times in the database.

Do you see any use cases that require custom key ids apart from storing symmetric keys? As a user of aries-framework-go, I'd prefer to not deal with custom key ids at all because it requires additional work and it might even cause problems by signing a VC with a key that's not associated with the DID that I'm using.

[baha-ai]

@Baha-sk Unfortunately, the PR isn't ready yet. A bunch of tests are failing due to the changes that I made. -> This brings me to a larger question:

I noticed that the ImportPrivateKey function accepts a custom key id which serves as the handle to the key store for future operations like issuing a credential. At least for the REST interface it's currently not possible to specify a custom key id that produces a signed VC. Instead, the key id is deterministically derived from the public key that's referenced in the DID document. As a developer I don't have to do anything to manage the key id. This behavior makes a lot of sense to me, because operations like issuing a VC require an identifier - if I can pass in the identifier/controller and the verification method that points to a public key in the DID document I see lots of consistency and easy to follow references.

With custom key ids, I have a much harder time grasping what's going on because the custom key ids are not part of any DID document. However, the current tests are using custom key ids a lot to store the same key multiple times in the database.

Do you see any use cases that require custom key ids apart from storing symmetric keys? As a user of aries-framework-go, I'd prefer to not deal with custom key ids at all because it requires additional work and it might even cause problems by signing a VC with a key that's not associated with the DID that I'm using.

@jceb setting a custom KID in the ImportPrivateKey is optional, it was added for users who want to preset it to whatever KID they have externally (but doesn't guarantee the key with custom KID will work in all cases in the framework). The KMS does not require it as it uses the JWK thumbprint of a key as the KID.

In other words, you don't have to set a custom KID when calling ImportPrivateKey(), simply use the generated KID returned by the function to keep track of the key in AFGO's KMS.

[baha-ai]

I see now, the unit-tests are failing because the KID is not found:

--- FAIL: TestWallet_Issue (0.54s)
    --- FAIL: TestWallet_Issue/Test_VC_wallet_issue_using_controller_-_success (0.04s)
        wallet_test.go:1271: 
            	Error Trace:	wallet_test.go:1271
            	Error:      	Received unexpected error:
            	            	failed to issue credential: getKeySet: failed to read json keyset from reader: cannot read data for keysetID dfiDkP85Xq5Cald-Q7nT4515MNAcguRJzJpD4CsepAg: data not found

I would remove , kms.WithKeyID(kid) at line 1265 of wallet_test.go: https://github.com/hyperledger/aries-framework-go/blob/main/pkg/wallet/wallet_test.go#L1265

and all other tests explicitly calling WithKeyID(kid)

@jceb so instead of:

		kmgr.ImportPrivateKey(edPriv, kms.ED25519, kms.WithKeyID(kid))

do:

		kid, _, err := kmgr.ImportPrivateKey(edPriv, kms.ED25519)
		require.NoError(t, err)
		require.NotEmpty(t, kid)

[jceb]

Unfortunately, the kid produced by ImportPrivateKey and CreateKeyPair are different. I'll update the PR to fix that issue. I'll also implement the use case that allows for a custom kid while unifying the kids returned by the create and import methods.

[baha-ai]

Unfortunately, the kid produced by ImportPrivateKey and CreateKeyPair are different.

as per the wallet's CreateKeyPair() function, it uses the KMS to create the key and KID, so it must match AFGO's KMS (unless the unit-tests use a mock KMS that returns a fixed key/KID): https://github.com/hyperledger/aries-framework-go/blob/main/pkg/wallet/wallet.go#L608

[codecov[bot]]

Codecov Report

Merging #3049 (2f6d860) into main (8144206) will decrease coverage by 0.05%. The diff coverage is 71.54%.

@@            Coverage Diff             @@
##             main    #3049      +/-   ##
==========================================
- Coverage   88.73%   88.68%   -0.06%     
==========================================
  Files         309      309              
  Lines       41644    41735      +91     
==========================================
+ Hits        36954    37013      +59     
- Misses       3415     3438      +23     
- Partials     1275     1284       +9     

Impacted Files	Coverage Δ
pkg/kms/localkms/kid_creator.go	100.00% <ø> (ø)
pkg/wallet/wallet.go	96.01% <48.78%> (-2.32%)	:arrow_down:
pkg/controller/command/verifiable/command.go	89.43% <66.66%> (-0.11%)	:arrow_down:
pkg/kms/localkms/privkey_import.go	82.37% <79.06%> (-0.84%)	:arrow_down:
pkg/wallet/kmsclient.go	93.83% <89.65%> (-0.20%)	:arrow_down:
pkg/doc/util/jwkkid/kid_creator.go	97.22% <100.00%> (ø)
pkg/wallet/contents.go	98.95% <100.00%> (ø)

---

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 dbf49cb...2f6d860. Read the comment docs.

[jceb]

@Baha-sk the pull request turned out bigger than expected. Please have a look whether the changes are okay.

I'll continue to make the remainder of the tests pass.

[jceb]

@Baha-sk one more thing - add content (https://github.com/identinet/aries-framework-go/blob/fix-vcwallet-correct-issue-vc/pkg/wallet/wallet.go#L407) doesn't return the content id yet. So if a key gets added, the developer/user has to manually compute the key id from the public key to find out where the key's stored. This is inconvenient and I'd prefer as a developer to just receive back the id of the key in particular and everything that I add in general, to have an easy way of tracking what I added to my wallet.

Non-key material can easily be queried but stored keys can't be queried. So for an application to keep track of the keys that the user added, it's important to receive back the id.

What do you think, shall we make this change to the Add function signature in this PR as well?

[baha-ai]

@Baha-sk one more thing - add content (https://github.com/identinet/aries-framework-go/blob/fix-vcwallet-correct-issue-vc/pkg/wallet/wallet.go#L407) doesn't return the content id yet. So if a key gets added, the developer/user has to manually compute the key id from the public key to find out where the key's stored. This is inconvenient and I'd prefer as a developer to just receive back the id of the key in particular and everything that I add in general, to have an easy way of tracking what I added to my wallet.

Non-key material can easily be queried but stored keys can't be queried. So for an application to keep track of the keys that the user added, it's important to receive back the id.

What do you think, shall we make this change to the Add function signature in this PR as well?

cc @sudeshrshetty, the wallet implementer. I believe Add() should return the kid as string as well. I don't see the Universal Wallet API imposing specific returns for the Add function so it should be able to return kid. @sudeshrshetty thoughts?

[baha-ai]

@jceb the pr looks good to me, regardless of the above discussion about returning the kid when adding a key to the wallet.

cc @troyronda @fqutishat @sudeshrshetty

[sudeshrshetty]

@jceb @Baha-sk would love to get on a call to solve this issue quickly.

[jceb]

Let me know when it suits you. I'm in the CET timezone. You can send me an invite to [email protected]

[sudeshrshetty]

Let me know when it suits you. I'm in the CET timezone. You can send me an invite to [email protected]

thanks for the time @Baha-sk @jceb

here are the outcomes of our discussion:

• in case of signing, if KID is provided by client then key will be fetched from KMS using that key ID and verification method splitting logic will be skipped .
• verification method splitting logic will have fallback in case of KEY NOT FOUND and wallet will try to fetch key again using fingerprint as you mentioned.
• vcwallet.Add(key) to accept custom key ID or compute one if missing (need update in universal wallet spec).

[jceb]

Hi @sudeshrshetty

here are the outcomes of our discussion:

* in case of signing, if `KID` is provided by client then key will be fetched from KMS using that key ID and verification method splitting logic will be skipped .

👍 has been implemented.

* verification method splitting logic will have fallback in case of KEY NOT FOUND and wallet will try to fetch key again using fingerprint as you mentioned.

I'm not sure about the refetch logic. At the moment the key ID that's provided or if none is provided, it's computed, is used.

* `vcwallet.Add(key)` to accept custom key ID or compute one if missing (need update in universal wallet spec).

From analyzing the code I learned that the Universal Wallet spec lets us add a key. In the content of the key there's an ID field. With this in place, I don't see the need to extend the specification because we can use that field. The current implementation either stores the key under the provided ID or, if ID is an empty string, the key ID is computed.

Is there anything else that you'd like me to change in the PR?

[jceb]

@sudeshrshetty @Baha-sk happy new year to you! Is there anything else I can do to get the PR merged?

[jceb]

@Baha-sk @sudeshrshetty I updated the patch to the version in main and extracted #3222 as it's not directly related. How could we get the patch in shape to be merged into main?

[fastman61]

@jceb, @Baha-sk , @sudeshrshetty

How can I solve this issue? Will the PR be merged? Is there a plan?

When I merged this PR with solving merge conflicts to my local main branch, It gives the error below. What can I do to solve this issue using BbsBlsSignature2020 signature?

{
    "code": 12010,
    "message": "failed to issue credential: getKeySet: failed to read json keyset from reader: keyset.Handle: decryption failed: cipher: message authentication failed"
}

/vcwallet/issue
{
    "auth": "{{wallet_token}}",
    "credential": {
        "@context": [
            "https://www.w3.org/2018/credentials/v1",
            "https://w3id.org/citizenship/v1"
        ],
        "type": [
            "VerifiableCredential",
            "PermanentResident"
        ],
        "id": "https://credential.example.com/residents/1234567890",
        "issuer": "did:key:zUC74hLbUrGUFKyVb3ehRU3rvpeb7HVYrCEQsbki5B18r5K4X2iKxBgygjbLstfLa7ZNLoQrsWxUcNJpjSBD5fDbXgyrm9opUPieU6dP98HGg1uKhq4yUNCq8gp4a363BzVUEcx",
        "issuanceDate": "2020-01-01T12:00:00Z",
        "credentialSubject": {
            "type": [
                "PermanentResident"
            ],
            "id": "did:peer:1zQmSiUedvbpDzi245g1wWXPz97kaBbRS6BBALW9DnP5Nk7Z",
            "givenName": "ALICE",
            "familyName": "SMITH",
            "gender": "Female",
            "birthCountry": "Bahamas",
            "birthDate": "1958-07-17"
        }
    },
    "proofOptions": {
        "controller": "did:key:zUC74hLbUrGUFKyVb3ehRU3rvpeb7HVYrCEQsbki5B18r5K4X2iKxBgygjbLstfLa7ZNLoQrsWxUcNJpjSBD5fDbXgyrm9opUPieU6dP98HGg1uKhq4yUNCq8gp4a363BzVUEcx",
        "proofRepresentation": 0,
        "proofType": "BbsBlsSignature2020"
    },
    "userID": "{{wallet_user_id}}"
}

[jceb]

Not sure, I haven't tried it but BLS signatures yet. Not sure either why this PR isn't going forward.