-
Notifications
You must be signed in to change notification settings - Fork 200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: dpop support #1966
feat: dpop support #1966
Changes from 9 commits
a3fe108
88e2ee5
8765c0b
cb3e749
c49466b
c599fa9
7a656c8
f250d57
7b13e97
e55bb2f
004eb51
bc92691
7273ca1
f2d73af
33ed706
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,80 +1,89 @@ | ||||||
import type { | ||||||
OpenId4VciAcceptCredentialOfferOptions, | ||||||
OpenId4VciAuthCodeFlowOptions, | ||||||
OpenId4VciProofOfPossessionRequirements, | ||||||
OpenId4VciCredentialBindingResolver, | ||||||
OpenId4VciResolvedCredentialOffer, | ||||||
OpenId4VciCredentialResponse, | ||||||
OpenId4VciNotificationEvent, | ||||||
OpenId4VciProofOfPossessionRequirements, | ||||||
OpenId4VciResolvedAuthorizationRequest, | ||||||
OpenId4VciResolvedAuthorizationRequestWithCode, | ||||||
OpenId4VciResolvedCredentialOffer, | ||||||
OpenId4VciSupportedCredentialFormats, | ||||||
OpenId4VciCredentialResponse, | ||||||
OpenId4VciNotificationEvent, | ||||||
OpenId4VciAcceptCredentialOfferOptions, | ||||||
OpenId4VciTokenRequestOptions, | ||||||
} from './OpenId4VciHolderServiceOptions' | ||||||
import type { | ||||||
OpenId4VciCredentialConfigurationsSupported, | ||||||
OpenId4VciCredentialConfigurationSupported, | ||||||
OpenId4VciCredentialConfigurationsSupported, | ||||||
OpenId4VciCredentialSupported, | ||||||
OpenId4VciIssuerMetadata, | ||||||
} from '../shared' | ||||||
import type { AgentContext, JwaSignatureAlgorithm, Key, JwkJson } from '@credo-ts/core' | ||||||
import type { AgentContext, JwaSignatureAlgorithm, JwkJson, Key } from '@credo-ts/core' | ||||||
import type { | ||||||
AccessTokenResponse, | ||||||
CredentialResponse, | ||||||
Jwt, | ||||||
OpenIDResponse, | ||||||
AuthorizationDetails, | ||||||
AuthorizationDetailsJwtVcJson, | ||||||
CredentialIssuerMetadataV1_0_11, | ||||||
CredentialIssuerMetadataV1_0_13, | ||||||
AuthorizationDetailsJwtVcJsonLdAndLdpVc, | ||||||
AuthorizationDetailsSdJwtVc, | ||||||
CredentialIssuerMetadataV1_0_11, | ||||||
CredentialIssuerMetadataV1_0_13, | ||||||
CredentialResponse, | ||||||
Jwt, | ||||||
OpenIDResponse, | ||||||
} from '@sphereon/oid4vci-common' | ||||||
|
||||||
import { | ||||||
SdJwtVcApi, | ||||||
getJwkFromJson, | ||||||
DidsApi, | ||||||
CredoError, | ||||||
DidsApi, | ||||||
Hasher, | ||||||
InjectionSymbols, | ||||||
JsonEncoder, | ||||||
Jwk, | ||||||
JwsService, | ||||||
Logger, | ||||||
SdJwtVcApi, | ||||||
SignatureSuiteRegistry, | ||||||
TypedArrayEncoder, | ||||||
W3cCredentialService, | ||||||
W3cJsonLdVerifiableCredential, | ||||||
W3cJwtVerifiableCredential, | ||||||
getJwkClassFromJwaSignatureAlgorithm, | ||||||
getJwkFromJson, | ||||||
getJwkFromKey, | ||||||
getKeyFromVerificationMethod, | ||||||
getSupportedVerificationMethodTypesFromKeyType, | ||||||
inject, | ||||||
injectable, | ||||||
parseDid, | ||||||
} from '@credo-ts/core' | ||||||
import { CreateDPoPClientOpts, SigningAlgo } from '@sphereon/oid4vc-common' | ||||||
import { | ||||||
AccessTokenClient, | ||||||
CredentialRequestClientBuilder, | ||||||
ProofOfPossessionBuilder, | ||||||
OpenID4VCIClient, | ||||||
OpenID4VCIClientStateV1_0_13, | ||||||
OpenID4VCIClientV1_0_11, | ||||||
OpenID4VCIClientV1_0_13, | ||||||
OpenID4VCIClientStateV1_0_13, | ||||||
ProofOfPossessionBuilder, | ||||||
} from '@sphereon/oid4vci-client' | ||||||
import { CodeChallengeMethod, OpenId4VCIVersion, PARMode, post } from '@sphereon/oid4vci-common' | ||||||
import { | ||||||
CodeChallengeMethod, | ||||||
DPoPResponseParams, | ||||||
EndpointMetadataResult, | ||||||
OpenId4VCIVersion, | ||||||
PARMode, | ||||||
post, | ||||||
} from '@sphereon/oid4vci-common' | ||||||
|
||||||
import { OpenId4VciCredentialFormatProfile } from '../shared' | ||||||
import { | ||||||
getTypesFromCredentialSupported, | ||||||
getOfferedCredentials, | ||||||
credentialsSupportedV11ToV13, | ||||||
getOfferedCredentials, | ||||||
getTypesFromCredentialSupported, | ||||||
} from '../shared/issuerMetadataUtils' | ||||||
import { OpenId4VciCredentialSupportedWithId } from '../shared/models/index' | ||||||
import { getSupportedJwaSignatureAlgorithms, isCredentialOfferV1Draft13 } from '../shared/utils' | ||||||
import { getCreateJwtCallback, getSupportedJwaSignatureAlgorithms, isCredentialOfferV1Draft13 } from '../shared/utils' | ||||||
|
||||||
import { openId4VciSupportedCredentialFormats, OpenId4VciNotificationMetadata } from './OpenId4VciHolderServiceOptions' | ||||||
import { OpenId4VciNotificationMetadata, openId4VciSupportedCredentialFormats } from './OpenId4VciHolderServiceOptions' | ||||||
|
||||||
@injectable() | ||||||
export class OpenId4VciHolderService { | ||||||
|
@@ -266,14 +275,60 @@ | |||||
} | ||||||
} | ||||||
|
||||||
private async getCreateDPoPOptions( | ||||||
agentContext: AgentContext, | ||||||
metadata: Pick<EndpointMetadataResult, 'authorizationServerMetadata'> & { | ||||||
credentialIssuerMetadata: OpenId4VciIssuerMetadata | ||||||
} | ||||||
) { | ||||||
const dpopSigningAlgValuesSupported = | ||||||
metadata.authorizationServerMetadata?.dpop_signing_alg_values_supported ?? | ||||||
metadata.credentialIssuerMetadata.dpop_signing_alg_values_supported | ||||||
|
||||||
if (!dpopSigningAlgValuesSupported) return undefined | ||||||
|
||||||
const alg = dpopSigningAlgValuesSupported.find((alg) => getJwkClassFromJwaSignatureAlgorithm(alg)) | ||||||
|
||||||
const JwkClass = alg ? getJwkClassFromJwaSignatureAlgorithm(alg) : undefined | ||||||
|
||||||
if (!JwkClass) { | ||||||
throw new CredoError( | ||||||
`No supported dpop signature algorithms found in dpop_signing_alg_values_supported '${dpopSigningAlgValuesSupported.join( | ||||||
', ' | ||||||
)}'` | ||||||
) | ||||||
} | ||||||
|
||||||
const key = await agentContext.wallet.createKey({ keyType: JwkClass.keyType }) | ||||||
const jwk = getJwkFromKey(key) | ||||||
|
||||||
const createDPoPOpts: CreateDPoPClientOpts = { | ||||||
jwtIssuer: { alg: alg as unknown as SigningAlgo, jwk: jwk.toJson() }, | ||||||
dPoPSigningAlgValuesSupported: dpopSigningAlgValuesSupported, | ||||||
jwtPayloadProps: {}, | ||||||
createJwtCallback: getCreateJwtCallback(agentContext), | ||||||
} | ||||||
return createDPoPOpts | ||||||
} | ||||||
|
||||||
public async requestAccessToken(agentContext: AgentContext, options: OpenId4VciTokenRequestOptions) { | ||||||
const { resolvedCredentialOffer, txCode, resolvedAuthorizationRequest, code } = options | ||||||
const { metadata, credentialOfferRequestWithBaseUrl } = resolvedCredentialOffer | ||||||
|
||||||
// acquire the access token | ||||||
let accessTokenResponse: OpenIDResponse<AccessTokenResponse> | ||||||
let accessTokenResponse: OpenIDResponse<AccessTokenResponse, DPoPResponseParams> | ||||||
|
||||||
const accessTokenClient = new AccessTokenClient() | ||||||
|
||||||
const createDPoPOpts = await this.getCreateDPoPOptions(agentContext, metadata) | ||||||
|
||||||
let dpopJwk: Jwk | undefined | ||||||
if (createDPoPOpts) { | ||||||
if (!createDPoPOpts.jwtIssuer.jwk.kty) { | ||||||
throw new CredoError('Missing required key type (kty) in the jwk.') | ||||||
} | ||||||
Comment on lines
+330
to
+332
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this needed? In theory the jwk should have this property, so I'm not sure why we check for this specific property |
||||||
dpopJwk = getJwkFromJson(createDPoPOpts.jwtIssuer.jwk as JwkJson) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why first transform from JWK class to json and then back to jwk, shouldn't the createDpopOptions just return the instance? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm. I don't think so because we create the options which are passed to sphereons library and there we don't have the jwk class |
||||||
} | ||||||
if (resolvedAuthorizationRequest) { | ||||||
const { codeVerifier, redirectUri } = resolvedAuthorizationRequest | ||||||
accessTokenResponse = await accessTokenClient.acquireAccessToken({ | ||||||
|
@@ -283,12 +338,14 @@ | |||||
code, | ||||||
codeVerifier, | ||||||
redirectUri, | ||||||
createDPoPOpts, | ||||||
}) | ||||||
} else { | ||||||
accessTokenResponse = await accessTokenClient.acquireAccessToken({ | ||||||
metadata: metadata, | ||||||
credentialOffer: { credential_offer: credentialOfferRequestWithBaseUrl.credential_offer }, | ||||||
pin: txCode, | ||||||
createDPoPOpts, | ||||||
}) | ||||||
} | ||||||
|
||||||
|
@@ -300,7 +357,10 @@ | |||||
|
||||||
this.logger.debug('Requested OpenId4VCI Access Token.') | ||||||
|
||||||
return accessTokenResponse.successBody | ||||||
return { | ||||||
...accessTokenResponse.successBody, | ||||||
...(dpopJwk && { dpop: { dpopJwk: dpopJwk, dpopNonce: accessTokenResponse.params?.dpop?.dpopNonce } }), | ||||||
} | ||||||
} | ||||||
|
||||||
public async acceptCredentialOffer( | ||||||
|
@@ -311,6 +371,7 @@ | |||||
resolvedAuthorizationRequestWithCode?: OpenId4VciResolvedAuthorizationRequestWithCode | ||||||
accessToken?: string | ||||||
cNonce?: string | ||||||
dpop?: { dpopJwk: Jwk; dpopNonce?: string } | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just thinking: is it needed to prefix dpop if we already are in |
||||||
clientId?: string | ||||||
} | ||||||
) { | ||||||
|
@@ -324,7 +385,9 @@ | |||||
return [] | ||||||
} | ||||||
|
||||||
this.logger.info(`Accepting the following credential offers '${credentialsToRequest}'`) | ||||||
this.logger.info( | ||||||
`Accepting the following credential offers '${credentialsToRequest ? credentialsToRequest.join(', ') : 'all'}` | ||||||
) | ||||||
|
||||||
const supportedJwaSignatureAlgorithms = getSupportedJwaSignatureAlgorithms(agentContext) | ||||||
|
||||||
|
@@ -351,7 +414,11 @@ | |||||
} as OpenId4VciTokenRequestOptions | ||||||
|
||||||
const tokenResponse = options.accessToken | ||||||
? { access_token: options.accessToken, c_nonce: options.cNonce } | ||||||
? { | ||||||
access_token: options.accessToken, | ||||||
c_nonce: options.cNonce, | ||||||
...(options.dpop && { dpop: { dpopJwk: options.dpop.dpopJwk, dpopNonce: options.dpop?.dpopNonce } }), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this won't work?
Suggested change
|
||||||
} | ||||||
: await this.requestAccessToken(agentContext, tokenRequestOptions) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the service we can already change it (only api we need to worry about breaking changes) |
||||||
|
||||||
const receivedCredentials: Array<OpenId4VciCredentialResponse> = [] | ||||||
|
@@ -425,10 +492,24 @@ | |||||
.withToken(tokenResponse.access_token) | ||||||
|
||||||
const credentialRequestClient = credentialRequestBuilder.build() | ||||||
|
||||||
let createDpopOpts: CreateDPoPClientOpts | undefined | ||||||
if (tokenResponse.dpop) { | ||||||
const jwk = tokenResponse.dpop.dpopJwk | ||||||
const alg = jwk.supportedSignatureAlgorithms[0] | ||||||
|
||||||
createDpopOpts = { | ||||||
jwtIssuer: { alg: alg as unknown as SigningAlgo, jwk: jwk.toJson() }, | ||||||
jwtPayloadProps: { accessToken: tokenResponse.access_token, nonce: tokenResponse.dpop?.dpopNonce }, | ||||||
createJwtCallback: getCreateJwtCallback(agentContext), | ||||||
} | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not reuse Also, accessing index [0] is unsafe again There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also should we check if dpop is required? Can we see that using the access token? Or how do we know it is required? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if the tokenresponse contains dpop it is required There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But token response is passed by the user There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, IMO, the user should not modify the object unless he knows what he is doing. |
||||||
|
||||||
const credentialResponse = await credentialRequestClient.acquireCredentialsUsingProof({ | ||||||
proofInput: proofOfPossession, | ||||||
credentialTypes: getTypesFromCredentialSupported(offeredCredentialConfiguration), | ||||||
format: offeredCredentialConfiguration.format, | ||||||
createDPoPOpts: createDpopOpts, | ||||||
}) | ||||||
|
||||||
newCNonce = credentialResponse.successBody?.c_nonce | ||||||
|
@@ -625,7 +706,7 @@ | |||||
|
||||||
private async handleCredentialResponse( | ||||||
agentContext: AgentContext, | ||||||
credentialResponse: OpenIDResponse<CredentialResponse>, | ||||||
credentialResponse: OpenIDResponse<CredentialResponse, DPoPResponseParams>, | ||||||
options: { | ||||||
verifyCredentialStatus: boolean | ||||||
credentialIssuerMetadata: OpenId4VciIssuerMetadata | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,21 +63,22 @@ export class OpenId4VcIssuerApi { | |
} | ||
|
||
public async updateIssuerMetadata( | ||
options: Pick<OpenId4VcIssuerRecordProps, 'issuerId' | 'display'> & | ||
options: Pick<OpenId4VcIssuerRecordProps, 'issuerId' | 'display' | 'dpopSigningAlgValuesSupported'> & | ||
(OpenId4VcIssuerRecordCredentialSupportedProps | OpenId4VcIssuerRecordCredentialConfigurationsSupportedProps) | ||
) { | ||
const issuer = await this.openId4VcIssuerService.getIssuerByIssuerId(this.agentContext, options.issuerId) | ||
const { issuerId, credentialConfigurationsSupported, credentialsSupported, ...issuerOptions } = options | ||
|
||
if (options.credentialConfigurationsSupported) { | ||
issuer.credentialConfigurationsSupported = options.credentialConfigurationsSupported | ||
issuer.credentialsSupported = credentialsSupportedV13ToV11(options.credentialConfigurationsSupported) | ||
const issuer = await this.openId4VcIssuerService.getIssuerByIssuerId(this.agentContext, issuerId) | ||
|
||
if (credentialConfigurationsSupported) { | ||
issuer.credentialConfigurationsSupported = credentialConfigurationsSupported | ||
issuer.credentialsSupported = credentialsSupportedV13ToV11(credentialConfigurationsSupported) | ||
} else { | ||
issuer.credentialsSupported = options.credentialsSupported | ||
issuer.credentialsSupported = credentialsSupported | ||
issuer.credentialConfigurationsSupported = undefined | ||
} | ||
issuer.display = options.display | ||
|
||
return this.openId4VcIssuerService.updateIssuer(this.agentContext, issuer) | ||
return this.openId4VcIssuerService.updateIssuer(this.agentContext, Object.assign(issuer, issuerOptions)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather manually assign the props that can be updated instead of doing object.assign. Especially since if you're not using TypeScript it will just assign any property to the record that you pass here. You could provide e.g. |
||
} | ||
|
||
/** | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can change the service interface just fine, so let's change the service return value to:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
? what should the response contain? the whole response?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it would be a start at solving #1965, which was requested. Returning the response means better extensibility as you can extract the headers, etc..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the PR where I implemented the tokenRequestfunctionality, I had that behaviour, and you made me change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay then i misunderstood the previous implementation. What i don't want is requiring the whole access token response as input to the request credentials method.
I want to have separation between the required fields for credo to work, and additional params you may need for extension. And as I said in the previous PR, i'm not a fan of mixing the successBody with extra params we extracted from e.g. the header.