From e66a800fb9ab18907ba42b86de1a82c479901bf6 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Fri, 5 Apr 2019 11:09:03 -0700 Subject: [PATCH 01/12] audit: log invalid wrapping token request/response --- helper/consts/error.go | 3 ++ http/handler.go | 19 ----------- http/logical.go | 15 ++++----- vault/request_handling.go | 69 ++++++++++++++++++++++++++++++++++----- vault/wrapping.go | 22 ++++++++----- 5 files changed, 83 insertions(+), 45 deletions(-) diff --git a/helper/consts/error.go b/helper/consts/error.go index 06977d5d5a4c..ec1967815d73 100644 --- a/helper/consts/error.go +++ b/helper/consts/error.go @@ -13,4 +13,7 @@ var ( // Used when .. is used in a path ErrPathContainsParentReferences = errors.New("path cannot contain parent references") + + // ErrInvalidToken + ErrInvalidWrappingToken = errors.New("wrapping token is not valid or does not exist") ) diff --git a/http/handler.go b/http/handler.go index abd2ca303bc5..b31a8d833101 100644 --- a/http/handler.go +++ b/http/handler.go @@ -284,25 +284,6 @@ func WrapForwardedForHandler(h http.Handler, authorizedAddrs []*sockaddr.SockAdd }) } -// A lookup on a token that is about to expire returns nil, which means by the -// time we can validate a wrapping token lookup will return nil since it will -// be revoked after the call. So we have to do the validation here. -func wrappingVerificationFunc(ctx context.Context, core *vault.Core, req *logical.Request) error { - if req == nil { - return fmt.Errorf("invalid request") - } - - valid, err := core.ValidateWrappingToken(ctx, req) - if err != nil { - return errwrap.Wrapf("error validating wrapping token: {{err}}", err) - } - if !valid { - return fmt.Errorf("wrapping token is not valid or does not exist") - } - - return nil -} - // stripPrefix is a helper to strip a prefix from the path. It will // return false from the second return value if it the prefix doesn't exist. func stripPrefix(prefix, path string) (string, bool) { diff --git a/http/logical.go b/http/logical.go index 99bd65813a8e..21f24328bb17 100644 --- a/http/logical.go +++ b/http/logical.go @@ -12,7 +12,7 @@ import ( "time" "github.com/hashicorp/errwrap" - uuid "github.com/hashicorp/go-uuid" + "github.com/hashicorp/go-uuid" "github.com/hashicorp/vault/helper/namespace" "github.com/hashicorp/vault/logical" "github.com/hashicorp/vault/vault" @@ -187,14 +187,11 @@ func handleLogicalInternal(core *vault.Core, injectDataIntoTopLevel bool) http.H switch req.Path { case "sys/wrapping/lookup", "sys/wrapping/rewrap", "sys/wrapping/unwrap": r = r.WithContext(newCtx) - if err := wrappingVerificationFunc(r.Context(), core, req); err != nil { - if errwrap.Contains(err, logical.ErrPermissionDenied.Error()) { - respondError(w, http.StatusForbidden, err) - } else { - respondError(w, http.StatusBadRequest, err) - } - return - } + // We perform validation but don't check the error here yet so + // that the request/response can be logged once we're in the + // logical layer. This is more for logical.Request modification + // depending on how the wrapping token was provided. + _, _ = core.ValidateWrappingToken(r.Context(), req, true) // The -self paths have no meaning outside of the token NS, so // requests for these paths always go to the token NS diff --git a/vault/request_handling.go b/vault/request_handling.go index c753b9129d74..a9aaa1dce3ce 100644 --- a/vault/request_handling.go +++ b/vault/request_handling.go @@ -584,6 +584,32 @@ func (c *Core) handleRequest(ctx context.Context, req *logical.Request) (retResp return nil, nil, ctErr } + // If this is a wrapping request, check for token validity before routing + switch req.Path { + case "sys/wrapping/rewrap", "sys/wrapping/unwrap": + valid, err := c.ValidateWrappingToken(ctx, req, false) + // Perform audit logging before returning if there's an issue with checking + // the wrapping token + if err != nil || !valid { + logInput := &audit.LogInput{ + Auth: auth, + Request: req, + OuterErr: ctErr, + NonHMACReqDataKeys: nonHMACReqDataKeys, + } + if err := c.auditBroker.LogRequest(ctx, logInput, c.auditedHeaders); err != nil { + c.logger.Error("failed to audit request", "path", req.Path, "error", err) + } + } + + if err != nil { + return nil, nil, errwrap.Wrapf("error validating wrapping token: {{err}}", err) + } + if !valid { + return logical.ErrorResponse(consts.ErrInvalidWrappingToken.Error()), nil, logical.ErrInvalidRequest + } + } + // We run this logic first because we want to decrement the use count even // in the case of an error (assuming we can successfully look up; if we // need to forward, we exit before now) @@ -901,6 +927,15 @@ func (c *Core) handleRequest(ctx context.Context, req *logical.Request) (retResp func (c *Core) handleLoginRequest(ctx context.Context, req *logical.Request) (retResp *logical.Response, retAuth *logical.Auth, retErr error) { defer metrics.MeasureSince([]string{"core", "handle_login_request"}, time.Now()) + var nonHMACReqDataKeys []string + entry := c.router.MatchingMountEntry(ctx, req.Path) + if entry != nil { + // Get and set ignored HMAC'd value. + if rawVals, ok := entry.synthesizedConfigCache.Load("audit_non_hmac_request_keys"); ok { + nonHMACReqDataKeys = rawVals.([]string) + } + } + req.Unauthenticated = true var auth *logical.Auth @@ -922,15 +957,6 @@ func (c *Core) handleLoginRequest(ctx context.Context, req *logical.Request) (re errType = logical.ErrInvalidRequest } - var nonHMACReqDataKeys []string - entry := c.router.MatchingMountEntry(ctx, req.Path) - if entry != nil { - // Get and set ignored HMAC'd value. - if rawVals, ok := entry.synthesizedConfigCache.Load("audit_non_hmac_request_keys"); ok { - nonHMACReqDataKeys = rawVals.([]string) - } - } - logInput := &audit.LogInput{ Auth: auth, Request: req, @@ -951,6 +977,31 @@ func (c *Core) handleLoginRequest(ctx context.Context, req *logical.Request) (re return logical.ErrorResponse(ctErr.Error()), auth, retErr } + // If this is a wrapping token lookup, validate the token and produce an audit log + switch req.Path { + case "sys/wrapping/lookup": + valid, err := c.ValidateWrappingToken(ctx, req, false) + // Perform audit logging before returning if there's an issue with checking + // the wrapping token + if err != nil || !valid { + logInput := &audit.LogInput{ + Auth: auth, + Request: req, + OuterErr: ctErr, + NonHMACReqDataKeys: nonHMACReqDataKeys, + } + if err := c.auditBroker.LogRequest(ctx, logInput, c.auditedHeaders); err != nil { + c.logger.Error("failed to audit request", "path", req.Path, "error", err) + } + } + if err != nil { + return nil, nil, errwrap.Wrapf("error validating wrapping token: {{err}}", err) + } + if !valid { + return logical.ErrorResponse(consts.ErrInvalidWrappingToken.Error()), nil, logical.ErrInvalidRequest + } + } + // Create an audit trail of the request. Attach auth if it was returned, // e.g. if a token was provided. logInput := &audit.LogInput{ diff --git a/vault/wrapping.go b/vault/wrapping.go index e3e59c68de79..41f0b6387730 100644 --- a/vault/wrapping.go +++ b/vault/wrapping.go @@ -309,16 +309,20 @@ DONELISTHANDLING: return nil, nil } -// ValidateWrappingToken checks whether a token is a wrapping token. -func (c *Core) ValidateWrappingToken(ctx context.Context, req *logical.Request) (bool, error) { +// ValidateWrappingToken checks whether a token is a wrapping token. The passed +// in logical request can be optionally updated if the wrapping token was +// provided within a JWT token. +func (c *Core) ValidateWrappingToken(ctx context.Context, req *logical.Request, updateRequest bool) (bool, error) { if req == nil { return false, fmt.Errorf("invalid request") } var err error - var token string var thirdParty bool + + // Check if the wrapping token is coming from the request body, and it not + // assume that req.ClientToken is the wrapping token if req.Data != nil && req.Data["token"] != nil { thirdParty = true if tokenStr, ok := req.Data["token"].(string); !ok { @@ -360,10 +364,12 @@ func (c *Core) ValidateWrappingToken(ctx context.Context, req *logical.Request) if typeClaim != "wrapping" { return false, errors.New("unexpected type claim") } - if !thirdParty { - req.ClientToken = claims.ID - } else { - req.Data["token"] = claims.ID + if updateRequest { + if !thirdParty { + req.ClientToken = claims.ID + } else { + req.Data["token"] = claims.ID + } } token = claims.ID } @@ -398,7 +404,7 @@ func (c *Core) ValidateWrappingToken(ctx context.Context, req *logical.Request) return false, nil } - if !thirdParty { + if !thirdParty && updateRequest { req.ClientTokenAccessor = te.Accessor req.ClientTokenRemainingUses = te.NumUses req.SetTokenEntry(te) From b6c2edf2d3d39ae588859b7ff239304722e64d89 Mon Sep 17 00:00:00 2001 From: Vishal Nayak Date: Fri, 5 Apr 2019 11:25:05 -0700 Subject: [PATCH 02/12] Update helper/consts/error.go Co-Authored-By: calvn --- helper/consts/error.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/helper/consts/error.go b/helper/consts/error.go index ec1967815d73..bcdf50197d62 100644 --- a/helper/consts/error.go +++ b/helper/consts/error.go @@ -14,6 +14,6 @@ var ( // Used when .. is used in a path ErrPathContainsParentReferences = errors.New("path cannot contain parent references") - // ErrInvalidToken + // ErrInvalidWrappingToken ErrInvalidWrappingToken = errors.New("wrapping token is not valid or does not exist") ) From dabb42704e8703f655999e782eff20084e5ec32d Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Fri, 5 Apr 2019 11:27:38 -0700 Subject: [PATCH 03/12] update error comments --- helper/consts/error.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/helper/consts/error.go b/helper/consts/error.go index bcdf50197d62..d4e60e54e347 100644 --- a/helper/consts/error.go +++ b/helper/consts/error.go @@ -11,9 +11,11 @@ var ( // No operation is expected to succeed until active. ErrStandby = errors.New("Vault is in standby mode") - // Used when .. is used in a path + // ErrPathContainsParentReferences is returned when a path contains parent + // references. ErrPathContainsParentReferences = errors.New("path cannot contain parent references") - // ErrInvalidWrappingToken + // ErrInvalidWrappingToken is returned when checking for the validity of + // a wrapping token that turns out to be invalid. ErrInvalidWrappingToken = errors.New("wrapping token is not valid or does not exist") ) From d120e8dab3286f5fdad8507640d08cf24253c19a Mon Sep 17 00:00:00 2001 From: Vishal Nayak Date: Fri, 5 Apr 2019 11:28:46 -0700 Subject: [PATCH 04/12] Update vault/wrapping.go Co-Authored-By: calvn --- vault/wrapping.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vault/wrapping.go b/vault/wrapping.go index 41f0b6387730..16b79a02662c 100644 --- a/vault/wrapping.go +++ b/vault/wrapping.go @@ -321,7 +321,7 @@ func (c *Core) ValidateWrappingToken(ctx context.Context, req *logical.Request, var token string var thirdParty bool - // Check if the wrapping token is coming from the request body, and it not + // Check if the wrapping token is coming from the request body, and if not // assume that req.ClientToken is the wrapping token if req.Data != nil && req.Data["token"] != nil { thirdParty = true From 94b9ee66784167a932019339471c69bd72f7bb70 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Fri, 5 Apr 2019 11:54:10 -0700 Subject: [PATCH 05/12] update comment --- vault/request_handling.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/vault/request_handling.go b/vault/request_handling.go index a9aaa1dce3ce..73580512ce23 100644 --- a/vault/request_handling.go +++ b/vault/request_handling.go @@ -584,7 +584,8 @@ func (c *Core) handleRequest(ctx context.Context, req *logical.Request) (retResp return nil, nil, ctErr } - // If this is a wrapping request, check for token validity before routing + // If this is a wrapping request that contains a wrapping token, check for + // token validity before routing switch req.Path { case "sys/wrapping/rewrap", "sys/wrapping/unwrap": valid, err := c.ValidateWrappingToken(ctx, req, false) From 5fcf2f4f9711f4cdf4227b17b1a7ac3f067913b1 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Mon, 8 Apr 2019 12:41:39 -0700 Subject: [PATCH 06/12] move validateWrappingToken out of http and into logical --- http/logical.go | 6 +-- vault/request_handling.go | 78 ++++++++++++++++++++------------------- vault/wrapping.go | 17 ++++----- 3 files changed, 50 insertions(+), 51 deletions(-) diff --git a/http/logical.go b/http/logical.go index 21f24328bb17..fa4f9080c8de 100644 --- a/http/logical.go +++ b/http/logical.go @@ -185,13 +185,9 @@ func handleLogicalInternal(core *vault.Core, injectDataIntoTopLevel bool) http.H } } switch req.Path { + // Route the token wrapping request to its respective sys NS case "sys/wrapping/lookup", "sys/wrapping/rewrap", "sys/wrapping/unwrap": r = r.WithContext(newCtx) - // We perform validation but don't check the error here yet so - // that the request/response can be logged once we're in the - // logical layer. This is more for logical.Request modification - // depending on how the wrapping token was provided. - _, _ = core.ValidateWrappingToken(r.Context(), req, true) // The -self paths have no meaning outside of the token NS, so // requests for these paths always go to the token NS diff --git a/vault/request_handling.go b/vault/request_handling.go index 73580512ce23..38833898ea68 100644 --- a/vault/request_handling.go +++ b/vault/request_handling.go @@ -578,24 +578,20 @@ func (c *Core) handleRequest(ctx context.Context, req *logical.Request) (retResp return } - // Validate the token - auth, te, ctErr := c.checkToken(ctx, req, false) - if ctErr == logical.ErrPerfStandbyPleaseForward { - return nil, nil, ctErr - } - // If this is a wrapping request that contains a wrapping token, check for - // token validity before routing + // token validity. We perform this before c.checkToken since wrapping token + // validation performs a token store lookup, and can potentially modify + // logical.Request's token-related fields. switch req.Path { case "sys/wrapping/rewrap", "sys/wrapping/unwrap": - valid, err := c.ValidateWrappingToken(ctx, req, false) + valid, err := c.validateWrappingToken(ctx, req) + // Perform audit logging before returning if there's an issue with checking // the wrapping token if err != nil || !valid { logInput := &audit.LogInput{ - Auth: auth, + Auth: req.Auth, Request: req, - OuterErr: ctErr, NonHMACReqDataKeys: nonHMACReqDataKeys, } if err := c.auditBroker.LogRequest(ctx, logInput, c.auditedHeaders); err != nil { @@ -611,6 +607,12 @@ func (c *Core) handleRequest(ctx context.Context, req *logical.Request) (retResp } } + // Validate the token + auth, te, ctErr := c.checkToken(ctx, req, false) + if ctErr == logical.ErrPerfStandbyPleaseForward { + return nil, nil, ctErr + } + // We run this logic first because we want to decrement the use count even // in the case of an error (assuming we can successfully look up; if we // need to forward, we exit before now) @@ -937,11 +939,38 @@ func (c *Core) handleLoginRequest(ctx context.Context, req *logical.Request) (re } } - req.Unauthenticated = true + // If this is a wrapping token lookup, validate the token and produce an + // audit log. We perform this before c.checkToken since wrapping token + // validation performs a token store lookup, and can potentially modify + // logical.Request's token-related fields. + switch req.Path { + case "sys/wrapping/lookup": + valid, err := c.validateWrappingToken(ctx, req) + // Perform audit logging before returning if there's an issue with checking + // the wrapping token + if err != nil || !valid { - var auth *logical.Auth + logInput := &audit.LogInput{ + Auth: req.Auth, + Request: req, + NonHMACReqDataKeys: nonHMACReqDataKeys, + } + if err := c.auditBroker.LogRequest(ctx, logInput, c.auditedHeaders); err != nil { + c.logger.Error("failed to audit request", "path", req.Path, "error", err) + } + } + if err != nil { + return nil, nil, errwrap.Wrapf("error validating wrapping token: {{err}}", err) + } + if !valid { + return logical.ErrorResponse(consts.ErrInvalidWrappingToken.Error()), nil, logical.ErrInvalidRequest + } + } + + req.Unauthenticated = true // Do an unauth check. This will cause EGP policies to be checked + var auth *logical.Auth var ctErr error auth, _, ctErr = c.checkToken(ctx, req, true) if ctErr == logical.ErrPerfStandbyPleaseForward { @@ -978,31 +1007,6 @@ func (c *Core) handleLoginRequest(ctx context.Context, req *logical.Request) (re return logical.ErrorResponse(ctErr.Error()), auth, retErr } - // If this is a wrapping token lookup, validate the token and produce an audit log - switch req.Path { - case "sys/wrapping/lookup": - valid, err := c.ValidateWrappingToken(ctx, req, false) - // Perform audit logging before returning if there's an issue with checking - // the wrapping token - if err != nil || !valid { - logInput := &audit.LogInput{ - Auth: auth, - Request: req, - OuterErr: ctErr, - NonHMACReqDataKeys: nonHMACReqDataKeys, - } - if err := c.auditBroker.LogRequest(ctx, logInput, c.auditedHeaders); err != nil { - c.logger.Error("failed to audit request", "path", req.Path, "error", err) - } - } - if err != nil { - return nil, nil, errwrap.Wrapf("error validating wrapping token: {{err}}", err) - } - if !valid { - return logical.ErrorResponse(consts.ErrInvalidWrappingToken.Error()), nil, logical.ErrInvalidRequest - } - } - // Create an audit trail of the request. Attach auth if it was returned, // e.g. if a token was provided. logInput := &audit.LogInput{ diff --git a/vault/wrapping.go b/vault/wrapping.go index 16b79a02662c..2dbd4370a68c 100644 --- a/vault/wrapping.go +++ b/vault/wrapping.go @@ -309,10 +309,10 @@ DONELISTHANDLING: return nil, nil } -// ValidateWrappingToken checks whether a token is a wrapping token. The passed +// validateWrappingToken checks whether a token is a wrapping token. The passed // in logical request can be optionally updated if the wrapping token was // provided within a JWT token. -func (c *Core) ValidateWrappingToken(ctx context.Context, req *logical.Request, updateRequest bool) (bool, error) { +func (c *Core) validateWrappingToken(ctx context.Context, req *logical.Request) (bool, error) { if req == nil { return false, fmt.Errorf("invalid request") } @@ -364,13 +364,12 @@ func (c *Core) ValidateWrappingToken(ctx context.Context, req *logical.Request, if typeClaim != "wrapping" { return false, errors.New("unexpected type claim") } - if updateRequest { - if !thirdParty { - req.ClientToken = claims.ID - } else { - req.Data["token"] = claims.ID - } + if !thirdParty { + req.ClientToken = claims.ID + } else { + req.Data["token"] = claims.ID } + token = claims.ID } @@ -404,7 +403,7 @@ func (c *Core) ValidateWrappingToken(ctx context.Context, req *logical.Request, return false, nil } - if !thirdParty && updateRequest { + if !thirdParty { req.ClientTokenAccessor = te.Accessor req.ClientTokenRemainingUses = te.NumUses req.SetTokenEntry(te) From c4c2d72fe2641761ae959f747c64fb614c270b85 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Fri, 12 Apr 2019 11:45:06 -0700 Subject: [PATCH 07/12] minor refactor, add test cases --- vault/core_test.go | 147 +++++++++++++++++++++++++++++++++++++- vault/request_handling.go | 43 +++++++---- 2 files changed, 174 insertions(+), 16 deletions(-) diff --git a/vault/core_test.go b/vault/core_test.go index 4bf7e8c0ce77..89a813eddb8c 100644 --- a/vault/core_test.go +++ b/vault/core_test.go @@ -9,7 +9,7 @@ import ( "github.com/go-test/deep" "github.com/hashicorp/errwrap" log "github.com/hashicorp/go-hclog" - uuid "github.com/hashicorp/go-uuid" + "github.com/hashicorp/go-uuid" "github.com/hashicorp/vault/audit" "github.com/hashicorp/vault/helper/consts" "github.com/hashicorp/vault/helper/logging" @@ -904,7 +904,150 @@ func TestCore_HandleRequest_AuditTrail_noHMACKeys(t *testing.T) { } } -// Ensure we get a client token +func TestCore_HandleRequest_AuditTrail_invalidWrappingToken(t *testing.T) { + // Create a noop audit backend + var noop *NoopAudit + c, _, root := TestCoreUnsealed(t) + c.auditBackends["noop"] = func(ctx context.Context, config *audit.BackendConfig) (audit.Backend, error) { + noop = &NoopAudit{ + Config: config, + } + return noop, nil + } + + // Enable the audit backend + req := logical.TestRequest(t, logical.UpdateOperation, "sys/audit/noop") + req.Data["type"] = "noop" + req.ClientToken = root + _, err := c.HandleRequest(namespace.RootContext(nil), req) + if err != nil { + t.Fatalf("err: %v", err) + } + + { + // Make a wrapping/unwrap request with an invalid token + req := &logical.Request{ + Operation: logical.UpdateOperation, + Path: "sys/wrapping/unwrap", + Data: map[string]interface{}{ + "token": "foo", + }, + ClientToken: root, + } + resp, err := c.HandleRequest(namespace.RootContext(nil), req) + if err == nil { + t.Fatal("expected invalid request error") + } + + // Check the audit trail on request and response + if len(noop.ReqAuth) != 1 { + t.Fatalf("bad: %#v", noop) + } + auth := noop.ReqAuth[0] + if auth.ClientToken != root { + t.Fatalf("bad client token: %#v", auth) + } + if len(noop.Req) != 1 || !reflect.DeepEqual(req, noop.Req[0]) { + t.Fatalf("bad:\nexpected:\n%#v\ngot:\n%#v", req, noop.Req[0]) + } + + if len(noop.RespAuth) != 2 { + t.Fatalf("bad: %#v", noop) + } + if !reflect.DeepEqual(auth, noop.RespAuth[1]) { + t.Fatalf("bad:\nexpected:\n%#v\ngot:\n%#v", auth, noop.RespAuth[1]) + } + if len(noop.RespReq) != 2 || !reflect.DeepEqual(req, noop.RespReq[1]) { + t.Fatalf("bad:\nexpected:\n%#v\ngot:\n%#v", req, noop.RespReq[1]) + } + if len(noop.Resp) != 2 || !reflect.DeepEqual(resp, noop.Resp[1]) { + t.Fatalf("bad:\nexpected:\n%#v\ngot:\n%#v", resp, noop.Resp[1]) + } + } + + { + // Make a wrapping/rewrap request with an invalid token + req := &logical.Request{ + Operation: logical.UpdateOperation, + Path: "sys/wrapping/rewrap", + Data: map[string]interface{}{ + "token": "foo", + }, + ClientToken: root, + } + resp, err := c.HandleRequest(namespace.RootContext(nil), req) + if err == nil { + t.Fatal("expected invalid request error") + } + + // Check the audit trail on request and response + if len(noop.ReqAuth) != 2 { + t.Fatalf("bad: %#v", noop) + } + auth := noop.ReqAuth[1] + if auth.ClientToken != root { + t.Fatalf("bad client token: %#v", auth) + } + if len(noop.Req) != 2 || !reflect.DeepEqual(req, noop.Req[1]) { + t.Fatalf("bad:\nexpected:\n%#v\ngot:\n%#v", req, noop.Req[1]) + } + + if len(noop.RespAuth) != 3 { + t.Fatalf("bad: %#v", noop) + } + if !reflect.DeepEqual(auth, noop.RespAuth[2]) { + t.Fatalf("bad:\nexpected:\n%#v\ngot:\n%#v", auth, noop.RespAuth[2]) + } + if len(noop.RespReq) != 3 || !reflect.DeepEqual(req, noop.RespReq[2]) { + t.Fatalf("bad:\nexpected:\n%#v\ngot:\n%#v", req, noop.RespReq[2]) + } + if len(noop.Resp) != 3 || !reflect.DeepEqual(resp, noop.Resp[2]) { + t.Fatalf("bad:\nexpected:\n%#v\ngot:\n%#v", resp, noop.Resp[2]) + } + } + + { + // Make a wrapping/lookup request with an invalid token + req := &logical.Request{ + Operation: logical.UpdateOperation, + Path: "sys/wrapping/lookup", + Data: map[string]interface{}{ + "token": "foo", + }, + ClientToken: root, + } + resp, err := c.HandleRequest(namespace.RootContext(nil), req) + if err == nil { + t.Fatal("expected invalid request error") + } + + // Check the audit trail on request and response + if len(noop.ReqAuth) != 3 { + t.Fatalf("bad: %#v", noop) + } + auth := noop.ReqAuth[2] + if auth.ClientToken != root { + t.Fatalf("bad client token: %#v", auth) + } + if len(noop.Req) != 3 || !reflect.DeepEqual(req, noop.Req[2]) { + t.Fatalf("bad:\nexpected:\n%#v\ngot:\n%#v", req, noop.Req[2]) + } + + if len(noop.RespAuth) != 4 { + t.Fatalf("bad: %#v", noop) + } + if !reflect.DeepEqual(auth, noop.RespAuth[3]) { + t.Fatalf("bad:\nexpected:\n%#v\ngot:\n%#v", auth, noop.RespAuth[3]) + } + if len(noop.RespReq) != 4 || !reflect.DeepEqual(req, noop.RespReq[3]) { + t.Fatalf("bad:\nexpected:\n%#v\ngot:\n%#v", req, noop.RespReq[3]) + } + if len(noop.Resp) != 4 || !reflect.DeepEqual(resp, noop.Resp[3]) { + t.Fatalf("bad:\nexpected:\n%#v\ngot:\n%#v", resp, noop.Resp[3]) + } + } +} + func TestCore_HandleLogin_AuditTrail(t *testing.T) { // Create a badass credential backend that always logs in as armon noop := &NoopAudit{} diff --git a/vault/request_handling.go b/vault/request_handling.go index 38833898ea68..134eb30b4049 100644 --- a/vault/request_handling.go +++ b/vault/request_handling.go @@ -589,21 +589,28 @@ func (c *Core) handleRequest(ctx context.Context, req *logical.Request) (retResp // Perform audit logging before returning if there's an issue with checking // the wrapping token if err != nil || !valid { + // We log the Auth object like so here since the wrapping token can + // come from the header, which gets set as the ClientToken + auth := &logical.Auth{ + ClientToken: req.ClientToken, + Accessor: req.ClientTokenAccessor, + } + logInput := &audit.LogInput{ - Auth: req.Auth, + Auth: auth, Request: req, NonHMACReqDataKeys: nonHMACReqDataKeys, } if err := c.auditBroker.LogRequest(ctx, logInput, c.auditedHeaders); err != nil { c.logger.Error("failed to audit request", "path", req.Path, "error", err) } - } - if err != nil { - return nil, nil, errwrap.Wrapf("error validating wrapping token: {{err}}", err) - } - if !valid { - return logical.ErrorResponse(consts.ErrInvalidWrappingToken.Error()), nil, logical.ErrInvalidRequest + switch { + case err != nil: + return nil, auth, errwrap.Wrapf("error validating wrapping token: {{err}}", err) + case !valid: + return logical.ErrorResponse(consts.ErrInvalidWrappingToken.Error()), auth, logical.ErrInvalidRequest + } } } @@ -949,21 +956,29 @@ func (c *Core) handleLoginRequest(ctx context.Context, req *logical.Request) (re // Perform audit logging before returning if there's an issue with checking // the wrapping token if err != nil || !valid { + // We log the Auth object like so here since the wrapping token can + // come from the header, which gets set as the ClientToken + auth := &logical.Auth{ + ClientToken: req.ClientToken, + Accessor: req.ClientTokenAccessor, + } logInput := &audit.LogInput{ - Auth: req.Auth, + Auth: auth, Request: req, NonHMACReqDataKeys: nonHMACReqDataKeys, } if err := c.auditBroker.LogRequest(ctx, logInput, c.auditedHeaders); err != nil { c.logger.Error("failed to audit request", "path", req.Path, "error", err) } - } - if err != nil { - return nil, nil, errwrap.Wrapf("error validating wrapping token: {{err}}", err) - } - if !valid { - return logical.ErrorResponse(consts.ErrInvalidWrappingToken.Error()), nil, logical.ErrInvalidRequest + + switch { + case err != nil: + return nil, auth, errwrap.Wrapf("error validating wrapping token: {{err}}", err) + case !valid: + return logical.ErrorResponse(consts.ErrInvalidWrappingToken.Error()), auth, logical.ErrInvalidRequest + + } } } From 51390e9dfc8d3fa3994ee0a38cdc4fd7b58ac602 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Fri, 12 Apr 2019 13:28:37 -0700 Subject: [PATCH 08/12] comment rewording --- vault/wrapping.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/vault/wrapping.go b/vault/wrapping.go index 2dbd4370a68c..79d283ab76d9 100644 --- a/vault/wrapping.go +++ b/vault/wrapping.go @@ -310,8 +310,8 @@ DONELISTHANDLING: } // validateWrappingToken checks whether a token is a wrapping token. The passed -// in logical request can be optionally updated if the wrapping token was -// provided within a JWT token. +// in logical request will be updated if the wrapping token was provided within +// a JWT token. func (c *Core) validateWrappingToken(ctx context.Context, req *logical.Request) (bool, error) { if req == nil { return false, fmt.Errorf("invalid request") From 37556fba0bace8fe20defc740978110c441f4e89 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Wed, 3 Jul 2019 14:32:40 -0700 Subject: [PATCH 09/12] refactor validateWrappingToken to perform audit logging --- vault/request_handling.go | 66 +++++++-------------------------------- vault/wrapping.go | 57 ++++++++++++++++++++++----------- 2 files changed, 51 insertions(+), 72 deletions(-) diff --git a/vault/request_handling.go b/vault/request_handling.go index c8dada573bf4..7dcfa00fde92 100644 --- a/vault/request_handling.go +++ b/vault/request_handling.go @@ -590,33 +590,12 @@ func (c *Core) handleRequest(ctx context.Context, req *logical.Request) (retResp // logical.Request's token-related fields. switch req.Path { case "sys/wrapping/rewrap", "sys/wrapping/unwrap": - valid, err := c.validateWrappingToken(ctx, req) - - // Perform audit logging before returning if there's an issue with checking - // the wrapping token - if err != nil || !valid { - // We log the Auth object like so here since the wrapping token can - // come from the header, which gets set as the ClientToken - auth := &logical.Auth{ - ClientToken: req.ClientToken, - Accessor: req.ClientTokenAccessor, - } - - logInput := &audit.LogInput{ - Auth: auth, - Request: req, - NonHMACReqDataKeys: nonHMACReqDataKeys, - } - if err := c.auditBroker.LogRequest(ctx, logInput, c.auditedHeaders); err != nil { - c.logger.Error("failed to audit request", "path", req.Path, "error", err) - } - - switch { - case err != nil: - return nil, auth, errwrap.Wrapf("error validating wrapping token: {{err}}", err) - case !valid: - return logical.ErrorResponse(consts.ErrInvalidWrappingToken.Error()), auth, logical.ErrInvalidRequest - } + valid, wtAuth, err := c.validateWrappingToken(ctx, req, nonHMACReqDataKeys) + if err != nil { + return nil, wtAuth, errwrap.Wrapf("error validating wrapping token: {{err}}", err) + } + if !valid { + return logical.ErrorResponse(consts.ErrInvalidWrappingToken.Error()), wtAuth, logical.ErrInvalidRequest } } @@ -958,33 +937,12 @@ func (c *Core) handleLoginRequest(ctx context.Context, req *logical.Request) (re // logical.Request's token-related fields. switch req.Path { case "sys/wrapping/lookup": - valid, err := c.validateWrappingToken(ctx, req) - // Perform audit logging before returning if there's an issue with checking - // the wrapping token - if err != nil || !valid { - // We log the Auth object like so here since the wrapping token can - // come from the header, which gets set as the ClientToken - auth := &logical.Auth{ - ClientToken: req.ClientToken, - Accessor: req.ClientTokenAccessor, - } - - logInput := &audit.LogInput{ - Auth: auth, - Request: req, - NonHMACReqDataKeys: nonHMACReqDataKeys, - } - if err := c.auditBroker.LogRequest(ctx, logInput, c.auditedHeaders); err != nil { - c.logger.Error("failed to audit request", "path", req.Path, "error", err) - } - - switch { - case err != nil: - return nil, auth, errwrap.Wrapf("error validating wrapping token: {{err}}", err) - case !valid: - return logical.ErrorResponse(consts.ErrInvalidWrappingToken.Error()), auth, logical.ErrInvalidRequest - - } + valid, wtAuth, err := c.validateWrappingToken(ctx, req, nonHMACReqDataKeys) + if err != nil { + return nil, wtAuth, errwrap.Wrapf("error validating wrapping token: {{err}}", err) + } + if !valid { + return logical.ErrorResponse(consts.ErrInvalidWrappingToken.Error()), wtAuth, logical.ErrInvalidRequest } } diff --git a/vault/wrapping.go b/vault/wrapping.go index b538c463eb61..4f7582553926 100644 --- a/vault/wrapping.go +++ b/vault/wrapping.go @@ -312,12 +312,33 @@ DONELISTHANDLING: // validateWrappingToken checks whether a token is a wrapping token. The passed // in logical request will be updated if the wrapping token was provided within // a JWT token. -func (c *Core) validateWrappingToken(ctx context.Context, req *logical.Request) (bool, error) { +func (c *Core) validateWrappingToken(ctx context.Context, req *logical.Request, nonHMACReqDataKeys []string) (valid bool, auth *logical.Auth, err error) { + defer func() { + // Perform audit logging before returning if there's an issue with checking + // the wrapping token + if err != nil || !valid { + // We log the Auth object like so here since the wrapping token can + // come from the header, which gets set as the ClientToken + auth = &logical.Auth{ + ClientToken: req.ClientToken, + Accessor: req.ClientTokenAccessor, + } + + logInput := &logical.LogInput{ + Auth: auth, + Request: req, + NonHMACReqDataKeys: nonHMACReqDataKeys, + } + if err := c.auditBroker.LogRequest(ctx, logInput, c.auditedHeaders); err != nil { + c.logger.Error("failed to audit request", "path", req.Path, "error", err) + } + } + }() + if req == nil { - return false, fmt.Errorf("invalid request") + return false, nil, fmt.Errorf("invalid request") } - var err error var token string var thirdParty bool @@ -326,9 +347,9 @@ func (c *Core) validateWrappingToken(ctx context.Context, req *logical.Request) if req.Data != nil && req.Data["token"] != nil { thirdParty = true if tokenStr, ok := req.Data["token"].(string); !ok { - return false, fmt.Errorf("could not decode token in request body") + return false, nil, fmt.Errorf("could not decode token in request body") } else if tokenStr == "" { - return false, fmt.Errorf("empty token in request body") + return false, nil, fmt.Errorf("empty token in request body") } else { token = tokenStr } @@ -346,23 +367,23 @@ func (c *Core) validateWrappingToken(ctx context.Context, req *logical.Request) // Implement the jose library way parsedJWT, err := squarejwt.ParseSigned(token) if err != nil { - return false, errwrap.Wrapf("wrapping token could not be parsed: {{err}}", err) + return false, nil, errwrap.Wrapf("wrapping token could not be parsed: {{err}}", err) } var claims squarejwt.Claims var allClaims = make(map[string]interface{}) if err = parsedJWT.Claims(&c.wrappingJWTKey.PublicKey, &claims, &allClaims); err != nil { - return false, errwrap.Wrapf("wrapping token signature could not be validated: {{err}}", err) + return false, nil, errwrap.Wrapf("wrapping token signature could not be validated: {{err}}", err) } typeClaimRaw, ok := allClaims["type"] if !ok { - return false, errors.New("could not validate type claim") + return false, nil, errors.New("could not validate type claim") } typeClaim, ok := typeClaimRaw.(string) if !ok { - return false, errors.New("could not parse type claim") + return false, nil, errors.New("could not parse type claim") } if typeClaim != "wrapping" { - return false, errors.New("unexpected type claim") + return false, nil, errors.New("unexpected type claim") } if !thirdParty { req.ClientToken = claims.ID @@ -374,33 +395,33 @@ func (c *Core) validateWrappingToken(ctx context.Context, req *logical.Request) } if token == "" { - return false, fmt.Errorf("token is empty") + return false, nil, fmt.Errorf("token is empty") } if c.Sealed() { - return false, consts.ErrSealed + return false, nil, consts.ErrSealed } c.stateLock.RLock() defer c.stateLock.RUnlock() if c.standby && !c.perfStandby { - return false, consts.ErrStandby + return false, nil, consts.ErrStandby } te, err := c.tokenStore.Lookup(ctx, token) if err != nil { - return false, err + return false, nil, err } if te == nil { - return false, nil + return false, nil, nil } if len(te.Policies) != 1 { - return false, nil + return false, nil, nil } if te.Policies[0] != responseWrappingPolicyName && te.Policies[0] != controlGroupPolicyName { - return false, nil + return false, nil, nil } if !thirdParty { @@ -409,5 +430,5 @@ func (c *Core) validateWrappingToken(ctx context.Context, req *logical.Request) req.SetTokenEntry(te) } - return true, nil + return true, nil, nil } From 732d3b511950360eea19267fe2c1b0a6305a49f6 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Wed, 3 Jul 2019 17:21:02 -0700 Subject: [PATCH 10/12] move ValidateWrappingToken back to wrappingVerificationFunc --- http/handler.go | 19 ++++++++++++++++ http/logical.go | 7 ++++++ vault/request_handling.go | 30 ------------------------- vault/wrapping.go | 46 ++++++++++++++++++++++++--------------- 4 files changed, 54 insertions(+), 48 deletions(-) diff --git a/http/handler.go b/http/handler.go index 92661ed1637d..7a8d2023e871 100644 --- a/http/handler.go +++ b/http/handler.go @@ -295,6 +295,25 @@ func WrapForwardedForHandler(h http.Handler, authorizedAddrs []*sockaddr.SockAdd }) } +// A lookup on a token that is about to expire returns nil, which means by the +// time we can validate a wrapping token lookup will return nil since it will +// be revoked after the call. So we have to do the validation here. +func wrappingVerificationFunc(ctx context.Context, core *vault.Core, req *logical.Request) error { + if req == nil { + return fmt.Errorf("invalid request") + } + + valid, err := core.ValidateWrappingToken(ctx, req) + if err != nil { + return errwrap.Wrapf("error validating wrapping token: {{err}}", err) + } + if !valid { + return fmt.Errorf("wrapping token is not valid or does not exist") + } + + return nil +} + // stripPrefix is a helper to strip a prefix from the path. It will // return false from the second return value if it the prefix doesn't exist. func stripPrefix(prefix, path string) (string, bool) { diff --git a/http/logical.go b/http/logical.go index 50529f945866..6b2d6c8ddc1c 100644 --- a/http/logical.go +++ b/http/logical.go @@ -211,6 +211,13 @@ func handleLogicalInternal(core *vault.Core, injectDataIntoTopLevel bool) http.H // Route the token wrapping request to its respective sys NS case "sys/wrapping/lookup", "sys/wrapping/rewrap", "sys/wrapping/unwrap": r = r.WithContext(newCtx) + if err := wrappingVerificationFunc(r.Context(), core, req); err != nil { + if errwrap.Contains(err, logical.ErrPermissionDenied.Error()) { + respondError(w, http.StatusForbidden, err) + } else { + respondError(w, http.StatusBadRequest, err) + } + } // The -self paths have no meaning outside of the token NS, so // requests for these paths always go to the token NS diff --git a/vault/request_handling.go b/vault/request_handling.go index 7dcfa00fde92..0e3f6d54b934 100644 --- a/vault/request_handling.go +++ b/vault/request_handling.go @@ -584,21 +584,6 @@ func (c *Core) handleRequest(ctx context.Context, req *logical.Request) (retResp return } - // If this is a wrapping request that contains a wrapping token, check for - // token validity. We perform this before c.checkToken since wrapping token - // validation performs a token store lookup, and can potentially modify - // logical.Request's token-related fields. - switch req.Path { - case "sys/wrapping/rewrap", "sys/wrapping/unwrap": - valid, wtAuth, err := c.validateWrappingToken(ctx, req, nonHMACReqDataKeys) - if err != nil { - return nil, wtAuth, errwrap.Wrapf("error validating wrapping token: {{err}}", err) - } - if !valid { - return logical.ErrorResponse(consts.ErrInvalidWrappingToken.Error()), wtAuth, logical.ErrInvalidRequest - } - } - // Validate the token auth, te, ctErr := c.checkToken(ctx, req, false) if ctErr == logical.ErrPerfStandbyPleaseForward { @@ -931,21 +916,6 @@ func (c *Core) handleLoginRequest(ctx context.Context, req *logical.Request) (re } } - // If this is a wrapping token lookup, validate the token and produce an - // audit log. We perform this before c.checkToken since wrapping token - // validation performs a token store lookup, and can potentially modify - // logical.Request's token-related fields. - switch req.Path { - case "sys/wrapping/lookup": - valid, wtAuth, err := c.validateWrappingToken(ctx, req, nonHMACReqDataKeys) - if err != nil { - return nil, wtAuth, errwrap.Wrapf("error validating wrapping token: {{err}}", err) - } - if !valid { - return logical.ErrorResponse(consts.ErrInvalidWrappingToken.Error()), wtAuth, logical.ErrInvalidRequest - } - } - req.Unauthenticated = true // Do an unauth check. This will cause EGP policies to be checked diff --git a/vault/wrapping.go b/vault/wrapping.go index 4f7582553926..d635ac4a0b9d 100644 --- a/vault/wrapping.go +++ b/vault/wrapping.go @@ -312,14 +312,24 @@ DONELISTHANDLING: // validateWrappingToken checks whether a token is a wrapping token. The passed // in logical request will be updated if the wrapping token was provided within // a JWT token. -func (c *Core) validateWrappingToken(ctx context.Context, req *logical.Request, nonHMACReqDataKeys []string) (valid bool, auth *logical.Auth, err error) { +func (c *Core) ValidateWrappingToken(ctx context.Context, req *logical.Request) (valid bool, err error) { defer func() { // Perform audit logging before returning if there's an issue with checking // the wrapping token if err != nil || !valid { + // Get non-HMAC'ed request data keys + var nonHMACReqDataKeys []string + entry := c.router.MatchingMountEntry(ctx, req.Path) + if entry != nil { + // Get and set ignored HMAC'd value. + if rawVals, ok := entry.synthesizedConfigCache.Load("audit_non_hmac_request_keys"); ok { + nonHMACReqDataKeys = rawVals.([]string) + } + } + // We log the Auth object like so here since the wrapping token can // come from the header, which gets set as the ClientToken - auth = &logical.Auth{ + auth := &logical.Auth{ ClientToken: req.ClientToken, Accessor: req.ClientTokenAccessor, } @@ -336,7 +346,7 @@ func (c *Core) validateWrappingToken(ctx context.Context, req *logical.Request, }() if req == nil { - return false, nil, fmt.Errorf("invalid request") + return false, fmt.Errorf("invalid request") } var token string @@ -347,9 +357,9 @@ func (c *Core) validateWrappingToken(ctx context.Context, req *logical.Request, if req.Data != nil && req.Data["token"] != nil { thirdParty = true if tokenStr, ok := req.Data["token"].(string); !ok { - return false, nil, fmt.Errorf("could not decode token in request body") + return false, fmt.Errorf("could not decode token in request body") } else if tokenStr == "" { - return false, nil, fmt.Errorf("empty token in request body") + return false, fmt.Errorf("empty token in request body") } else { token = tokenStr } @@ -367,23 +377,23 @@ func (c *Core) validateWrappingToken(ctx context.Context, req *logical.Request, // Implement the jose library way parsedJWT, err := squarejwt.ParseSigned(token) if err != nil { - return false, nil, errwrap.Wrapf("wrapping token could not be parsed: {{err}}", err) + return false, errwrap.Wrapf("wrapping token could not be parsed: {{err}}", err) } var claims squarejwt.Claims var allClaims = make(map[string]interface{}) if err = parsedJWT.Claims(&c.wrappingJWTKey.PublicKey, &claims, &allClaims); err != nil { - return false, nil, errwrap.Wrapf("wrapping token signature could not be validated: {{err}}", err) + return false, errwrap.Wrapf("wrapping token signature could not be validated: {{err}}", err) } typeClaimRaw, ok := allClaims["type"] if !ok { - return false, nil, errors.New("could not validate type claim") + return false, errors.New("could not validate type claim") } typeClaim, ok := typeClaimRaw.(string) if !ok { - return false, nil, errors.New("could not parse type claim") + return false, errors.New("could not parse type claim") } if typeClaim != "wrapping" { - return false, nil, errors.New("unexpected type claim") + return false, errors.New("unexpected type claim") } if !thirdParty { req.ClientToken = claims.ID @@ -395,33 +405,33 @@ func (c *Core) validateWrappingToken(ctx context.Context, req *logical.Request, } if token == "" { - return false, nil, fmt.Errorf("token is empty") + return false, fmt.Errorf("token is empty") } if c.Sealed() { - return false, nil, consts.ErrSealed + return false, consts.ErrSealed } c.stateLock.RLock() defer c.stateLock.RUnlock() if c.standby && !c.perfStandby { - return false, nil, consts.ErrStandby + return false, consts.ErrStandby } te, err := c.tokenStore.Lookup(ctx, token) if err != nil { - return false, nil, err + return false, err } if te == nil { - return false, nil, nil + return false, nil } if len(te.Policies) != 1 { - return false, nil, nil + return false, nil } if te.Policies[0] != responseWrappingPolicyName && te.Policies[0] != controlGroupPolicyName { - return false, nil, nil + return false, nil } if !thirdParty { @@ -430,5 +440,5 @@ func (c *Core) validateWrappingToken(ctx context.Context, req *logical.Request, req.SetTokenEntry(te) } - return true, nil, nil + return true, nil } From d7df2b2009802e55e0b6417ff188e6f52f30d059 Mon Sep 17 00:00:00 2001 From: Brian Kassouf Date: Fri, 5 Jul 2019 12:12:18 -0700 Subject: [PATCH 11/12] Fix tests --- http/handler.go | 2 +- http/logical.go | 1 + http/logical_test.go | 90 ++++++++++++++++++++++++ vault/audit_test.go | 84 ---------------------- vault/core_test.go | 144 -------------------------------------- vault/request_handling.go | 4 +- vault/testing.go | 82 ++++++++++++++++++++++ vault/wrapping.go | 6 ++ 8 files changed, 182 insertions(+), 231 deletions(-) diff --git a/http/handler.go b/http/handler.go index 7a8d2023e871..6d10a5bc113a 100644 --- a/http/handler.go +++ b/http/handler.go @@ -308,7 +308,7 @@ func wrappingVerificationFunc(ctx context.Context, core *vault.Core, req *logica return errwrap.Wrapf("error validating wrapping token: {{err}}", err) } if !valid { - return fmt.Errorf("wrapping token is not valid or does not exist") + return consts.ErrInvalidWrappingToken } return nil diff --git a/http/logical.go b/http/logical.go index 6b2d6c8ddc1c..ac9326af7e5b 100644 --- a/http/logical.go +++ b/http/logical.go @@ -217,6 +217,7 @@ func handleLogicalInternal(core *vault.Core, injectDataIntoTopLevel bool) http.H } else { respondError(w, http.StatusBadRequest, err) } + return } // The -self paths have no meaning outside of the token NS, so diff --git a/http/logical_test.go b/http/logical_test.go index f7498733ec6d..284d94c11425 100644 --- a/http/logical_test.go +++ b/http/logical_test.go @@ -2,6 +2,7 @@ package http import ( "bytes" + "context" "encoding/json" "io" "io/ioutil" @@ -16,6 +17,7 @@ import ( "github.com/go-test/deep" log "github.com/hashicorp/go-hclog" + "github.com/hashicorp/vault/audit" "github.com/hashicorp/vault/helper/namespace" "github.com/hashicorp/vault/sdk/helper/consts" "github.com/hashicorp/vault/sdk/helper/logging" @@ -347,3 +349,91 @@ func TestLogical_RespondWithStatusCode(t *testing.T) { t.Fatalf("bad response: %s", string(bodyRaw[:])) } } + +func TestLogical_Audit_invalidWrappingToken(t *testing.T) { + // Create a noop audit backend + var noop *vault.NoopAudit + c, _, root := vault.TestCoreUnsealedWithConfig(t, &vault.CoreConfig{ + AuditBackends: map[string]audit.Factory{ + "noop": func(ctx context.Context, config *audit.BackendConfig) (audit.Backend, error) { + noop = &vault.NoopAudit{ + Config: config, + } + return noop, nil + }, + }, + }) + ln, addr := TestServer(t, c) + defer ln.Close() + + // Enable the audit backend + + resp := testHttpPost(t, root, addr+"/v1/sys/audit/noop", map[string]interface{}{ + "type": "noop", + }) + testResponseStatus(t, resp, 204) + + { + // Make a wrapping/unwrap request with an invalid token + resp := testHttpPost(t, root, addr+"/v1/sys/wrapping/unwrap", map[string]interface{}{ + "token": "foo", + }) + testResponseStatus(t, resp, 400) + body := map[string][]string{} + testResponseBody(t, resp, &body) + if body["errors"][0] != "wrapping token is not valid or does not exist" { + t.Fatal(body) + } + + // Check the audit trail on request and response + if len(noop.ReqAuth) != 1 { + t.Fatalf("bad: %#v", noop) + } + auth := noop.ReqAuth[0] + if auth.ClientToken != root { + t.Fatalf("bad client token: %#v", auth) + } + if len(noop.Req) != 1 || noop.Req[0].Path != "sys/wrapping/unwrap" { + t.Fatalf("bad:\ngot:\n%#v", noop.Req[0]) + } + + if len(noop.ReqErrs) != 1 { + t.Fatalf("bad: %#v", noop.RespErrs) + } + if noop.ReqErrs[0] != consts.ErrInvalidWrappingToken { + t.Fatalf("bad: %#v", noop.ReqErrs) + } + } + + { + // Make a wrapping/unwrap request with an invalid token + resp := testHttpPost(t, root, addr+"/v1/sys/wrapping/unwrap", map[string]interface{}{ + "token": "foo", + }) + testResponseStatus(t, resp, 400) + body := map[string][]string{} + testResponseBody(t, resp, &body) + if body["errors"][0] != "wrapping token is not valid or does not exist" { + t.Fatal(body) + } + + // Check the audit trail on request and response + if len(noop.ReqAuth) != 2 { + t.Fatalf("bad: %#v", noop) + } + auth := noop.ReqAuth[1] + if auth.ClientToken != root { + t.Fatalf("bad client token: %#v", auth) + } + if len(noop.Req) != 2 || noop.Req[1].Path != "sys/wrapping/unwrap" { + t.Fatalf("bad:\ngot:\n%#v", noop.Req[1]) + } + + if len(noop.ReqErrs) != 2 { + t.Fatalf("bad: %#v", noop.RespErrs) + } + if noop.ReqErrs[1] != consts.ErrInvalidWrappingToken { + t.Fatalf("bad: %#v", noop.ReqErrs) + } + } +} diff --git a/vault/audit_test.go b/vault/audit_test.go index dd7bba4cf069..c0d1f4149c40 100644 --- a/vault/audit_test.go +++ b/vault/audit_test.go @@ -5,7 +5,6 @@ import ( "fmt" "reflect" "strings" - "sync" "testing" "time" @@ -18,93 +17,10 @@ import ( "github.com/hashicorp/vault/helper/namespace" "github.com/hashicorp/vault/sdk/helper/jsonutil" "github.com/hashicorp/vault/sdk/helper/logging" - "github.com/hashicorp/vault/sdk/helper/salt" "github.com/hashicorp/vault/sdk/logical" "github.com/mitchellh/copystructure" ) -type NoopAudit struct { - Config *audit.BackendConfig - ReqErr error - ReqAuth []*logical.Auth - Req []*logical.Request - ReqHeaders []map[string][]string - ReqNonHMACKeys []string - ReqErrs []error - - RespErr error - RespAuth []*logical.Auth - RespReq []*logical.Request - Resp []*logical.Response - RespNonHMACKeys []string - RespReqNonHMACKeys []string - RespErrs []error - - salt *salt.Salt - saltMutex sync.RWMutex -} - -func (n *NoopAudit) LogRequest(ctx context.Context, in *logical.LogInput) error { - n.ReqAuth = append(n.ReqAuth, in.Auth) - n.Req = append(n.Req, in.Request) - n.ReqHeaders = append(n.ReqHeaders, in.Request.Headers) - n.ReqNonHMACKeys = in.NonHMACReqDataKeys - n.ReqErrs = append(n.ReqErrs, in.OuterErr) - return n.ReqErr -} - -func (n *NoopAudit) LogResponse(ctx context.Context, in *logical.LogInput) error { - n.RespAuth = append(n.RespAuth, in.Auth) - n.RespReq = append(n.RespReq, in.Request) - n.Resp = append(n.Resp, in.Response) - n.RespErrs = append(n.RespErrs, in.OuterErr) - - if in.Response != nil { - n.RespNonHMACKeys = in.NonHMACRespDataKeys - n.RespReqNonHMACKeys = in.NonHMACReqDataKeys - } - - return n.RespErr -} - -func (n *NoopAudit) Salt(ctx context.Context) (*salt.Salt, error) { - n.saltMutex.RLock() - if n.salt != nil { - defer n.saltMutex.RUnlock() - return n.salt, nil - } - n.saltMutex.RUnlock() - n.saltMutex.Lock() - defer n.saltMutex.Unlock() - if n.salt != nil { - return n.salt, nil - } - salt, err := salt.NewSalt(ctx, n.Config.SaltView, n.Config.SaltConfig) - if err != nil { - return nil, err - } - n.salt = salt - return salt, nil -} - -func (n *NoopAudit) GetHash(ctx context.Context, data string) (string, error) { - salt, err := n.Salt(ctx) - if err != nil { - return "", err - } - return salt.GetIdentifiedHMAC(data), nil -} - -func (n *NoopAudit) Reload(ctx context.Context) error { - return nil -} - -func (n *NoopAudit) Invalidate(ctx context.Context) { - n.saltMutex.Lock() - defer n.saltMutex.Unlock() - n.salt = nil -} - func TestAudit_ReadOnlyViewDuringMount(t *testing.T) { c, _, _ := TestCoreUnsealed(t) c.auditBackends["noop"] = func(ctx context.Context, config *audit.BackendConfig) (audit.Backend, error) { diff --git a/vault/core_test.go b/vault/core_test.go index a457c1eb4bc7..11ed21b72c86 100644 --- a/vault/core_test.go +++ b/vault/core_test.go @@ -904,150 +904,6 @@ func TestCore_HandleRequest_AuditTrail_noHMACKeys(t *testing.T) { } } -func TestCore_HandleRequest_AuditTrail_invalidWrappingToken(t *testing.T) { - // Create a noop audit backend - var noop *NoopAudit - c, _, root := TestCoreUnsealed(t) - c.auditBackends["noop"] = func(ctx context.Context, config *audit.BackendConfig) (audit.Backend, error) { - noop = &NoopAudit{ - Config: config, - } - return noop, nil - } - - // Enable the audit backend - req := logical.TestRequest(t, logical.UpdateOperation, "sys/audit/noop") - req.Data["type"] = "noop" - req.ClientToken = root - _, err := c.HandleRequest(namespace.RootContext(nil), req) - if err != nil { - t.Fatalf("err: %v", err) - } - - { - // Make a wrapping/unwrap request with an invalid token - req := &logical.Request{ - Operation: logical.UpdateOperation, - Path: "sys/wrapping/unwrap", - Data: map[string]interface{}{ - "token": "foo", - }, - ClientToken: root, - } - resp, err := c.HandleRequest(namespace.RootContext(nil), req) - if err == nil { - t.Fatal("expected invalid request error") - } - - // Check the audit trail on request and response - if len(noop.ReqAuth) != 1 { - t.Fatalf("bad: %#v", noop) - } - auth := noop.ReqAuth[0] - if auth.ClientToken != root { - t.Fatalf("bad client token: %#v", auth) - } - if len(noop.Req) != 1 || !reflect.DeepEqual(req, noop.Req[0]) { - t.Fatalf("bad:\nexpected:\n%#v\ngot:\n%#v", req, noop.Req[0]) - } - - if len(noop.RespAuth) != 2 { - t.Fatalf("bad: %#v", noop) - } - if !reflect.DeepEqual(auth, noop.RespAuth[1]) { - t.Fatalf("bad:\nexpected:\n%#v\ngot:\n%#v", auth, noop.RespAuth[1]) - } - if len(noop.RespReq) != 2 || !reflect.DeepEqual(req, noop.RespReq[1]) { - t.Fatalf("bad:\nexpected:\n%#v\ngot:\n%#v", req, noop.RespReq[1]) - } - if len(noop.Resp) != 2 || !reflect.DeepEqual(resp, noop.Resp[1]) { - t.Fatalf("bad:\nexpected:\n%#v\ngot:\n%#v", resp, noop.Resp[1]) - } - } - - { - // Make a wrapping/rewrap request with an invalid token - req := &logical.Request{ - Operation: logical.UpdateOperation, - Path: "sys/wrapping/rewrap", - Data: map[string]interface{}{ - "token": "foo", - }, - ClientToken: root, - } - resp, err := c.HandleRequest(namespace.RootContext(nil), req) - if err == nil { - t.Fatal("expected invalid request error") - } - - // Check the audit trail on request and response - if len(noop.ReqAuth) != 2 { - t.Fatalf("bad: %#v", noop) - } - auth := noop.ReqAuth[1] - if auth.ClientToken != root { - t.Fatalf("bad client token: %#v", auth) - } - if len(noop.Req) != 2 || !reflect.DeepEqual(req, noop.Req[1]) { - t.Fatalf("bad:\nexpected:\n%#v\ngot:\n%#v", req, noop.Req[1]) - } - - if len(noop.RespAuth) != 3 { - t.Fatalf("bad: %#v", noop) - } - if !reflect.DeepEqual(auth, noop.RespAuth[2]) { - t.Fatalf("bad:\nexpected:\n%#v\ngot:\n%#v", auth, noop.RespAuth[2]) - } - if len(noop.RespReq) != 3 || !reflect.DeepEqual(req, noop.RespReq[2]) { - t.Fatalf("bad:\nexpected:\n%#v\ngot:\n%#v", req, noop.RespReq[2]) - } - if len(noop.Resp) != 3 || !reflect.DeepEqual(resp, noop.Resp[2]) { - t.Fatalf("bad:\nexpected:\n%#v\ngot:\n%#v", resp, noop.Resp[2]) - } - } - - { - // Make a wrapping/lookup request with an invalid token - req := &logical.Request{ - Operation: logical.UpdateOperation, - Path: "sys/wrapping/lookup", - Data: map[string]interface{}{ - "token": "foo", - }, - ClientToken: root, - } - resp, err := c.HandleRequest(namespace.RootContext(nil), req) - if err == nil { - t.Fatal("expected invalid request error") - } - - // Check the audit trail on request and response - if len(noop.ReqAuth) != 3 { - t.Fatalf("bad: %#v", noop) - } - auth := noop.ReqAuth[2] - if auth.ClientToken != root { - t.Fatalf("bad client token: %#v", auth) - } - if len(noop.Req) != 3 || !reflect.DeepEqual(req, noop.Req[2]) { - t.Fatalf("bad:\nexpected:\n%#v\ngot:\n%#v", req, noop.Req[2]) - } - - if len(noop.RespAuth) != 4 { - t.Fatalf("bad: %#v", noop) - } - if !reflect.DeepEqual(auth, noop.RespAuth[3]) { - t.Fatalf("bad:\nexpected:\n%#v\ngot:\n%#v", auth, noop.RespAuth[3]) - } - if len(noop.RespReq) != 4 || !reflect.DeepEqual(req, noop.RespReq[3]) { - t.Fatalf("bad:\nexpected:\n%#v\ngot:\n%#v", req, noop.RespReq[3]) - } - if len(noop.Resp) != 4 || !reflect.DeepEqual(resp, noop.Resp[3]) { - t.Fatalf("bad:\nexpected:\n%#v\ngot:\n%#v", resp, noop.Resp[3]) - } - } -} - func TestCore_HandleLogin_AuditTrail(t *testing.T) { // Create a badass credential backend that always logs in as armon noop := &NoopAudit{} diff --git a/vault/request_handling.go b/vault/request_handling.go index 0e3f6d54b934..fb5b192e7cae 100644 --- a/vault/request_handling.go +++ b/vault/request_handling.go @@ -907,6 +907,8 @@ func (c *Core) handleRequest(ctx context.Context, req *logical.Request) (retResp func (c *Core) handleLoginRequest(ctx context.Context, req *logical.Request) (retResp *logical.Response, retAuth *logical.Auth, retErr error) { defer metrics.MeasureSince([]string{"core", "handle_login_request"}, time.Now()) + req.Unauthenticated = true + var nonHMACReqDataKeys []string entry := c.router.MatchingMountEntry(ctx, req.Path) if entry != nil { @@ -916,8 +918,6 @@ func (c *Core) handleLoginRequest(ctx context.Context, req *logical.Request) (re } } - req.Unauthenticated = true - // Do an unauth check. This will cause EGP policies to be checked var auth *logical.Auth var ctErr error diff --git a/vault/testing.go b/vault/testing.go index e8fc5a2abf17..9e19db4cd631 100644 --- a/vault/testing.go +++ b/vault/testing.go @@ -1739,3 +1739,85 @@ func (m *mockBuiltinRegistry) Keys(pluginType consts.PluginType) []string { func (m *mockBuiltinRegistry) Contains(name string, pluginType consts.PluginType) bool { return false } + +type NoopAudit struct { + Config *audit.BackendConfig + ReqErr error + ReqAuth []*logical.Auth + Req []*logical.Request + ReqHeaders []map[string][]string + ReqNonHMACKeys []string + ReqErrs []error + + RespErr error + RespAuth []*logical.Auth + RespReq []*logical.Request + Resp []*logical.Response + RespNonHMACKeys []string + RespReqNonHMACKeys []string + RespErrs []error + + salt *salt.Salt + saltMutex sync.RWMutex +} + +func (n *NoopAudit) LogRequest(ctx context.Context, in *logical.LogInput) error { + n.ReqAuth = append(n.ReqAuth, in.Auth) + n.Req = append(n.Req, in.Request) + n.ReqHeaders = append(n.ReqHeaders, in.Request.Headers) + n.ReqNonHMACKeys = in.NonHMACReqDataKeys + n.ReqErrs = append(n.ReqErrs, in.OuterErr) + return n.ReqErr +} + +func (n *NoopAudit) LogResponse(ctx context.Context, in *logical.LogInput) error { + n.RespAuth = append(n.RespAuth, in.Auth) + n.RespReq = append(n.RespReq, in.Request) + n.Resp = append(n.Resp, in.Response) + n.RespErrs = append(n.RespErrs, in.OuterErr) + + if in.Response != nil { + n.RespNonHMACKeys = in.NonHMACRespDataKeys + n.RespReqNonHMACKeys = in.NonHMACReqDataKeys + } + + return n.RespErr +} + +func (n *NoopAudit) Salt(ctx context.Context) (*salt.Salt, error) { + n.saltMutex.RLock() + if n.salt != nil { + defer n.saltMutex.RUnlock() + return n.salt, nil + } + n.saltMutex.RUnlock() + n.saltMutex.Lock() + defer n.saltMutex.Unlock() + if n.salt != nil { + return n.salt, nil + } + salt, err := salt.NewSalt(ctx, n.Config.SaltView, n.Config.SaltConfig) + if err != nil { + return nil, err + } + n.salt = salt + return salt, nil +} + +func (n *NoopAudit) GetHash(ctx context.Context, data string) (string, error) { + salt, err := n.Salt(ctx) + if err != nil { + return "", err + } + return salt.GetIdentifiedHMAC(data), nil +} + +func (n *NoopAudit) Reload(ctx context.Context) error { + return nil +} + +func (n *NoopAudit) Invalidate(ctx context.Context) { + n.saltMutex.Lock() + defer n.saltMutex.Unlock() + n.salt = nil +} diff --git a/vault/wrapping.go b/vault/wrapping.go index d635ac4a0b9d..2f461073c0d0 100644 --- a/vault/wrapping.go +++ b/vault/wrapping.go @@ -339,6 +339,12 @@ func (c *Core) ValidateWrappingToken(ctx context.Context, req *logical.Request) Request: req, NonHMACReqDataKeys: nonHMACReqDataKeys, } + if err != nil { + logInput.OuterErr = errwrap.Wrapf("error validating wrapping token: {{err}}", err) + } + if !valid { + logInput.OuterErr = consts.ErrInvalidWrappingToken + } if err := c.auditBroker.LogRequest(ctx, logInput, c.auditedHeaders); err != nil { c.logger.Error("failed to audit request", "path", req.Path, "error", err) } From b327f2416b7778e37e5c3dd7f00db6f38be85606 Mon Sep 17 00:00:00 2001 From: Brian Kassouf Date: Fri, 5 Jul 2019 14:14:09 -0700 Subject: [PATCH 12/12] Review feedback --- http/http_test.go | 22 +++++++++++++++------- http/logical_test.go | 32 ++++++++++++++++---------------- vault/wrapping.go | 17 +++-------------- 3 files changed, 34 insertions(+), 37 deletions(-) diff --git a/http/http_test.go b/http/http_test.go index 1f2b20444762..e37b9c3d7693 100644 --- a/http/http_test.go +++ b/http/http_test.go @@ -22,32 +22,36 @@ func testHttpGet(t *testing.T, token string, addr string) *http.Response { loggedToken = "" } t.Logf("Token is %s", loggedToken) - return testHttpData(t, "GET", token, addr, nil, false) + return testHttpData(t, "GET", token, addr, nil, false, 0) } func testHttpDelete(t *testing.T, token string, addr string) *http.Response { - return testHttpData(t, "DELETE", token, addr, nil, false) + return testHttpData(t, "DELETE", token, addr, nil, false, 0) } // Go 1.8+ clients redirect automatically which breaks our 307 standby testing func testHttpDeleteDisableRedirect(t *testing.T, token string, addr string) *http.Response { - return testHttpData(t, "DELETE", token, addr, nil, true) + return testHttpData(t, "DELETE", token, addr, nil, true, 0) +} + +func testHttpPostWrapped(t *testing.T, token string, addr string, body interface{}, wrapTTL time.Duration) *http.Response { + return testHttpData(t, "POST", token, addr, body, false, wrapTTL) } func testHttpPost(t *testing.T, token string, addr string, body interface{}) *http.Response { - return testHttpData(t, "POST", token, addr, body, false) + return testHttpData(t, "POST", token, addr, body, false, 0) } func testHttpPut(t *testing.T, token string, addr string, body interface{}) *http.Response { - return testHttpData(t, "PUT", token, addr, body, false) + return testHttpData(t, "PUT", token, addr, body, false, 0) } // Go 1.8+ clients redirect automatically which breaks our 307 standby testing func testHttpPutDisableRedirect(t *testing.T, token string, addr string, body interface{}) *http.Response { - return testHttpData(t, "PUT", token, addr, body, true) + return testHttpData(t, "PUT", token, addr, body, true, 0) } -func testHttpData(t *testing.T, method string, token string, addr string, body interface{}, disableRedirect bool) *http.Response { +func testHttpData(t *testing.T, method string, token string, addr string, body interface{}, disableRedirect bool, wrapTTL time.Duration) *http.Response { bodyReader := new(bytes.Buffer) if body != nil { enc := json.NewEncoder(bodyReader) @@ -68,6 +72,10 @@ func testHttpData(t *testing.T, method string, token string, addr string, body i req.Header.Set("Content-Type", "application/json") + if wrapTTL > 0 { + req.Header.Set("X-Vault-Wrap-TTL", wrapTTL.String()) + } + if len(token) != 0 { req.Header.Set(consts.AuthHeaderName, token) } diff --git a/http/logical_test.go b/http/logical_test.go index 284d94c11425..df385bd05ee4 100644 --- a/http/logical_test.go +++ b/http/logical_test.go @@ -406,34 +406,34 @@ func TestLogical_Audit_invalidWrappingToken(t *testing.T) { } { + resp := testHttpPostWrapped(t, root, addr+"/v1/auth/token/create", nil, 10*time.Second) + testResponseStatus(t, resp, 200) + body := map[string]interface{}{} + testResponseBody(t, resp, &body) + + wrapToken := body["wrap_info"].(map[string]interface{})["token"].(string) + // Make a wrapping/unwrap request with an invalid token - resp := testHttpPost(t, root, addr+"/v1/sys/wrapping/unwrap", map[string]interface{}{ - "token": "foo", + resp = testHttpPost(t, root, addr+"/v1/sys/wrapping/unwrap", map[string]interface{}{ + "token": wrapToken, }) - testResponseStatus(t, resp, 400) - body := map[string][]string{} - testResponseBody(t, resp, &body) - if body["errors"][0] != "wrapping token is not valid or does not exist" { - t.Fatal(body) - } + testResponseStatus(t, resp, 200) // Check the audit trail on request and response - if len(noop.ReqAuth) != 2 { + if len(noop.ReqAuth) != 3 { t.Fatalf("bad: %#v", noop) } - auth := noop.ReqAuth[1] + auth := noop.ReqAuth[2] if auth.ClientToken != root { t.Fatalf("bad client token: %#v", auth) } - if len(noop.Req) != 2 || noop.Req[1].Path != "sys/wrapping/unwrap" { - t.Fatalf("bad:\ngot:\n%#v", noop.Req[1]) + if len(noop.Req) != 3 || noop.Req[2].Path != "sys/wrapping/unwrap" { + t.Fatalf("bad:\ngot:\n%#v", noop.Req[2]) } - if len(noop.ReqErrs) != 2 { + // Make sure there is only one error in the logs + if noop.ReqErrs[1] != nil || noop.ReqErrs[2] != nil { t.Fatalf("bad: %#v", noop.RespErrs) } - if noop.ReqErrs[1] != consts.ErrInvalidWrappingToken { - t.Fatalf("bad: %#v", noop.ReqErrs) - } } } diff --git a/vault/wrapping.go b/vault/wrapping.go index 2f461073c0d0..a861e14ed26a 100644 --- a/vault/wrapping.go +++ b/vault/wrapping.go @@ -317,16 +317,6 @@ func (c *Core) ValidateWrappingToken(ctx context.Context, req *logical.Request) // Perform audit logging before returning if there's an issue with checking // the wrapping token if err != nil || !valid { - // Get non-HMAC'ed request data keys - var nonHMACReqDataKeys []string - entry := c.router.MatchingMountEntry(ctx, req.Path) - if entry != nil { - // Get and set ignored HMAC'd value. - if rawVals, ok := entry.synthesizedConfigCache.Load("audit_non_hmac_request_keys"); ok { - nonHMACReqDataKeys = rawVals.([]string) - } - } - // We log the Auth object like so here since the wrapping token can // come from the header, which gets set as the ClientToken auth := &logical.Auth{ @@ -335,12 +325,11 @@ func (c *Core) ValidateWrappingToken(ctx context.Context, req *logical.Request) } logInput := &logical.LogInput{ - Auth: auth, - Request: req, - NonHMACReqDataKeys: nonHMACReqDataKeys, + Auth: auth, + Request: req, } if err != nil { - logInput.OuterErr = errwrap.Wrapf("error validating wrapping token: {{err}}", err) + logInput.OuterErr = errors.New("error validating wrapping token") } if !valid { logInput.OuterErr = consts.ErrInvalidWrappingToken