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

Consistent Token Renewal Logic When Configured Policies Not Equivalent #8449

Open
mdgreenfield opened this issue Mar 2, 2020 · 3 comments
Open
Labels
bug Used to indicate a potential bug core/auth core/policy

Comments

@mdgreenfield
Copy link
Contributor

Describe the bug
The following auth plugins renew logic all enforce equivalent policies between the requests token and in the configured auth role.

However, the following auth backends do not enforce this.

Per the docs, "Lease renewal will fail if the token is not renewable, the token has already been revoked, or if the token has already reached its maximum TTL." Admittedly, "if the token is not renewable" is open to interpretation. However, all auth backends should be consistent in how they handle token renewals.

I realize that having the logic to fail a token renewal if the policies differ forces clients to re-authenticate to get a more up-to-date token more quickly, however AFAIK there is no security reason not to allow token renewal just because the policies configured on an auth role has changed.

Additionally, because other auth backends do not enforce this, adding the equivalent policy check to those could potentially break some clients, though, hopefully those clients are smart enough to re-authenticate with Vault.

Expected behavior
For all auth backends, if a token has been issued a set of policies, re-configuration of the corresponding auth role policies should not cause token renewals to fail.

Environment:

  • Vault Server Version (retrieve with vault status): 1.3.1 (but appears to affect master)
  • Vault CLI Version (retrieve with vault version): 1.3.1 (but appears to affect master)
@jefferai
Copy link
Member

jefferai commented Mar 3, 2020

We can't just change this...it's changed behavior. Inconsistency is bad but sudden changes on upgrade are worse. This needs to have a broader discussion.

Also, you're changing it in those PRs the opposite way of how we have been trending -- there's a reason that the newer backends do it differently (maybe, I think -- point is, we need a discussion).

@catsby catsby added bug Used to indicate a potential bug core/policy core/auth labels Mar 4, 2020
@mdgreenfield
Copy link
Contributor Author

Thanks @jefferai. Definitely hear you about sudden changes.

Regarding the PRs, should the Vault maintainer choose to pursue addressing this I'm happy to update the PRs to be consistent with how your team(s) is trending. My hope is to have the renew logic consistent across plugins. From the vault-plugin-auth-example repo and the relatively recent vault-plugin-auth-cf repo it looked like the auth plugins were not enforcing equivalent policy checks on renew.

@remilapeyre
Copy link
Contributor

There is also a bug in the way policies are compared for the auth method that check them during renewal: the comparison is case sensitive but policy names are actually case insensitive.

This means that having a policy name with an uppercase letter will make it impossible to renew a token. I opened a PR for this at: #16484

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Used to indicate a potential bug core/auth core/policy
Projects
None yet
Development

No branches or pull requests

4 participants