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

Update SessionManager implementation for non-renewable tokens #863

Open
ashtonwebster opened this issue Apr 12, 2024 · 8 comments
Open

Update SessionManager implementation for non-renewable tokens #863

ashtonwebster opened this issue Apr 12, 2024 · 8 comments
Labels
type: enhancement A general enhancement

Comments

@ashtonwebster
Copy link

My organization has a vault policy which does not allow renewal of vault tokens. Therefore I am looking into creating a custom implementation of SessionManager which regenerates the token rather than attempting to renew. I have a simple implementation which seems to work:

Component
@Slf4j
public class VaultCustomSessionManager implements SessionManager {

    private Optional<VaultToken> actualToken = Optional.empty();
    private Optional<Long> expirationTime = Optional.empty();
    private final ClientAuthentication clientAuthentication;


    public VaultCustomSessionManager(final ClientAuthentication clientAuthentication) {
        this.clientAuthentication = clientAuthentication;
    }

    @Synchronized
    @Override
    public VaultToken getSessionToken() {
        boolean isExpired = this.expirationTime.map(expiration -> expiration < System.currentTimeMillis()).orElse(false);
        if (this.actualToken.isEmpty() || isExpired) {
            VaultToken newToken = this.clientAuthentication.login();
            if (newToken instanceof LoginToken loginToken) {
                this.expirationTime = Optional.of(System.currentTimeMillis() + loginToken.getLeaseDuration().toMillis());
            } else {
                // default duration to zero - do not refresh
                this.expirationTime = Optional.empty();
            }
            this.actualToken = Optional.of(newToken);
        }
        return this.actualToken.get();
    }
}

Is this something that could be contributed back as an autoconfiguration option for those who cannot renew tokens?

@mp911de
Copy link
Member

mp911de commented Apr 15, 2024

I appreciate your suggestion. What is missing in the existing session manager to meet your requirements?

We would rather want to reuse existing infrastructure (also for the reactive side) to avoid introducing additional unrelated complexity.

@mp911de mp911de added the status: waiting-for-triage An issue we've not yet triaged label Apr 15, 2024
@ashtonwebster
Copy link
Author

What is missing in the existing session manager to meet your requirements?

Our requirement is to allow regeneration of token (not renewal - because our policy does not allow renewal) in the session manager. If I'm understanding correctly, the current LifecycleAwareSessionManager which gets autoconfigured can only renew (if the token can be renewed), but not regenerate the token. To maintain the current behavior, this feature could be implemented behind a configuration option.

I would be happy to assist with contribution if this is something that would be desired.

@mp911de
Copy link
Member

mp911de commented Apr 15, 2024

If I'm understanding correctly, the current LifecycleAwareSessionManager which gets autoconfigured can only renew (if the token can be renewed), but not regenerate the token.

Session managers obtain a new token if the current one is expired. Renewal is attempted until hitting max_ttl. Since I'm not familiar with the actual setup for your tokens, help is appreciated to try out our session managers and report back what is happening vs. what needs to be done to enable your use case.

I could well imagine that if the token isn't renewable, then our renewal scheduler doesn't get activated and therefore, an expired token remains active. With that scenario in mind, we could still enable a background thread to monitor the token. If the token is about to expire, we drop the token and request a new one.

That's similar to what we do now, except that we attempt renewal before we detect whether a token is about to expire.

@ashtonwebster
Copy link
Author

Ah I see, it seems I was misunderstanding. I now see this comment at the top of the LifeCycleSessionManager:

 * If Token renewal runs into a client-side error, it assumes the token was
 * revoked/expired. It discards the token state so the next attempt will lead to another
 * login attempt.

I think the bug here is that the renewal does not get scheduled if the token can't be renewed, therefore the token never gets revoked, and the subsequent request does not regenerate the token/login again. I can confirm this behavior with our application and provide a minimal replicable example, but I think it's pretty clear from the existing session manager code that this is the behavior.

With that scenario in mind, we could still enable a background thread to monitor the token. If the token is about to expire, we drop the token and request a new one.

I think this is a good solution. I'm thinking it could be implemented in a similar way to token renewal - schedule a task to set the current token to Optional.empty() at the lease_duration timeout. Then the next request will regenerate the token. Does that work for you?

@mp911de
Copy link
Member

mp911de commented Apr 15, 2024

That sounds straight-forward. We have a few utilities to calculate a point in time when the token is expected to expire with an additional threshold that can drop the token a tad earlier to avoid token usage in the second it is about to expire.

@mp911de mp911de added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 15, 2024
@mp911de mp911de changed the title Add SessionManager implementation for non-renewable tokens Update SessionManager implementation for non-renewable tokens Apr 15, 2024
@ashtonwebster
Copy link
Author

Ok, that makes sense, I'm familiar with the configs you mentioned. I'll try to make this contribution soon.

@danivichkin
Copy link

@ashtonwebster Hey, do you steel work on the problem? I can make this contribution by myself

@ashtonwebster
Copy link
Author

I don't think I will have time to work on this, so feel free to pick it up. We found some workaround on our end with another sessionamanger implementation.
One other issue we ran into, probably not fixable here, but the AWS credentials we were using also expired and we had to renew those as well, which required some reinitialization of the beans...but probably out of scope for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants