From e6f00445221beb11813974a2f58aabadcfdb2d3c Mon Sep 17 00:00:00 2001 From: mgritter Date: Fri, 26 Apr 2019 15:57:00 -0700 Subject: [PATCH 1/3] Check nil parameter value when processing an ACL. --- vault/acl.go | 9 ++++++++- vault/acl_test.go | 3 +++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/vault/acl.go b/vault/acl.go index 4105b3583f..1313fa33b5 100644 --- a/vault/acl.go +++ b/vault/acl.go @@ -709,7 +709,14 @@ func valueInParameterList(v interface{}, list []interface{}) bool { func valueInSlice(v interface{}, list []interface{}) bool { for _, el := range list { - if reflect.TypeOf(el).String() == "string" && reflect.TypeOf(v).String() == "string" { + if el == nil || v == nil { + // It doesn't seem possible to set up a nil entry in the list, but it is possible + // to pass in a null entry in the API request being checked. Just in case, + // nil will match nil. + if el == v { + return true + } + } else if reflect.TypeOf(el).String() == "string" && reflect.TypeOf(v).String() == "string" { item := el.(string) val := v.(string) diff --git a/vault/acl_test.go b/vault/acl_test.go index 5a2de1b7fc..a8cb634dd8 100644 --- a/vault/acl_test.go +++ b/vault/acl_test.go @@ -549,6 +549,8 @@ func testACLValuePermissions(t *testing.T, ns *namespace.Namespace) { {"foo/bar", []string{"deny"}, []interface{}{"bad glob"}, false}, {"foo/bar", []string{"deny"}, []interface{}{"good"}, true}, {"foo/bar", []string{"allow"}, []interface{}{"good"}, true}, + {"foo/bar", []string{"deny"}, []interface{}{nil}, true}, + {"foo/bar", []string{"allow"}, []interface{}{nil}, true}, {"foo/baz", []string{"aLLow"}, []interface{}{"good"}, true}, {"foo/baz", []string{"deny"}, []interface{}{"bad"}, false}, {"foo/baz", []string{"deny"}, []interface{}{"good"}, false}, @@ -557,6 +559,7 @@ func testACLValuePermissions(t *testing.T, ns *namespace.Namespace) { {"foo/baz", []string{"deNy", "allow"}, []interface{}{"bad", "good"}, false}, {"foo/baz", []string{"aLLow"}, []interface{}{"bad"}, false}, {"foo/baz", []string{"Neither"}, []interface{}{"bad"}, false}, + {"foo/baz", []string{"allow"}, []interface{}{nil}, false}, {"fizz/buzz", []string{"allow_multi"}, []interface{}{"good"}, true}, {"fizz/buzz", []string{"allow_multi"}, []interface{}{"good1"}, true}, {"fizz/buzz", []string{"allow_multi"}, []interface{}{"good2"}, true}, From 11cc73299ebf1804acfdde7d0456d252182df41d Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Mon, 29 Apr 2019 13:14:26 -0700 Subject: [PATCH 2/3] core: honor non-HMAC keys in audit requests (#6653) --- vault/request_handling.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/vault/request_handling.go b/vault/request_handling.go index b2f0a9a202..d3e9bb04a6 100644 --- a/vault/request_handling.go +++ b/vault/request_handling.go @@ -914,9 +914,17 @@ func (c *Core) handleLoginRequest(ctx context.Context, req *logical.Request) (re req.Unauthenticated = true - var auth *logical.Auth + 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) + } + } // 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 { @@ -933,15 +941,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, @@ -965,8 +964,9 @@ func (c *Core) handleLoginRequest(ctx context.Context, req *logical.Request) (re // Create an audit trail of the request. Attach auth if it was returned, // e.g. if a token was provided. logInput := &audit.LogInput{ - Auth: auth, - Request: req, + 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) From 0fe1077b853ed35c3cb46eebdeb020645829c661 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Mon, 29 Apr 2019 13:16:40 -0700 Subject: [PATCH 3/3] changelog++ --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ea375c691e..a9a8f5b6e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ IMPROVEMENTS: BUG FIXES: + * core: Correctly honor non-HMAC request keys when auditing requests [GH-6653] * replication: Fix an issue causing startup problems if a namespace policy wasn't replicated properly * replication: Properly update mount entry cache on a secondary to apply all