Skip to content

userinfo: verify the signature on JWT-encoded composite claims when verification keys are configured - #1395

Open
Pyolar wants to merge 1 commit into
OpenIDC:masterfrom
Pyolar:fix/composite-claims-signature-verification
Open

userinfo: verify the signature on JWT-encoded composite claims when verification keys are configured#1395
Pyolar wants to merge 1 commit into
OpenIDC:masterfrom
Pyolar:fix/composite-claims-signature-verification

Conversation

@Pyolar

@Pyolar Pyolar commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

oidc_proto_userinfo_composite_decode_source took the JWT from an aggregated/distributed (composite) claim source apart with a bare oidc_jwt_parse and merged its payload into the userinfo claims unconditionally, without any signature verification.

With this change, when static verification keys are configured (OIDCProviderVerifyCertFiles), the composite-claims JWT is signature validated against them first, and one that fails validation is discarded - its claims are not applied. Without configured verification keys the previous behavior is preserved, since OIDC Core 1.0 section 5.6.2 does not require signature validation of these JWTs when the Claims Provider keys cannot be obtained.

…erification keys are configured

oidc_proto_userinfo_composite_decode_source parsed aggregated/distributed
(composite) claim JWTs with a bare oidc_jwt_parse and merged their payload
unconditionally. When static verification keys are configured
(OIDCProviderVerifyCertFiles), the composite-claims JWT is now signature
validated against them first, and one that fails validation is discarded.
Without configured keys the behavior is unchanged, since OIDC Core 1.0
section 5.6.2 does not require signature validation of these JWTs when the
Claims Provider keys cannot be obtained.

@zandbelt zandbelt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for following up on the composite-claims path — the gap you identified is real.
oidc_proto_userinfo_composite_decode_source does parse the JWT and merge its payload with no
signature check at all, and that should not stand. The patch also builds clean and the whole test
suite passes here.

I don't want to take it in this shape though, because it validates against the wrong party's keys.

The keys belong to the Claims Provider, not the OP

OpenID Connect Core 1.0 section 5.6.2:

An iss (issuer) Claim SHOULD be included in any JWT issued by a Claims Provider so that the
Claims Provider's keys can be retrieved for signature validation of the JWT. The value of the
Claim is the Claims Provider's Issuer Identifier URL.

The aggregated/distributed claims JWT is signed by the Claims Provider, which is by definition a
different entity from the OpenID Provider. OIDCProviderVerifyCertFiles is the OP's key set — see
auth_openidc.conf, where it is documented as the keys "that can be used for ID Token
verification". Verifying a Claims Provider's JWT against the OP's ID token keys will not line up in
any deployment that uses composite claims the way the spec describes them.

Three things follow from that.

1. It breaks working deployments. Anyone who uses composite claims and has
OIDCProviderVerifyCertFiles configured will have legitimate Claims Provider JWTs discarded from
this release on: the claims silently disappear, authorization that used to succeed starts failing,
and all the operator gets is a line in the error log. Note that the negative case in
test_proto_userinfo_request_composite_claims_verify_keys — a JWT signed with a key that is not in
the configured set — is indistinguishable from a perfectly legitimate third-party Claims Provider.
The test asserts the regression as though it were the fix.

2. It does not address the threat that matters. In the aggregated case the only party that can
place a JWT in _claim_sources is the OP, and the OP can already assert any claim it likes directly
in the UserInfo response. Checking that JWT against the OP's own keys therefore adds nothing over
the TLS channel we already have to the OP. The case actually worth defending against is a
substituted or tampered Claims Provider assertion, and catching that requires the Claims
Provider's
key — precisely the one this does not use.

3. Most deployments get no protection anyway. The check is gated on whether an unrelated
directive happens to be set. Installations that configure the provider through
OIDCProviderMetadataURL or OIDCProviderJwksUri — the common case — keep the unverified path
exactly as it is today. So the change leaves the reported gap open for the majority while breaking a
subset of everyone else.

The ChangeLog entry overstates the spec

since OpenID Connect Core 1.0 section 5.6.2 does not require signature validation of these JWTs
when the Claims Provider keys cannot be obtained

Section 5.6.2 does not say this. It says iss SHOULD be included so that the keys can be
retrieved for signature validation. The absence of an explicit requirement is not an explicit
exemption, and this sentence is doing a lot of work to make the opt-in design look sanctioned by the
specification.

Smaller points

  • Changing const oidc_cfg_t *cfg to oidc_cfg_t *cfg on
    oidc_proto_userinfo_composite_decode_source is a const regression, forced by
    oidc_proto_jwt_verify taking a non-const cfg. I would rather fix the const-correctness at the
    callee than propagate it outward.
  • oidc_util_key_symmetric_merge(r->pool, verify_keys, NULL) supplies no symmetric key, unlike the
    signed-UserInfo path a few dozen lines above that derives one from the client secret. An HS-signed
    composite JWT can therefore never verify.
  • The negative test signs with HS256 under an unknown kid, which really only demonstrates
    "different algorithm plus unknown key fails". A tampered signature under the same alg and kid
    would be the stronger assertion. There is also no test covering the compatibility promise the
    change is built on, i.e. that behaviour is unchanged when no verification keys are configured.
  • I have a ChangeLog entry of my own landing under 08/03/2026, so that section will need a rebase
    once it is on master.

Where I would like this to go

The gap is worth closing, so please do not drop it. The shape I would take is one that follows
5.6.2: resolve the Claims Provider's keys from the iss claim in the composite JWT. That needs
configuration that does not exist yet — a JWKS URI or a static key set per Claims Provider issuer —
and the verification should be gated on that new setting, so the behaviour change is opt-in on a
knob that actually means what it does, rather than riding on OIDCProviderVerifyCertFiles.

Happy to discuss the configuration surface for that before you invest in another round.

@Pyolar

Pyolar commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

@zandbelt

Thanks for the detailed review — you're right on all three points, and reusing OIDCProviderVerifyCertFiles was a bad call. The OP's ID token keys are not the Claims Provider's keys, so that check would break exactly the deployments it was meant to help, and it adds nothing against a substituted Claims Provider assertion. I'll drop that direction.

Agreed on the way forward: resolve the Claims Provider's verification material from the iss claim per section 5.6.2, gated on configuration that is actually about Claims Providers. A few questions before I build it:

  • Configuration surface: one directive per Claims Provider issuer — e.g. OIDCClaimsProviderJwksUri <issuer> <jwks_uri> / OIDCClaimsProviderVerifyCertFiles <issuer> <keyfiles> — or would you prefer a single directive that maps an issuer to its key source?
  • Static key files, JWKS URI, or both? A per-issuer JWKS URI seems the more practical default for providers that publish one, with static public keys as a fallback.
  • Issuer matching: my preference is exact iss matching rather than wildcard/prefix matching, possibly retaining the module's existing trailing-slash normalization semantics. The iss value would only be used to select an administrator-configured Claims Provider entry; I would not perform discovery or fetch an arbitrary JWKS URI based directly on the unverified iss.
  • Symmetric signatures: for the first revision, would it make sense to support only asymmetric Claims Provider signatures? Reusing the OP's OIDCClientSecret for HS-signed composite JWTs would have the same trust-domain problem as reusing the OP's verification keys. If HS support is needed, I think it should require a separate per-Claims-Provider shared secret.

I'll also fix the smaller items in the same pass: keep const-correctness at the callee instead of widening the parameter where possible, strengthen the negative test by tampering with the signature/payload while keeping the same algorithm and kid, add a test for unchanged behavior when no Claims Provider verification configuration is present, and rebase the ChangeLog entry onto yours.

Happy to wait for your take on the configuration surface before writing the next revision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants