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

add explanation for oidc refresh token issue #1808

Conversation

soer3n
Copy link
Contributor

@soer3n soer3n commented Jan 8, 2025

This pr adds information about a problem regarding oidc authentication on kkp usercluster observed in kubermatic/kubermatic#13573 to known issues.

@kubermatic-bot kubermatic-bot added dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 8, 2025
* adding explanation on the known issue
* adding a way to configure dex to handle multiple refresh tokens per user/client
** adding explanation on security considerations

Signed-off-by: soer3n <[email protected]>
@soer3n soer3n force-pushed the add-oidc-refresh-token-info-to-known-issues branch from 7291351 to 088d438 Compare January 9, 2025 11:00
Comment on lines 26 to 27
You can either change this in dex helm values by setting `userIDKey` to `jti` and `userNameKey` for example to `email` in the config section of a connector or you could configure an other oidc provider which supports multiple refresh tokens per user-client pair like keycloak does by default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please provide an example config in yaml format as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spliited the solution section into dex and external provider and added an example connector snippet for dex.

Comment on lines 21 to 22
With the default helm chart values for dex it is only possible to have one refresh token per user/client pair for security reasons. The refresh token has by default also no expiration set. This is useful to stay logged in over a longer time because the id_token can be refreshed unless the refresh token is invalidated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the default helm chart values for dex

It's a limitation in dex, not the helm chart of dex so i'd avoid mentioning helm chart here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed wording to dex configuration instead of helm values.


### Root Cause

With the default helm chart values for dex it is only possible to have one refresh token per user/client pair for security reasons. The refresh token has by default also no expiration set. This is useful to stay logged in over a longer time because the id_token can be refreshed unless the refresh token is invalidated.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add link to the upstream issues for dex

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.


### Problem

One example would be to download a kubeconfig of one cluster and then of another with the same user. You should only be able to use the first kubeconfig until the id_token expires because the refresh token was already invalidated by the download of the second one. This happens when a new one was requested.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wording needs to be improved a bit, i'd also suggest first mentioning the issue and then giving an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the example to the root cause section and generalized the explanation in the problem section.


You can either change this in dex helm values by setting `userIDKey` to `jti` and `userNameKey` for example to `email` in the config section of a connector or you could configure an other oidc provider which supports multiple refresh tokens per user-client pair like keycloak does by default.

For dex this has some implications. With this configuration a token is generated for each user session. The number of objects stored in kubernetes regarding refresh tokens has no limit anymore. The principle that one refresh belongs to one user/client pair is a security consideration which would be ignored in that case. The only way to revoke a refresh token is then to do it via grpc api which is not exposed by default or by manually deleting the related refreshtoken resource in the kubernetes cluster.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this can have it's own heading since this section(known limitations or similar) is quite important; this needs more visibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the hint. I added another section for it.

@kubermatic-bot kubermatic-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 15, 2025
@soer3n soer3n force-pushed the add-oidc-refresh-token-info-to-known-issues branch from 6ca77ba to e3779c2 Compare January 15, 2025 08:24
* add an example yaml for connector configuration
* move implications of the dex solution to an own heading
* reword problem description
* move example explanation to root cause section

Signed-off-by: soer3n <[email protected]>
@soer3n soer3n force-pushed the add-oidc-refresh-token-info-to-known-issues branch from e3779c2 to ae7b671 Compare January 15, 2025 08:28
Copy link
Member

@ahmedwaleedmalik ahmedwaleedmalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

Thanks for the great work.

@kubermatic-bot kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2025
@kubermatic-bot
Copy link
Contributor

LGTM label has been added.

Git tree hash: e82c9de0fc02c1969d49a93071be2869e85ff5f1

@kubermatic-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmedwaleedmalik

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubermatic-bot kubermatic-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 15, 2025
@kubermatic-bot kubermatic-bot merged commit 0ab1a09 into kubermatic:main Jan 15, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants