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

Re-read JWT file for every authentication #491

Merged
merged 3 commits into from
Nov 25, 2024

Conversation

ramondeklein
Copy link
Contributor

@ramondeklein ramondeklein commented Nov 25, 2024

The JWT for authentication to Hashicorp Keyvault is only read during initialization or when SIGHUP is received. This results in issues when the SA token expires and the Keyvault client token needs to be refreshed. This PR will re-read the JWT file for every authentication.

Related to https://subnet.min.io/issues/12909.

b, err := os.ReadFile(y.KeyStore.Vault.Kubernetes.JWT.Value)
_, err := os.ReadFile(y.KeyStore.Vault.Kubernetes.JWT.Value)
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 kept this check to ensure that the file can be read during start-up for verification only.

@klauspost
Copy link
Contributor

How often is this expected to be called worst case?

@aead
Copy link
Member

aead commented Nov 25, 2024

How often is this expected to be called worst case?

On every re-autentication to Vault, when the session token from Vault expires. Hence, not often. E.g. once an hour @klauspost

@ramondeklein
Copy link
Contributor Author

@klauspost Authentication should be done only once and each time the token expires, which should be once every few minutes if you really have short TTLs. The only way this can go crazy if it can't authenticate. But calling the authentication endpoint would be much more expensive compared to reading the file.

hour, min, sec := info.CreatedAt.Clock()
hour, minute, sec := info.CreatedAt.Clock()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was needed to fix the lint complaint that min redefinitions the built-in function min.

Copy link
Contributor

@klauspost klauspost left a comment

Choose a reason for hiding this comment

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

ok, should be fine then. I see the file name is provided by the config, so AFAICT it should be safe.

@ramondeklein
Copy link
Contributor Author

@aead I "fixed" a unit test, but it doesn't really read the JWT from the file anymore. If we do want to do that, then we need to create a mocked Vault client. I could do that, but not sure if I should...

@ramondeklein
Copy link
Contributor Author

@aead Feel free to merge (I can't do it).

@aead aead merged commit 2e4e7be into minio:master Nov 25, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants