Does it makes sense to also verify here that issuerUrl is the only audience and there are not any other audiences included?

So maybe something like:

if (!token.hasAnyAudience(expectedAudiences) && token.getAudience().length == 1) {

In the past, there was security issue found in the private_key_jwt authentication that servers accepted the JWT with multiple audiences and same JWT token, which was used for client authentication, was also used for some other party. That is why the recent draft of OIDC core mentions explicitly single audience https://openid.net/specs/openid-connect-core-1_0-36.html#ClientAuthentication . The FAPI2 seems to not mention it explicitly for the server, however it indirectly mention this as well in the requirements for client, where it mentions - The issuer identifier value shall be sent as a string not as an item in an array. . .

I am not 100% sure if it is possible to reliably check that aud was sent as a string and not an array (also there is likely not any security benefit in that). However for better security, it makes sense to me to check that the issuer is single audience and not other audiences are included. WDYT?

Thank you for your comments.

As you suggested, I will add the "aud" check.

I know the secuirty issue about specification and you have resolved that.
#38819
#38830

OIDF FAPI 2.0 Final conformance tests include the test for check whether "aud" claim value is single.
So, when running the conformance tests, I do the following settings.

spi-login-protocol-openid-connect-allow-multiple-audiences-for-jwt-client-authentication=true

When a keycloak user tries to apply FAPI 2.0 Final security profile, they need to do the setting, which checks the "aud" single value check, so I omitted the same check in the PR.

However, if the PR includes the "aud" check, then the user need not do the setting because the PR itself checks that. It might be helpful for the user.

In order to do that, what about the following?

        if (token.getAudience().length != 1) {
            logger.warnf("invalid audience in client assertion - no audience or multiple audiences found");
            throw new ClientPolicyException(OAuthErrorException.INVALID_REQUEST, "invalid audience in client assertion");
        }
        if (!token.hasAnyAudience(expectedAudiences)) {
            logger.warnf("invalid audience in client assertion - audience not issuer URL");
            throw new ClientPolicyException(OAuthErrorException.INVALID_REQUEST, "invalid audience in client assertion");
        }

I modified the PR as just above and confirmed that Keycloak incoporating the PR can pass FAPI 2.0 Security Profile Final and FAPI 2.0 Message Singing Final conformance tests with both the following settings (current Keycloak's default setting):

spi-login-protocol-openid-connect-allow-multiple-audiences-for-jwt-client-authentication=true

and

#spi-login-protocol-openid-connect-allow-multiple-audiences-for-jwt-client-authentication=true

The PR was force-pushed.

@tnorimat Probably a detail, but I afraid a bit of NPE when there are not any audience in the token as in this case token.getAudience().length might probably throw NPE? Is it ok to rather change the condition for length to if (token.getAudience() == null || token.getAudience().length != 1) ?

Another option is to make sure that length is checked after token.hasAudience call (as token.hasAudience would fail in case that token does not have any audience and hence no chance of NPE).

@mposolda Thank you. I will revise the PR as you suggested.

@tnorimat Probably a detail, but I afraid a bit of NPE when there are not any audience in the token as in this case token.getAudience().length might probably throw NPE? Is it ok to rather change the condition for length to if (token.getAudience() == null || token.getAudience().length != 1) ?

Another option is to make sure that length is checked after token.hasAudience call (as token.hasAudience would fail in case that token does not have any audience and hence no chance of NPE).