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

First pass at JWT auto-deletion flag #11969

Merged
merged 7 commits into from Jul 25, 2022

Conversation

tdsacilowski
Copy link
Contributor

Fixes #11968

Still need to write tests.

@tdsacilowski tdsacilowski requested a review from a team June 30, 2021 21:45
@hashicorp-cla
Copy link

hashicorp-cla commented Jun 30, 2021

CLA assistant check
All committers have signed the CLA.

@calvn
Copy link
Member

calvn commented Jun 30, 2021

Are there potential (re-)authentication issues that could arise if we don't delete a stale JWT?

@@ -17,3 +17,7 @@ JWT to perform a reauthentication.
- `path` `(string: required)` - The path to the JWT file

- `role` `(string: required)` - The role to authenticate against on Vault

- `remove_jwt_after_reading` `(bool: optional, defaults to true)` -
Copy link
Member

Choose a reason for hiding this comment

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

Can we flip this around and have it be keep_after_reading or something along those lines? This way the default value is also the zero value (false).

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 thought about that but I thought it more prudent to retain the current default behavior and opt-out if needed. I'd assume that in most cases, the default behavior is more appropriate. This change is mostly to allow for using Kubernetes bound service account tokens with JWT auth. In this use case, Kubernetes automatically rotates the JWT based on a configurable TTL.

Copy link
Member

@calvn calvn Jun 30, 2021

Choose a reason for hiding this comment

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

keep_after_reading with it being defaulted false should still retain the current behavior though. Note that we're flipping the naming and also the default so keeping the file would still be an opt-in. We'd only not delete the file if this gets set to true explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok, I see what you mean. I was following the same model that the AppRole auto-auth was using but I can change this around if it makes more sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think keep_jwt_after_reading works, but I am not convinced that it needs to be this verbose. I think any of : skip_jwt_cleanup, no_jwt_cleanup, preseve_jwt could probably work well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually like remove_jwt_after_reading for its consistency with AppRole's option: https://www.vaultproject.io/docs/agent/autoauth/methods/approle#remove_secret_id_file_after_reading

IMO the bar to break from this pattern needs to be higher than making it terser. If it wasn't for the prior art, I would 100% agree with both the flip + naming comments above, but they're minor nits and they're secondary to preventing confusion for consumers.

@tdsacilowski
Copy link
Contributor Author

Are there potential (re-)authentication issues that could arise if we don't delete a stale JWT?

I wouldn't think so. JWTs should have an expiration set so if re-auth is attempted with an expired JWT auth should fail. In this scenario, the underlying system that is responsible for delivering an updated JWT (e.g. Kubernetes) should have provided a newer JWT. But I may be missing some of the finer points here.

@calvn
Copy link
Member

calvn commented Jun 30, 2021

It looks like ingressToken() relies on the absence on the file to short-circuit from the function, so on non-kubernetes workflows in which someone opts in for this I could see Agent doing the opposite -- reading the file every 500ms. We do end up comparing the value of this token with the existing one so it's mostly benign, but it does result in more filesystem reads.

@arianvp
Copy link

arianvp commented Oct 13, 2021

As an alternative of using the absence as the edge trigger; we could use inotify to see when the token file is modified. That sounds like the better design here. This would be really cool to have as it makes a dedicated kubernetes auth method redundant! Since Kubernetes 1.21 k8s even has a /.well-known/openid-configuration endpoint so you can use the jwt auth method without any complicated configuration. Would be really cool if this could land

@benashz
Copy link
Contributor

benashz commented Oct 15, 2021

As an alternative of using the absence as the edge trigger; we could use inotify to see when the token file is modified. That sounds like the better design here. This would be really cool to have as it makes a dedicated kubernetes auth method redundant! Since Kubernetes 1.21 k8s even has a /.well-known/openid-configuration endpoint so you can use the jwt auth method without any complicated configuration. Would be really cool if this could land

That's a very interesting idea. I don't really see kubernetes being made redundant, rather it could be extended to support openid-connect-tokens as alternative to relying on the k8s TokenReview API.

@arianvp
Copy link

arianvp commented Oct 15, 2021

Just to avoid confusion. Note that the document you're linking to is the wrong side of the coin. That document is about kubernetes utilising an external openid connect provider to allow externals to authenticate to K8s.

We're talking about https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/#service-account-issuer-discovery instead.

Which is the opposite of that. Allow serviceaccounts to connect to external services like vault by registering the Kubernetes apiserver as an OpenID connect provider in vault

@arianvp
Copy link

arianvp commented Oct 18, 2021

@benashz here is the full example using the jwt auth method including an explanation why I'd like a (variant) of this PR merged.

https://github.com/arianvp/vault-k8s-jwt-auth-demo

@aelbarkani
Copy link

aelbarkani commented Jun 17, 2022

@arianvp @tdsacilowski since this PR was not merged, did you find a workaround ?

Copy link
Contributor

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

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

Taking another look as there has been some renewed interest in this. @tdsacilowski I'm happy to take this on if you'd prefer not to pick this up again.

@@ -17,3 +17,7 @@ JWT to perform a reauthentication.
- `path` `(string: required)` - The path to the JWT file

- `role` `(string: required)` - The role to authenticate against on Vault

- `remove_jwt_after_reading` `(bool: optional, defaults to true)` -
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually like remove_jwt_after_reading for its consistency with AppRole's option: https://www.vaultproject.io/docs/agent/autoauth/methods/approle#remove_secret_id_file_after_reading

IMO the bar to break from this pattern needs to be higher than making it terser. If it wasn't for the prior art, I would 100% agree with both the flip + naming comments above, but they're minor nits and they're secondary to preventing confusion for consumers.

command/agent/auth/jwt/jwt.go Outdated Show resolved Hide resolved
* Unit test to test default behaviour, and config parsing for delete_jwt_after_reading option
* Slow down read period of token when we're not deleting it to 1 minute. This is fast enough
  for the shortest TTL possible in Kubernetes (10m), which seems like a sensible default. We
  may need to make this period configurable in the future though.
* Remove usage of the deprecated ioutil library
@tomhjp tomhjp requested review from benashz and calvn July 21, 2022 15:40
@tomhjp tomhjp requested a review from tvoran July 22, 2022 15:47
Copy link
Member

@tvoran tvoran left a comment

Choose a reason for hiding this comment

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

I'm ambivalent about the option name.

command/agent/auth/jwt/jwt.go Outdated Show resolved Hide resolved
@tomhjp tomhjp enabled auto-merge (squash) July 25, 2022 13:33
@tomhjp tomhjp merged commit dac99be into main Jul 25, 2022
@tomhjp tomhjp deleted the Enable-JWT-auto-deletion-to-be-optional branch September 12, 2022 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vault Agent: make auto-delete of JWT file a configurable option
9 participants