refactor: make ldp_vc logic reusable

[dbluhm]

This PR is a refactor of LDP VC handling within ICv2 and PPv2 to make it usable outside of those protocol's handlers. I believe this results in a cleaner implementation, separating the concerns of preparing a credential from the ICv2 protocol and its messages and handlers for those messages.

This refactor enables the other major addition in this PR, adding a couple of new endpoints for LDP VC issue and verify, POST /vc/ldp/issue and POST /vc/ldp/verify. These endpoints are similar to the /jsonld/sign and /jsonld/verify endpoints. However, the big difference is that the /jsonld endpoints did not use the same LDP signing methods used in ICv2 or PPv2. This has caused the two implementations to diverge more and more over time. With this PR, these endpoints are still available but are marked as deprecated. One other significant difference, the inputs for endpoints have changed to be more specific to signing and verifying LDP VCs and not just arbitrary JSON-LD documents.

Fixes #2468.

[dbluhm]

Some additional testing and fixes pending for this PR. I wanted to open this to see if there were any thoughts on this question:

Should I also move the holder related functions of storing LDP VCs to this interface?

[swcurran]

Assuming that we will soon be supporting signing and verifying AnonCreds in W3C format proofs, is there anything else that needs to be done in preparation for that? Will we be able to use the same interface to sign such credentials? And ideally, sign a VC twice — once with AnonCreds, once with another key type? AFAIK, the AnonCreds signature will be aligned with Linked Data Proofs.

Another thought. I believe the “LDP” term has been replaced with Data Integrity Proof…

I’ll edit this message — have to run right now...

[dbluhm]

Good questions. I really enjoy how nice and simple everything is when it comes to VC formats 😄

Based on my understanding, AnonCreds in W3C format would be formatted as JSON-LD but would not be using Linked Data Proof signatures since they will of course use CL Signatures. So it would technically not fall in the category of ldp_vc. And to your point on Data Integrity Proofs replacing Linked Data Proofs, I believe you're correct. Based on casual observation, it seems the term ldp_vc will be sticking around for a while at least, given that the format is included in the OpenID4VCI spec as "VC secured using Data Integrity, using JSON-LD, with proof suite requiring Linked Data canonicalization" (which is hilariously verbose). That section clarifies:

Note that Data Integrity used to be called Linked Data Proofs, hence "ldp" in the Credential format identifier

I haven't looked deeply at Data Integrity Proofs but I believe there would be changes required at layers lower than the ones I've touched in this PR. Superficially, it seems the biggest change would be how the signature is represented.

I think the interface I've added here would be beneficial to later adding DIP support, switching behavior by processing the proof_type option. Perhaps that means the manager itself should be renamed 🤔

I don't think this extends to supporting AnonCreds in W3C format, though. AnonCreds would need a different set of options, at a minimum, which I think makes it not fit particularly well into this interface. I could be persuaded otherwise.

[dbluhm]

Oh, I just read more of https://w3c.github.io/vc-data-integrity/; it seems that AnonCreds would actually be able to be considered a generic Data Integrity Proof so I think I'm wrong about what I just said.

[andrewwhitehead]

Looks like a nice clean-up to me!

[dbluhm]

Brief update: I'm working through the test failures and moving tests that used to apply to the ICv2 and PPv2 format handlers for ld_proof and dif respectively to tests for the added manager!

[dbluhm]

This is now ready for review! @swcurran I think this is a good change for JSON-LD creds but there's probably a conversation worth having about how this should interact with the DataIntegrityProofs in the future. This is a good stepping stone in that direction; it will be easier to make that switch when it's time.

[swcurran]

@PatStLouis — this might be relevant to the work you are doing — being able to use the JSON-LD credentials independent of DIDComm protocols. I think you mentioned that yesterday? Please let me know. Your review of this PR would also be welcome! :-)

[PatStLouis]

@swcurran thank you for pointing this out to me. @dbluhm I will have a thorough look tomorrow but this seems like a great functionality to have. I'd like to have a discussion with you around the differences you highlighted between these new endpoints and the json-ld endpoints. I understand this will replace the existing json-ld endpoints?

2 comments/question I have

1. Have you considered using the endpoints /credentials/issue and /credentials/verify to align with the vc-api credential management spec? This would enable aca-py to be interoperable with w3c test-suites out of the box. They also have different endpoints for presentations, /presentations/sign and /presentations/verify which could be interesting to consider. I'm currently writing a PR on the vc-api spec around how error messages should be returned, I'd like to know your approach on this as well! I will base this on the VC data model spec problem details section.
   https://w3c-ccg.github.io/vc-api/#issue-credential
   https://pr-preview.s3.amazonaws.com/w3c/vc-data-model/pull/1338.html#problem-details

2. What are your thought's on credential storage? Currently the only way to store a json-ld credential is through the issue-credential v2 protocol. There is however some logic to query those credentials through the /credentials/w3c endpoint. Do you see adding another mechanism to store an already issued credential?

And you are correct, CL signatures are considered data integrity proofs. However, AnonCreds VC type will be difficult to be issued with an endpoint like this because the process for the credential request requires a back and forth between the issuer and the holder. There would need to be some reflections about how to provide the request to this endpoint and how it will look. Super interested in helping with this however.

[PatStLouis]

Pulling in @wip-abramson since we recently had discussions around this for a project we are working on

[dbluhm]

Hey @PatStLouis, thanks for your interest! I'm not necessarily opposed to implementing the VC-API; the interface is inspired by the VC-API, as was the LD-Proof-VC-Detail attachment format that this work kind of inherits from. There are subtle differences right now and it's been a minute since I did the bulk of this work. I'll take some time to compare and see if there's a clear reason for the differences.

[PatStLouis]

One of the key differences I noticed with the json-ld endpoints was that you needed to provide the verkey associated with the wallet you wanted to sign the credential with at a level higher in the request body. The options field in the vc-api could be used for optionally providing a verkey, and the agent would have one set as the default. The vc-api is designed to be implementation agnostic and I feel this would be a reasonable use of the field.

The vc-api spec could also be included in the aca-py backchannel to enable running w3c test suites this way instead, however having these routes in the aca-py spec itself could also be interesting while making use of the already existing /credentials section. For example, the vc data model 2.0 test suites interfaces with the /credentials/issue endpoint as defined in the vc-api. When aca-py will want to enable the data model 2.0 as a vc issuance format, this could be demonstrated by directly exposing a demo instance's admin api to the test-suite instead of having a backchannel layer in between.
https://github.com/w3c/vc-data-model-2.0-test-suite#setup

Other noteworthy test suites includes:
https://github.com/w3c-ccg/did-key-test-suite
https://github.com/w3c-ccg/status-list-2021-test-suite

It could also open the door for integration with the vcplaygroud

Overall great for interoperability.

These are just considerations I wanted to bring to the decision making, I think the work done here is fantastic regardless and much needed.

[dbluhm]

Disclaimer that I haven't yet taken that closer look yet lol but it sounds like simply moving the added endpoints from /vc/ldp to /credentials might be enough?
I have mixed feelings about the implications of endpoints named that way, though, especially since, as you noted, they wouldn't be able to issue AnonCreds VCs. Can we put the endpoints under /vc/ldp/credentials and then provide a base url including /vc/ldp to test suites?

[PatStLouis]

Yes that would also work, if the endpoints are conformant with the expected request body / response body. You can have a look here at what the configuration files for these test suites look like.

I have a vc-api implementation of the anoncreds-rs python wrapper where I do issue an anoncreds vc with this endpoint. It would take into account that you already received the credential request from the holder and provide it in the options field....not ideal but it works. For demonstration you can also simply use the endpoint as is and have the issuer create the request himself (thus binding the credential to a link secret he manages). However this is highly unpractical...it would however demonstrate interoperability...

Once the anoncreds in w3c work is completed for aca-py, we should have a lot more clarity about what is possible here. Are you looking to merge this PR soon or are you planning to leave it open for some time?

[PatStLouis]

Have a look here if you want an easy to run test-suite that would interact the same way. Just plug in the endpoints and see what happens: https://github.com/credential-handler/verifiable-credential-playground-test-suite

[wip-abramson]

I agree with @PatStLouis.

I think we need an endpoint to store an issued JSON-LD VC. Or is it imagined that applications would handle storage of credentials themselves?

I also think supporting AnonCreds issuance in this way will be hard. AnonCreds issuance requires interaction between issuer and holder passing cryptographic information that needs to be used in protocols other that json-ld sign. To implement this outside of DIDComm, I think we would need API calls to represent each of these interactions (leaving the communication out of band - potentially CHAPI or something else).

Really, AnonCreds jsonld issuance isn't JSON-LD sign at all (I don't think). Isn't it just JSONLD pack? Or W3C VC pack? Some transformation that takes a signed AnonCred and represents it in W3C VC format.

[PatStLouis]

@wip-abramson You had some interesting points relating to didauth and credential storage, would you mind sharing these as well? This should be documented and discussed (its out of scope for this PR however discussing it doesn't hurt)

[wip-abramson]

I'm not sure what I said more about credential storage.

But DIDAuth, is generally recommended when issuing a credential to a DID. First the issuer must request a DIDAuth from the holder for the DID they wish to include in the credential. See https://chapi.io/developers/wallets/#did-authentication-with-chapi

I actually think this would be possible with the endpoints proposed here. You would just JSONLD sign something like this:

const didAuthPresentation = {
    '@context': [
        'https://www.w3.org/2018/credentials/v1',
        'https://w3id.org/security/suites/ed25519-2020/v1'
    ],
    type: ['VerifiablePresentation'],
    holder: 'did:key:z6MkeprvBw4RFHJPQEmtioq4xRrN6Tk8EBSJ37eBCBQNHRjZ',
};

[PatStLouis]

Bold of you to post javascript snippets in an aca-py thread. Thanks for the input!

[dbluhm]

Are you looking to merge this PR soon or are you planning to leave it open for some time?

I think it will be better to get it merged sooner than later; even as is, I think it's a significant improvement over what was previously supported with the /jsonld/sign and /jsonld/verify endpoints (critically, resolving the split in the implementations).

Still reading deeper on your other points!

[swcurran]

@PatStLouis — any feedback on this? I’d like to get this merged. Thanks!

[PatStLouis]

@swcurran testing now

[PatStLouis]

This looks great and works really well. I would have 2 comments:

Would it make sense to make the proofType optional? AFAIK in acapy the key type is selected upon wallet creation, so by providing a did:key wouldn't it be already bound to a specific proof type? If I'm wrong please enlighten me!

The created timestamp could be formatted to use the 'Z' alternative to specify UTC. Also according to the XMLSCHEMA11-2 date-time specification the milliseconds should be omitted.

2023-12-07T17:58:51.962131+00:00 -> 2023-12-07T17:58:51Z

Question, when you verify a vp, do you also verify the vc signature it holds?

I like the included details upon verification, great context and useful.

Overall I like this alot in comparison to the json-ld endpoints

[swcurran]

@dbluhm — let me know how you want to handle @PatStLouis ’s comments — include or merge as is.

Thanks @PatStLouis — good stuff!

[PatStLouis]

@dbluhm you should come present this at the vc-api meeting next week, I know a few people who will be thrilled to see this alignment :)

[dbluhm]

Would it make sense to make the proofType optional? AFAIK in acapy the key type is selected upon wallet creation, so by providing a did:key wouldn't it be already bound to a specific proof type? If I'm wrong please enlighten me!

There might be a better way to address this but the proof type is currently required because the same keys associated with a DID can create more than one signature type, e.g. Ed25519Signature2018 or Ed25519Signature2020. Kind of dumb that that split exists in the first place; I think this will be cleaner with Data Integrity Proofs.

The created timestamp could be formatted to use the 'Z' alternative to specify UTC. Also according to the XMLSCHEMA11-2 date-time specification the milliseconds should be omitted.

2023-12-07T17:58:51.962131+00:00 -> 2023-12-07T17:58:51Z

Good catch, I'll get that fixed!

Question, when you verify a vp, do you also verify the vc signature it holds?

Yep, this is handled a layer down from these changes and can be found here: https://github.com/dbluhm/aries-cloudagent-python/blob/37c4bc27d3c38b9843d696213d096b3fd4762dc2/aries_cloudagent/vc/vc_ld/verify.py#L142

I like the included details upon verification, great context and useful.

Overall I like this alot in comparison to the json-ld endpoints

👍 Glad to hear it!

[dbluhm]

@PatStLouis Was it the proof created at timestamp specifically that you were referring to?

[dbluhm]

@PatStLouis I believe I located the bad timestamp formatting; if you are willing, I wouldn't mind another look from you to validate it is as expected!

Update: The change has actually caused some test cases (that have been around since before my changes on this PR) to fail. I'm either missing the mark or there might be test cases that need to be updated. See: https://github.com/hyperledger/aries-cloudagent-python/actions/runs/7132863110/job/19424316476?pr=2533

I'll revert my change for now.

[PatStLouis]

Sounds good, I was able to sign most of the credentials from this list

Thumbs up from me!

[swcurran]

Approved based on evaluation of @PatStLouis . I’ll leave it to you @dbluhm to decide when to merge.

Awesome work developing and reviewing — thanks!

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information