Allow for multiple "aud" values - #4147

Thanks for offering this @the-voidl.

While I respect that RFC7519 does allow multiple aud values, the comment wording (and related logic) removed in this PR comes directly from the OIDC implementers guide in regard to ID Token Validation (Section 2.2.1 -> Point 2):

The Client MUST validate that the aud (audience) Claim contains its client_id value registered at the Issuer identified by the iss (issuer) Claim as an audience. The ID Token MUST be rejected if the ID Token does not list the Client as a valid audience, or if it contains additional audiences not trusted by the Client.

Emphasis in the above is mine.

In my view, from the BookStack perspective we would only be trusting the client_id for BookStack, so we'd only expect that one value to be part of the aud list, otherwise reject as per the line above.

Therefore I'm hesitant to change the existing logic since I'm specifically working to OIDC guidance, instead of the wider scope of what's possible in a JWT.

Let me know if I've misunderstood though, or if you think I'm misinterpreting the OIDC guidance.

There is a whole discussion about the use of azp in the OpenID repo at Bitbucket: https://bitbucket.org/openid/connect/issues/973/
I see that there is much ambiguity about the form and validation of azp in conjunction with aud, in some documents azp doesn't event occur.

OpenID seems to be inconsistent about itself in this manner:
The draft to openid-basic you sent says:

azp: [...] This Claim is only needed when the ID Token has a single audience value and that audience is
different than the authorized party.

While the errata to openid-core states following:

If the ID Token contains multiple audiences, the Client SHOULD verify that an azp Claim is present. 

My interpretation therefore is:

1. There can be multiple aud values in the token
2. aud, no matter if single value or array, must contain the client_id
3. apz can always be present, whether there is a single aud value or multiple
4. If azp is present, it must contain the client_id as well
5. Other aud values than client_id can be ignored, especially if an azp claim is present

The only trustworthy conclusion weather azp should be present or not - in my opinion - is if the issuer uses extensions which extend the openid specification. While a client can't tell fur sure if that is the case, it should not reject azp in any case as long as it contains the client_id.

To answer the question about the comment I removed:
I also cannot tell under what circumstances an aud value should be trusted or not. We could define an array of untrusted auds, which would again lead to confusing over-interpretation of the specs, so ignoring different values should be the preferred way to use.

There is a whole discussion about the use of azp in the OpenID repo at Bitbucket:

Thanks for that info, but the conversation does not alter or lead to changes in the lines I've followed here. The proposals right at the end even specifically retain the or if it contains additional audiences not trusted by the Client line.

I also cannot tell under what circumstances an aud value should be trusted or not. ... so ignoring different values should be the preferred way to use.

Since this related to auth, I'd side on the safe default of not trusting anything unless there's a reason to, and therefore stick to the official guidance in strict terms. I'd much rather "over-interpret" than under-interpret.

To be honest, I'd question why extra aud values are being provided in this context by zitadel. Having a real quick look at their source, I couldn't immediately see where multiple values are coming from, I could only find single value instances although there were quite a lot of steps between audience source and usage in the JWT so I may have got lost along the way.

You are right. The phrase contains additional audiences was retained in the spec.
What lets me believe it's safe to ignore different audiences were these sentences:

openid-connect-core: 2. (ID Token)

aud [...] It MAY also contain identifiers for other audiences. In the general case, the aud value is an array of case sensitive strings. In the common special case when there is one audience, the aud value MAY be a single case sensitive string.

And this answer on StackOverflow.

And Michael Jones in the referred discussion on Bitbucket:

We need to clarify that "the Client" in item 5 is the issued-to client identified by the "azp" claim - not the requesting claim identified by the "aud" claim.

and

I still think that we should recommend using multi-party audience values for multi-party scenarios, rather than recommending any further use of "azp".

William Denniss also figured scenarios there, which comply with what I understand (search for "Multiple Audience Format" or "Dual Audience Format").

Zitadel implements this hierarchy: Instance > Project > Client while roles/groups are project-wide.
The five different aud values I was receiving in the above mentioned ID Token correspond the four different clients we defined in this project as well as the project id. So it makes totally sense to send all these audiences within the payload. One of the clients could pass the token to one of the other clients, because all of them are in the same scope. But the token was originally issued for azp which can be seen in the token by any client. I think this would be called token exchange.

This scenario, that multiple audiences occur, can also happen with ORY, AzureAD, and Keycloak as well.

Also interesting is that ORY say:

aud claim of an OAuth 2.0 access token defines the endpoints at which the token can be used.

Further I'd consider the payload trustworthy as long as a single azp value corresponds to the client_id. I see no reason to reject the token solely because there are other IDs in the token, while the actual client_id is present AND confirmed by being the single azp value.

What lets me believe it's safe to ignore different audiences were these sentences:

None of those quoted lines, nor the stack overflow answer, lead me to think it's valid to not strictly follow the OIDC wording. Nor do they seem incompatible with the wording I'm following.

Zitadel implements this hierarchy: Instance > Project > Client while roles/groups are project-wide.
The five different aud values I was receiving in the above mentioned ID Token correspond the four different clients we defined in this project as well as the project id. So it makes totally sense to send all these audiences within the payload.

That makes sense, where project is an application, especially in the context of potential micro services that would maybe share tokens at some level as part of a single application/project.
Can you not just put BookStack into its own project then?

This scenario, that multiple audiences occur, can also happen with ORY, AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#39, and Keycloak as well.

I'm sure there are valid multi-audience OIDC scenarios, like the touched on above. I'm just not sure the scenario in question is one of those. BookStack with OIDC has been used in many environments, including in AzureAD and Keycloak, without this being an issue. I believe the existence/use of multi-audience for OIDC would be conditional based upon context of the client application, and how it's auth is shared/connected with other applications (If any).

I see no reason to reject the token solely because there are other IDs in the token, while the actual client_id is present AND confirmed by being the single azp value.

No, there may be absolutely no reason to reject like this, or there may be a non-obvious reason which is what prompted the wording to be written into the implementers guide in the first place. Either way, I'd play it safe and stick to the wording of the guide.

Can you not just put BookStack into its own project then?

Sadly that's not possible because, like mentioned before, Zitadel sends the project id as audience as well, so we always get at least two values.

We could also thump on a change in Zitadel to be able to send a single audience. I'd rather like to see BookStack more flexible in accepting OICD servers than authentication providers adopting to single instances they need to authenticate for. Also since both of us clearly agree that it's valid to have more than one audience.

I support you in being more strict about authentication, but in this particular case here I'd wish to see more flexibility.

If I understand you right, the only problem is that one cannot determine if an audience different from client_id can be trusted?
What about a config parameter that allows for multiple audiences if wanted? I suppose that anyone using an authentication server trusts this endpoint and so trusts the received audiences. Additionally we could define an array of trusted audiences. I'd offer to add this to my PR.

If I understand you right, the only problem is that one cannot determine if an audience different from client_id can be trusted? What about a config parameter that allows for multiple audiences if wanted? I suppose that anyone using an authentication server trusts this endpoint and so trusts the received audiences. Additionally we could define an array of trusted audiences.

I'm not too comfortable with that either, it seems like an option that would only act as a hack around this exact scenario. From the BookStack perspective, it should not need to trust other audiences, it's not like it's expected that auth/tokens need to be shared.

I understand the desire for us to be more flexible, but I'm hesitant on adding additional specific options, and widening the scope of what we support, to specifically address outlying cases from specific identity providers.

That said, as touched upon in your other PR, I'd be happy to add a new logical theme event to allow customization of incoming user token data, which would allow generic ultimate flexibility, allowing any shape of ID token to be manipulated to the format that BookStack supports as required.

I'm going to go ahead and close this off since, as explained above, I don't see myself going against the OIDC guidance or adding scenario/vendor specific workarounds.

As said above, happy to add a new logical theme event to allow customization of incoming user token data. Just shout if you want that to be added and a feature request issue can be added for that.

Well, I still don't see that this is against OIDC guideline, while I understand your doubts.

Thus I think I will be fine with a customization of the user token data in the new system.

@the-voidl Thanks for the understanding. I've opened a new issue for the mentioned theme even in #4200. Have assigned to the current milestone with an aim to sneak this into our next feature release.