aries-cloudagent-python icon indicating copy to clipboard operation
aries-cloudagent-python copied to clipboard

Fix verkey validation to support did:key with BBS+

Open ff137 opened this issue 2 years ago • 5 comments

Resolves #2437

This PR essentially updates the model schema definitions (used in the Swagger/OpenAPI spec) to support verkeys generated with did:key + BBS signature type. At present, the OpenAPI schema models only support ED25519 verkeys, and will raise validation errors when request/responses contain a verkey that is longer than expected.

In summary:

  • created a class DidKeyBbsRawPublicKey to define the regex pattern for did:key + BBS verkeys:
    PATTERN = rf"^[{B58}]{{131,132}}$"
  • created a class IndyOrKeyBbsPublicKey to define a validation pattern that accepts either an ED25519 or a BBS verkey
  • replaced all cases where validation uses INDY_RAW_PUBLIC_KEY_VALIDATE to rather use the "indy or key bbs" pattern.

The affected models are as follows:

  • ConnRecord - invitation_key field can now be a BBS verkey. Likewise for:
  • ConnectionTarget - recipient_keys, routing_keys, and sender_key
  • verkey fields in RegisterLedgerNymQueryString and GetDIDVerkeyResponse
  • signer in SignatureDecorator
  • recipient_keys and routing_keys in ServiceDecorator, in ConnectionInvitation, and in CreateInvitationRequest
  • my_verkey and their_verkey in ConnectionStaticResult
  • invitation_key in ConnectionsListQueryString
  • lastly: verkey in DIDSchema and DIDListQueryStringSchema

To reiterate: all of the above fields were previously specified to use the indy verkey validation. This is now updated so that did-key/BBS verkeys can also pass validation.

There are no changes to the internal processing of did:key / BBS verkey support for methods that use these schemas; this PR simply updates the schema's validation patterns so that BBS verkeys can be passed in request/response bodies.

If there are any schema fields above that specifically require validating for only ED25519 verkeys, do let me know! Otherwise, this change will help client libraries that are generated from the OpenAPI spec (like aries-cloudcontroller) to use did:key with BBS signature type.

ff137 avatar Sep 06 '23 07:09 ff137

Great stuff — thanks! @andrewwhitehead , can you please review?

What is the BBS+ dependency being used? AFAIK — we are/were using Ursa, which in turn was using Mattr’s BBS+ implementation. The Ursa BBS+ support was moved to aries-bbssignatures-rs. However, the transition to that repo is still in progress such that the ACA-Py dependency is still from the (archived) Ursa project.

A further practical pain was that the BBS+ support in Mattr does not publish an Arm/M1 compatible artifact, so we can’t produce a “pure” M1 friendly ACA-Py artifact. Does this change help with that?

swcurran avatar Sep 06 '23 13:09 swcurran

@ff137 For many of the updated schemas, Ed25519 keys are strictly expected because the DIDComm pack and unpack operations only support Ed25519/X25519 keys.

  • ConnRecord - invitation_key is a key used to pack/unpack initial connection messages
  • ConnectionTarget - recipient_keys, routing_keys, and sender_key - each of these is also a key to be used as part pack/unpack
  • verkey fields in RegisterLedgerNymQueryString and GetDIDVerkeyResponse - These are referencing Indy ledger specific data structures; the verkey associated with nyms are always Ed25519 keys
  • signer in SignatureDecorator - This one could probably be more flexible. The signature decorator is essentially deprecated though and the only defined signature type supported in the RFC is ed25519Sha512_single.
  • recipient_keys and routing_keys in ServiceDecorator, in ConnectionInvitation, and in CreateInvitationRequest - these are pack/unpack keys again
  • my_verkey and their_verkey in ConnectionStaticResult - Same; pack/unpack keys
  • invitation_key in ConnectionsListQueryString - Same as invitation_key above
  • verkey in DIDSchema and DIDListQueryStringSchema - This one is probably okay but there are deeper problems in how ACA-Py uses the DID objects since it always expects it to be an Ed25519 key, base58 encoded.

dbluhm avatar Sep 06 '23 17:09 dbluhm

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

sonarqubecloud[bot] avatar Sep 07 '23 06:09 sonarqubecloud[bot]

@dbluhm Thank you! That overview is very helpful.

So, in summary, it'll be best for all of the models to stick with validating for an Ed25519 key? Except for maybe DIDSchema and DIDListQueryStringSchema.

Initially it was only DIDSchema that I wanted to update, because that's the response schema for the create-did route. Using did:key + BBS, the response doesn't pass validation / yields 422 unprocessable entity (when using OpenAPI clients)

Naturally, if we can create it, then we'd want to be able to use the BBS+ verkey everywhere, so I figured update all the models, and get feedback on what needs to stay with Ed25519. But, it looks like everything does!

So, I'm wondering, how big of an undertaking is it for me to try contribute BBS+ support for some of the methods associated with these schemas? I take it that it mainly comes down to updating the DIDComm pack/unpack operation. How complex is that? And is it already on the roadmap / a goal in the short term? If it's only a long term goal / somebody not assigned to it, then I'd be willing to take a look and help to expedite it.

A simple compromise for now would be to only fix the schema for the create-did response, and then add BBS verkey validation for the other models once their methods have support for it.

ff137 avatar Sep 07 '23 07:09 ff137

As a quick but somewhat incomplete response:

The pack/unpack crypto is pretty rigidly defined at this point. I think it's unlikely that it would be worth the effort to change it for DIDComm v1. DIDComm v2 I think is a bit more flexible but I haven't looked too deeply at the crypto in use there.

dbluhm avatar Sep 07 '23 16:09 dbluhm

I think this concept would need to be revisited at some point in the future but from the perspective of DIDComm v2 support for different crypto suites. I think this PR is probably DOA until there is further discussions. Thanks for your contributions!

dbluhm avatar Mar 19 '24 15:03 dbluhm