-
Notifications
You must be signed in to change notification settings - Fork 21
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
Refresh Token Concurrency issue #225
Comments
@elestedt Thanks for the report. I'll try to reproduce this on our end. |
Add read/write mutex for refreshing access token
I'm not sure if this new version is the issue here, but after upgrading to this version I'm seeing 504 errors that we didn't have before - and the requests never reach Artifactory
The timeout is maybe 30s. It looks to be a vault internal error, and not something we ever got on 1.8.1. |
@elestedt I think it's very unlikely due to the changes in 1.8.2. I added write mutex around the operations of updating the Vault store to ensure concurrency is correct. Nothing else was changed. The error is from Nginx so I assume this is from Artifactory? |
Actually the artifactory logs show nothing at all - the request never reaches Artifactory at all as far as I can see. |
@elestedt In that case you're probably right that this is Vault server related. Perhaps there are clues in the verbose/trace logs. |
@alexhung I reconfigured the entire vault intance and things are still not working properly .but now at least I can use the plugin again. So, I configure the plugin for a user with a refreshable token. An hour later, when that token has expired and only the refreshable token is valid it fails to renew it. Logs:
Plugin config:
Command used to get token:
Seems pretty clear that it fails to refresh the expired refreshable token, why I cannot say. |
I did manage to find these request in our Artifactory logs - just a single request to the token endpoint and it's rejected with a 401. I tried to capture the traffic - but didn't manage (encrypted) |
After upgrading prod to 1.8.2 - more and more users are starting to report Context Deadline Exceeded responses from vault itself. It seems to be a deadlock in the mutex that was added. There is no logs in the plugin when this happens and it's impossible to do anything against the plugin... The rest of vault works fine. |
@elestedt From my testing, I ran into this issue (that wasn't present when I implemented the token refresh) where soon after the token expired, the requests to the Artifactory API still works (i.e. no token expired error) but fails with requests to Access API. The delay (of token expiration between Access and Artifactory services) lasted about 1 min, so using the Artifactory version API passes while the create token API with Access fails. This accounts for the behavior you reported so I'm switching from the version API to role API (which is from Access service). Hopefully the delay will not be presented and thus more reliable. Since the issue is unrelated to the additional mutex I added, they will be removed along with the API switch. |
Switched to use Access API for checking expired token
Describe the bug
When we have multiple clients using the same account (read: pipelines in a CI environment) and it happens to try to refresh the token at the same time this seems to cause the refreshable token to become invalid and future refreshes fails
To Reproduce
Steps to reproduce the behavior:
After these steps there is a chance that the stored token has been "corrupted" and can no longer be used.
Artifactory version 7.98.7
Plugin version 1.8.1
Vault version 1.17.3+ent
Expected behavior
Refresh events (when the refresh token is used to renew the generator token) should not cause the refresh token to become invalidated.
Additional context
I suspect this happens due to concurrency issues and the fact that a refresh token is invalid once it's been used (i.e. can only be used once).
The text was updated successfully, but these errors were encountered: