From 6d45a421ff8987f0414cb57c7be71a9901ea00e9 Mon Sep 17 00:00:00 2001 From: Anton Averchenkov <84287187+averche@users.noreply.github.com> Date: Fri, 12 Aug 2022 19:29:42 -0400 Subject: [PATCH] Add a sentinel error for missing KV secrets (#16699) --- api/kv.go | 6 ++ api/kv_v1.go | 2 +- api/kv_v2.go | 58 ++++++++----- changelog/16699.txt | 3 + vault/external_tests/api/kv_helpers_test.go | 95 ++++++++++++++++----- 5 files changed, 120 insertions(+), 44 deletions(-) create mode 100644 changelog/16699.txt diff --git a/api/kv.go b/api/kv.go index 16437582e..37699df26 100644 --- a/api/kv.go +++ b/api/kv.go @@ -1,5 +1,11 @@ package api +import "errors" + +// ErrSecretNotFound is returned by KVv1 and KVv2 wrappers to indicate that the +// secret is missing at the given location. +var ErrSecretNotFound = errors.New("secret not found") + // A KVSecret is a key-value secret returned by Vault's KV secrets engine, // and is the most basic type of secret stored in Vault. // diff --git a/api/kv_v1.go b/api/kv_v1.go index d269070bc..22ba99238 100644 --- a/api/kv_v1.go +++ b/api/kv_v1.go @@ -19,7 +19,7 @@ func (kv *KVv1) Get(ctx context.Context, secretPath string) (*KVSecret, error) { return nil, fmt.Errorf("error encountered while reading secret at %s: %w", pathToRead, err) } if secret == nil { - return nil, fmt.Errorf("no secret found at %s", pathToRead) + return nil, fmt.Errorf("%w: at %s", ErrSecretNotFound, pathToRead) } return &KVSecret{ diff --git a/api/kv_v2.go b/api/kv_v2.go index f0f59abfe..7a98cfeef 100644 --- a/api/kv_v2.go +++ b/api/kv_v2.go @@ -2,7 +2,9 @@ package api import ( "context" + "errors" "fmt" + "net/http" "sort" "strconv" "time" @@ -115,7 +117,7 @@ func (kv *KVv2) Get(ctx context.Context, secretPath string) (*KVSecret, error) { return nil, fmt.Errorf("error encountered while reading secret at %s: %w", pathToRead, err) } if secret == nil { - return nil, fmt.Errorf("no secret found at %s", pathToRead) + return nil, fmt.Errorf("%w: at %s", ErrSecretNotFound, pathToRead) } kvSecret, err := extractDataAndVersionMetadata(secret) @@ -149,7 +151,7 @@ func (kv *KVv2) GetVersion(ctx context.Context, secretPath string, version int) return nil, err } if secret == nil { - return nil, fmt.Errorf("no secret with version %d found at %s", version, pathToRead) + return nil, fmt.Errorf("%w: for version %d at %s", ErrSecretNotFound, version, pathToRead) } kvSecret, err := extractDataAndVersionMetadata(secret) @@ -175,7 +177,7 @@ func (kv *KVv2) GetVersionsAsList(ctx context.Context, secretPath string) ([]KVV return nil, err } if secret == nil || secret.Data == nil { - return nil, fmt.Errorf("no secret metadata found at %s", pathToRead) + return nil, fmt.Errorf("%w: no metadata at %s", ErrSecretNotFound, pathToRead) } md, err := extractFullMetadata(secret) @@ -202,7 +204,7 @@ func (kv *KVv2) GetMetadata(ctx context.Context, secretPath string) (*KVMetadata return nil, err } if secret == nil || secret.Data == nil { - return nil, fmt.Errorf("no secret metadata found at %s", pathToRead) + return nil, fmt.Errorf("%w: no metadata at %s", ErrSecretNotFound, pathToRead) } md, err := extractFullMetadata(secret) @@ -244,7 +246,7 @@ func (kv *KVv2) Put(ctx context.Context, secretPath string, data map[string]inte return nil, fmt.Errorf("error writing secret to %s: %w", pathToWriteTo, err) } if secret == nil { - return nil, fmt.Errorf("no secret was written to %s", pathToWriteTo) + return nil, fmt.Errorf("%w: after writing to %s", ErrSecretNotFound, pathToWriteTo) } metadata, err := extractVersionMetadata(secret) @@ -325,19 +327,19 @@ func (kv *KVv2) Patch(ctx context.Context, secretPath string, newData map[string // Determine which kind of patch to use, // the newer HTTP Patch style or the older read-then-write style var kvs *KVSecret - var perr error + var err error switch patchMethod { case "rw": - kvs, perr = readThenWrite(ctx, kv.c, kv.mountPath, secretPath, newData) + kvs, err = readThenWrite(ctx, kv.c, kv.mountPath, secretPath, newData) case "patch": - kvs, perr = mergePatch(ctx, kv.c, kv.mountPath, secretPath, newData, opts...) + kvs, err = mergePatch(ctx, kv.c, kv.mountPath, secretPath, newData, opts...) case "": - kvs, perr = mergePatch(ctx, kv.c, kv.mountPath, secretPath, newData, opts...) + kvs, err = mergePatch(ctx, kv.c, kv.mountPath, secretPath, newData, opts...) default: return nil, fmt.Errorf("unsupported patch method provided; value for patch method should be string \"rw\" or \"patch\"") } - if perr != nil { - return nil, fmt.Errorf("unable to perform patch: %w", perr) + if err != nil { + return nil, fmt.Errorf("unable to perform patch: %w", err) } if kvs == nil { return nil, fmt.Errorf("no secret was written to %s", secretPath) @@ -478,7 +480,7 @@ func (kv *KVv2) Rollback(ctx context.Context, secretPath string, toVersion int) // Now run it again and read the version we want to roll back to rollbackVersion, err := kv.GetVersion(ctx, secretPath, toVersion) if err != nil { - return nil, fmt.Errorf("unable to get previous version %d of secret: %s", toVersion, err) + return nil, fmt.Errorf("unable to get previous version %d of secret: %w", toVersion, err) } err = validateRollbackVersion(rollbackVersion) @@ -687,18 +689,28 @@ func mergePatch(ctx context.Context, client *Client, mountPath string, secretPat secret, err := client.Logical().JSONMergePatch(ctx, pathToMergePatch, wrappedData) if err != nil { - // If it's a 405, that probably means the server is running a pre-1.9 - // Vault version that doesn't support the HTTP PATCH method. - // Fall back to the old way of doing it. - if re, ok := err.(*ResponseError); ok && re.StatusCode == 405 { - return readThenWrite(ctx, client, mountPath, secretPath, newData) + var re *ResponseError + + if errors.As(err, &re) { + switch re.StatusCode { + // 403 + case http.StatusForbidden: + return nil, fmt.Errorf("received 403 from Vault server; please ensure that token's policy has \"patch\" capability: %w", err) + + // 404 + case http.StatusNotFound: + return nil, fmt.Errorf("%w: performing merge patch to %s", ErrSecretNotFound, pathToMergePatch) + + // 405 + case http.StatusMethodNotAllowed: + // If it's a 405, that probably means the server is running a pre-1.9 + // Vault version that doesn't support the HTTP PATCH method. + // Fall back to the old way of doing it. + return readThenWrite(ctx, client, mountPath, secretPath, newData) + } } - if re, ok := err.(*ResponseError); ok && re.StatusCode == 403 { - return nil, fmt.Errorf("received 403 from Vault server; please ensure that token's policy has \"patch\" capability: %w", err) - } - - return nil, fmt.Errorf("error performing merge patch to %s: %s", pathToMergePatch, err) + return nil, fmt.Errorf("error performing merge patch to %s: %w", pathToMergePatch, err) } metadata, err := extractVersionMetadata(secret) @@ -730,7 +742,7 @@ func readThenWrite(ctx context.Context, client *Client, mountPath string, secret // Make sure the secret already exists if existingVersion == nil || existingVersion.Data == nil { - return nil, fmt.Errorf("no existing secret was found at %s when doing read-then-write patch operation: %w", secretPath, err) + return nil, fmt.Errorf("%w: at %s as part of read-then-write patch operation", ErrSecretNotFound, secretPath) } // Verify existing secret has metadata diff --git a/changelog/16699.txt b/changelog/16699.txt new file mode 100644 index 000000000..4a96e868a --- /dev/null +++ b/changelog/16699.txt @@ -0,0 +1,3 @@ +```release-note:improvement +api: Add a sentinel error for missing KV secrets +``` diff --git a/vault/external_tests/api/kv_helpers_test.go b/vault/external_tests/api/kv_helpers_test.go index 83f83d42e..f005a35d9 100644 --- a/vault/external_tests/api/kv_helpers_test.go +++ b/vault/external_tests/api/kv_helpers_test.go @@ -2,6 +2,7 @@ package api import ( "context" + "errors" "reflect" "testing" "time" @@ -80,7 +81,7 @@ func TestKVHelpers(t *testing.T) { } //// v1 //// - t.Run("kv v1 helpers", func(t *testing.T) { + t.Run("kv v1: put, get, and delete data", func(t *testing.T) { if err := client.KVv1(v1MountPath).Put(context.Background(), secretPath, secretData); err != nil { t.Fatal(err) } @@ -97,10 +98,25 @@ func TestKVHelpers(t *testing.T) { if err := client.KVv1(v1MountPath).Delete(context.Background(), secretPath); err != nil { t.Fatal(err) } + + _, err = client.KVv1(v1MountPath).Get(context.Background(), secretPath) + if !errors.Is(err, api.ErrSecretNotFound) { + t.Fatalf("KVv1.Get is expected to return an api.ErrSecretNotFound wrapped error after secret had been deleted; got %v", err) + } + }) + + t.Run("kv v1: get secret that does not exist", func(t *testing.T) { + _, err = client.KVv1(v1MountPath).Get(context.Background(), "does/not/exist") + if err == nil { + t.Fatalf("KVv1.Get is expected to return an error for a missing secret") + } + if !errors.Is(err, api.ErrSecretNotFound) { + t.Fatalf("KVv1.Get is expected to return an api.ErrSecretNotFound wrapped error for a missing secret; got %v", err) + } }) //// v2 //// - t.Run("get data and full metadata", func(t *testing.T) { + t.Run("kv v2: get data and full metadata", func(t *testing.T) { teardownTest, originalSecret := setupKVv2Test(t) defer teardownTest(t) @@ -125,7 +141,27 @@ func TestKVHelpers(t *testing.T) { } }) - t.Run("multiple versions", func(t *testing.T) { + t.Run("kv v2: get secret that does not exist", func(t *testing.T) { + teardownTest, _ := setupKVv2Test(t) + defer teardownTest(t) + + _, err = client.KVv2(v1MountPath).Get(context.Background(), "does/not/exist") + if !errors.Is(err, api.ErrSecretNotFound) { + t.Fatalf("KVv2.Get is expected to return an api.ErrSecretNotFound wrapped error for a missing secret; got %v", err) + } + + _, err = client.KVv2(v1MountPath).GetMetadata(context.Background(), "does/not/exist") + if !errors.Is(err, api.ErrSecretNotFound) { + t.Fatalf("KVv2.GetMetadata is expected to return an api.ErrSecretNotFound wrapped error for a missing secret; got %v", err) + } + + _, err = client.KVv2(v1MountPath).GetVersion(context.Background(), secretPath, 99) + if !errors.Is(err, api.ErrSecretNotFound) { + t.Fatalf("KVv2.GetVersion is expected to return an api.ErrSecretNotFound wrapped error for a missing secret version; got %v", err) + } + }) + + t.Run("kv v2: multiple versions", func(t *testing.T) { teardownTest, _ := setupKVv2Test(t) defer teardownTest(t) @@ -149,7 +185,7 @@ func TestKVHelpers(t *testing.T) { } }) - t.Run("delete and undelete", func(t *testing.T) { + t.Run("kv v2: delete and undelete", func(t *testing.T) { teardownTest, _ := setupKVv2Test(t) defer teardownTest(t) @@ -193,9 +229,18 @@ func TestKVHelpers(t *testing.T) { if err != nil { t.Fatal(err) } + + s1AfterUndelete, err := client.KVv2(v2MountPath).GetVersion(context.Background(), secretPath, 1) + if err != nil { + t.Fatal(err) + } + + if s1AfterUndelete.Data == nil { + t.Fatalf("data is empty for the first version of the secret despite this version being undeleted") + } }) - t.Run("destroy", func(t *testing.T) { + t.Run("kv v2: destroy", func(t *testing.T) { teardownTest, _ := setupKVv2Test(t) defer teardownTest(t) @@ -214,7 +259,7 @@ func TestKVHelpers(t *testing.T) { } }) - t.Run("use named functional options and generic WithOption", func(t *testing.T) { + t.Run("kv v2: use named functional options and generic WithOption", func(t *testing.T) { teardownTest, _ := setupKVv2Test(t) defer teardownTest(t) @@ -238,7 +283,7 @@ func TestKVHelpers(t *testing.T) { } }) - t.Run("patch", func(t *testing.T) { + t.Run("kv v2: patch", func(t *testing.T) { teardownTest, _ := setupKVv2Test(t) defer teardownTest(t) @@ -275,14 +320,6 @@ func TestKVHelpers(t *testing.T) { t.Fatalf("incorrect version %d, expected 4", patchRW.VersionMetadata.Version) } - // patch something that doesn't exist - _, err = client.KVv2(v2MountPath).Patch(context.Background(), "nonexistent-secret", map[string]interface{}{ - "no": "nope", - }) - if err == nil { - t.Fatal("expected error from trying to patch something that doesn't exist") - } - secretAfterPatches, err := client.KVv2(v2MountPath).Get(context.Background(), secretPath) if err != nil { t.Fatal(err) @@ -353,7 +390,25 @@ func TestKVHelpers(t *testing.T) { } }) - t.Run("roll back to an old version", func(t *testing.T) { + t.Run("kv v2: patch a secret that does not exist", func(t *testing.T) { + for _, method := range [][]api.KVOption{ + {}, + {api.WithMergeMethod(api.KVMergeMethodPatch)}, + {api.WithMergeMethod(api.KVMergeMethodReadWrite)}, + } { + _, err = client.KVv2(v2MountPath).Patch( + context.Background(), + "does/not/exist", + map[string]interface{}{"no": "nope"}, + method..., + ) + if !errors.Is(err, api.ErrSecretNotFound) { + t.Fatalf("expected an api.ErrSecretNotFound wrapped error from trying to patch something that doesn't exist for %v method; got: %v", method, err) + } + } + }) + + t.Run("kv v2: roll back to an old version", func(t *testing.T) { teardownTest, _ := setupKVv2Test(t) defer teardownTest(t) @@ -402,7 +457,7 @@ func TestKVHelpers(t *testing.T) { } }) - t.Run("delete all versions of a secret", func(t *testing.T) { + t.Run("kv v2: delete all versions of a secret", func(t *testing.T) { teardownTest, _ := setupKVv2Test(t) defer teardownTest(t) @@ -429,7 +484,7 @@ func TestKVHelpers(t *testing.T) { } }) - t.Run("create a secret with metadata but no data", func(t *testing.T) { + t.Run("kv v2: create a secret with metadata but no data", func(t *testing.T) { // put and patch metadata //// noDataSecretPath := "empty" @@ -457,7 +512,7 @@ func TestKVHelpers(t *testing.T) { } }) - t.Run("put and patch metadata", func(t *testing.T) { + t.Run("kv v2: put and patch metadata", func(t *testing.T) { teardownTest, _ := setupKVv2Test(t) defer teardownTest(t) @@ -529,7 +584,7 @@ func TestKVHelpers(t *testing.T) { } }) - t.Run("patch with explicit zero values", func(t *testing.T) { + t.Run("kv v2: patch with explicit zero values", func(t *testing.T) { teardownTest, _ := setupKVv2Test(t) defer teardownTest(t)