From 4572a9f9a1bd4b12c9007d6edae186bd36421f7c Mon Sep 17 00:00:00 2001 From: Teddy Sacilowski Date: Wed, 30 Jun 2021 14:23:32 -0700 Subject: [PATCH 1/6] First pass at JWT auto-deletion flag --- command/agent/auth/jwt/jwt.go | 63 +++++++++++-------- .../docs/agent/autoauth/methods/jwt.mdx | 4 ++ 2 files changed, 42 insertions(+), 25 deletions(-) diff --git a/command/agent/auth/jwt/jwt.go b/command/agent/auth/jwt/jwt.go index 0c97bee905ec3..78130038735ab 100644 --- a/command/agent/auth/jwt/jwt.go +++ b/command/agent/auth/jwt/jwt.go @@ -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 @@ -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("") @@ -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 + } + switch { case j.path == "": return nil, errors.New("'path' value is empty") @@ -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{}{} } } @@ -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. @@ -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) + } } } diff --git a/website/content/docs/agent/autoauth/methods/jwt.mdx b/website/content/docs/agent/autoauth/methods/jwt.mdx index 41c0bdd0cbe61..4dbcfaf8c2d3f 100644 --- a/website/content/docs/agent/autoauth/methods/jwt.mdx +++ b/website/content/docs/agent/autoauth/methods/jwt.mdx @@ -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)` - + This can be set to `false` to disable the default behavior of removing the + JWT after it's been read. From bf94d2113c53048269f52457d495301ca2e9d612 Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Thu, 21 Jul 2022 16:31:13 +0100 Subject: [PATCH 2/6] Unit test for delete option + slow down read period * 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 --- command/agent/auth/jwt/jwt.go | 20 ++++++--- command/agent/auth/jwt/jwt_test.go | 67 ++++++++++++++++++++++++++++-- 2 files changed, 77 insertions(+), 10 deletions(-) diff --git a/command/agent/auth/jwt/jwt.go b/command/agent/auth/jwt/jwt.go index 78130038735ab..8c83ad17f9c9b 100644 --- a/command/agent/auth/jwt/jwt.go +++ b/command/agent/auth/jwt/jwt.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "io/fs" - "io/ioutil" "net/http" "os" "sync" @@ -76,13 +75,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 { + if removeJWTAfterReadingRaw, ok := conf.Config["remove_jwt_after_reading"]; 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 + } else { + // Default to true for maximum backwards compatibility. + j.removeJWTAfterReading = true } switch { @@ -92,7 +93,14 @@ func NewJWTAuthMethod(conf *auth.AuthConfig) (auth.AuthMethod, error) { return nil, errors.New("'role' value is empty") } - j.ticker = time.NewTicker(500 * time.Millisecond) + // If we don't delete the JWT after reading, use a slower reload period, + // otherwise we would re-read the whole file every 500ms, instead of just + // doing a stat on the file every 500ms. + readPeriod := 1 * time.Minute + if j.removeJWTAfterReading { + readPeriod = 500 * time.Millisecond + } + j.ticker = time.NewTicker(readPeriod) go j.runWatcher() @@ -176,7 +184,7 @@ func (j *jwtMethod) ingressToken() { // 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. + // but os.ReadFile below will return a descriptive error. switch mode := fi.Mode(); { case mode.IsRegular(): // regular file @@ -187,7 +195,7 @@ func (j *jwtMethod) ingressToken() { return } - token, err := ioutil.ReadFile(j.path) + token, err := os.ReadFile(j.path) if err != nil { j.logger.Error("failed to read jwt file", "error", err) return diff --git a/command/agent/auth/jwt/jwt_test.go b/command/agent/auth/jwt/jwt_test.go index ef33bfb7e720b..5582743366dc7 100644 --- a/command/agent/auth/jwt/jwt_test.go +++ b/command/agent/auth/jwt/jwt_test.go @@ -2,7 +2,6 @@ package jwt import ( "bytes" - "io/ioutil" "os" "path" "strings" @@ -10,6 +9,7 @@ import ( "testing" "github.com/hashicorp/go-hclog" + "github.com/hashicorp/vault/command/agent/auth" ) func TestIngressToken(t *testing.T) { @@ -21,18 +21,18 @@ func TestIngressToken(t *testing.T) { symlinked = "symlinked" ) - rootDir, err := ioutil.TempDir("", "vault-agent-jwt-auth-test") + rootDir, err := os.MkdirTemp("", "vault-agent-jwt-auth-test") if err != nil { t.Fatalf("failed to create temp dir: %s", err) } defer os.RemoveAll(rootDir) setupTestDir := func() string { - testDir, err := ioutil.TempDir(rootDir, "") + testDir, err := os.MkdirTemp(rootDir, "") if err != nil { t.Fatal(err) } - err = ioutil.WriteFile(path.Join(testDir, file), []byte("test"), 0644) + err = os.WriteFile(path.Join(testDir, file), []byte("test"), 0644) if err != nil { t.Fatal(err) } @@ -106,3 +106,62 @@ func TestIngressToken(t *testing.T) { } } } + +func TestDeleteAfterReading(t *testing.T) { + for _, tc := range map[string]struct { + configValue string + shouldDelete bool + }{ + "default": { + "", + true, + }, + "explicit true": { + "true", + true, + }, + "false": { + "false", + false, + }, + } { + rootDir, err := os.MkdirTemp("", "vault-agent-jwt-auth-test") + if err != nil { + t.Fatalf("failed to create temp dir: %s", err) + } + defer os.RemoveAll(rootDir) + tokenPath := path.Join(rootDir, "token") + err = os.WriteFile(tokenPath, []byte("test"), 0644) + if err != nil { + t.Fatal(err) + } + + config := &auth.AuthConfig{ + Config: map[string]interface{}{ + "path": tokenPath, + "role": "unusedrole", + }, + Logger: hclog.Default(), + } + if tc.configValue != "" { + config.Config["remove_jwt_after_reading"] = tc.configValue + } + + jwtAuth, err := NewJWTAuthMethod(config) + if err != nil { + t.Fatal(err) + } + + jwtAuth.(*jwtMethod).ingressToken() + + if _, err := os.Lstat(tokenPath); tc.shouldDelete { + if err == nil || !os.IsNotExist(err) { + t.Fatal(err) + } + } else { + if err != nil { + t.Fatal(err) + } + } + } +} From 033c1cfbce809fa37a35dc6defa984ab8cf2db60 Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Thu, 21 Jul 2022 16:39:48 +0100 Subject: [PATCH 3/6] Add changelog note --- changelog/11969.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/11969.txt diff --git a/changelog/11969.txt b/changelog/11969.txt new file mode 100644 index 0000000000000..5128ac65bd2ba --- /dev/null +++ b/changelog/11969.txt @@ -0,0 +1,3 @@ +```release-note:improvement +agent: JWT auto auth now supports a `delete_jwt_after_reading` which defaults to true. +``` \ No newline at end of file From c713a6bd497b80dff1d8f3419031176306e03d45 Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Thu, 21 Jul 2022 16:44:44 +0100 Subject: [PATCH 4/6] Changelog typo --- changelog/11969.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog/11969.txt b/changelog/11969.txt index 5128ac65bd2ba..47718be291e6a 100644 --- a/changelog/11969.txt +++ b/changelog/11969.txt @@ -1,3 +1,3 @@ ```release-note:improvement -agent: JWT auto auth now supports a `delete_jwt_after_reading` which defaults to true. +agent: JWT auto auth now supports a `delete_jwt_after_reading` config option which defaults to true. ``` \ No newline at end of file From d07e9756c1161bf96bfe77eac4333df0989912fc Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Thu, 21 Jul 2022 16:48:41 +0100 Subject: [PATCH 5/6] Another changelog typo --- changelog/11969.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog/11969.txt b/changelog/11969.txt index 47718be291e6a..668093565d33c 100644 --- a/changelog/11969.txt +++ b/changelog/11969.txt @@ -1,3 +1,3 @@ ```release-note:improvement -agent: JWT auto auth now supports a `delete_jwt_after_reading` config option which defaults to true. +agent: JWT auto auth now supports a `remove_jwt_after_reading` config option which defaults to true. ``` \ No newline at end of file From 174eaf81fae7431a2a64aa10d6746af752f58c5f Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Mon, 25 Jul 2022 14:27:08 +0100 Subject: [PATCH 6/6] Remove unnecessary second defaulting of value --- command/agent/auth/jwt/jwt.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/command/agent/auth/jwt/jwt.go b/command/agent/auth/jwt/jwt.go index 8c83ad17f9c9b..8f088eb199e5e 100644 --- a/command/agent/auth/jwt/jwt.go +++ b/command/agent/auth/jwt/jwt.go @@ -81,9 +81,6 @@ func NewJWTAuthMethod(conf *auth.AuthConfig) (auth.AuthMethod, error) { return nil, fmt.Errorf("error parsing 'remove_jwt_after_reading' value: %w", err) } j.removeJWTAfterReading = removeJWTAfterReading - } else { - // Default to true for maximum backwards compatibility. - j.removeJWTAfterReading = true } switch {