VAULT-39294: Deprecate recover_snapshot_id query param and use a header instead (#8834) (#9042)

* deprecate snapshot query params, use a header instead

* keep read query param, but deprecate recover one

* fix test

* remove list change

* add changelog

* rename header, allow request method

* update changelog

Co-authored-by: miagilepner <mia.epner@hashicorp.com>

api/client.go

@@ -70,6 +70,9 @@ const (
 	// SSRF protection.
 	RequestHeaderName = "X-Vault-Request"
 
+	SnapshotHeaderName          = "X-Vault-Recover-Snapshot-Id"
+	RecoverSourcePathHeaderName = "X-Vault-Recover-Source-Path"
+
 	TLSErrorString = "This error usually means that the server is running with TLS disabled\n" +
 		"but the client is configured to use TLS. Please either enable TLS\n" +
 		"on the server or run the client with -address set to an address\n" +

api/logical.go

@@ -309,8 +309,9 @@ func (c *Logical) Recover(ctx context.Context, path string, snapshotID string) (
 func (c *Logical) RecoverFromPath(ctx context.Context, newPath string, snapshotID string, originalPath string) (*Secret, error) {
 	r := c.c.NewRequest(http.MethodPut, "/v1/"+newPath)
 	r.Params.Set("recover_snapshot_id", snapshotID)
+	r.Headers.Set(SnapshotHeaderName, snapshotID)
 	if originalPath != "" && originalPath != newPath {
-		r.Params.Set("recover_source_path", url.QueryEscape(originalPath))
+		r.Headers.Set(RecoverSourcePathHeaderName, originalPath)
 	}
 	return c.write(ctx, originalPath, r)
 }

changelog/_8834.txt

@@ -0,0 +1,3 @@
+```release-note:change
+Secrets Recovery (enterprise): Deprecate the `recover_snapshot_id` query parameter to pass the snapshot ID for recover operations, in favor of a `X-Vault-Recover-Snapshot-Id` header. Vault will still accept the query parameter for backward compatibility. Also support setting the HTTP method to `RECOVER` for recover operations, in addition to `POST` and `PUT`.
+```

http/handler.go

@@ -84,10 +84,15 @@ const (
 	VaultSnapshotReadParam = "read_snapshot_id"
 	// VaultSnapshotRecoverParam is the query parameter sent when Vault should
 	// recover the data from a loaded snapshot
+	// Deprecated: use VaultSnapshotRecoverHeader
 	VaultSnapshotRecoverParam = "recover_snapshot_id"
-	// VaultRecoverSourcePathParam contains an optional source path
+	// VaultRecoverSourcePathHeader contains an optional source path
 	// to read the data from when performing a recover operation
-	VaultRecoverSourcePathParam = consts.RecoverSourcePathParam
+	VaultRecoverSourcePathHeader = consts.RecoverSourcePathHeader
+	// VaultSnapshotRecoverHeader holds the snapshot ID to use for a read, list, or
+	// recover from snapshot operation. This replaces the use of query parameters
+	// to pass the snapshot ID
+	VaultSnapshotRecoverHeader = "X-Vault-Recover-Snapshot-Id"
 	// CustomMaxJSONDepth specifies the maximum nesting depth of a JSON object.
 	// This limit is designed to prevent stack exhaustion attacks from deeply
 	// nested JSON payloads, which could otherwise lead to a denial-of-service
@@ -1505,7 +1510,7 @@ func requiresSnapshot(r *http.Request) bool {
 	case http.MethodGet, "LIST":
 		return query.Has(VaultSnapshotReadParam)
 	case http.MethodPut, http.MethodPost:
-		return query.Has(VaultSnapshotRecoverParam)
+		return query.Has(VaultSnapshotRecoverParam) || r.Header.Get(VaultSnapshotRecoverParam) != ""
 	}
 	return false
 }

http/logical.go

@@ -12,7 +12,6 @@ import (
 	"mime"
 	"net"
 	"net/http"
-	"net/url"
 	"strconv"
 	"strings"
 	"time"
@@ -98,8 +97,12 @@ func buildLogicalRequestNoAuth(perfStandby bool, ra *vault.RouterAccess, w http.
 			responseWriter = w
 		}
 
-	case "POST", "PUT":
-		op = logical.UpdateOperation
+	case "POST", "PUT", "RECOVER":
+		if r.Method == "RECOVER" {
+			op = logical.RecoverOperation
+		} else {
+			op = logical.UpdateOperation
+		}
 
 		// Buffer the request body in order to allow us to peek at the beginning
 		// without consuming it. This approach involves no copying.
@@ -219,25 +222,28 @@ func buildLogicalRequestNoAuth(perfStandby bool, ra *vault.RouterAccess, w http.
 				data = nil
 			}
 		}
-	case logical.UpdateOperation:
-		queryVals := r.URL.Query()
-		if queryVals.Has(VaultSnapshotRecoverParam) {
-			snapshotID := queryVals.Get(VaultSnapshotRecoverParam)
-			if snapshotID != "" {
-				requiredSnapshotID = snapshotID
-				op = logical.RecoverOperation
+	case logical.UpdateOperation, logical.RecoverOperation:
+		snapshotHeaderID := r.Header.Get(VaultSnapshotRecoverHeader)
+		if snapshotHeaderID != "" {
+			requiredSnapshotID = snapshotHeaderID
+		} else {
+			queryVals := r.URL.Query()
+			if queryVals.Has(VaultSnapshotRecoverParam) {
+				snapshotID := queryVals.Get(VaultSnapshotRecoverParam)
+				if snapshotID != "" {
+					requiredSnapshotID = snapshotID
+				}
 			}
 		}
-		if op == logical.RecoverOperation {
-			if queryVals.Has(VaultRecoverSourcePathParam) {
-				sourcePath := queryVals.Get(VaultRecoverSourcePathParam)
-				if sourcePath != "" {
-					unescapedPath, err := url.QueryUnescape(sourcePath)
-					if err != nil {
-						return nil, nil, http.StatusBadRequest, fmt.Errorf("failed to unescape %s query parameter: %w", VaultRecoverSourcePathParam, err)
-					}
-					recoverSourcePath = trimPath(ns, unescapedPath)
-				}
+		if requiredSnapshotID == "" && op == logical.RecoverOperation {
+			return nil, nil, http.StatusBadRequest, fmt.Errorf("missing required snapshot ID")
+		}
+
+		if requiredSnapshotID != "" {
+			op = logical.RecoverOperation
+			sourcePath := r.Header.Get(VaultRecoverSourcePathHeader)
+			if sourcePath != "" {
+				recoverSourcePath = trimPath(ns, sourcePath)
 			}
 		}
 	}

http/logical_test.go

@@ -1071,9 +1071,11 @@ func TestLogical_SnapshotParams(t *testing.T) {
 		method                 string
 		url                    string
 		body                   []byte
+		headers                map[string]string
 		wantData               map[string]interface{}
 		wantOperation          logical.Operation
 		wantRequiresSnapshotID string
+		wantError              bool
 	}{
 		{
 			name:                   "normal get",
@@ -1109,6 +1111,31 @@ func TestLogical_SnapshotParams(t *testing.T) {
 			wantOperation:          logical.ReadOperation,
 			wantRequiresSnapshotID: "1234",
 		},
+		{
+			name:   "snapshot list header",
+			method: http.MethodGet,
+			url:    "https://example.com?list=true",
+			body:   nil,
+			headers: map[string]string{
+				VaultSnapshotRecoverHeader: "1234",
+			},
+			wantData:               nil,
+			wantOperation:          logical.ListOperation,
+			wantRequiresSnapshotID: "",
+		},
+		{
+			name:   "snapshot read header",
+			method: http.MethodGet,
+			url:    "https://example.com",
+			headers: map[string]string{
+				VaultSnapshotRecoverHeader: "1234",
+			},
+			body:                   nil,
+			wantData:               nil,
+			wantOperation:          logical.ReadOperation,
+			wantRequiresSnapshotID: "",
+		},
+
 		{
 			name:   "normal update",
 			method: http.MethodPost,
@@ -1130,6 +1157,44 @@ func TestLogical_SnapshotParams(t *testing.T) {
 			wantOperation:          logical.RecoverOperation,
 			wantRequiresSnapshotID: "1234",
 		},
+		{
+			name:   "snapshot update header",
+			method: http.MethodPost,
+			url:    "https://example.com",
+			body:   []byte(`{"other_data":"abcd"}`),
+			headers: map[string]string{
+				VaultSnapshotRecoverHeader: "1234",
+			},
+			wantData: map[string]interface{}{
+				"other_data": "abcd",
+			},
+			wantOperation:          logical.RecoverOperation,
+			wantRequiresSnapshotID: "1234",
+		},
+		{
+			name:   "recover operation no snapshot",
+			method: "RECOVER",
+			url:    "https://example.com",
+			body:   []byte(`{"other_data":"abcd"}`),
+			headers: map[string]string{
+				"other_header": "value",
+			},
+			wantError: true,
+		},
+		{
+			name:   "recover operation with snapshot",
+			method: "RECOVER",
+			url:    "https://example.com",
+			body:   []byte(`{"other_data":"abcd"}`),
+			headers: map[string]string{
+				VaultSnapshotRecoverHeader: "1234",
+			},
+			wantOperation: logical.RecoverOperation,
+			wantData: map[string]interface{}{
+				"other_data": "abcd",
+			},
+			wantRequiresSnapshotID: "1234",
+		},
 	}
 
 	for _, tc := range testCases {
@@ -1137,8 +1202,14 @@ func TestLogical_SnapshotParams(t *testing.T) {
 			req, _ := http.NewRequest(tc.method, tc.url, bytes.NewReader(tc.body))
 			req = req.WithContext(namespace.RootContext(nil))
 			req.Header.Add(consts.AuthHeaderName, rootToken)
-
+			for k, v := range tc.headers {
+				req.Header.Add(k, v)
+			}
 			lreq, _, status, err := buildLogicalRequest(core, nil, req, "")
+			if tc.wantError {
+				require.Error(t, err)
+				return
+			}
 			require.NoError(t, err)
 			require.Equal(t, 0, status)
 			require.Equal(t, tc.wantOperation, lreq.Operation)

sdk/framework/openapi.go

@@ -403,15 +403,21 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc *
 						Name:        "recover_snapshot_id",
 						Description: "Triggers a recover operation using the given snapshot ID. Request body is ignored when a recover operation is requested.",
 						In:          "query",
+						Deprecated:  true,
+						Schema:      &OASSchema{Type: "string"},
+					}, OASParameter{
+						Name:        "X-Vault-Recover-Snapshot-Id",
+						Description: "Triggers a recover operation using the given snapshot ID. Request body is ignored when a recover operation is requested.",
+						In:          "header",
 						Schema:      &OASSchema{Type: "string"},
 					})
 					// If there are path fields, it will also be possible to recover from a path with
 					// different path field values than the request path.
 					if len(pathFields) > 0 {
 						op.Parameters = append(op.Parameters, OASParameter{
-							Name:        "recover_source_path",
-							Description: "The source path to recover from. Only used if recover_snapshot_id parameter is also supplied. If not specified, the source path is assumed to be the same as the request path.",
-							In:          "query",
+							Name:        "X-Vault-Recover-Source-Path",
+							Description: "The source path to recover from. Only used if a snapshot ID is also supplied. If not specified, the source path is assumed to be the same as the request path.",
+							In:          "header",
 							Schema:      &OASSchema{Type: "string"},
 						})
 					}

sdk/framework/testdata/operations.json

@@ -71,14 +71,23 @@
             "name": "recover_snapshot_id",
             "description": "Triggers a recover operation using the given snapshot ID. Request body is ignored when a recover operation is requested.",
             "in": "query",
+            "schema": {
+              "type": "string"
+            },
+            "deprecated": true
+          },
+          {
+            "name": "X-Vault-Recover-Snapshot-Id",
+            "description": "Triggers a recover operation using the given snapshot ID. Request body is ignored when a recover operation is requested.",
+            "in": "header",
             "schema": {
               "type": "string"
             }
           },
           {
-            "name": "recover_source_path",
-            "description": "The source path to recover from. Only used if recover_snapshot_id parameter is also supplied. If not specified, the source path is assumed to be the same as the request path.",
-            "in": "query",
+            "name": "X-Vault-Recover-Source-Path",
+            "description": "The source path to recover from. Only used if a snapshot ID is also supplied. If not specified, the source path is assumed to be the same as the request path.",
+            "in": "header",
             "schema": {
               "type": "string"
             }

sdk/framework/testdata/operations_recover.json

@@ -43,11 +43,20 @@
         "parameters": [
           {
             "name": "recover_snapshot_id",
+            "deprecated": true,
             "description": "Triggers a recover operation using the given snapshot ID. Request body is ignored when a recover operation is requested.",
             "in": "query",
             "schema": {
               "type": "string"
             }
+          },
+          {
+            "name": "X-Vault-Recover-Snapshot-Id",
+            "description": "Triggers a recover operation using the given snapshot ID. Request body is ignored when a recover operation is requested.",
+            "in": "header",
+            "schema": {
+              "type": "string"
+            }
           }
         ],
         "requestBody": {

sdk/helper/consts/consts.go

@@ -48,5 +48,5 @@ const (
 
 	DRReplicationPathTarget = "dr"
 
-	RecoverSourcePathParam = "recover_source_path"
+	RecoverSourcePathHeader = "X-Vault-Recover-Source-Path"
 )