From d6d0a526cfc194b9083e1117cde217aa20a0669a Mon Sep 17 00:00:00 2001 From: Peter Schultz Date: Thu, 29 Jun 2017 21:38:59 +0200 Subject: [PATCH] Issue #274: Avoid premature vault token renewals Don't attempt Vault token renewals if the token isn't renewable. If it is, don't renew the token with each refresh; only do so if the token would expire shortly. Increase the token's lifetime by its original TTL. Fixes #274 --- cert/source_test.go | 36 +++++------ cert/vault_source.go | 142 ++++++++++++++++++++++++++++++++++--------- 2 files changed, 131 insertions(+), 47 deletions(-) diff --git a/cert/source_test.go b/cert/source_test.go index 88f76f41c..316bde34d 100644 --- a/cert/source_test.go +++ b/cert/source_test.go @@ -140,6 +140,8 @@ func TestNewSource(t *testing.T) { desc: "vault", cfg: certsource("vault"), src: &VaultSource{ + Addr: os.Getenv("VAULT_ADDR"), + vaultToken: os.Getenv("VAULT_TOKEN"), CertPath: "cert", ClientCAPath: "clientca", CAUpgradeCN: "upgcn", @@ -391,28 +393,28 @@ func TestVaultSource(t *testing.T) { // run tests tests := []struct { - desc string - wrapTTL string - req *vaultapi.TokenCreateRequest - dropErr bool + desc string + wrapTTL string + req *vaultapi.TokenCreateRequest + dropWarn bool }{ { desc: "renewable token", req: &vaultapi.TokenCreateRequest{Lease: "1m", TTL: "1m", Policies: []string{"fabio"}}, }, { - desc: "non-renewable token", - req: &vaultapi.TokenCreateRequest{Lease: "1m", TTL: "1m", Renewable: newBool(false), Policies: []string{"fabio"}}, - dropErr: true, + desc: "non-renewable token", + req: &vaultapi.TokenCreateRequest{Lease: "1m", TTL: "1m", Renewable: newBool(false), Policies: []string{"fabio"}}, + dropWarn: true, }, { desc: "renewable orphan token", req: &vaultapi.TokenCreateRequest{Lease: "1m", TTL: "1m", NoParent: true, Policies: []string{"fabio"}}, }, { - desc: "non-renewable orphan token", - req: &vaultapi.TokenCreateRequest{Lease: "1m", TTL: "1m", NoParent: true, Renewable: newBool(false), Policies: []string{"fabio"}}, - dropErr: true, + desc: "non-renewable orphan token", + req: &vaultapi.TokenCreateRequest{Lease: "1m", TTL: "1m", NoParent: true, Renewable: newBool(false), Policies: []string{"fabio"}}, + dropWarn: true, }, { desc: "renewable wrapped token", @@ -420,10 +422,10 @@ func TestVaultSource(t *testing.T) { req: &vaultapi.TokenCreateRequest{Lease: "1m", TTL: "1m", Policies: []string{"fabio"}}, }, { - desc: "non-renewable wrapped token", - wrapTTL: "10s", - req: &vaultapi.TokenCreateRequest{Lease: "1m", TTL: "1m", Renewable: newBool(false), Policies: []string{"fabio"}}, - dropErr: true, + desc: "non-renewable wrapped token", + wrapTTL: "10s", + req: &vaultapi.TokenCreateRequest{Lease: "1m", TTL: "1m", Renewable: newBool(false), Policies: []string{"fabio"}}, + dropWarn: true, }, } @@ -438,11 +440,11 @@ func TestVaultSource(t *testing.T) { vaultToken: makeToken(t, client, tt.wrapTTL, tt.req), } - // suppress the log warning about a non-renewable lease + // suppress the log warning about a non-renewable token // since this is the expected behavior. - dropNotRenewableError = tt.dropErr + dropNotRenewableWarning = tt.dropWarn testSource(t, src, pool, timeout) - dropNotRenewableError = false + dropNotRenewableWarning = false }) } } diff --git a/cert/vault_source.go b/cert/vault_source.go index 78c0e2ac4..c1729058f 100644 --- a/cert/vault_source.go +++ b/cert/vault_source.go @@ -3,6 +3,7 @@ package cert import ( "crypto/tls" "crypto/x509" + "encoding/json" "errors" "fmt" "log" @@ -29,8 +30,21 @@ type VaultSource struct { Refresh time.Duration mu sync.Mutex - token string // actual token vaultToken string // VAULT_TOKEN env var. Might be wrapped. + auth struct { + // token is the actual Vault token. + token string + + // expireTime is the time at which the token expires (becomes useless). + // The zero value indicates that the token is not renewable or never + // expires. + expireTime time.Time + + // renewTTL is the desired token lifetime after renewal, in seconds. + // This value is advisory and the Vault server may ignore or silently + // change it. + renewTTL int + } } func (s *VaultSource) client() (*api.Client, error) { @@ -45,20 +59,20 @@ func (s *VaultSource) client() (*api.Client, error) { if err != nil { return nil, err } - if err := s.setToken(c); err != nil { + + if err := s.setAuth(c); err != nil { return nil, err } + return c, nil } -func (s *VaultSource) setToken(c *api.Client) error { +func (s *VaultSource) setAuth(c *api.Client) error { s.mu.Lock() - defer func() { - c.SetToken(s.token) - s.mu.Unlock() - }() + defer s.mu.Unlock() - if s.token != "" { + if s.auth.token != "" { + c.SetToken(s.auth.token) return nil } @@ -68,20 +82,66 @@ func (s *VaultSource) setToken(c *api.Client) error { // did we get a wrapped token? resp, err := c.Logical().Unwrap(s.vaultToken) - if err != nil { - // not a wrapped token? - if strings.HasPrefix(err.Error(), "no value found at") { - s.token = s.vaultToken - return nil - } + switch { + case err == nil: + log.Printf("[INFO] vault: Unwrapped token %s", s.vaultToken) + s.auth.token = resp.Auth.ClientToken + case strings.HasPrefix(err.Error(), "no value found at"): + // not a wrapped token + s.auth.token = s.vaultToken + default: return err } - log.Printf("[INFO] vault: Unwrapped token %s", s.vaultToken) - s.token = resp.Auth.ClientToken + c.SetToken(s.auth.token) + s.checkRenewal(c) + return nil } +// dropNotRenewableWarning controls whether the 'Token is not renewable' +// warning is logged. This is useful for testing where this is the expected +// behavior. On production, this should always be set to false. +var dropNotRenewableWarning bool + +// checkRenewal checks if the Vault token can be renewed, and if so when it +// expires and how big the renewal increment should be. +func (s *VaultSource) checkRenewal(c *api.Client) { + resp, err := c.Auth().Token().LookupSelf() + if err != nil { + log.Printf("[WARN] vault: lookup-self failed, token renewal is disabled: %s", err) + return + } + + b, _ := json.Marshal(resp.Data) + var data struct { + CreationTTL int `json:"creation_ttl"` + ExpireTime time.Time `json:"expire_time"` + Renewable bool `json:"renewable"` + } + if err := json.Unmarshal(b, &data); err != nil { + log.Printf("[WARN] vault: lookup-self failed, token renewal is disabled: %s", err) + return + } + + switch { + case data.Renewable: + s.auth.renewTTL = data.CreationTTL + s.auth.expireTime = data.ExpireTime + case data.ExpireTime.IsZero(): + // token doesn't expire + return + case dropNotRenewableWarning: + return + default: + ttl := time.Until(data.ExpireTime) + ttl = ttl / time.Second * time.Second // truncate to seconds + log.Printf("[WARN] vault: Token is not renewable and will expire %s from now at %s", + ttl, data.ExpireTime.Format(time.RFC3339)) + } + +} + func (s *VaultSource) LoadClientCAs() (*x509.CertPool, error) { return newCertPool(s.ClientCAPath, s.CAUpgradeCN, s.load) } @@ -92,11 +152,6 @@ func (s *VaultSource) Certificates() chan []tls.Certificate { return ch } -// dropNotRenewableError controls whether the 'lease is not renewable' -// error is logged. This is useful for testing where this is the expected -// behavior. On production, this should always be set to false. -var dropNotRenewableError bool - func (s *VaultSource) load(path string) (pemBlocks map[string][]byte, err error) { pemBlocks = map[string][]byte{} @@ -130,15 +185,8 @@ func (s *VaultSource) load(path string) (pemBlocks map[string][]byte, err error) return nil, fmt.Errorf("vault: client: %s", err) } - // renew token - // TODO(fs): make configurable - const oneHour = 3600 - _, err = c.Auth().Token().RenewSelf(oneHour) - if err != nil { - // TODO(fs): danger of filling up log since default refresh is 1s - if !dropNotRenewableError { - log.Printf("[WARN] vault: Failed to renew token. %s", err) - } + if err := s.renewToken(c); err != nil { + log.Printf("[WARN] vault: Failed to renew token: %s", err) } // get the subkeys under 'path'. @@ -165,3 +213,37 @@ func (s *VaultSource) load(path string) (pemBlocks map[string][]byte, err error) return pemBlocks, nil } + +func (s *VaultSource) renewToken(c *api.Client) error { + s.mu.Lock() + defer s.mu.Unlock() + + ttl := time.Until(s.auth.expireTime) + switch { + case s.auth.expireTime.IsZero(): + // Token isn't renewable. + return nil + case ttl < 2*s.Refresh: + // Renew the token if it isn't valid for two more refresh intervals. + break + case ttl < time.Minute: + // Renew the token if it isn't valid for one more minute. This happens + // if s.Refresh is small, say one second. It is risky to renew the + // token just one or two seconds before expiration; networks are + // unreliable, clocks can be skewed, etc. + break + default: + // Token doesn't need to be renewed yet. + return nil + } + + resp, err := c.Auth().Token().RenewSelf(s.auth.renewTTL) + if err != nil { + return err + } + + leaseDuration := time.Duration(resp.Auth.LeaseDuration) * time.Second + s.auth.expireTime = time.Now().Add(leaseDuration) + + return nil +}