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

Support refresh of limited use tokens (num_use > 0) #360

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kdubb
Copy link
Contributor

@kdubb kdubb commented Jan 23, 2025

Fixes #338

While enhancing the test for caching tokens I realized supporting limited use tokens should be fairly straightforward.

@vsevel What do you think? The only tricky thing is that we copy token values (e.g., for caching) so I used an AtomicReference as a shared reference count. Other than that, I added getClientTokenForUsage to decrement the count when it's being used for authorization and overloaded the expiration tests to take into account it's max # of uses.

I added a test to the caching token provider tests suite that tests limited use tokens and it seems to perform as expected.

It does not attempt to extend limited use tokens that are over their use limit because from my investigation it doesn't seem that's allowed.

@alexjaravete Can you build this this locally and see if this "just works" for you?

@kdubb kdubb requested a review from a team as a code owner January 23, 2025 20:30
@kdubb kdubb force-pushed the feature/limited-use-tokens branch from 0f40ded to 4d3cb81 Compare January 23, 2025 20:38
@kdubb kdubb requested a review from vsevel January 23, 2025 21:00
@vsevel
Copy link
Contributor

vsevel commented Jan 27, 2025

ideally we would want decrementing the token close to where it is actually being used, so in the real VaultRequestExecutor, to make sure it is actually called exactly once. VaultClient is an executor, but so are JDKVaultHttpClient, VertxVaultHttpClient. are we sure we always go through VaultClient first, then to one of the concrete classes? it seems so.
for my own comprehension what are you supposed to do when you exhausted the number of uses? you can still renew it?
so when we call getClientTokenForUsage and it fails with a VaultException do we auto renew it?
I see the overload on isExpired(), so the sequence what should happen is that when we reaches 0, we should renew the token. correct?
I suppose the only pitfall would be if there was 1 use left, with 2 concurrent calls. they would both decrement the counter, after they have called isExpired(), then fail when making the real call. is that correct?
if true, then this approach would not be reliable in highly concurrent applications.
a solution then would be that once isExpired() has returned false, we can't be rejected because of the num uses.
but that might not be easily implementable?

@kdubb
Copy link
Contributor Author

kdubb commented Jan 28, 2025

VaultClient is an executor, but so are JDKVaultHttpClient, VertxVaultHttpClient. are we sure we always go through VaultClient first, then to one of the concrete classes? it seems so.

Yes all roads go through VaultClient, it basically wraps another executor that is the client implementation. The only calls to getClientToken were the one in VaultClient and one in VaultCachingTokenProvider. I added getClientTokenForUsage only to be very clear about what it's doing.

I suppose the only pitfall would be if there was 1 use left, with 2 concurrent calls. they would both decrement the counter, after they have called isExpired(), then fail when making the real call. is that correct?

This is a great point. I'll look into solving this problem and testing it.

@alexjaravete
Copy link

@kdubb Thanks for taking a look at it and creating a fix. I will try to run it locally in the next few days and let you know if it works.

@alexjaravete
Copy link

I did a small test in our system with the new fix, and it seems to work as expected. The potential pitfall mentioned by @vsevel could still be a problem, but I was not able to test this in my limited test run.
And thank you @kdubb for tackling this issue.

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.

No support for token_num_uses in auth token
3 participants