feat: auth + eudi disclosure policies #3

berendsliedrecht · 2025-06-16T11:33:49Z

feat: Relying Party Authorization
feat: authz disclosure policy verification

Signed-off-by: Berend Sliedrecht <[email protected]>

TimoGlastra · 2025-06-16T11:58:56Z

src/disclosure-policy/validateAttributeBasedAccessControlPolicy.ts

+    })
+  }
+
+  const result = runDcqlQuery(attributeBasedAccessControlPolicy.attribute_based_access_control as Query.Output, {


i think we should use the approach as used by Credo: https://github.com/openwallet-foundation/credo-ts/blob/1ac3c1d258fbef3fea655072bdf61d73960733c6/packages/core/src/modules/dcql/DcqlService.ts#L153

And actually parse the query first instead of casting it as .Output

TimoGlastra · 2025-06-16T11:59:25Z

src/disclosure-policy/validateAttributeBasedAccessControlPolicy.ts

+    credentials.push({
+      credential_format: 'vc+sd-jwt',
+      vct: sdJwtCredential.prettyClaims.vct,
+      claims: sdJwtCredential.prettyClaims,
+    })


I think just using dc+sd-jwt is fine here, since the DCQL queries can only contain dc+sd-jwt right?

I got this from Credo.

But I see now in the changelog of openid4vp they renamed vc+... to dc+....

Removed it.

TimoGlastra · 2025-06-16T12:00:24Z

src/disclosure-policy/validateAttributeBasedAccessControlPolicy.ts

+  const credentials: DcqlCredential[] = []
+  const sdJwtService = agentContext.dependencyManager.resolve(SdJwtVcService)
+  for (const authorizationAttestation of authorizationAttestations) {
+    const sdJwtCredential = sdJwtService.fromCompact(authorizationAttestation)


Do you filter the attestations beforehand, so we always know this will be an Sd-JWT?

There is a check, whether it is not a registration certificate. I assumed this method would handle it, but I can add a layer around it.

TimoGlastra · 2025-06-16T12:01:37Z

src/disclosure-policy/validateAllowlistPolicy.ts

+export const validateAllowListPolicy = (
+  allowListPolicy: AllowListPolicy,
+  relyingPartyAccessCertificate: X509Certificate
+) => allowListPolicy.allowlist.includes(relyingPartyAccessCertificate.subject)


I prefer returning objects with keys for these kinds of things. I think there's a good chance we need to add more context/metadata in the future. Especially if we e.g. want to return the entry at some piont.

I think for this case, returning a boolean makes sense as it is just a validation API, not an extraction. Also, including the entry does not really make sense as the entry (the rpAccessCert is provided by the user).

Can change it for consistency, though.

TimoGlastra · 2025-06-16T12:01:49Z

src/disclosure-policy/validateAttributeBasedAccessControlPolicy.ts

+    presentation: false,
+  })
+
+  return result.canBeSatisfied


Same here, prefer objects which allow for extension

Same answer as above.

TimoGlastra · 2025-06-16T12:04:07Z

src/disclosure-policy/verifyAuthorizationAttestation.ts

+  try {
+    const { isValid } = await jwsService.verifyJws(agentContext, {
+      jws: authorizationAttestation,
+      trustedCertificates,
+    })
+    isValidAndTrusted = isValid
+  } catch {
+    if (allowUntrustedSigned) {
+      const { isValid } = await jwsService.verifyJws(agentContext, {
+        jws: authorizationAttestation,
+        trustedCertificates: jwt.header.x5c ?? [],
+      })
+      isValidButUntrusted = isValid
+    }
+  }


I think we need to tweak the API in Credo to handle this better. Verifying twice is not a good approach

TimoGlastra · 2025-06-16T12:04:48Z

src/disclosure-policy/verifyAuthorizationAttestation.ts

+  if (!signedAuthorizationRequest) {
+    throw new Error('Authorization request must be signed for the authorization attestation')
+  }
+
+  if (signedAuthorizationRequest.signer.method !== 'x5c') {
+    throw new Error(
+      'x5c is only supported for key derivation on the authorization request containing a authorization attestation'
+    )
+  }


We discussed returning metadata over throwing errors. Any reason to still throw errors here. If we're doing this we at least need to start throwing catchable errors so you can handle the errors correctly

Not sure what makes this error uncatchable, but I changed it to a context object.

TimoGlastra · 2025-06-16T12:05:07Z

src/disclosure-policy/verifyAuthorizationAttestation.ts

+
+  // TODO: confirm that the `signingCertificate.subject` is the DN of the RP
+  if (payload.sub !== accessCertificate.subject) {
+    throw new Error('The Subject of the Authorization Attestation should equal the distinguished of the Relying Party')


Suggested change

throw new Error('The Subject of the Authorization Attestation should equal the distinguished of the Relying Party')

throw new Error('The Subject of the Authorization Attestation should equal the distinguished name of the Relying Party')

TimoGlastra · 2025-06-16T12:06:37Z

src/disclosure-policy/verifyAuthorizationAttestation.ts

+  })
+
+  if (!isValid) {
+    throw new Error(


it would be good to have a cause here, to not swallow the error.

TimoGlastra · 2025-06-16T12:07:12Z

src/disclosure-policy/verifyAuthorizationForOpenid4VpAuthorizationRequest.ts

+ *
+ * Checks all the matched credentials for additional disclosure policies as set by the issuer
+ *
+ * Make sure the disclosure policies are set manually on the metadata of the record, under the `__disclosurePolicy` key


can we include something that identifies it as eudi in the keyname

TimoGlastra · 2025-06-16T12:07:45Z

src/disclosure-policy/verifyAuthorizationForOpenid4VpAuthorizationRequest.ts

+
+export type AuthzPolicy = AllowListPolicy | RootOfTrustPolicy | AttributeBasedAccessControlPolicy
+
+type VerifyAuthorizationForOpenid4VpAuthorizationRequestOptions = {


Suggested change

type VerifyAuthorizationForOpenid4VpAuthorizationRequestOptions = {

export type VerifyAuthorizationForOpenid4VpAuthorizationRequestOptions = {

TimoGlastra · 2025-06-16T12:08:35Z

src/disclosure-policy/verifyAuthorizationForOpenid4VpAuthorizationRequest.ts

+import { type RootOfTrustPolicy, validateRootOfTrustPolicy } from './validateRootOfTrustPolicy'
+import { isAuthorizationAttestation } from './verifyAuthorizationAttestation'
+
+export type AuthzPolicy = AllowListPolicy | RootOfTrustPolicy | AttributeBasedAccessControlPolicy


We generally don't use abbreviations

Suggested change

export type AuthzPolicy = AllowListPolicy | RootOfTrustPolicy | AttributeBasedAccessControlPolicy

export type AuthorizationPolicy = AllowListPolicy | RootOfTrustPolicy | AttributeBasedAccessControlPolicy

TimoGlastra · 2025-06-16T12:10:54Z

src/registration-certificate/verifyRegistrationCertificate.ts

+    credentials: z.array(
+      z.object({
+        format: z.string(),
+        multiple: z.boolean().default(false),
+        meta: z
+          .object({
+            vct_values: z.array(z.string()).optional(),
+            doctype_value: z.string().optional(),
+          })
+          .optional(),
+        trusted_authorities: z
+          .array(z.object({ type: z.string(), values: z.array(z.string()) }))
+          .nonempty()
+          .optional(),
+        require_cryptographic_holder_binding: z.boolean().default(true),
+        claims: z
+          .array(
+            z.object({
+              id: z.string().optional(),
+              path: z.array(z.string()).nonempty().nonempty(),
+              values: z.array(z.number().or(z.boolean())).optional(),
+            })
+          )
+          .nonempty()
+          .optional(),
+        claim_sets: z.array(z.array(z.string())).nonempty().optional(),


Any way that we could reuse the DCQL query type, or do we really need to redefine this?

Maybe, if they are both in zod otherwise I think it would add unnecessary complexity.

TimoGlastra · 2025-06-16T12:12:21Z

src/registration-certificate/verifyRegistrationCertificateInOpenid4VpAuthorizationRequest.ts

+    if (typeof va.data !== 'string') {
+      throw new Error('Only inline data supported')
+    }


data can also be an object, in which case the error is a bit confusing

TimoGlastra

Took a quick look, still need to do a thorough review. I'm missing some of the returning context instead of throwing errors that we discussed.

berendsliedrecht · 2025-06-16T13:12:29Z

I wanted to handle the errors with context in a different pr, as it also includes the registration certificate verification, but I can also do it here.

berendsliedrecht · 2025-06-16T14:28:23Z

Added a return context to the disclosurePolicy validation:

export type VerifyDisclosurePoliciesForOpenId4VpAuthorizationRequestReturnContext = {
  isValid: boolean
  isSignedWithX509: boolean
  disclosurePolicies: {
    [credentialId: string]: {
      isAllowListPolicyValid?: boolean
      isRootOfTrustPolicyValid?: boolean
      isAttributeBasedAccessControlValid?: boolean
    }
  }
}

I think this provides enough context for a wallet to display information to the user about the request, lmk if something is missing.

Signed-off-by: Berend Sliedrecht <[email protected]>

TimoGlastra · 2025-07-17T08:30:04Z

tests/verifyOpenid4VpAuthorizationRequest.test.ts

+      strictEqual(result?.isValid, false)
+      equal(result?.isRegistrationCertificateQueryEqualOrSubsetOfAuthorizationRequestQuery, false)


Suggested change

strictEqual(result?.isValid, false)

equal(result?.isRegistrationCertificateQueryEqualOrSubsetOfAuthorizationRequestQuery, false)

strictEqual(result.isValid, false)

equal(result.isRegistrationCertificateQueryEqualOrSubsetOfAuthorizationRequestQuery, false)

TimoGlastra · 2025-07-17T08:30:47Z

tests/verifyOpenid4VpAuthorizationRequest.test.ts


      await rejects(
-        verifyOpenid4VpAuthorizationRequest(agent.context, {
+        verifyRegistrationCertificateInOpenid4VpAuthorizationRequest(agent.context, {


When does it throw vs return return invalid?

TimoGlastra

Left some comments. Overall looks good, but we need a bit more consistency in when we throw errors vs return isValid: false

TimoGlastra · 2025-07-17T08:30:58Z

tests/verifyOpenid4VpAuthorizationRequest.test.ts


-      equal(result?.[0].isValidAndTrusted, false)
-      equal(result?.[0].isValidButUntrusted, true)
+      equal(result?.isValid, true)


Suggested change

equal(result?.isValid, true)

equal(result.isValid, true)

TimoGlastra · 2025-07-17T08:31:42Z

src/registration-certificate/verifyRegistrationCertificateInOpenid4VpAuthorizationRequest.ts

+  if (!resolvedAuthorizationRequest.authorizationRequestPayload.verifier_attestations) return
+  for (const va of resolvedAuthorizationRequest.authorizationRequestPayload.verifier_attestations) {
+    if (typeof va.data !== 'string') {
+      throw new Error('Authorization Attestations of string are currently only supported')


Can we create a specific error for this library? So we can catch error as being an EUDI verification error?

TimoGlastra · 2025-07-17T08:36:15Z

src/registration-certificate/verifyRegistrationCertificate.ts

+  registrationCertificateHeaderSchema.parse(jwt.header)
+  const parsedPayload = registrationCertificatePayloadSchema.parse(jwt.payload.toJson())


I feel this should also be part of the returned context, (and we should add an human error message there).

It's very random now:

JWS signature verification fails -> return isValid: false

registration certificate header schema parsing fails -> throw error

TimoGlastra · 2025-07-17T08:38:19Z

src/registration-certificate/verifyRegistrationCertificate.ts

+  const rpCert = X509Certificate.fromEncodedCertificate(rpCertEncoded)
+
+  if (rpCert.subject !== parsedPayload.sub) {
+    returnContext.isAccessCertificateSubjectEqualToRegistrationCertificate = false


We should also set these to true if they did succeed, so you know which checks were performed.

TimoGlastra · 2025-07-17T08:39:38Z

src/registration-certificate/verifyRegistrationCertificate.ts

+  const isValidDcqlQuery = isDcqlQueryEqualOrSubset(dcql.queryResult, parsedPayload as unknown as DcqlQuery)
+  if (!isValidDcqlQuery) {
+    returnContext.isValid = false
+    returnContext.isRegistrationCertificateQueryEqualOrSubsetOfAuthorizationRequestQuery = false


Wouldn't this with a JSDoc comment be sufficient?

Suggested change

returnContext.isRegistrationCertificateQueryEqualOrSubsetOfAuthorizationRequestQuery = false

returnContext.isAuthorizationRequestQueryValid = false

Comment could be like:

/** * Returns whether the authorization request query is equal or a subset of the registration certificate query */

TimoGlastra · 2025-07-17T09:23:20Z

src/disclosure-policy/verifyAuthorizationAttestation.ts

+
+// TODO:
+//    - Should we support more hashing confirmation claim values?
+const authorizationAttestationPayloadSchema = z.object({


It would be great if we don't have to redefine the JWT payload every time 🤔 . Fine for now, but might be interesting to build on top of a base defined in e.g. openid4vc libs

TimoGlastra · 2025-07-17T09:25:37Z

src/disclosure-policy/validateRootOfTrustPolicy.ts

+// TODO: I am not sure how to validate this, based on the description
+//
+// rootOfTrust: the certificate of a RP must be derived from a list of specific root certificates. In the Implementing Acts it's called Specific root of trust. The value has to be the distinguished name of the root certificate.
+export const validateRootOfTrustPolicy = (rootOfTrustPolicy: RootOfTrustPolicy) => true


I think we just need to do a match on the distinguished name (issuer or subject) of the certificate?

So probaby the issuer field of the root/intermediate certificate in the authorization request must match one of the distinguides names from the list passed

TimoGlastra · 2025-07-17T09:26:06Z

src/disclosure-policy/validateAttributeBasedAccessControlPolicy.ts

+    const queryResult = Query.query(
+      Query.parse(attributeBasedAccessControlPolicy.attribute_based_access_control),
+      credentials
+    )


This can throw if an invalid query is used. Do we catch that somewhere?

berendsliedrecht added 2 commits June 13, 2025 10:25

feat: Relying Party Authorization

85c0155

Signed-off-by: Berend Sliedrecht <[email protected]>

feat: authz disclosure policy verification

ff50735

Signed-off-by: Berend Sliedrecht <[email protected]>

TimoGlastra reviewed Jun 16, 2025

View reviewed changes

TimoGlastra requested changes Jun 16, 2025

View reviewed changes

chore: resolved feedback

193202a

Signed-off-by: Berend Sliedrecht <[email protected]>

berendsliedrecht force-pushed the authz branch from 3161a30 to 193202a Compare June 17, 2025 09:00

feat: add error context to registration certificate validation

faf6ee1

Signed-off-by: Berend Sliedrecht <[email protected]>

berendsliedrecht force-pushed the authz branch from d9f7a6e to faf6ee1 Compare June 17, 2025 13:43

TimoGlastra reviewed Jul 17, 2025

View reviewed changes

	throw new Error('The Subject of the Authorization Attestation should equal the distinguished of the Relying Party')
	throw new Error('The Subject of the Authorization Attestation should equal the distinguished name of the Relying Party')


		export type AuthzPolicy = AllowListPolicy \| RootOfTrustPolicy \| AttributeBasedAccessControlPolicy

		type VerifyAuthorizationForOpenid4VpAuthorizationRequestOptions = {

	type VerifyAuthorizationForOpenid4VpAuthorizationRequestOptions = {
	export type VerifyAuthorizationForOpenid4VpAuthorizationRequestOptions = {

	export type AuthzPolicy = AllowListPolicy \| RootOfTrustPolicy \| AttributeBasedAccessControlPolicy
	export type AuthorizationPolicy = AllowListPolicy \| RootOfTrustPolicy \| AttributeBasedAccessControlPolicy

		strictEqual(result?.isValid, false)
		equal(result?.isRegistrationCertificateQueryEqualOrSubsetOfAuthorizationRequestQuery, false)

		registrationCertificateHeaderSchema.parse(jwt.header)
		const parsedPayload = registrationCertificatePayloadSchema.parse(jwt.payload.toJson())

	returnContext.isRegistrationCertificateQueryEqualOrSubsetOfAuthorizationRequestQuery = false
	returnContext.isAuthorizationRequestQueryValid = false

feat: auth + eudi disclosure policies #3

Are you sure you want to change the base?

feat: auth + eudi disclosure policies #3

Uh oh!

Conversation

berendsliedrecht commented Jun 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimoGlastra left a comment

Choose a reason for hiding this comment

Uh oh!

berendsliedrecht commented Jun 16, 2025

Uh oh!

berendsliedrecht commented Jun 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimoGlastra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels