Add a sentinel error for missing KV secrets (#16699)

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.
 //

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{

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)
 
-		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)
+			// 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)
+			}
 		}
 
-		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

changelog/16699.txt

@@ -0,0 +1,3 @@
+```release-note:improvement
+api: Add a sentinel error for missing KV secrets
+```

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)