From 146e8f6c9c5d31df28b98ad2b546ed01540de1b5 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Thu, 10 May 2018 15:40:03 -0400 Subject: [PATCH] Fix response wrapping from K/V version 2 (#4511) This takes place in two parts, since working on this exposed an issue with response wrapping when there is a raw body set. The changes are (in diff order): * A CurrentWrappingLookupFunc has been added to return the current value. This is necessary for the lookahead call since we don't want the lookahead call to be wrapped. * Support for unwrapping < 0.6.2 tokens via the API/CLI has been removed, because we now have backends returning 404s with data and can't rely on the 404 trick. These can still be read manually via cubbyhole/response. * KV preflight version request now ensures that its calls is not wrapped, and restores any given function after. * When responding with a raw body, instead of always base64-decoding a string value and erroring on failure, on failure we assume that it simply wasn't a base64-encoded value and use it as is. * A test that fails on master and works now that ensures that raw body responses that are wrapped and then unwrapped return the expected values. * A flag for response data that indicates to the wrapping handling that the data contained therein is already JSON decoded (more later). * RespondWithStatusCode now defaults to a string so that the value is HMAC'd during audit. The function always JSON encodes the body, so before now it was always returning []byte which would skip HMACing. We don't know what's in the data, so this is a "better safe than sorry" issue. If different behavior is needed, backends can always manually populate the data instead of relying on the helper function. * We now check unwrapped data after unwrapping to see if there were raw flags. If so, we try to detect whether the value can be unbase64'd. The reason is that if it can it was probably originally a []byte and shouldn't be audit HMAC'd; if not, it was probably originally a string and should be. In either case, we then set the value as the raw body and hit the flag indicating that it's already been JSON decoded so not to try again before auditing. Doing it this way ensures the right typing. * There is now a check to see if the data coming from unwrapping is already JSON decoded and if so the decoding is skipped before setting the audit response. --- CHANGELOG.md | 4 +++ api/client.go | 9 +++++ api/logical.go | 60 ++++++++----------------------- command/kv_helpers.go | 6 ++++ http/logical.go | 12 ++++--- http/unwrapping_raw_body_test.go | 62 ++++++++++++++++++++++++++++++++ logical/response.go | 13 +++++-- vault/logical_system.go | 47 ++++++++++++++++++++++++ vault/request_handling.go | 18 ++++++---- 9 files changed, 171 insertions(+), 60 deletions(-) create mode 100644 http/unwrapping_raw_body_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 41aecd277f67..462328bbe29d 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/api/client.go b/api/client.go index 1471b6185629..0780baddcffa 100644 --- a/api/client.go +++ b/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) { diff --git a/api/logical.go b/api/logical.go index d5e5afae867a..ceb2b72f8e62 100644 --- a/api/logical.go +++ b/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) } diff --git a/command/kv_helpers.go b/command/kv_helpers.go index b0e984783595..1c3c74a8c907 100644 --- a/command/kv_helpers.go +++ b/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 { diff --git a/http/logical.go b/http/logical.go index 2d09f67e5147..7b31c8751d8c 100644 --- a/http/logical.go +++ b/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) diff --git a/http/unwrapping_raw_body_test.go b/http/unwrapping_raw_body_test.go new file mode 100644 index 000000000000..358c81f5e882 --- /dev/null +++ b/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") + } +} diff --git a/logical/response.go b/logical/response.go index acfe63945c91..723f88e7d07f 100644 --- a/logical/response.go +++ b/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 } diff --git a/vault/logical_system.go b/vault/logical_system.go index f565241a03b2..afd395eaa355 100644 --- a/vault/logical_system.go +++ b/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 { diff --git a/vault/request_handling.go b/vault/request_handling.go index 022952a16e56..0e2a7892bed3 100644 --- a/vault/request_handling.go +++ b/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