Issue fabiolb#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.

cert/source_test.go

@@ -394,25 +394,22 @@ func TestVaultSource(t *testing.T) {
 		desc    string
 		wrapTTL string
 		req     *vaultapi.TokenCreateRequest
-		dropErr 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"}},
 		},
 		{
 			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"}},
 		},
 		{
 			desc:    "renewable wrapped token",
@@ -423,7 +420,6 @@ func TestVaultSource(t *testing.T) {
 			desc:    "non-renewable wrapped token",
 			wrapTTL: "10s",
 			req:     &vaultapi.TokenCreateRequest{Lease: "1m", TTL: "1m", Renewable: newBool(false), Policies: []string{"fabio"}},
-			dropErr: true,
 		},
 	}
 
@@ -438,11 +434,7 @@ func TestVaultSource(t *testing.T) {
 				vaultToken: makeToken(t, client, tt.wrapTTL, tt.req),
 			}
 
-			// suppress the log warning about a non-renewable lease
-			// since this is the expected behavior.
-			dropNotRenewableError = tt.dropErr
 			testSource(t, src, pool, timeout)
-			dropNotRenewableError = false
 		})
 	}
 }

cert/vault_source.go

@@ -3,6 +3,7 @@ package cert
 import (
 	"crypto/tls"
 	"crypto/x509"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"log"
@@ -29,8 +30,12 @@ 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          string    // actual token
+		expireTime     time.Time // zero value if the token isn't renewable
+		renewIncrement int
+	}
 }
 
 func (s *VaultSource) client() (*api.Client, error) {
@@ -45,20 +50,27 @@ 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 {
+	firstCall := true
 	s.mu.Lock()
 	defer func() {
-		c.SetToken(s.token)
+		c.SetToken(s.auth.token)
+		if firstCall {
+			s.checkRenewal(c)
+		}
 		s.mu.Unlock()
 	}()
 
-	if s.token != "" {
+	if s.auth.token != "" {
+		firstCall = false
 		return nil
 	}
 
@@ -71,17 +83,50 @@ func (s *VaultSource) setToken(c *api.Client) error {
 	if err != nil {
 		// not a wrapped token?
 		if strings.HasPrefix(err.Error(), "no value found at") {
-			s.token = s.vaultToken
+			s.auth.token = s.vaultToken
 			return nil
 		}
 		return err
 	}
 	log.Printf("[INFO] vault: Unwrapped token %s", s.vaultToken)
 
-	s.token = resp.Auth.ClientToken
+	s.auth.token = resp.Auth.ClientToken
 	return nil
 }
 
+// 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) {
+	// Check when the token expires (if at all).
+	response, 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(response.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
+	}
+
+	if data.Renewable {
+		s.auth.renewIncrement = data.CreationTTL
+		s.auth.expireTime = data.ExpireTime
+		log.Printf("[INFO] vault: Token will be renewed")
+	} else if !data.ExpireTime.IsZero() {
+		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 +137,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 +170,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 +198,36 @@ 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()
+
+	switch {
+	case s.auth.expireTime.IsZero():
+		// Token isn't renewable.
+		return nil
+	case time.Until(s.auth.expireTime) < 2*s.Refresh:
+		// Renew the token if it isn't valid for two more refresh intervals.
+		break
+	case time.Until(s.auth.expireTime) < 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 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
+	}
+
+	response, err := c.Auth().Token().RenewSelf(s.auth.renewIncrement)
+	if err != nil {
+		return err
+	}
+
+	leaseDuration := time.Duration(response.Auth.LeaseDuration) * time.Second
+	s.auth.expireTime = time.Now().Add(leaseDuration)
+
+	return nil
+}