Fix response wrapping from K/V version 2

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 DEPRECATIONS/CHANGES:
 
+ * As of this release, the Vault CLI (via `vault unwrap`) and Go API (via
+   `Logical().Unwrap()`) can no longer unwrap response-wrapped tokens produced
+   by Vault prior to 0.6.2. These can still be read manually by performing a
+   read on `cubbyhole/response` and decoding the JSON-encoded value.
  * PKI duration return types: The PKI backend now returns durations (e.g. when
    reading a role) as an integer number of seconds instead of a Go-style
    string, in line with how the rest of Vault's API returns durations.

api/client.go

@@ -389,6 +389,15 @@ func (c *Client) SetClientTimeout(timeout time.Duration) {
 	c.config.Timeout = timeout
 }
 
+// CurrentWrappingLookupFunc sets a lookup function that returns desired wrap TTLs
+// for a given operation and path
+func (c *Client) CurrentWrappingLookupFunc() WrappingLookupFunc {
+	c.modifyLock.RLock()
+	defer c.modifyLock.RUnlock()
+
+	return c.wrappingLookupFunc
+}
+
 // SetWrappingLookupFunc sets a lookup function that returns desired wrap TTLs
 // for a given operation and path
 func (c *Client) SetWrappingLookupFunc(lookupFunc WrappingLookupFunc) {

api/logical.go

@@ -1,14 +1,8 @@
 package api
 
 import (
-	"bytes"
-	"fmt"
 	"io"
-	"net/http"
 	"os"
-
-	"github.com/hashicorp/errwrap"
-	"github.com/hashicorp/vault/helper/jsonutil"
 )
 
 const (
@@ -188,49 +182,23 @@ func (c *Logical) Unwrap(wrappingToken string) (*Secret, error) {
 	if resp != nil {
 		defer resp.Body.Close()
 	}
-
-	// Return all errors except those that are from a 404 as we handle the not
-	// found error as a special case.
-	if err != nil && (resp == nil || resp.StatusCode != 404) {
-		return nil, err
-	}
-	if resp == nil {
-		return nil, nil
-	}
-
-	switch resp.StatusCode {
-	case http.StatusOK: // New method is supported
-		return ParseSecret(resp.Body)
-	case http.StatusNotFound: // Fall back to old method
-	default:
+	if resp != nil && resp.StatusCode == 404 {
+		secret, parseErr := ParseSecret(resp.Body)
+		switch parseErr {
+		case nil:
+		case io.EOF:
+			return nil, nil
+		default:
+			return nil, err
+		}
+		if secret != nil && (len(secret.Warnings) > 0 || len(secret.Data) > 0) {
+			return secret, nil
+		}
 		return nil, nil
 	}
-
-	if wrappingToken != "" {
-		origToken := c.c.Token()
-		defer c.c.SetToken(origToken)
-		c.c.SetToken(wrappingToken)
-	}
-
-	secret, err := c.Read(wrappedResponseLocation)
 	if err != nil {
-		return nil, errwrap.Wrapf(fmt.Sprintf("error reading %q: {{err}}", wrappedResponseLocation), err)
-	}
-	if secret == nil {
-		return nil, fmt.Errorf("no value found at %q", wrappedResponseLocation)
-	}
-	if secret.Data == nil {
-		return nil, fmt.Errorf("\"data\" not found in wrapping response")
-	}
-	if _, ok := secret.Data["response"]; !ok {
-		return nil, fmt.Errorf("\"response\" not found in wrapping response \"data\" map")
-	}
-
-	wrappedSecret := new(Secret)
-	buf := bytes.NewBufferString(secret.Data["response"].(string))
-	if err := jsonutil.DecodeJSONFromReader(buf, wrappedSecret); err != nil {
-		return nil, errwrap.Wrapf("error unmarshalling wrapped secret: {{err}}", err)
+		return nil, err
 	}
 
-	return wrappedSecret, nil
+	return ParseSecret(resp.Body)
 }

command/kv_helpers.go

@@ -41,6 +41,12 @@ func kvReadRequest(client *api.Client, path string, params map[string]string) (*
 }
 
 func kvPreflightVersionRequest(client *api.Client, path string) (string, int, error) {
+	// We don't want to use a wrapping call here so save any custom value and
+	// restore after
+	currentWrappingLookupFunc := client.CurrentWrappingLookupFunc()
+	client.SetWrappingLookupFunc(nil)
+	defer client.SetWrappingLookupFunc(currentWrappingLookupFunc)
+
 	r := client.NewRequest("GET", "/v1/sys/internal/ui/mounts/"+path)
 	resp, err := client.RawRequest(r)
 	if resp != nil {

http/logical.go

@@ -274,11 +274,13 @@ func respondRaw(w http.ResponseWriter, r *http.Request, resp *logical.Response)
 
 		switch bodyRaw.(type) {
 		case string:
-			var err error
-			body, err = base64.StdEncoding.DecodeString(bodyRaw.(string))
-			if err != nil {
-				retErr(w, "cannot decode body")
-				return
+			// This is best effort. The value may already be base64-decoded so
+			// if it doesn't work we just use as-is
+			bodyDec, err := base64.StdEncoding.DecodeString(bodyRaw.(string))
+			if err == nil {
+				body = bodyDec
+			} else {
+				body = []byte(bodyRaw.(string))
 			}
 		case []byte:
 			body = bodyRaw.([]byte)

http/unwrapping_raw_body_test.go

@@ -0,0 +1,62 @@
+package http
+
+import (
+	"testing"
+
+	kv "github.com/hashicorp/vault-plugin-secrets-kv"
+	"github.com/hashicorp/vault/api"
+	"github.com/hashicorp/vault/logical"
+	"github.com/hashicorp/vault/vault"
+)
+
+func TestUnwrapping_Raw_Body(t *testing.T) {
+	coreConfig := &vault.CoreConfig{
+		LogicalBackends: map[string]logical.Factory{
+			"kv": kv.Factory,
+		},
+	}
+	cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{
+		HandlerFunc: Handler,
+	})
+	cluster.Start()
+	defer cluster.Cleanup()
+
+	core := cluster.Cores[0].Core
+	vault.TestWaitActive(t, core)
+	client := cluster.Cores[0].Client
+
+	// Mount a k/v backend, version 2
+	err := client.Sys().Mount("kv", &api.MountInput{
+		Type:    "kv",
+		Options: map[string]string{"version": "2"},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	client.SetWrappingLookupFunc(func(operation, path string) string {
+		return "5m"
+	})
+	secret, err := client.Logical().Write("kv/foo/bar", map[string]interface{}{
+		"a": "b",
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if secret == nil {
+		t.Fatal("nil secret")
+	}
+	if secret.WrapInfo == nil {
+		t.Fatal("nil wrap info")
+	}
+	wrapToken := secret.WrapInfo.Token
+
+	client.SetWrappingLookupFunc(nil)
+	secret, err = client.Logical().Unwrap(wrapToken)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(secret.Warnings) != 1 {
+		t.Fatal("expected 1 warning")
+	}
+}

logical/response.go

@@ -24,6 +24,12 @@ const (
 	// This can only be specified for non-secrets, and should should be similarly
 	// avoided like the HTTPContentType. The value must be an integer.
 	HTTPStatusCode = "http_status_code"
+
+	// For unwrapping we may need to know whether the value contained in the
+	// raw body is already JSON-unmarshaled. The presence of this key indicates
+	// that it has already been unmarshaled. That way we don't need to simply
+	// ignore errors.
+	HTTPRawBodyAlreadyJSONDecoded = "http_raw_body_already_json_decoded"
 )
 
 // Response is a struct that stores the response of a request.
@@ -146,8 +152,11 @@ func RespondWithStatusCode(resp *Response, req *Request, code int) (*Response, e
 	return &Response{
 		Data: map[string]interface{}{
 			HTTPContentType: "application/json",
-			HTTPRawBody:     body,
-			HTTPStatusCode:  code,
+			// We default to string here so that the value is HMAC'd via audit.
+			// Since this function is always marshaling to JSON, this is
+			// appropriate.
+			HTTPRawBody:    string(body),
+			HTTPStatusCode: code,
 		},
 	}, nil
 }

vault/logical_system.go

@@ -23,6 +23,7 @@ import (
 	"github.com/hashicorp/vault/helper/compressutil"
 	"github.com/hashicorp/vault/helper/consts"
 	"github.com/hashicorp/vault/helper/identity"
+	"github.com/hashicorp/vault/helper/jsonutil"
 	"github.com/hashicorp/vault/helper/parseutil"
 	"github.com/hashicorp/vault/helper/strutil"
 	"github.com/hashicorp/vault/helper/wrapping"
@@ -3116,6 +3117,52 @@ func (b *SystemBackend) handleWrappingUnwrap(ctx context.Context, req *logical.R
 	resp := &logical.Response{
 		Data: map[string]interface{}{},
 	}
+
+	// Most of the time we want to just send over the marshalled HTTP bytes.
+	// However there is a sad separate case: if the original response was using
+	// bare values we need to use those or else what comes back is garbled.
+	httpResp := &logical.HTTPResponse{}
+	err = jsonutil.DecodeJSON([]byte(response), httpResp)
+	if err != nil {
+		return nil, errwrap.Wrapf("error decoding wrapped response: {{err}}", err)
+	}
+	if httpResp.Data != nil &&
+		(httpResp.Data[logical.HTTPStatusCode] != nil ||
+			httpResp.Data[logical.HTTPRawBody] != nil ||
+			httpResp.Data[logical.HTTPContentType] != nil) {
+		if httpResp.Data[logical.HTTPStatusCode] != nil {
+			resp.Data[logical.HTTPStatusCode] = httpResp.Data[logical.HTTPStatusCode]
+		}
+		if httpResp.Data[logical.HTTPContentType] != nil {
+			resp.Data[logical.HTTPContentType] = httpResp.Data[logical.HTTPContentType]
+		}
+
+		rawBody := httpResp.Data[logical.HTTPRawBody]
+		if rawBody != nil {
+			// Decode here so that we can audit properly
+			switch rawBody.(type) {
+			case string:
+				// Best effort decoding; if this works, the original value was
+				// probably a []byte instead of a string, but was marshaled
+				// when the value was saved, so this restores it as it was
+				decBytes, err := base64.StdEncoding.DecodeString(rawBody.(string))
+				if err == nil {
+					// We end up with []byte, will not be HMAC'd
+					resp.Data[logical.HTTPRawBody] = decBytes
+				} else {
+					// We end up with string, will be HMAC'd
+					resp.Data[logical.HTTPRawBody] = rawBody
+				}
+			default:
+				b.Core.Logger().Error("unexpected type of raw body when decoding wrapped token", "type", fmt.Sprintf("%T", rawBody))
+			}
+
+			resp.Data[logical.HTTPRawBodyAlreadyJSONDecoded] = true
+		}
+
+		return resp, nil
+	}
+
 	if len(response) == 0 {
 		resp.Data[logical.HTTPStatusCode] = 204
 	} else {

vault/request_handling.go

@@ -105,14 +105,18 @@ func (c *Core) HandleRequest(req *logical.Request) (resp *logical.Response, err
 		resp.Data[logical.HTTPRawBody] != nil {
 
 		// Decode the JSON
-		httpResp := &logical.HTTPResponse{}
-		err := jsonutil.DecodeJSON(resp.Data[logical.HTTPRawBody].([]byte), httpResp)
-		if err != nil {
-			c.logger.Error("failed to unmarshal wrapped HTTP response for audit logging", "error", err)
-			return nil, ErrInternalError
-		}
+		if resp.Data[logical.HTTPRawBodyAlreadyJSONDecoded] != nil {
+			delete(resp.Data, logical.HTTPRawBodyAlreadyJSONDecoded)
+		} else {
+			httpResp := &logical.HTTPResponse{}
+			err := jsonutil.DecodeJSON(resp.Data[logical.HTTPRawBody].([]byte), httpResp)
+			if err != nil {
+				c.logger.Error("failed to unmarshal wrapped HTTP response for audit logging", "error", err)
+				return nil, ErrInternalError
+			}
 
-		auditResp = logical.HTTPResponseToLogicalResponse(httpResp)
+			auditResp = logical.HTTPResponseToLogicalResponse(httpResp)
+		}
 	}
 
 	var nonHMACReqDataKeys []string