Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OIDC: backchannel logout default configuration only accepts a single client id #6149

Closed
rhafer opened this issue Apr 26, 2023 · 3 comments
Closed
Labels

Comments

@rhafer
Copy link
Contributor

rhafer commented Apr 26, 2023

Describe the bug

Our code verifying the backchannel logout token, tries to verify the aud claim of the token by default. It is possible to configure a single value that is allowed to appear in that claim (defaulting to web).
I think we're doing something wrong here. Keycloak for example by default sets the aud claim to the value of the clientid for which the session was created. So with the current approach only sessions initiated one specific oidc client can be logged out (web by default). For sessions created by any other clients (i.e. the desktop of mobil apps) the validation of the logout token will fail because the aud claim does not match the expected value.

As a workaround we can disable the validation of the aud claim for now with PROXY_OIDC_SKIP_CLIENT_ID_CHECK=true

I am still not sure what the correct validation of the aud claim would look like in ocis. We simply don't know all the valid client ids that could appear in logout tokens. (At least when DCR is enabled in the idp).

@rhafer
Copy link
Contributor Author

rhafer commented Apr 26, 2023

cc @micbar @wkloucek

rhafer added a commit to rhafer/ocis that referenced this issue Apr 26, 2023
The "aud" claim of the logout token is supposed to contain the client-id
of the client for which the token was issued. Our current implementation of
validating that claim is somewhat broken. We only allow to configure a single
value for the allowed client id. But we have different client-ids
accessing oCIS.

This completely removes the current validation of the `aud` claim until
we come up with a working solution. As we currently require a session id
to be present in the logout token the risk not validating the `aud`
claim is pretty low.

Related: owncloud#6149
butonic pushed a commit that referenced this issue Apr 27, 2023
The "aud" claim of the logout token is supposed to contain the client-id
of the client for which the token was issued. Our current implementation of
validating that claim is somewhat broken. We only allow to configure a single
value for the allowed client id. But we have different client-ids
accessing oCIS.

This completely removes the current validation of the `aud` claim until
we come up with a working solution. As we currently require a session id
to be present in the logout token the risk not validating the `aud`
claim is pretty low.

Related: #6149
ownclouders pushed a commit that referenced this issue Apr 27, 2023
The "aud" claim of the logout token is supposed to contain the client-id
of the client for which the token was issued. Our current implementation of
validating that claim is somewhat broken. We only allow to configure a single
value for the allowed client id. But we have different client-ids
accessing oCIS.

This completely removes the current validation of the `aud` claim until
we come up with a working solution. As we currently require a session id
to be present in the logout token the risk not validating the `aud`
claim is pretty low.

Related: #6149
@micbar
Copy link
Contributor

micbar commented Apr 27, 2023

Thanks. I see the PR was merged. IMO that makes sense for the time being.

@micbar
Copy link
Contributor

micbar commented May 10, 2023

Fixed.

@micbar micbar closed this as completed May 10, 2023
@github-project-automation github-project-automation bot moved this from Qualification to Done in Infinite Scale Team Board May 10, 2023
fschade pushed a commit that referenced this issue Jul 10, 2023
The "aud" claim of the logout token is supposed to contain the client-id
of the client for which the token was issued. Our current implementation of
validating that claim is somewhat broken. We only allow to configure a single
value for the allowed client id. But we have different client-ids
accessing oCIS.

This completely removes the current validation of the `aud` claim until
we come up with a working solution. As we currently require a session id
to be present in the logout token the risk not validating the `aud`
claim is pretty low.

Related: #6149
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

No branches or pull requests

2 participants