From 1f78c55e6f5b09d6170ff42d0ce1fd19a842b169 Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Thu, 14 May 2020 10:56:58 -0400 Subject: [PATCH 1/2] vault: failing test for repeated revocation --- nomad/vault_test.go | 126 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 126 insertions(+) diff --git a/nomad/vault_test.go b/nomad/vault_test.go index 552ef9908e1c..99d7c78e40b6 100644 --- a/nomad/vault_test.go +++ b/nomad/vault_test.go @@ -1409,6 +1409,52 @@ func TestVaultClient_RevokeTokens_PreEstablishs(t *testing.T) { } } +// TestVaultClient_RevokeTokens_failures_TTL asserts that +// the registered TTL doesn't get extended on retries +func TestVaultClient_RevokeTokens_Failures_TTL(t *testing.T) { + t.Parallel() + vconfig := &config.VaultConfig{ + Enabled: helper.BoolToPtr(true), + Token: uuid.Generate(), + Addr: "http://127.0.0.1:0", + } + logger := testlog.HCLogger(t) + client, err := NewVaultClient(vconfig, logger, nil) + if err != nil { + t.Fatalf("failed to build vault client: %v", err) + } + client.SetActive(true) + defer client.Stop() + + // Create some VaultAccessors + vas := []*structs.VaultAccessor{ + mock.VaultAccessor(), + mock.VaultAccessor(), + } + + err = client.RevokeTokens(context.Background(), vas, true) + require.NoError(t, err) + + // Was committed + require.Len(t, client.revoking, 2) + + // set TTL + ttl := time.Now().Add(50 * time.Second) + client.revoking[vas[0]] = ttl + client.revoking[vas[1]] = ttl + + // revoke again and ensure that TTL isn't extended + err = client.RevokeTokens(context.Background(), vas, true) + require.NoError(t, err) + + require.Len(t, client.revoking, 2) + expected := map[*structs.VaultAccessor]time.Time{ + vas[0]: ttl, + vas[1]: ttl, + } + require.Equal(t, expected, client.revoking) +} + func TestVaultClient_RevokeTokens_Root(t *testing.T) { t.Parallel() v := testutil.NewTestVault(t) @@ -1541,6 +1587,86 @@ func TestVaultClient_RevokeTokens_Role(t *testing.T) { } } +// TestVaultClient_RevokeTokens_Idempotent asserts that token revocation +// is idempotent, and can cope with cases if token was deleted out of band. +func TestVaultClient_RevokeTokens_Idempotent(t *testing.T) { + t.Parallel() + v := testutil.NewTestVault(t) + defer v.Stop() + + // Set the configs token in a new test role + v.Config.Token = defaultTestVaultWhitelistRoleAndToken(v, t, 5) + + purged := map[string]struct{}{} + purge := func(accessors []*structs.VaultAccessor) error { + for _, accessor := range accessors { + purged[accessor.Accessor] = struct{}{} + } + return nil + } + + logger := testlog.HCLogger(t) + client, err := NewVaultClient(v.Config, logger, purge) + if err != nil { + t.Fatalf("failed to build vault client: %v", err) + } + client.SetActive(true) + defer client.Stop() + + waitForConnection(client, t) + + // Create some vault tokens + auth := v.Client.Auth().Token() + req := vapi.TokenCreateRequest{ + Policies: []string{"default"}, + } + t1, err := auth.Create(&req) + require.NoError(t, err) + require.NotNil(t, t1) + require.NotNil(t, t1.Auth) + + t2, err := auth.Create(&req) + require.NoError(t, err) + require.NotNil(t, t2) + require.NotNil(t, t2.Auth) + + t3, err := auth.Create(&req) + require.NoError(t, err) + require.NotNil(t, t3) + require.NotNil(t, t3.Auth) + + // revoke t3 out of band + err = auth.RevokeAccessor(t3.Auth.Accessor) + require.NoError(t, err) + + // Create two VaultAccessors + vas := []*structs.VaultAccessor{ + {Accessor: t1.Auth.Accessor}, + {Accessor: t2.Auth.Accessor}, + {Accessor: t3.Auth.Accessor}, + } + + // Issue a token revocation + err = client.RevokeTokens(context.Background(), vas, true) + require.NoError(t, err) + require.Empty(t, client.revoking) + + // revoke token again + err = client.RevokeTokens(context.Background(), vas, true) + require.NoError(t, err) + require.Empty(t, client.revoking) + + // Lookup the token and make sure we get an error + require.Len(t, purged, 3) + require.Contains(t, purged, t1.Auth.Accessor) + require.Contains(t, purged, t2.Auth.Accessor) + require.Contains(t, purged, t3.Auth.Accessor) + s, err := auth.Lookup(t1.Auth.ClientToken) + require.Errorf(t, err, "failed to purge token: %v", s) + s, err = auth.Lookup(t2.Auth.ClientToken) + require.Errorf(t, err, "failed to purge token: %v", s) +} + func waitForConnection(v *vaultClient, t *testing.T) { testutil.WaitForResult(func() (bool, error) { return v.ConnectionEstablished() From ff3cf8f33bd942177b3dc54e3b054cf7bfda04d8 Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Thu, 14 May 2020 10:57:43 -0400 Subject: [PATCH 2/2] vault: ensure that token revocation is idempotent This ensures that token revocation is idempotent and can handle when tokens are revoked out of band. Idempotency is important to handle some transient failures and retries. Consider when a single token of a batch fails to be revoked, nomad would retry revoking the entire batch; tokens already revoked should be gracefully handled, otherwise, nomad may retry revoking the same tokens forever. --- nomad/vault.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/nomad/vault.go b/nomad/vault.go index 4a36471810a5..9dbcd0660088 100644 --- a/nomad/vault.go +++ b/nomad/vault.go @@ -6,6 +6,7 @@ import ( "fmt" "math/rand" "strconv" + "strings" "sync" "sync/atomic" "time" @@ -1135,7 +1136,9 @@ func (v *vaultClient) storeForRevocation(accessors []*structs.VaultAccessor) { now := time.Now() for _, a := range accessors { - v.revoking[a] = now.Add(time.Duration(a.CreationTTL) * time.Second) + if _, ok := v.revoking[a]; !ok { + v.revoking[a] = now.Add(time.Duration(a.CreationTTL) * time.Second) + } } v.revLock.Unlock() } @@ -1176,7 +1179,8 @@ func (v *vaultClient) parallelRevoke(ctx context.Context, accessors []*structs.V return nil } - if err := v.auth.RevokeAccessor(va.Accessor); err != nil { + err := v.auth.RevokeAccessor(va.Accessor) + if err != nil && !strings.Contains(err.Error(), "invalid accessor") { return fmt.Errorf("failed to revoke token (alloc: %q, node: %q, task: %q): %v", va.AllocID, va.NodeID, va.Task, err) } case <-pCtx.Done():