If public_key is specified, it will not verify issuers. Are you sure this is expected?

you are right, we should verify issuer when use public_key too.

This has already had another implementation that was approved and we have to decide which one is better.

#11987

Your PR adds audience validation and my PR adds issuer validation.
Audience validation matches "aud" in the JWT with "client_id"
Issuer validation matches "issuer" in the JWT with discovery.issuer/or passed by user.

If the problem that I am trying to solve is this "I want to reject a request because customer passed a JWT with unrecognised issuer."

@bzp2010

@Revolyssup 🙌

I certainly know what I did, and know what you did, and I mentioned that we have to choose one of the better configuration schema designs, while accommodating design that are similar in use but so different is unacceptable.
Specifically, if they are tiled, then this and all subsequent validator configuration items need to be tiled, otherwise it's going to be structured like that PR #11987.

That's exactly why that PR hasn't merged yet, it's waiting for some feedback.

@Revolyssup please refer to #11987 to update the configuration schema, thanks.

When using the Introspection API, don't we check the issuer again locally? If so, it seems a bit confusing.

I.e. when exactly does the configuration of “valid_issuers” take effect. If this is as expected, it should be documented.

I understand that this valid_issuers is one of the options in lua-resty-jwt, but this is not exactly for OpenID Connect (JWT subset), but for whole JWT.
As documented in the OpenID Connect Core 1.0 specification, it should be present and must be equal to an exact match of the issuer identifier (obtained from discovery), and it seems that this value should not be configurable by the user, or according to the OIDC specification, the user doesn't need to configure this value.

Ref:

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.
https://openid.net/specs/openid-connect-core-1_0.html#AggregatedDistributedClaims

It is mentioned here that the iss declaration must be present in any IDP-issued JWT, so this is not limited to ID Token, but also Access Token.

The Issuer Identifier for the OpenID Provider (which is typically obtained during Discovery) MUST exactly match the value of the iss (issuer) Claim.
https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation

Although this is the section used to describe the ID Token rules, it mentions the exact rules about iss claim.

issuer: REQUIRED. [skiped] This also MUST be identical to the iss Claim value in ID Tokens issued from this Issuer.
https://openid.net/specs/openid-connect-discovery-1_0-final.html#ProviderMetadata

The issuer must be included in the discovery response.

So if the use case here is oriented towards openid connect, and IDP servers that do conform to openid connect, the value should not even be allowed to be user-defined, but always obtained from discovery.
But I understand that we may have to deal with some corner cases, then the configuration design should explicitly reflect that this is an override of the default behavior (which according to the spec iss validation should be mandatory).

So I would propose a design similar to the audience one:

{
  "claim_validator": {
    // !!! If you feel that it is sufficient for us to obey the OIDC specification (i.e. not need allow the user to customize the issuer list), then this field can be simply specified as boolean, which defaults to true.
    "issuer": true,
    // !!! Otherwise a nested structure can be provided that has two settings, whether to enforce issuer validation or not, and another to store a list of custom issuers.
    "issuer": {
      "enforce": true // I'm not sure what it should be called, but this should indicate if we MUST perform the iss check.
      "valid_issuers": ["xxx"]
    }
  }
}

WDYT?

• Must users still be allowed to customize the issuer list?
• How should the configuration look?

I think the users should be able to customise the list, that was the original requirement for this from the customer.
From usage perspective, I think the original configuration is simple

{
  "claim_validator": {
      "valid_issuers": []
   }
}

If valid_issuers is not explicitly set then it is understood that there wont be issuer verification.
The use case this tries to solve is to make sure that even if discovery endpoint returns a different issuers in response, user can override that by manually passing the valid_issuer.

The claim_validator should only store claims without any prefixes, just like audience. The valid_issuers clearly violates this convention.

I've made proposals that would satisfy either the option to support setting arbitrary issuers or the strict OIDC specification of not supporting arbitrary issuer setting.

• If OIDC is strictly followed, use issuer: true
• If customization is allowed, use nested structured configuration.

If no agreement can be reached on this, then other reviewers will need to comment on this.

Okay second option looks good to me.