Add flags that allow using an IDP that puts multiple audiences into an ID token

[rssc]

These commits add two flags to the code that allows using IDPs that set more than one audience in the ID token, like for example Zitadel.

The two flags / environment variables are:

• ACCEPTED_AUDIENCES: takes a list of space-separated audiences that are permitted to be present in the ID token
• ACCEPT_UNKNOWN_AUDIENCES: a boolean where, if set, all unknown audiences in the ID token are accepted; this isn't necessarily ideal (and strictly speaking I think goes against the OpenID Connect spec), but this may be the only reasonable solution for IDPs like Zitadel, where adding a new application to a project will add an additional audience to all applications in that project, which would require the config of clients like this one to be updated

I also updated the README file to explain those two new flags.

I believe this pull request would fix issue #341 (which is actually why I created this fix, because I am also using Zitadel with this code).

As a caveat, I am a complete beginner in Rust, so this code may be far from canonical, elegant, whatever, so I am happy to either make additional changes to the code, or let you use this as a basis for your own fix.

[MarcelCoding]

Hi, thanks for your contribution. I think it would be better to provide a required audience and just allow that the idp provides additional audiences. But the one configured in jitsi openid should always be provided.

[rssc]

Hi, thanks for your contribution. I think it would be better to provide a required audience and just allow that the idp provides additional audiences. But the one configured in jitsi openid should always be provided.

Hi, thanks for looking at this!

My understanding from reading through the code is that the OpenID library always checks that the CLIENT_ID is in the returned audiences. Setting either ACCEPTED_AUDIENCES or ACCEPT_UNKNOWN_AUDIENCES does not change that.

But if I understand what you're saying correctly, I think ACCEPT_UNKNOWN_AUDIENCES is doing what you suggested? It still checks that the SECRET_ID is in the list of audiences (and fails if it isn't), but if there are any additional audiences, it ignores those.

I added ACCEPTED_AUDIENCES in addition because ideally people didn't have to accept unknown audiences, and at least the way I am using Zitadel I can make sure that even though it sends one additional audience (which is an identifier for the project the client is configured in), I specifically trust that additional audience as well.

But because it is possible that people will use Zitadel differently and add additional applications to the same project that this client will be configured in, I created the ACCEPT_UNKNOWN_AUDIENCES, so that making changes in Zitadel does not break this client.

Does that make sense, or did I misunderstand what you meant?

[MarcelCoding]

Sorry. I've misunderstood your code. Thanks. Although I would strongly avoid adding accept_unknown_audiences. Could ACCEPTED_AUDIENCES be removed by just using the client id. So the client is musst be in the array of audienced? If I read the issue again Zitadel should be including the client id of this application as one of them.

[rssc]

Hi,

I don't have a strong opinion on ACCEPT_UNKNOWN_AUDIENCES, because I personally don't need it for my use-case, but I initially added it because there are use-cases where it can be useful when using Zitadel.
I'll try to give an example: when using Zitadel, OpenID clients are grouped by projects, for example like this:

• project1
  • app1 (jitsi-openid)
  • app2 (some other client)

When authenticating with app1, Zitadel will send an audience for project1, an audience for app1, and an audience for app2. Let's assume that app1 is jitsi-openid. To make this work, I would need to set ACCEPTED_AUDIENCES to contain project1 ID and app2 ID, because openidconnect will fail the validation of an ID token if it encounters an audience it does not know about (because that is what the spec says). The ID of app1 is the client ID, and that one is already checked when validating the ID token, with the code here. So I would set "ACCEPTED_AUDIENCES=<project1> <app2>", and everything works.
Now assume I add a new app to project1, say, "app3". The next time I authenticate against jitsi-openid / app1, Zitadel will send the audiences for project1, app1, app2, and app3. app1 is still validated through the client ID, however, my ACCEPTED_AUDIENCES only contains the IDs of project1 and app2, but not the project ID of app3, and the authentication will fail, because it encounters an unknown audience. So this means, that openid-jitsi / app1 will suddenly fail to authenticate even though I have not made any changes to app1, but simply added a new app to project1. That certainly seems to violate the principle of least surprise.
So it is for this use case that I added ACCEPT_UNKNOWN_AUDIENCES, so that adding / removing apps in a Zitadel project does not suddenly break existing openid-jitsi clients. This is not ideal, but because it still validates that the client ID is contained in the audiences, I don't think it is entirely horrible.

Anywho, as I said, that use-case is not relevant for me, so I have removed the ACCEPT_UNKNOWN_AUDIENCES flag from the pull request. And it can be avoided by keeping the openid-jitsi app in its own project with no other clients, but I just didn't want to presume how people use Zitadel.

To your second point: Could ACCEPTED_AUDIENCES be removed by just using the client id.
It cannot. As I outlined above, let's assume I have configured project1 -> app1, then Zitadel will send the ID for project1 and app1 as audiences, and this code will already validate that app1 is contained in the audiences. But unless I also set ACCEPTED_AUDIENCES to the ID of project1, the validation of the ID token will fail, because it encountered an unknown audience (project1). So I must tell openidconnect about the ID for project1 for it to be able to validate all audiences that Zitadel sends.

I hope that helps, but let me know if anything is still unclear (and I'm not saying I understand everything here, it's just that all the testing and debugging I have done indicates to me that this is how it works).

[MarcelCoding]

But unless I also set ACCEPTED_AUDIENCES to the ID of project1, the validation of the ID token will fail, because it encountered an unknown audience (project1). So I must tell openidconnect about the ID for project1 for it to be able to validate all audiences that Zitadel sends.

I meant, that jitsi-openid should accept the token, as long as the client id is contained in the audiences. I would argue it should not be a failure if there are more audiences. Therefore, jitsi-openid should not fail if the project id is also included in the audiences.

[rssc]

But unless I also set ACCEPTED_AUDIENCES to the ID of project1, the validation of the ID token will fail, because it encountered an unknown audience (project1). So I must tell openidconnect about the ID for project1 for it to be able to validate all audiences that Zitadel sends.

I meant, that jitsi-openid should accept the token, as long as the client id is contained in the audiences. I would argue it should not be a failure if there are more audiences. Therefore, jitsi-openid should not fail if the project id is also included in the audiences.

Hm, wouldn't that essentially bring us back to the ACCEPT_UNKNOWN_AUDIENCES that I had originally implemented?

[MarcelCoding]

I've imagined the following behavior, sorry if I did not express that that well. But I would not add any additional configuration options compared to current master branch.

Configured Client ID in jitsi-openid: abc

IDP Audiences	Result
abc	pass
abc, xyz	pass
xyz	fail

[rssc]

I've imagined the following behavior, sorry if I did not express that that well. But I would not add any additional configuration options compared to current master branch.

Configured Client ID in jitsi-openid: abc

IDP Audiences	Result	Comment
abc	pass	<--- this already succeeds
abc, xyz	pass	<--- this is technically against the spec
xyz	fail	<--- this already fails

Passing the authentication if an audience is added in addition that the client does not know about technically seems to go against the spec:

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 aud (audience) Claim MAY contain an array with
more than one element. 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.

Adding "ACCEPTED_AUDIENCES" fulfils the spec, because then the additional audiences are no longer unknown/untrusted.

However, because of quirks with IDPs like Zitadel where making changes unrelated to the jitsi-openid client can break existing, configured clients, I've also added ACCEPT_UNKNOWN_AUDIENCES, which is technically against the spec, but really the only way to avoid getting randomly broken with such IDPs. But because it is technically against the spec, I though it is better to make that choice explicit with a flag.

But it is your repo, so if you prefer the functionality of ACCEPT_UNKNOWN_AUDIENCES but without the actual flag I can do that.

[rssc]

I've updated the pull request, and I think now it behaves the way you prefer. Let me know if that works?

[MarcelCoding]

Thx, sorry for discussion. I would argue that it's fine to go against the spec for this. I can't imagine any way this can be bad or a security issue. (Please correct me, if I am wrong) And considering this is no an enterprise solution for jitsi it should be fine, if it is not 100% spec complaint. This software just bridges the gap, because jitsi does not provide any sensible way on its own. When I've started this, I never knew about all the IDP's that existed and people are opening issues here ;D But this is how it goes. I think the current implementation now offers a compromise without bloating this software, while also supporting all IDP's.