JwtAuthnPolicy could validate the key_issuer against iss

[achamayou]

JwtAuthnPolicy currently exposes an authenticated key_issuer, and an un-checked iss field from the token. It is up to the application code to check that they match (which should be the case when the standard is followed), and to include logic used to implement multi-tenant support for example, when applicable.

We could improve on this by:

1. Checking key_issuer == iss
2. If not, instantiate the key_issuer using the AAD-specific logic described in the link above and the tid

This would break neither standards-compliant, nor AAD use cases, and would reduce the risk of incorrect token validation by the application code.

Tasks

Preview Give feedback

1. Do the issuer validation against the token
2. Migration: check how the old code works with new nodes in the network and prepare a PR to 4.x if needed
3. Migration: make sure we get rid of the old table with time

[eddyashton]

@maxtropets

[maxtropets]

Key points

• We improve token authorization by adding checks against issuer data for both key and token.
• In the scope of this ticket we assume we can’t have more than one unique kid. The documentation doesn’t mention what should we do if doesn’t come true. It is up to Duplicate JWK kids across issuers are not handled correctly #5177 to either:
  • prove the point, or
  • prove that wrong, but not an issue, or
  • prove that wrong and to be an issue
    • come up with the fix in this case
• Fixups we consider
  • Check token.iss and key.issuer match (caveats for multi-tenancy mode, see below)
  • token.iss’s tenant-id and token.tid claim match
  • key.issuer matches the expected issuer when fetching
    • To be discussed further
    • Doc says
      • Identify key in the metadata based on the kid header. Check the "issuer" property attached to the key in the metadata document

      // ...
      if (configuration.allowedIssuer != "*"
          && configuration.allowedIssuer != issuer)
          throw validationException;
      // ...

• The algorithm to be implemented is nicely summarized in the documentation
• Multi-tenancy VS single-tenancy
  • First we should check for single/muti-tenancy: key.issuer = prefix/{tenant-id}/suffix stands for multi-tenancy and everything else means single-tenancy. We will choose the mode by {tenant_id} presence (curly braces included).
  • Now, for single-tenancy, token.iss has to match key.issuer
  • For multi-tenancy
    • Replace {tenant-id} with token.tid , so consider issuer* = prefix/token.tid/suffix
    • Now, token.iss has to match issuer*

[maxtropets]

After implementing the above, I've faced a complication.

Currently, we have only one tables in the KV to map the kid to issuer. The problem is, that for MSFT Entra multitenancy mode, we have to compare the token.iss claim against the "issuer" specified in the key.

E.g., as the doc above says, we will have the issuer argument =https://login.microsoftonline.com/common/v2.0/, but each key from it will have either issuer=https://login.microsoftonline.com/{tenantid}/v2.0 or issuer=https://login.microsoftonline.com/HEX-STRING/v2.0

As soon as the multitenant endpoint can (and probably will) provide multiple keys with different issuers, I first decided to overwrite the original issue in the kid->issuer table and this worked well.

However, as I revealed in tests, this breaks down remove_jwt_keys., because we lose the original issuer, and now we can't find out which keys to remove for any particular issuer anymore.

The only way to fix it I came up with is change the schema (extend kid->issuer or add new table). Needs to be discussed further.

[achamayou]

@maxtropets I think what this effectively means is that fixing #5177 first is necessary. Let's discuss this with @eddyashton.

[achamayou]

To clarify, there are potentially two tables where this is a problem:

jwt.public_signing_keys, which was the concern raised in #5177. Two different issuers could be using the same kid for different public keys. I don't believe this is a problem if all the issuers are AAD with a different tenant, but remains a problem in general.

jwt.public_signing_key_issuer, which is the concern in this case. Two different issuers could be applying different issuer constraints to the same kid. This may happen in AAD, where in practice it will be two tenants saying they both accept a kid for their respective tenant.

In addition to this, I think we also need to keep a mapping of kid -> issuer as configured, as opposed to the issuer constraint fetched from the keys. This is because we must not allow a configured issuer A to give us as constraint that the keys they expose can be used for issuer B.

[maxtropets]

Regarding the first one - it seems they carry the same public key.

In common Entra endpoint, the first non-common tenant in the keys is 9188040d-6c67-4c5b-b112-36a304b66dad with kid=SP89B0VYWSffbnOcvhkYBmPdo3M.

It exposes it's own OpenID keys at https://login.microsoftonline.com/9188040d-6c67-4c5b-b112-36a304b66dad/v2.0/.well-known/openid-configuration

If we go to the keys in there, we can find the kid=SP89B0VYWSffbnOcvhkYBmPdo3M

Both keys with kid=SP89B0VYWSffbnOcvhkYBmPdo3M, from the common endpoint and specified endpoint, carry the same public key.

However we don't see anything said that this's an expected behaviour, and I don't really see how we can deal with that.

[maxtropets]

Regarding the second point about different issuer constraints.

It seems it's not true for the keys published right now, because the keys they point to, which are

• https://login.microsoftonline.com/common/discovery/v2.0/keys
• and https://login.microsoftonline.com/9188040d-6c67-4c5b-b112-36a304b66dad/discovery/v2.0/keys

seem to an alias of the same document - the contents are identical.

However we also don't have anything saying it's guaranteed to not happen.

[achamayou]

Regarding the first one - it seems they carry the same public key.

I believe that for the same top level issuer, but not across actually different issuers (not just ADD/tenant).

Regarding the second point about different issuer constraints.
However we also don't have anything saying it's guaranteed to not happen.

Exactly. I think it's probably mostly true, but it's very easy to imagine configurations where that would not be. So I think we want a design that can cope with that, even if it maybe doesn't have to.

[maxtropets]

#6175 opened for review.

Must-knows before review

• We will refer to issuer as a url where we get key from, but we also refer to it as a value we compare the token.iss claim to. The latter we might also call "issuer constraint".
  • These don't always match, for AAD they differ a lot, for Facebook they also differ, for others we've checked they matched, but the point is that we shouldn't rely on that.
• We never stored the constraint, we only had kid -> issuer in the KV. Two things changed
  • Issuer is stored alongside with the constraint. We still need issuer to know who we got the key from (in case we want to delete all keys for the issuer, for example), and we need a constraint to check against.
  • We now store a list of those pairs for each kid, because you might have the same key added by different tenants with different constraints. On auth, we'll go through all matching issuers for the kid until we either find the match or reject the auth request otherwise.
  • We haven't allowed multiple kids for different issuers YET. This will be done in Duplicate JWK kids across issuers are not handled correctly #5177. Moving to a list of values is a good preparation for that change, and we'll avoid changing the schema twice.
• We also made sure we properly handle cases from the old schema, meaning we simply don't validate the issuer until we refresh the keys on a node with a new version of the code which will properly populate the constraints.
  • In request handles that ask for JWT information we don't provide the issuer at all in case it's not there, which is the only change in the behavior. We believe this shouldn't cause problems, and is not worth keeping the old table around.
• Also, added Verify JWT issuer constraint against the issuer url #6204 to implement domain check on key fetch as we spoke with @achamayou

[maxtropets]

Decided to complete follow-up tickets in this PR to avoid scattering between releases

[maxtropets]

Done in #6175