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
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
63 changes: 38 additions & 25 deletions command/agent/auth/jwt/jwt.go
Expand Up @@ -15,21 +15,23 @@ import (
hclog "github.com/hashicorp/go-hclog"
"github.com/hashicorp/vault/api"
"github.com/hashicorp/vault/command/agent/auth"
"github.com/hashicorp/vault/sdk/helper/parseutil"
)

type jwtMethod struct {
logger hclog.Logger
path string
mountPath string
role string
credsFound chan struct{}
watchCh chan string
stopCh chan struct{}
doneCh chan struct{}
credSuccessGate chan struct{}
ticker *time.Ticker
once *sync.Once
latestToken *atomic.Value
logger hclog.Logger
path string
mountPath string
role string
removeJWTAfterReading bool
credsFound chan struct{}
watchCh chan string
stopCh chan struct{}
doneCh chan struct{}
credSuccessGate chan struct{}
ticker *time.Ticker
once *sync.Once
latestToken *atomic.Value
}

// NewJWTAuthMethod returns an implementation of Agent's auth.AuthMethod
Expand All @@ -43,15 +45,16 @@ func NewJWTAuthMethod(conf *auth.AuthConfig) (auth.AuthMethod, error) {
}

j := &jwtMethod{
logger: conf.Logger,
mountPath: conf.MountPath,
credsFound: make(chan struct{}),
watchCh: make(chan string),
stopCh: make(chan struct{}),
doneCh: make(chan struct{}),
credSuccessGate: make(chan struct{}),
once: new(sync.Once),
latestToken: new(atomic.Value),
logger: conf.Logger,
mountPath: conf.MountPath,
removeJWTAfterReading: true,
credsFound: make(chan struct{}),
watchCh: make(chan string),
stopCh: make(chan struct{}),
doneCh: make(chan struct{}),
credSuccessGate: make(chan struct{}),
once: new(sync.Once),
latestToken: new(atomic.Value),
}
j.latestToken.Store("")

Expand All @@ -73,6 +76,15 @@ func NewJWTAuthMethod(conf *auth.AuthConfig) (auth.AuthMethod, error) {
return nil, errors.New("could not convert 'role' config value to string")
}

removeJWTAfterReadingRaw, ok := conf.Config["remove_jwt_after_reading"]
if ok {
removeJWTAfterReading, err := parseutil.ParseBool(removeJWTAfterReadingRaw)
if err != nil {
return nil, fmt.Errorf("error parsing 'remove_jwt_after_reading' value: %w", err)
}
j.removeJWTAfterReading = removeJWTAfterReading
}
tomhjp marked this conversation as resolved.
Show resolved Hide resolved

switch {
case j.path == "":
return nil, errors.New("'path' value is empty")
Expand Down Expand Up @@ -145,6 +157,7 @@ func (j *jwtMethod) runWatcher() {
j.ingressToken()
newToken := j.latestToken.Load().(string)
if newToken != latestToken {
j.logger.Debug("new jwt file found")
j.credsFound <- struct{}{}
}
}
Expand All @@ -161,8 +174,6 @@ func (j *jwtMethod) ingressToken() {
return
}

j.logger.Debug("new jwt file found")

// Check that the path refers to a file.
// If it's a symlink, it could still be a symlink to a directory,
// but ioutil.ReadFile below will return a descriptive error.
Expand Down Expand Up @@ -190,7 +201,9 @@ func (j *jwtMethod) ingressToken() {
j.latestToken.Store(string(token))
}

if err := os.Remove(j.path); err != nil {
j.logger.Error("error removing jwt file", "error", err)
if j.removeJWTAfterReading {
if err := os.Remove(j.path); err != nil {
j.logger.Error("error removing jwt file", "error", err)
}
}
}
4 changes: 4 additions & 0 deletions website/content/docs/agent/autoauth/methods/jwt.mdx
Expand Up @@ -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.

This can be set to `false` to disable the default behavior of removing the
JWT after it's been read.