Skip to content

Commit

Permalink
Issue #274: Avoid premature vault token renewals
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@pschultz @magiconair
pschultz authored and magiconair committed Jun 29, 2017
1 parent bbb0ca3 commit d6d0a52
Show file tree
Hide file tree
Showing 2 changed files with 131 additions and 47 deletions.
36 changes: 19 additions & 17 deletions cert/source_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -391,39 +393,39 @@ 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",
wrapTTL: "10s",
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,
},
}

Expand All @@ -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
})
}
}
Expand Down
142 changes: 112 additions & 30 deletions cert/vault_source.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package cert
import (
"crypto/tls"
"crypto/x509"
"encoding/json"
"errors"
"fmt"
"log"
Expand All @@ -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) {
Expand All @@ -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
}

Expand All @@ -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)
}
Expand All @@ -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{}

Expand Down Expand Up @@ -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'.
Expand All @@ -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
}

0 comments on commit d6d0a52

Please sign in to comment.