vault: improve Vault API interaction

This commit improves the Vault API interaction by using the `vaultapi.KVv1` and `vaultapi.KVv2` types where possible. This simplifies the `List`, `Delete` and `Get` APIs. Further, this commit changes the `List` API implementation by using HTTP GET instead of LIST. This is a workaround that prevents listing errors caused by some middleware/firewalls. Such systems may reject API calls with "non-standard" HTTP methods. Signed-off-by: Andreas Auernhammer <[email protected]>
minio · Apr 8, 2024 · 6946e60 · 6946e60
1 parent f7a894a
commit 6946e60
Show file tree

Hide file tree

Showing 5 changed files with 25 additions and 62 deletions.
diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml
@@ -16,7 +16,7 @@ jobs:
     - name: Set up Go
       uses: actions/setup-go@v3
       with:
-        go-version: 1.21.8
+        go-version: 1.22.2
         check-latest: true
       id: go
     - name: Check out code
@@ -34,7 +34,7 @@ jobs:
       - name: "Set up Go"
         uses: actions/setup-go@v3
         with:
-          go-version: 1.21.8
+          go-version: 1.22.2
         id: go
       - name: Check out code
         uses: actions/checkout@v3
@@ -54,7 +54,7 @@ jobs:
     - name: Set up Go
       uses: actions/setup-go@v3
       with:
-        go-version: 1.21.8
+        go-version: 1.22.2
         check-latest: true
       id: go
     - name: Check out code
@@ -74,7 +74,7 @@ jobs:
       uses: actions/checkout@v3
     - uses: actions/setup-go@v3
       with:
-        go-version: 1.21.8
+        go-version: 1.22.2
         check-latest: true
     - name: Get govulncheck
       run: go install golang.org/x/vuln/cmd/govulncheck@latest

diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml
@@ -20,7 +20,7 @@ jobs:
       - name: Set up Go
         uses: actions/setup-go@v3
         with:
-          go-version: 1.21.8
+          go-version: 1.22.2
           check-latest: true
       - name: Set up QEMU
         uses: docker/setup-qemu-action@v1

diff --git a/go.mod b/go.mod
@@ -82,7 +82,7 @@ require (
 	go.opentelemetry.io/otel v1.22.0 // indirect
 	go.opentelemetry.io/otel/metric v1.22.0 // indirect
 	go.opentelemetry.io/otel/trace v1.22.0 // indirect
-	golang.org/x/net v0.22.0 // indirect
+	golang.org/x/net v0.23.0 // indirect
 	golang.org/x/oauth2 v0.18.0 // indirect
 	golang.org/x/sync v0.6.0 // indirect
 	golang.org/x/text v0.14.0 // indirect

diff --git a/go.sum b/go.sum
@@ -250,8 +250,8 @@ golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v
 golang.org/x/net v0.0.0-20211112202133-69e39bad7dc2/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y=
 golang.org/x/net v0.0.0-20220722155237-a158d28d115b/go.mod h1:XRhObCWvk6IyKnWLug+ECip1KBveYUHfp+8e9klMJ9c=
 golang.org/x/net v0.6.0/go.mod h1:2Tu9+aMcznHK/AK1HMvgo6xiTLG5rD5rZLDS+rp2Bjs=
-golang.org/x/net v0.22.0 h1:9sGLhx7iRIHEiX0oAJ3MRZMUCElJgy7Br1nO+AMN3Tc=
-golang.org/x/net v0.22.0/go.mod h1:JKghWKKOSdJwpW2GEx0Ja7fmaKnMsbu+MWVZTokSYmg=
+golang.org/x/net v0.23.0 h1:7EYJ93RZ9vYSZAIb2x3lnuvqO5zneoD6IvWjuhfxjTs=
+golang.org/x/net v0.23.0/go.mod h1:JKghWKKOSdJwpW2GEx0Ja7fmaKnMsbu+MWVZTokSYmg=
 golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U=
 golang.org/x/oauth2 v0.18.0 h1:09qnuIAgzdx1XplqJvW6CQqMCtGZykZWcXzPMPUusvI=
 golang.org/x/oauth2 v0.18.0/go.mod h1:Wf7knwG0MPoWIMMBgFlEaSUDaKskp0dCfrlJRJXbBi8=

diff --git a/internal/keystore/vault/vault.go b/internal/keystore/vault/vault.go
@@ -226,7 +226,7 @@ func (s *Store) Create(ctx context.Context, name string, value []byte) error {
 	// But when the client returns an error it does not mean that
 	// the entry does not exist but that some other error (e.g.
 	// network error) occurred.
-	switch secret, err := s.client.Logical().Read(location); {
+	switch secret, err := s.client.Logical().ReadWithContext(ctx, location); {
 	case err == nil && secret != nil && s.config.APIVersion != APIv2:
 		if _, ok := secret.Data[name]; !ok {
 			return fmt.Errorf("vault: entry exist but failed to read '%s': invalid K/V v1 format", location)
@@ -340,52 +340,34 @@ func (s *Store) Create(ctx context.Context, name string, value []byte) error {
 	return nil
 }
 
-// Set creates the given key-value pair at Vault if and only
-// if the given key does not exist. If such an entry already exists
-// it returns kes.ErrKeyExists.
-func (s *Store) Set(ctx context.Context, name string, value []byte) error {
-	return s.Create(ctx, name, value)
-}
-
 // Get returns the value associated with the given key.
 // If no entry for the key exists it returns kes.ErrKeyNotFound.
 func (s *Store) Get(ctx context.Context, name string) ([]byte, error) {
 	if s.client.Sealed() {
 		return nil, errSealed
 	}
 
-	var location string
+	var (
+		location = path.Join(s.config.Prefix, name)
+		entry    *vaultapi.KVSecret
+		err      error
+	)
 	if s.config.APIVersion == APIv2 {
-		// See: https://www.vaultproject.io/api/secret/kv/kv-v2#read-secret-version
-		location = path.Join(s.config.Engine, "data", s.config.Prefix, name) // /<engine>/data/<location>/<name>
+		entry, err = s.client.KVv2(s.config.Engine).GetVersion(ctx, location, 1)
 	} else {
-		// See: https://www.vaultproject.io/api/secret/kv/kv-v1#read-secret
-		location = path.Join(s.config.Engine, s.config.Prefix, name) // /<engine>/<location>/<name>
+		entry, err = s.client.KVv1(s.config.Engine).Get(ctx, location)
 	}
-	entry, err := s.client.Logical().Read(location)
 	if err != nil || entry == nil {
 		// Vault will not return an error if e.g. the key existed but has
 		// been deleted. However, it will return (nil, nil) in this case.
-		if err == nil && entry == nil {
+		if (err == nil && entry == nil) || errors.Is(err, vaultapi.ErrSecretNotFound) {
 			return nil, kesdk.ErrKeyNotFound
 		}
 		return nil, fmt.Errorf("vault: failed to read '%s': %v", location, err)
 	}
 
-	data := entry.Data
-	if s.config.APIVersion == APIv2 { // See: https://www.vaultproject.io/api/secret/kv/kv-v2#sample-response-1 (differs from v1 format)
-		v, ok := entry.Data["data"]
-		if !ok || v == nil {
-			return nil, fmt.Errorf("vault: failed to read '%s': invalid K/V v2 format: missing 'data' entry", location)
-		}
-		data, ok = v.(map[string]interface{})
-		if !ok || data == nil {
-			return nil, fmt.Errorf("vault: failed to read '%s': invalid K/V v2 format: invalid 'data' entry", location)
-		}
-	}
-
 	// Verify that we got a well-formed response from Vault
-	v, ok := data[name]
+	v, ok := entry.Data[name]
 	if !ok || v == nil {
 		return nil, fmt.Errorf("vault: failed to read '%s': entry exists but no secret key is present", location)
 	}
@@ -446,36 +428,20 @@ func (s *Store) Delete(ctx context.Context, name string) error {
 		return errSealed
 	}
 
-	var location string
+	var (
+		location = path.Join(s.config.Prefix, name)
+		err      error
+	)
 	if s.config.APIVersion == APIv2 {
 		// See: https://www.vaultproject.io/api/secret/kv/kv-v2#delete-metadata-and-all-versions
-		location = path.Join(s.config.Engine, "metadata", s.config.Prefix, name) // /<engine>/metadata/<location>/<name>
+		err = s.client.KVv2(s.config.Engine).DeleteMetadata(ctx, location)
 	} else {
 		// See: https://www.vaultproject.io/api/secret/kv/kv-v1#delete-secret
-		location = path.Join(s.config.Engine, s.config.Prefix, name) // /<engine>/<location>/<name>
+		err = s.client.KVv1(s.config.Engine).Delete(ctx, location)
 	}
-
-	// The Vault SDK may not return an error even if it hasn't deleted
-	// an entry - e.g. in case of some network errors. Therefore, we
-	// implement the specific key deletion logic ourself.
-	//
-	// We expect HTTP 204 (No Content) when a key got deleted successfully.
-	// So, we check that Vault response with 204. Otherwise, we return an
-	// error.
-	req := s.client.Client.NewRequest(http.MethodDelete, "/v1/"+location)
-	resp, err := s.client.Client.RawRequestWithContext(ctx, req)
 	if err != nil {
 		return fmt.Errorf("vault: failed to delete '%s': %v", location, err)
 	}
-	if resp != nil && resp.Body != nil {
-		defer resp.Body.Close()
-	}
-	if resp.StatusCode != http.StatusNoContent {
-		if _, err := vaultapi.ParseSecret(resp.Body); err != nil {
-			return fmt.Errorf("vault: failed to delete '%s': %v", location, err)
-		}
-		return fmt.Errorf("vault: failed to delete '%s': expected response %s (%d) but received %s (%d)", location, resp.Status, resp.StatusCode, http.StatusText(http.StatusNoContent), http.StatusNoContent)
-	}
 	return nil
 }
 
@@ -509,10 +475,7 @@ func (s *Store) List(ctx context.Context, prefix string, n int) ([]string, strin
 		location = path.Join(s.config.Engine, s.config.Prefix)
 	}
 
-	r := s.client.NewRequest("LIST", "/v1/"+location)
-	r.Params.Set("list", "true")
-
-	resp, err := s.client.RawRequestWithContext(ctx, r)
+	resp, err := s.client.Logical().ReadRawWithDataWithContext(ctx, location, map[string][]string{"list": {"true"}})
 	if err != nil {
 		return nil, "", fmt.Errorf("vault: failed to list '%s': %v", location, err)
 	}