feat: remove did-resolver and did-jwt dependencies #82
Conversation
auer-martin
changed the title
Signed-off-by: Martin Auer <martin.auer97@gmail.com>
There was a problem hiding this comment.
I like the general direction and implementation of the PR. 🥇
A few notes though:
-
When working on this large of a PR next time probably it is best to have a small chat/discussion up front. To ensure direction is alligned, but also to ensure people are not by accident doing similar work
-
I think we should create another module with the DID and well-known DIDs, as the library now only shows how to implement it using tests. This means we lost Microsoft Entra Verified ID compatibility, universal resolution and DIDs in general for parties that are not using a framework
-
I see some expressions without accolades. We always require them to prevent any future potential bugs
-
I think we lost the support for OAuth2 style payloads only, or to convert these into OID request object style. Although most examples in the specs are using request objects there are cases where the spec also mentions to not use signed JWTs. I have seen some examples where an async jwt() method is replaced by the _jwt property, and also some cases where a authz payload was converted to the request object have been removed on first glance
src/id-token/Payload.ts
Outdated
| @@ -50,34 +37,11 @@ export const createIDTokenPayload = async ( | |||
| aud: responseOpts.audience || payload.client_id, | |||
| iat: Math.round(Date.now() / SEC_IN_MS - 60 * SEC_IN_MS), | |||
| exp: Math.round(Date.now() / SEC_IN_MS + (responseOpts.expiresIn || 600)), | |||
| sub: responseOpts.signature.did, | |||
| sub: undefined, | |||
There was a problem hiding this comment.
Why are we including it as undefined?
There was a problem hiding this comment.
So when you build an idtoken with the fromVerifiedAuthorizationRequest you dont' have the jwtIssuer available.
Thus you don't know the subject yet. I set this when calling the jwt() method on the idToken
There was a problem hiding this comment.
Yes, but why is the sub key/prop being included in that case to begin with?
|
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? |
|
Hello @nklomp, thanks for the feedback 👍🏻 , and apologies on my part for not opening an issue in advance. I will do so next time. |
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 :) |
Yeah our bad, we'll discuss it upfront next time --
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. |
|
Hey @nklomp is there anything else that needs to happen before we can merge this? |
There was a problem hiding this comment.
A few small comments left.
BTW we prefer to have the reviewer resolve the raised issues. Reason is that it ensures the reviewer actually has seen the change implemented and is okay with it. Otherwise it is easy to overlook and/or harder to find whether it was resolved appropriately
src/id-token/Payload.ts
Outdated
| @@ -50,34 +37,11 @@ export const createIDTokenPayload = async ( | |||
| aud: responseOpts.audience || payload.client_id, | |||
| iat: Math.round(Date.now() / SEC_IN_MS - 60 * SEC_IN_MS), | |||
| exp: Math.round(Date.now() / SEC_IN_MS + (responseOpts.expiresIn || 600)), | |||
| sub: responseOpts.signature.did, | |||
| sub: undefined, | |||
There was a problem hiding this comment.
Yes, but why is the sub key/prop being included in that case to begin with?
|
Okay, there are still some test failures. I guess some are related to ehtr:goerly not being around anymore, similar for ebsi testnetwork not being around anymore |
|
Do you want us to go ahead with moving the siop-oid4vp repo to the OID4VCI monorepo as well? I think we can change the package name at the same time (so it's not did-auth-siop anymore). Or do you want to wait with that? |
|
Sure. Suggest to call it siop-oid4vp for now though, as with the next refactor extracting SIOPv2 and OID4VP into respective modules is highly likely |
|
To make it not too hard for people to have to adjust their imports all over the place, we could opt for leaving the name as is for now. |
Sure, then we can rename when we separate the modules 👍 |
|
I created a PR to move this repo over to the OID4VC monorepo. Sphereon-Opensource/OID4VC#125 |
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.