Sphereon-Opensource / SIOP-OID4VP

Self Issued OpenID Provider v2 (SIOP) with optional OpenID for Verifiable Presentations (OpenID4VP)
Apache License 2.0
77 stars 25 forks source link

feat: remove did-resolver and did-jwt dependencies #82

Open auer-martin opened 1 week ago

auer-martin commented 1 week ago

This PR mainly removes the strong dependencies to the did-resolver and did-jwt dependencies and allows consumers to provide signing and verification mechanisms. This was discussed before in #55.

Some notable changes:

The Signing and Verification Capabilities now have to be provided externally. Thus, internal checks, such as verifying linked domains, were removed and must now be done as part of the pluggable verification process.

The different signature types INTERNAL | SUPPLIED | ... were removed. The behavior can be mimicked by implementing the signing callback accordingly.

The Verification Method types INTERNAL | EXTERNAL were removed. Users now must provide their own jwt verification implementation. The INTERNAL and EXTERNAL functionality again can be achieved by implementing the verification callback accordingly.

This PR provides verification and signing 'adapters' for x5c, jwk, and did protected jwts (x5c, and jwk functionality was not present/possible previously) to make the user-provided signing and verification process as easy as possible. For example, when creating an id-token that is jwk protected, you can pass a jwtIssuer with method jwk, and the jwt header and payload are prepared so that the consumer of the library should not need to modify the jwt header/payload content manually. For verification, the JwtVerifier provides properties to streamline the verification process.

With these changes, I believe this PR effectively addresses the issues raised in #67 and the discussions in #55.

TimoGlastra commented 5 days ago

Hey @nklomp, thanks for the feedback. I discussed with Martin all the changes, and we discussed briefly about opening an issue, but as it was already partly discussed in the other open issues we in the end didn't.

I feel like we've discussed in the past quite often about the general purpose of these libraries, which is to provide an unopiniated foundation of the specification. To achieve that we need to be flexible, and so it made sense to us to remove the did specific logic. We looked at OID4VCI as an example, which doesn't provide did resolving logic as well.

Nowhere in the OID4VP or SIOPv2 specification I see anything mentioned about DID Linked Domains. Would you prefer this is exctracted to a separate library in ssi-sdk? Or maybe we can in the JWT verification callback, optionally allow a did document to be returned like is done in OID4VCI, then we can do the verification there?

auer-martin commented 5 days ago

Hello @nklomp, thanks for the feedback 👍🏻 , and apologies on my part for not opening an issue in advance. I will do so next time. I will look into your feedback today and wait for a decision on how to proceed with the DID related functionality.

nklomp commented 5 days ago

Hey @nklomp, thanks for the feedback. I discussed with Martin all the changes, and we discussed briefly about opening an issue, but as it was already partly discussed in the other open issues we in the end didn't.

I feel like we've discussed in the past quite often about the general purpose of these libraries, which is to provide an unopiniated foundation of the specification. To achieve that we need to be flexible, and so it made sense to us to remove the did specific logic. We looked at OID4VCI as an example, which doesn't provide did resolving logic as well.

Nowhere in the OID4VP or SIOPv2 specification I see anything mentioned about DID Linked Domains. Would you prefer this is exctracted to a separate library in ssi-sdk? Or maybe we can in the JWT verification callback, optionally allow a did document to be returned like is done in OID4VCI, then we can do the verification there?

Yeah, point is more that this PR removed existing functionality. I rather would have seen that discussed upfont, so we could have discussed how to go about it.

As I already mentioned to you, what I want to do soon is to basically merge the current SIOP/OID4VP lib and the OID4VCI lib into a mono repo. Then to also split up SIOP and OID4VP more into separate modules and move a lot of the old version support. I also already want to make an abstraction for query language support for VP, so that we can plugin PEx and any future spec for it.

For now the easiest approach is to convert this repo already into a mono-repo. Then to add a module that supports DIDs including the universal resolver/registrar lib as that only depends on cross-fetch. Ideally well-known DIDs would be a separate module, but I would be okay with having it in the DID module for a first version.

Well-known DIDs are part of the JWT VC Presentation Interop profile which the lib supports. That DIF profile is also supported by different larger tech vendors, including Microsoft Entra Verified ID.

To conclude. I am happy with the approach as mentioned and this indeed is the route forward. But next time I would like to see some discussion upfront in case we will have to (partially) drop certain features present, so we can come up with a plan and decide who will do what at which specific time :)

TimoGlastra commented 5 days ago

Yeah, point is more that this PR removed existing functionality. I rather would have seen that discussed upfont, so we could have discussed how to go about it.

Yeah our bad, we'll discuss it upfront next time -- The burden is on us to fix it now 😉

For now the easiest approach is to convert this repo already into a mono-repo. Then to add a module that supports DIDs including the universal resolver/registrar lib as that only depends on cross-fetch. Ideally well-known DIDs would be a separate module, but I would be okay with having it in the DID module for a first version.

There's quite some work and overhead involved in moving a repository to a monorepo, including the release pipeline etc.. I think my preference would be to move this siop-oid4vp and the new did module already to the oid4vci monorepo for example and then later we can split it up. I think there's not too many PRs open now, so it should be quite straightforward. And by first moving this repo as is, means we have the code in one repo, without the overhead of separating this repo into seaprate modules at the same time (we can start doing that work over time). Are you okay with that approach?

In that case, it'd be good to:

This way the effort spent will be on the desired solution (one monorepo) instead of spending effort to transform this into a monorepo.

auer-martin commented 10 hours ago

Hey @nklomp is there anything else that needs to happen before we can merge this?