From e64bfecae97b97d22f364631e49d97b1d94d5832 Mon Sep 17 00:00:00 2001 From: Chris Capurso Date: Mon, 20 Sep 2021 09:43:43 -0400 Subject: [PATCH 01/30] handle HTTP PATCH requests as logical.PatchOperation --- http/logical.go | 31 +++++++++++++++++++++++++++++++ sdk/logical/request.go | 1 + 2 files changed, 32 insertions(+) diff --git a/http/logical.go b/http/logical.go index dd9abce34dfdb..31ba714b89808 100644 --- a/http/logical.go +++ b/http/logical.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" "io" + "mime" "net" "net/http" "strconv" @@ -38,6 +39,8 @@ func (b *bufferedReader) Close() error { return b.rOrig.Close() } +const MergePatchContentTypeHeader = "application/merge-patch+json" + func buildLogicalRequestNoAuth(perfStandby bool, w http.ResponseWriter, r *http.Request) (*logical.Request, io.ReadCloser, int, error) { ns, err := namespace.FromContext(r.Context()) if err != nil { @@ -139,6 +142,34 @@ func buildLogicalRequestNoAuth(perfStandby bool, w http.ResponseWriter, r *http. } } + case "PATCH": + op = logical.PatchOperation + + contentTypeHeader := r.Header.Get("Content-Type") + contentType, _, err := mime.ParseMediaType(contentTypeHeader) + if err != nil { + status := http.StatusBadRequest + logical.AdjustErrorStatusCode(&status, err) + return nil, nil, status, err + } + + if contentType != MergePatchContentTypeHeader { + return nil, nil, http.StatusBadRequest, fmt.Errorf("PATCH requires Content-Type of %s, provided %s", MergePatchContentTypeHeader, contentType) + } + + origBody, err = parseJSONRequest(perfStandby, r, w, &data) + + if err == io.EOF { + data = nil + err = nil + } + + if err != nil { + status := http.StatusBadRequest + logical.AdjustErrorStatusCode(&status, err) + return nil, nil, status, fmt.Errorf("error parsing JSON") + } + case "LIST": op = logical.ListOperation if !strings.HasSuffix(path, "/") { diff --git a/sdk/logical/request.go b/sdk/logical/request.go index b88aabce2df37..e683217a6efc5 100644 --- a/sdk/logical/request.go +++ b/sdk/logical/request.go @@ -350,6 +350,7 @@ const ( CreateOperation Operation = "create" ReadOperation = "read" UpdateOperation = "update" + PatchOperation = "patch" DeleteOperation = "delete" ListOperation = "list" HelpOperation = "help" From 11eaec8dc0ac9f62164047b47543f8e8823ac25e Mon Sep 17 00:00:00 2001 From: Chris Capurso Date: Mon, 20 Sep 2021 12:36:39 -0400 Subject: [PATCH 02/30] update go.mod, go.sum --- go.mod | 2 ++ go.sum | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/go.mod b/go.mod index f8283261ef794..0e6249154560e 100644 --- a/go.mod +++ b/go.mod @@ -6,6 +6,8 @@ replace github.com/hashicorp/vault/api => ./api replace github.com/hashicorp/vault/sdk => ./sdk +replace github.com/hashicorp/vault-plugin-secrets-kv => ../vault-plugin-secrets-kv + require ( cloud.google.com/go v0.56.0 cloud.google.com/go/spanner v1.5.1 diff --git a/go.sum b/go.sum index fa8f547f494bb..8f859f7244e32 100644 --- a/go.sum +++ b/go.sum @@ -358,6 +358,8 @@ github.com/envoyproxy/go-control-plane v0.9.4/go.mod h1:6rpuAdCZL397s3pYoYcLgu1m github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c= github.com/evanphx/json-patch v0.0.0-20190203023257-5858425f7550/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk= github.com/evanphx/json-patch v4.2.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk= +github.com/evanphx/json-patch/v5 v5.5.0 h1:bAmFiUJ+o0o2B4OiTFeE3MqCOtyo+jjPP9iZ0VRxYUc= +github.com/evanphx/json-patch/v5 v5.5.0/go.mod h1:G79N1coSVB93tBe7j6PhzjmR3/2VvlbKOFpnXhI9Bw4= github.com/fatih/color v1.7.0/go.mod h1:Zm6kSWBoL9eyXnKyktHP6abPY2pDugNf5KwzbycvMj4= github.com/fatih/color v1.9.0/go.mod h1:eQcE1qtQxscV5RaZvpXrrb8Drkc3/DdQ+uUYCNjL+zU= github.com/fatih/color v1.11.0 h1:l4iX0RqNnx/pU7rY2DB/I+znuYY0K3x6Ywac6EIr0PA= @@ -681,6 +683,8 @@ github.com/hashicorp/go-version v1.0.0/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09 github.com/hashicorp/go-version v1.2.0/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA= github.com/hashicorp/go-version v1.2.1 h1:zEfKbn2+PDgroKdiOzqiE8rsmLqU2uwi5PB5pBJ3TkI= github.com/hashicorp/go-version v1.2.1/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA= +github.com/hashicorp/go-version v1.3.0 h1:McDWVJIU/y+u1BRV06dPaLfLCaT7fUTJLp5r04x7iNw= +github.com/hashicorp/go-version v1.3.0/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA= github.com/hashicorp/go.net v0.0.1/go.mod h1:hjKkEWcCURg++eb33jQU7oqQcI9XDCnUzHA0oac0k90= github.com/hashicorp/golang-lru v0.5.0/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8= github.com/hashicorp/golang-lru v0.5.1/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8= @@ -814,6 +818,7 @@ github.com/jefferai/isbadcipher v0.0.0-20190226160619-51d2077c035f h1:E87tDTVS5W github.com/jefferai/isbadcipher v0.0.0-20190226160619-51d2077c035f/go.mod h1:3J2qVK16Lq8V+wfiL2lPeDZ7UWMxk5LemerHa1p6N00= github.com/jefferai/jsonx v1.0.0 h1:Xoz0ZbmkpBvED5W9W1B5B/zc3Oiq7oXqiW7iRV3B6EI= github.com/jefferai/jsonx v1.0.0/go.mod h1:OGmqmi2tTeI/PS+qQfBDToLHHJIy/RMp24fPo8vFvoQ= +github.com/jessevdk/go-flags v1.4.0/go.mod h1:4FA24M0QyGHXBuZZK/XkWh8h0e1EYbRYJSGM75WSRxI= github.com/jmespath/go-jmespath v0.0.0-20180206201540-c2b33e8439af/go.mod h1:Nht3zPeWKUH0NzdCt2Blrr5ys8VGpn0CEB0cQHVjt7k= github.com/jmespath/go-jmespath v0.3.0/go.mod h1:9QtRXoHjLGCJ5IBSaohpXITPlowMeeYCZ7fLUTSywik= github.com/jmespath/go-jmespath v0.4.0 h1:BEgLn5cpjn8UN1mAw4NjwDrS35OdebyEtFe+9YPoQUg= From 10ebff66dfe6ee3fa602864edea588a0844c46fd Mon Sep 17 00:00:00 2001 From: Chris Capurso Date: Mon, 20 Sep 2021 14:08:27 -0400 Subject: [PATCH 03/30] a nil response for logical.PatchOperation should result in 404 --- sdk/logical/response_util.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/logical/response_util.go b/sdk/logical/response_util.go index 6ae3005b735f1..5244069379864 100644 --- a/sdk/logical/response_util.go +++ b/sdk/logical/response_util.go @@ -17,7 +17,7 @@ import ( func RespondErrorCommon(req *Request, resp *Response, err error) (int, error) { if err == nil && (resp == nil || !resp.IsError()) { switch { - case req.Operation == ReadOperation: + case req.Operation == ReadOperation, req.Operation == PatchOperation: if resp == nil { return http.StatusNotFound, nil } From 0d6fab2c8ff992466f4178a3eb3a7f245badab8f Mon Sep 17 00:00:00 2001 From: Chris Capurso Date: Mon, 20 Sep 2021 15:23:32 -0400 Subject: [PATCH 04/30] respond with 415 for incorrect MIME type in PATCH Content-Type header --- http/logical.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http/logical.go b/http/logical.go index 31ba714b89808..7984f8ac06ba3 100644 --- a/http/logical.go +++ b/http/logical.go @@ -154,7 +154,7 @@ func buildLogicalRequestNoAuth(perfStandby bool, w http.ResponseWriter, r *http. } if contentType != MergePatchContentTypeHeader { - return nil, nil, http.StatusBadRequest, fmt.Errorf("PATCH requires Content-Type of %s, provided %s", MergePatchContentTypeHeader, contentType) + return nil, nil, http.StatusUnsupportedMediaType, fmt.Errorf("PATCH requires Content-Type of %s, provided %s", MergePatchContentTypeHeader, contentType) } origBody, err = parseJSONRequest(perfStandby, r, w, &data) From 1ab8d4c17cac53e51de58f2cf38fee97aa57772d Mon Sep 17 00:00:00 2001 From: Chris Capurso Date: Mon, 20 Sep 2021 16:18:34 -0400 Subject: [PATCH 05/30] add abstraction to handle PatchOperation requests --- go.sum | 2 ++ sdk/framework/backend.go | 51 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/go.sum b/go.sum index 8f859f7244e32..5744376461b41 100644 --- a/go.sum +++ b/go.sum @@ -357,6 +357,8 @@ github.com/envoyproxy/go-control-plane v0.9.1-0.20191026205805-5f8ba28d4473/go.m github.com/envoyproxy/go-control-plane v0.9.4/go.mod h1:6rpuAdCZL397s3pYoYcLgu1mIlRU8Am5FuJP05cCM98= github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c= github.com/evanphx/json-patch v0.0.0-20190203023257-5858425f7550/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk= +github.com/evanphx/json-patch v0.5.2/go.mod h1:ZWS5hhDbVDyob71nXKNL0+PWn6ToqBHMikGIFbs31qQ= +github.com/evanphx/json-patch v4.2.0+incompatible h1:fUDGZCv/7iAN7u0puUVhvKCcsR6vRfwrJatElLBEf0I= github.com/evanphx/json-patch v4.2.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk= github.com/evanphx/json-patch/v5 v5.5.0 h1:bAmFiUJ+o0o2B4OiTFeE3MqCOtyo+jjPP9iZ0VRxYUc= github.com/evanphx/json-patch/v5 v5.5.0/go.mod h1:G79N1coSVB93tBe7j6PhzjmR3/2VvlbKOFpnXhI9Bw4= diff --git a/sdk/framework/backend.go b/sdk/framework/backend.go index c2c3f1810008b..ef8ab07b2d483 100644 --- a/sdk/framework/backend.go +++ b/sdk/framework/backend.go @@ -3,7 +3,9 @@ package framework import ( "context" "crypto/rand" + "encoding/json" "fmt" + jsonpatch "github.com/evanphx/json-patch" "io" "io/ioutil" "net/http" @@ -120,6 +122,10 @@ type InvalidateFunc func(context.Context, string) // Initialize() just after a plugin has been mounted. type InitializeFunc func(context.Context, *logical.InitializationRequest) error +// PatchPreprocessorFunc is used by HandlePatchOperation in order to shape +// the input as defined by request handler prior to JSON marshaling +type PatchPreprocessorFunc func(map[string]interface{}) (map[string]interface{}, error) + // Initialize is the logical.Backend implementation. func (b *Backend) Initialize(ctx context.Context, req *logical.InitializationRequest) error { if b.InitializeFunc != nil { @@ -274,6 +280,51 @@ func (b *Backend) HandleRequest(ctx context.Context, req *logical.Request) (*log return callback(ctx, req, &fd) } + +func HandlePatchOperation(input *FieldData, resource map[string]interface{}, preprocessor PatchPreprocessorFunc) ([]byte, error) { + var err error + + if resource == nil { + return nil, fmt.Errorf("resource does not exist") + } + + inputMap := map[string]interface{}{} + + // Parse all fields to ensure data types are handled properly according to the FieldSchema + for key := range input.Raw { + val, ok := input.GetOk(key) + + // Only accept fields in the schema + if ok { + inputMap[key] = val + } + } + + if preprocessor!= nil { + inputMap, err = preprocessor(inputMap) + if err != nil { + return nil, err + } + } + + marshaledResource, err := json.Marshal(resource) + if err != nil { + return nil, err + } + + marshaledInput, err := json.Marshal(inputMap) + if err != nil { + return nil, err + } + + modified, err := jsonpatch.MergePatch(marshaledResource, marshaledInput) + if err != nil { + return nil, err + } + + return modified, nil +} + // SpecialPaths is the logical.Backend implementation. func (b *Backend) SpecialPaths() *logical.Paths { return b.PathsSpecial From dc649aa12f66d98b3c803eda9c64d26447cb88f9 Mon Sep 17 00:00:00 2001 From: Josh Black Date: Mon, 20 Sep 2021 14:41:30 -0700 Subject: [PATCH 06/30] add ACLs for patch --- go.sum | 1 + vault/acl.go | 7 ++++++- vault/acl_test.go | 18 ++++++++++++++++++ vault/logical_system.go | 6 +++++- vault/policy.go | 5 ++++- vault/policy_test.go | 10 ++++++++++ 6 files changed, 44 insertions(+), 3 deletions(-) diff --git a/go.sum b/go.sum index 8f859f7244e32..02e65706bebb6 100644 --- a/go.sum +++ b/go.sum @@ -357,6 +357,7 @@ github.com/envoyproxy/go-control-plane v0.9.1-0.20191026205805-5f8ba28d4473/go.m github.com/envoyproxy/go-control-plane v0.9.4/go.mod h1:6rpuAdCZL397s3pYoYcLgu1mIlRU8Am5FuJP05cCM98= github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c= github.com/evanphx/json-patch v0.0.0-20190203023257-5858425f7550/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk= +github.com/evanphx/json-patch v4.2.0+incompatible h1:fUDGZCv/7iAN7u0puUVhvKCcsR6vRfwrJatElLBEf0I= github.com/evanphx/json-patch v4.2.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk= github.com/evanphx/json-patch/v5 v5.5.0 h1:bAmFiUJ+o0o2B4OiTFeE3MqCOtyo+jjPP9iZ0VRxYUc= github.com/evanphx/json-patch/v5 v5.5.0/go.mod h1:G79N1coSVB93tBe7j6PhzjmR3/2VvlbKOFpnXhI9Bw4= diff --git a/vault/acl.go b/vault/acl.go index 3d07c4089c486..fc9f353aa8afb 100644 --- a/vault/acl.go +++ b/vault/acl.go @@ -305,6 +305,9 @@ func (a *ACL) Capabilities(ctx context.Context, path string) (pathCapabilities [ if capabilities&CreateCapabilityInt > 0 { pathCapabilities = append(pathCapabilities, CreateCapability) } + if capabilities&PatchCapabilityInt > 0 { + pathCapabilities = append(pathCapabilities, PatchCapability) + } // If "deny" is explicitly set or if the path has no capabilities at all, // set the path capabilities to "deny" @@ -406,6 +409,8 @@ CHECK: operationAllowed = capabilities&DeleteCapabilityInt > 0 case logical.CreateOperation: operationAllowed = capabilities&CreateCapabilityInt > 0 + case logical.PatchOperation: + operationAllowed = capabilities&PatchCapabilityInt > 0 // These three re-use UpdateCapabilityInt since that's the most appropriate // capability/operation mapping @@ -440,7 +445,7 @@ CHECK: // Only check parameter permissions for operations that can modify // parameters. - if op == logical.ReadOperation || op == logical.UpdateOperation || op == logical.CreateOperation { + if op == logical.ReadOperation || op == logical.UpdateOperation || op == logical.CreateOperation || op == logical.PatchOperation { for _, parameter := range permissions.RequiredParameters { if _, ok := req.Data[strings.ToLower(parameter)]; !ok { return diff --git a/vault/acl_test.go b/vault/acl_test.go index 29c690e01a882..fe3a33e8aa20d 100644 --- a/vault/acl_test.go +++ b/vault/acl_test.go @@ -238,6 +238,12 @@ func testACLSingle(t *testing.T, ns *namespace.Namespace) { {logical.UpdateOperation, "foo/bar", false, true}, {logical.CreateOperation, "foo/bar", true, true}, + {logical.ReadOperation, "baz/quux", true, false}, + {logical.CreateOperation, "baz/quux", true, false}, + {logical.PatchOperation, "baz/quux", true, false}, + {logical.ListOperation, "baz/quux", false, false}, + {logical.UpdateOperation, "baz/quux", false, false}, + // Path segment wildcards {logical.ReadOperation, "test/foo/bar/segment", false, false}, {logical.ReadOperation, "test/foo/segment", true, false}, @@ -341,6 +347,12 @@ func testLayeredACL(t *testing.T, acl *ACL, ns *namespace.Namespace) { {logical.ListOperation, "foo/bar", false, false}, {logical.UpdateOperation, "foo/bar", false, false}, {logical.CreateOperation, "foo/bar", false, false}, + + {logical.ReadOperation, "baz/quux", false, false}, + {logical.ListOperation, "baz/quux", false, false}, + {logical.UpdateOperation, "baz/quux", false, false}, + {logical.CreateOperation, "baz/quux", false, false}, + {logical.PatchOperation, "baz/quux", false, false}, } for _, tc := range tcases { @@ -864,6 +876,9 @@ path "sys/*" { path "foo/bar" { capabilities = ["read", "create", "sudo"] } +path "baz/quux" { + capabilities = ["read", "create", "patch"] +} path "test/+/segment" { capabilities = ["read"] } @@ -912,6 +927,9 @@ path "sys/seal" { path "foo/bar" { capabilities = ["deny"] } +path "baz/quux" { + capabilities = ["deny"] +} ` // test merging diff --git a/vault/logical_system.go b/vault/logical_system.go index 6179287818861..bdeb44f955c87 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -3395,7 +3395,8 @@ func hasMountAccess(ctx context.Context, acl *ACL, path string) bool { perms.CapabilitiesBitmap&ListCapabilityInt > 0, perms.CapabilitiesBitmap&ReadCapabilityInt > 0, perms.CapabilitiesBitmap&SudoCapabilityInt > 0, - perms.CapabilitiesBitmap&UpdateCapabilityInt > 0: + perms.CapabilitiesBitmap&UpdateCapabilityInt > 0, + perms.CapabilitiesBitmap&PatchCapabilityInt > 0: aclCapabilitiesGiven = true @@ -3681,6 +3682,9 @@ func (b *SystemBackend) pathInternalUIResultantACL(ctx context.Context, req *log if perms.CapabilitiesBitmap&UpdateCapabilityInt > 0 { capabilities = append(capabilities, UpdateCapability) } + if perms.CapabilitiesBitmap&PatchCapabilityInt > 0 { + capabilities = append(capabilities, PatchCapability) + } // If "deny" is explicitly set or if the path has no capabilities at all, // set the path capabilities to "deny" diff --git a/vault/policy.go b/vault/policy.go index a4686b1d81c74..e80d1657e98dd 100644 --- a/vault/policy.go +++ b/vault/policy.go @@ -26,6 +26,7 @@ const ( ListCapability = "list" SudoCapability = "sudo" RootCapability = "root" + PatchCapability = "patch" // Backwards compatibility OldDenyPathPolicy = "deny" @@ -42,6 +43,7 @@ const ( DeleteCapabilityInt ListCapabilityInt SudoCapabilityInt + PatchCapabilityInt ) // Error constants for testing @@ -83,6 +85,7 @@ var cap2Int = map[string]uint32{ DeleteCapability: DeleteCapabilityInt, ListCapability: ListCapabilityInt, SudoCapability: SudoCapabilityInt, + PatchCapability: PatchCapabilityInt, } type egpPath struct { @@ -390,7 +393,7 @@ func parsePaths(result *Policy, list *ast.ObjectList, performTemplating bool, en pc.Capabilities = []string{DenyCapability} pc.Permissions.CapabilitiesBitmap = DenyCapabilityInt goto PathFinished - case CreateCapability, ReadCapability, UpdateCapability, DeleteCapability, ListCapability, SudoCapability: + case CreateCapability, ReadCapability, UpdateCapability, DeleteCapability, ListCapability, SudoCapability, PatchCapability: pc.Permissions.CapabilitiesBitmap |= cap2Int[cap] default: return fmt.Errorf("path %q: invalid capability %q", key, cap) diff --git a/vault/policy_test.go b/vault/policy_test.go index 045fae8655387..7f09d7cb2be75 100644 --- a/vault/policy_test.go +++ b/vault/policy_test.go @@ -83,6 +83,9 @@ path "test/req" { capabilities = ["create", "sudo"] required_parameters = ["foo"] } +path "test/patch" { + capabilities = ["patch"] +} path "test/mfa" { capabilities = ["create", "sudo"] mfa_methods = ["my_totp", "my_totp2"] @@ -244,6 +247,13 @@ func TestPolicy_Parse(t *testing.T) { RequiredParameters: []string{"foo"}, }, }, + { + Path: "test/patch", + Capabilities: []string{"patch"}, + Permissions: &ACLPermissions{ + CapabilitiesBitmap: (PatchCapabilityInt), + }, + }, { Path: "test/mfa", Capabilities: []string{ From 6257873515ae1d2f60c626c504887d3b66eaad0d Mon Sep 17 00:00:00 2001 From: Josh Black Date: Tue, 21 Sep 2021 16:03:47 -0700 Subject: [PATCH 07/30] Adding JSON Merge support to the API client --- api/api_test.go | 3 +-- api/logical.go | 13 +++++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/api/api_test.go b/api/api_test.go index b2b851df6e89f..e4ba3153203eb 100644 --- a/api/api_test.go +++ b/api/api_test.go @@ -9,8 +9,7 @@ import ( // testHTTPServer creates a test HTTP server that handles requests until // the listener returned is closed. -func testHTTPServer( - t *testing.T, handler http.Handler) (*Config, net.Listener) { +func testHTTPServer(t *testing.T, handler http.Handler) (*Config, net.Listener) { ln, err := net.Listen("tcp", "127.0.0.1:0") if err != nil { t.Fatalf("err: %s", err) diff --git a/api/logical.go b/api/logical.go index cd950a2b78968..0e28844037dc9 100644 --- a/api/logical.go +++ b/api/logical.go @@ -5,6 +5,7 @@ import ( "context" "fmt" "io" + "net/http" "net/url" "os" @@ -138,6 +139,18 @@ func (c *Logical) Write(path string, data map[string]interface{}) (*Secret, erro return c.write(path, r) } +func (c *Logical) JSONMergePatch(path string, data map[string]interface{}) (*Secret, error) { + r := c.c.NewRequest("PATCH", "/v1/"+path) + r.Headers = http.Header{ + "Content-Type": []string{"application/merge-patch+json"}, + } + if err := r.SetJSONBody(data); err != nil { + return nil, err + } + + return c.write(path, r) +} + func (c *Logical) WriteBytes(path string, data []byte) (*Secret, error) { r := c.c.NewRequest("PUT", "/v1/"+path) r.BodyBytes = data From 8bd064d9e9481ba543a92ec06fc857e15b4b712d Mon Sep 17 00:00:00 2001 From: Chris Capurso Date: Wed, 22 Sep 2021 16:21:44 -0400 Subject: [PATCH 08/30] add HTTP PATCH tests to check high level response logic --- http/handler_test.go | 108 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 108 insertions(+) diff --git a/http/handler_test.go b/http/handler_test.go index c228629ea8dce..b22993f755b68 100644 --- a/http/handler_test.go +++ b/http/handler_test.go @@ -5,6 +5,7 @@ import ( "crypto/tls" "encoding/json" "errors" + "github.com/hashicorp/vault/api" "io/ioutil" "net/http" "net/http/httptest" @@ -16,6 +17,7 @@ import ( "github.com/go-test/deep" "github.com/hashicorp/go-cleanhttp" + kv "github.com/hashicorp/vault-plugin-secrets-kv" "github.com/hashicorp/vault/helper/namespace" "github.com/hashicorp/vault/sdk/helper/consts" "github.com/hashicorp/vault/sdk/logical" @@ -818,3 +820,109 @@ func TestHandler_Parse_Form(t *testing.T) { t.Fatal(diff) } } + +func TestHandler_Patch_BadContentTypeHeader(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() + + cores := cluster.Cores + + core := cores[0].Core + c := cluster.Cores[0].Client + vault.TestWaitActive(t, core) + + // Mount a KVv2 backend + err := c.Sys().Mount("kv", &api.MountInput{ + Type: "kv", + Options: map[string]string{"version": "2"}, + }) + if err != nil { + t.Fatal(err) + } + + kvData := map[string]interface{}{ + "data": map[string]interface{}{ + "bar": "a", + }, + } + + resp, err := c.Logical().Write("kv/data/foo", kvData) + if err != nil { + t.Fatalf("write failed - err :%#v, resp: %#v\n", err, resp) + } + + resp, err = c.Logical().Read("kv/data/foo") + if err != nil { + t.Fatalf("read failed - err :%#v, resp: %#v\n", err, resp) + } + + req := c.NewRequest("PATCH", "/v1/kv/data/foo") + req.Headers = http.Header{ + "Content-Type": []string{"application/json"}, + } + + if err := req.SetJSONBody(kvData); err != nil { + t.Fatal(err) + } + + apiResp, err := c.RawRequestWithContext(context.Background(), req) + if err == nil || apiResp.StatusCode != http.StatusUnsupportedMediaType { + t.Fatalf("expected PATCH request to fail with %d status code - err :%#v, resp: %#v\n", http.StatusUnsupportedMediaType, err, apiResp) + } +} + +func TestHandler_Patch_NotFound(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() + + cores := cluster.Cores + + core := cores[0].Core + c := cluster.Cores[0].Client + vault.TestWaitActive(t, core) + + // Mount a KVv2 backend + err := c.Sys().Mount("kv", &api.MountInput{ + Type: "kv", + Options: map[string]string{"version": "2"}, + }) + if err != nil { + t.Fatal(err) + } + + kvData := map[string]interface{}{ + "data": map[string]interface{}{ + "bar": "a", + }, + } + + resp, err := c.Logical().JSONMergePatch("kv/data/foo", kvData) + if err == nil { + t.Fatalf("expected PATCH request to fail, resp: %#v\n", resp) + } + + if err != nil { + responseError := err.(*api.ResponseError) + t.Fatalf("expected PATCH request to fail with %d status code - err: %#v, resp: %#v\n", http.StatusNotFound, responseError, resp) + } +} From 8bab71dc42366b397a3e878357183455a2524066 Mon Sep 17 00:00:00 2001 From: Chris Capurso Date: Thu, 23 Sep 2021 15:25:29 -0400 Subject: [PATCH 09/30] add permission-based 'kv patch' tests in prep to add HTTP PATCH --- command/kv_test.go | 200 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 200 insertions(+) diff --git a/command/kv_test.go b/command/kv_test.go index 83af43a4dd17f..473e5dbd6db6b 100644 --- a/command/kv_test.go +++ b/command/kv_test.go @@ -1,6 +1,7 @@ package command import ( + "fmt" "io" "strings" "testing" @@ -552,3 +553,202 @@ func TestKVMetadataGetCommand(t *testing.T) { assertNoTabs(t, cmd) }) } + +func testKVPatchCommand(tb testing.TB) (*cli.MockUi, *KVPatchCommand) { + tb.Helper() + + ui := cli.NewMockUi() + return ui, &KVPatchCommand{ + BaseCommand: &BaseCommand{ + UI: ui, + }, + } +} + +func TestKVPatchCommand_RWMethodNotExists(t *testing.T) { + client, closer := testVaultServer(t) + defer closer() + + if err := client.Sys().Mount("kv/", &api.MountInput{ + Type: "kv-v2", + }); err != nil { + t.Fatal(err) + } + + // Give time for the upgrade code to run/finish + time.Sleep(time.Second) + + ui, cmd := testKVPatchCommand(t) + cmd.client = client + + code := cmd.Run([]string{"kv/patch/foo", "foo==a"}) + if code != 2 { + t.Errorf("patch command failed with code %d", code) + } + + combined := ui.OutputWriter.String() + ui.ErrorWriter.String() + + expectedOutputSubstr := "No value found" + if !strings.Contains(combined, expectedOutputSubstr) { + t.Errorf("expected %q to contain %q", combined, expectedOutputSubstr) + } +} + +func TestKVPatchCommand_RWMethodSucceeds(t *testing.T) { + client, closer := testVaultServer(t) + defer closer() + + if err := client.Sys().Mount("kv/", &api.MountInput{ + Type: "kv-v2", + }); err != nil { + t.Fatal(err) + } + + // Give time for the upgrade code to run/finish + time.Sleep(time.Second) + + if _, err := client.Logical().Write("kv/data/patch/foo", map[string]interface{}{ + "data": map[string]interface{}{ + "foo": "a", + "bar": "b", + }, + }); err != nil { + t.Fatal(err) + } + + ui, cmd := testKVPatchCommand(t) + cmd.client = client + + code := cmd.Run([]string{"kv/patch/foo", "foo==aa"}) + if code != 0 { + t.Errorf("patch command failed with code %d", code) + } + + combined := ui.OutputWriter.String() + ui.ErrorWriter.String() + + expectedOutputSubstr := "created_time" + if !strings.Contains(combined, expectedOutputSubstr) { + t.Errorf("expected %q to contain %q", combined, expectedOutputSubstr) + } +} + +func createTokenForPolicy(t *testing.T, client *api.Client, policy string) (*api.SecretAuth, error) { + t.Helper() + + if err := client.Sys().PutPolicy("policy", policy); err != nil { + return nil, err + } + + secret, err := client.Auth().Token().Create(&api.TokenCreateRequest{ + Policies: []string{"policy"}, + TTL: "30m", + }) + + if err != nil { + return nil, err + } + + if secret == nil || secret.Auth == nil || secret.Auth.ClientToken == "" { + return nil, fmt.Errorf("missing auth data: %#v", secret) + } + + return secret.Auth, err +} + +func TestKVPatchCommand_RWMethodNoRead(t *testing.T) { + client, closer := testVaultServer(t) + defer closer() + + if err := client.Sys().Mount("kv/", &api.MountInput{ + Type: "kv-v2", + }); err != nil { + t.Fatal(err) + } + + // Give time for the upgrade code to run/finish + time.Sleep(time.Second) + + // The rw patch method requires the read capability - create a + // policy that does not have it in order to force command to fail + policy := `path "kv/data/patch/foo" { capabilities = ["create", "update"] }` + secretAuth, err := createTokenForPolicy(t, client, policy) + + if err != nil { + t.Fatal(err) + } + + client.SetToken(secretAuth.ClientToken) + + if _, err := client.Logical().Write("kv/data/patch/foo", map[string]interface{}{ + "data": map[string]interface{}{ + "foo": "a", + "bar": "b", + }, + }); err != nil { + t.Fatal(err) + } + + ui, cmd := testKVPatchCommand(t) + cmd.client = client + + code := cmd.Run([]string{"kv/patch/foo", "foo==aa"}) + if code != 2 { + t.Errorf("patch command failed with code %d", code) + } + + combined := ui.OutputWriter.String() + ui.ErrorWriter.String() + + expectedOutputSubstr := "Error doing pre-read" + if !strings.Contains(combined, expectedOutputSubstr) { + t.Errorf("expected %q to contain %q", combined, expectedOutputSubstr) + } +} + +func TestKVPatchCommand_RWMethodNoUpdate(t *testing.T) { + client, closer := testVaultServer(t) + defer closer() + + if err := client.Sys().Mount("kv/", &api.MountInput{ + Type: "kv-v2", + }); err != nil { + t.Fatal(err) + } + + // Give time for the upgrade code to run/finish + time.Sleep(time.Second) + + // The rw patch method requires the update capability - create a + // policy that does not have it in order to force command to fail + policy := `path "kv/data/patch/foo" { capabilities = ["create", "read"] }` + secretAuth, err := createTokenForPolicy(t, client, policy) + + if err != nil { + t.Fatal(err) + } + + client.SetToken(secretAuth.ClientToken) + + if _, err := client.Logical().Write("kv/data/patch/foo", map[string]interface{}{ + "data": map[string]interface{}{ + "foo": "a", + "bar": "b", + }, + }); err != nil { + t.Fatal(err) + } + + ui, cmd := testKVPatchCommand(t) + cmd.client = client + + code := cmd.Run([]string{"kv/patch/foo", "foo==aa"}) + if code != 2 { + t.Errorf("patch command failed with code %d", code) + } + + combined := ui.OutputWriter.String() + ui.ErrorWriter.String() + + expectedOutputSubstr := "Error writing data" + if !strings.Contains(combined, expectedOutputSubstr) { + t.Errorf("expected %q to contain %q", combined, expectedOutputSubstr) + } +} From 67f7a60b2957bc832c8d74d83562d5528f9b5054 Mon Sep 17 00:00:00 2001 From: Chris Capurso Date: Fri, 24 Sep 2021 15:58:13 -0400 Subject: [PATCH 10/30] adding more 'kv patch' CLI command tests --- command/kv_test.go | 255 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 220 insertions(+), 35 deletions(-) diff --git a/command/kv_test.go b/command/kv_test.go index 473e5dbd6db6b..155ced7bc9fd2 100644 --- a/command/kv_test.go +++ b/command/kv_test.go @@ -565,32 +565,204 @@ func testKVPatchCommand(tb testing.TB) (*cli.MockUi, *KVPatchCommand) { } } -func TestKVPatchCommand_RWMethodNotExists(t *testing.T) { +func TestKVPatchCommand_ArgValidation(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + args []string + out string + code int + }{ + { + "not_enough_args", + []string{}, + "Not enough arguments", + 1, + }, + { + "empty_kvs", + []string{"kv/patch/foo"}, + "Must supply data", + 1, + }, + { + "kvs_no_value", + []string{"kv/patch/foo", "foo"}, + "Failed to parse K=V data", + 1, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + client, closer := testVaultServer(t) + defer closer() + + if err := client.Sys().Mount("kv/", &api.MountInput{ + Type: "kv-v2", + }); err != nil { + t.Fatalf("kv-v2 mount attempt failed - err: %#v\n", err) + } + + ui, cmd := testKVPatchCommand(t) + cmd.client = client + + code := cmd.Run(tc.args) + + if code != tc.code { + t.Fatalf("expected code to be %d but was %d for cmd %#v with args %#v\n", tc.code, code, cmd, tc.args) + } + + combined := ui.OutputWriter.String() + ui.ErrorWriter.String() + + if !strings.Contains(combined, tc.out) { + t.Fatalf("expected output to be %q but was %q for cmd %#v with args %#v\n", tc.out, combined, cmd, tc.args) + } + }) + } +} + +func TestKvPatchCommand_StdinFull(t *testing.T) { + client, closer := testVaultServer(t) + defer closer() + + if err := client.Sys().Mount("kv/", &api.MountInput{ + Type: "kv-v2", + }); err != nil { + t.Fatalf("kv-v2 mount attempt failed - err: %#v\n", err) + } + + if _, err := client.Logical().Write("kv/data/patch/foo", map[string]interface{}{ + "data": map[string]interface{}{ + "foo": "a", + }, + }); err != nil { + t.Fatalf("write failed, err: %#v\n", err) + } + + stdinR, stdinW := io.Pipe() + go func() { + stdinW.Write([]byte(`{"foo":"bar"}`)) + stdinW.Close() + }() + + _, cmd := testKVPatchCommand(t) + cmd.client = client + + cmd.testStdin = stdinR + + args := []string{"kv/patch/foo", "-"} + code := cmd.Run(args) + if code != 0 { + t.Fatalf("expected code to be 0 but was %d for cmd %#v with args %#v\n", code, cmd, args) + } + + secret, err := client.Logical().Read("kv/data/patch/foo") + if err != nil { + t.Fatalf("read failed, err: %#v\n", err) + } + + if secret == nil || secret.Data == nil { + t.Fatal("expected secret to have data") + } + + secretDataRaw, ok := secret.Data["data"] + + if !ok { + t.Fatalf("expected secret to have nested data key, data: %#v", secret.Data) + } + + secretData := secretDataRaw.(map[string]interface{}) + + if exp, act := "bar", secretData["foo"].(string); exp != act { + t.Fatalf("expected %q to be %q, data: %#v\n", act, exp, secret.Data) + } +} + +func TestKvPatchCommand_StdinValue(t *testing.T) { client, closer := testVaultServer(t) defer closer() if err := client.Sys().Mount("kv/", &api.MountInput{ Type: "kv-v2", }); err != nil { - t.Fatal(err) + t.Fatalf("kv-v2 mount attempt failed - err: %#v\n", err) } - // Give time for the upgrade code to run/finish - time.Sleep(time.Second) + if _, err := client.Logical().Write("kv/data/patch/foo", map[string]interface{}{ + "data": map[string]interface{}{ + "foo": "a", + }, + }); err != nil { + t.Fatalf("write failed, err: %#v\n", err) + } + + stdinR, stdinW := io.Pipe() + go func() { + stdinW.Write([]byte("bar")) + stdinW.Close() + }() + + _, cmd := testKVPatchCommand(t) + cmd.client = client + + cmd.testStdin = stdinR + + args := []string{"kv/patch/foo", "foo=-"} + code := cmd.Run(args) + if code != 0 { + t.Fatalf("expected code to be 0 but was %d for cmd %#v with args %#v\n", code, cmd, args) + } + + secret, err := client.Logical().Read("kv/patch/foo") + if err != nil { + t.Fatalf("read failed, err: %#v\n", err) + } + + if secret == nil || secret.Data == nil { + t.Fatal("expected secret to have data") + } + + secretDataRaw, ok := secret.Data["data"] + + if !ok { + t.Fatalf("expected secret to have nested data key, data: %#v\n", secret.Data) + } + + secretData := secretDataRaw.(map[string]interface{}) + + if exp, act := "bar", secretData["foo"].(string); exp != act { + t.Fatalf("expected %q to be %q, data: %#v\n", act, exp, secret.Data) + } +} + +func TestKVPatchCommand_RWMethodNotExists(t *testing.T) { + client, closer := testVaultServer(t) + defer closer() + + if err := client.Sys().Mount("kv/", &api.MountInput{ + Type: "kv-v2", + }); err != nil { + t.Fatalf("kv-v2 mount attempt failed - err: %#v\n", err) + } ui, cmd := testKVPatchCommand(t) cmd.client = client - code := cmd.Run([]string{"kv/patch/foo", "foo==a"}) + args := []string{"kv/patch/foo", "foo=a"} + code := cmd.Run(args) if code != 2 { - t.Errorf("patch command failed with code %d", code) + t.Fatalf("expected code to be 2 but was %d for cmd %#v with args %#v\n", code, cmd, args) } combined := ui.OutputWriter.String() + ui.ErrorWriter.String() expectedOutputSubstr := "No value found" if !strings.Contains(combined, expectedOutputSubstr) { - t.Errorf("expected %q to contain %q", combined, expectedOutputSubstr) + t.Fatalf("expected output %q to contain %q for cmd %#v with args %#v\n", combined, expectedOutputSubstr, cmd, args) } } @@ -601,34 +773,46 @@ func TestKVPatchCommand_RWMethodSucceeds(t *testing.T) { if err := client.Sys().Mount("kv/", &api.MountInput{ Type: "kv-v2", }); err != nil { - t.Fatal(err) + t.Fatalf("kv-v2 mount attempt failed - err: %#v\n", err) } - // Give time for the upgrade code to run/finish - time.Sleep(time.Second) - if _, err := client.Logical().Write("kv/data/patch/foo", map[string]interface{}{ "data": map[string]interface{}{ "foo": "a", "bar": "b", }, }); err != nil { - t.Fatal(err) + t.Fatalf("write failed, err: %#v\n", err) } ui, cmd := testKVPatchCommand(t) cmd.client = client - code := cmd.Run([]string{"kv/patch/foo", "foo==aa"}) + // Test single value + args := []string{"kv/patch/foo", "foo=aa"} + code := cmd.Run(args) if code != 0 { - t.Errorf("patch command failed with code %d", code) + t.Fatalf("expected code to be 0 but was %d for cmd %#v with args %#v\n", code, cmd, args) } combined := ui.OutputWriter.String() + ui.ErrorWriter.String() expectedOutputSubstr := "created_time" if !strings.Contains(combined, expectedOutputSubstr) { - t.Errorf("expected %q to contain %q", combined, expectedOutputSubstr) + t.Fatalf("expected output %q to contain %q for cmd %#v with args %#v\n", combined, expectedOutputSubstr, cmd, args) + } + + // Test multi value + args = []string{"kv/patch/foo", "foo=aaa bar=bbb"} + code = cmd.Run(args) + if code != 0 { + t.Fatalf("expected code to be 0 but was %d for cmd %#v with args %#v\n", code, cmd, args) + } + + combined = ui.OutputWriter.String() + ui.ErrorWriter.String() + + if !strings.Contains(combined, expectedOutputSubstr) { + t.Fatalf("expected output %q to contain %q for cmd %#v with args %#v\n", combined, expectedOutputSubstr, cmd, args) } } @@ -662,19 +846,16 @@ func TestKVPatchCommand_RWMethodNoRead(t *testing.T) { if err := client.Sys().Mount("kv/", &api.MountInput{ Type: "kv-v2", }); err != nil { - t.Fatal(err) + t.Fatalf("kv-v2 mount attempt failed - err: %#v\n", err) } - // Give time for the upgrade code to run/finish - time.Sleep(time.Second) - // The rw patch method requires the read capability - create a // policy that does not have it in order to force command to fail policy := `path "kv/data/patch/foo" { capabilities = ["create", "update"] }` secretAuth, err := createTokenForPolicy(t, client, policy) if err != nil { - t.Fatal(err) + t.Fatalf("policy/token creation failed for policy %s, err: %#v\n", policy, err) } client.SetToken(secretAuth.ClientToken) @@ -685,22 +866,26 @@ func TestKVPatchCommand_RWMethodNoRead(t *testing.T) { "bar": "b", }, }); err != nil { - t.Fatal(err) + t.Fatalf("write failed, err: %#v\n", err) } ui, cmd := testKVPatchCommand(t) cmd.client = client - code := cmd.Run([]string{"kv/patch/foo", "foo==aa"}) - if code != 2 { - t.Errorf("patch command failed with code %d", code) + args := []string{"kv/patch/foo", "foo=aa"} + code := cmd.Run(args) + expectedCode := 2 + + if code != expectedCode { + t.Fatalf("expected code to be %d but was %d for cmd %#v with args %#v\n", expectedCode, code, cmd, args) } combined := ui.OutputWriter.String() + ui.ErrorWriter.String() expectedOutputSubstr := "Error doing pre-read" if !strings.Contains(combined, expectedOutputSubstr) { - t.Errorf("expected %q to contain %q", combined, expectedOutputSubstr) + t.Fatalf("expected output %q to contain %q for cmd %#v with args %#v\n", combined, expectedOutputSubstr, cmd, args) + } } @@ -711,19 +896,16 @@ func TestKVPatchCommand_RWMethodNoUpdate(t *testing.T) { if err := client.Sys().Mount("kv/", &api.MountInput{ Type: "kv-v2", }); err != nil { - t.Fatal(err) + t.Fatalf("kv-v2 mount attempt failed - err: %#v\n", err) } - // Give time for the upgrade code to run/finish - time.Sleep(time.Second) - // The rw patch method requires the update capability - create a // policy that does not have it in order to force command to fail policy := `path "kv/data/patch/foo" { capabilities = ["create", "read"] }` secretAuth, err := createTokenForPolicy(t, client, policy) if err != nil { - t.Fatal(err) + t.Fatalf("policy/token creation failed for policy %s, err: %#v\n", policy, err) } client.SetToken(secretAuth.ClientToken) @@ -734,21 +916,24 @@ func TestKVPatchCommand_RWMethodNoUpdate(t *testing.T) { "bar": "b", }, }); err != nil { - t.Fatal(err) + t.Fatalf("write failed, err: %#v\n", err) } ui, cmd := testKVPatchCommand(t) cmd.client = client - code := cmd.Run([]string{"kv/patch/foo", "foo==aa"}) - if code != 2 { - t.Errorf("patch command failed with code %d", code) + args := []string{"kv/patch/foo", "foo=aa"} + code := cmd.Run(args) + expectedCode := 2 + + if code != expectedCode { + t.Fatalf("expected code to be %d but was %d for cmd %#v with args %#v\n", expectedCode, code, cmd, args) } combined := ui.OutputWriter.String() + ui.ErrorWriter.String() expectedOutputSubstr := "Error writing data" if !strings.Contains(combined, expectedOutputSubstr) { - t.Errorf("expected %q to contain %q", combined, expectedOutputSubstr) + t.Fatalf("expected output %q to contain %q for cmd %#v with args %#v\n", combined, expectedOutputSubstr, cmd, args) } } From ecb1efea7a35ecf03375c4b38a47854317db215b Mon Sep 17 00:00:00 2001 From: Chris Capurso Date: Mon, 27 Sep 2021 10:51:43 -0400 Subject: [PATCH 11/30] fix TestHandler_Patch_NotFound --- http/handler_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/http/handler_test.go b/http/handler_test.go index b22993f755b68..1b2b486ff9b88 100644 --- a/http/handler_test.go +++ b/http/handler_test.go @@ -921,8 +921,8 @@ func TestHandler_Patch_NotFound(t *testing.T) { t.Fatalf("expected PATCH request to fail, resp: %#v\n", resp) } - if err != nil { - responseError := err.(*api.ResponseError) + responseError := err.(*api.ResponseError) + if responseError.StatusCode != http.StatusNotFound { t.Fatalf("expected PATCH request to fail with %d status code - err: %#v, resp: %#v\n", http.StatusNotFound, responseError, resp) } } From 0bfec354c09e476361a1dc055198656dc55efac3 Mon Sep 17 00:00:00 2001 From: Chris Capurso Date: Mon, 27 Sep 2021 12:09:57 -0400 Subject: [PATCH 12/30] Fix TestKvPatchCommand_StdinValue --- command/kv_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/kv_test.go b/command/kv_test.go index 155ced7bc9fd2..03d0ff219425d 100644 --- a/command/kv_test.go +++ b/command/kv_test.go @@ -717,7 +717,7 @@ func TestKvPatchCommand_StdinValue(t *testing.T) { t.Fatalf("expected code to be 0 but was %d for cmd %#v with args %#v\n", code, cmd, args) } - secret, err := client.Logical().Read("kv/patch/foo") + secret, err := client.Logical().Read("kv/data/patch/foo") if err != nil { t.Fatalf("read failed, err: %#v\n", err) } From 6b9aeca84b37865fe58c10bef8b5efd35109bba6 Mon Sep 17 00:00:00 2001 From: Chris Capurso Date: Mon, 27 Sep 2021 12:32:56 -0400 Subject: [PATCH 13/30] add audit log test for HTTP PATCH --- http/handler_test.go | 106 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 99 insertions(+), 7 deletions(-) diff --git a/http/handler_test.go b/http/handler_test.go index 1b2b486ff9b88..30994e60ac06f 100644 --- a/http/handler_test.go +++ b/http/handler_test.go @@ -5,7 +5,10 @@ import ( "crypto/tls" "encoding/json" "errors" + kv "github.com/hashicorp/vault-plugin-secrets-kv" "github.com/hashicorp/vault/api" + "github.com/hashicorp/vault/audit" + auditFile "github.com/hashicorp/vault/builtin/audit/file" "io/ioutil" "net/http" "net/http/httptest" @@ -14,10 +17,10 @@ import ( "reflect" "strings" "testing" + "time" "github.com/go-test/deep" "github.com/hashicorp/go-cleanhttp" - kv "github.com/hashicorp/vault-plugin-secrets-kv" "github.com/hashicorp/vault/helper/namespace" "github.com/hashicorp/vault/sdk/helper/consts" "github.com/hashicorp/vault/sdk/logical" @@ -824,7 +827,7 @@ func TestHandler_Parse_Form(t *testing.T) { func TestHandler_Patch_BadContentTypeHeader(t *testing.T) { coreConfig := &vault.CoreConfig{ LogicalBackends: map[string]logical.Factory{ - "kv": kv.Factory, + "kv": kv.VersionedKVFactory, }, } @@ -843,8 +846,7 @@ func TestHandler_Patch_BadContentTypeHeader(t *testing.T) { // Mount a KVv2 backend err := c.Sys().Mount("kv", &api.MountInput{ - Type: "kv", - Options: map[string]string{"version": "2"}, + Type: "kv-v2", }) if err != nil { t.Fatal(err) @@ -884,7 +886,7 @@ func TestHandler_Patch_BadContentTypeHeader(t *testing.T) { func TestHandler_Patch_NotFound(t *testing.T) { coreConfig := &vault.CoreConfig{ LogicalBackends: map[string]logical.Factory{ - "kv": kv.Factory, + "kv": kv.VersionedKVFactory, }, } @@ -903,8 +905,7 @@ func TestHandler_Patch_NotFound(t *testing.T) { // Mount a KVv2 backend err := c.Sys().Mount("kv", &api.MountInput{ - Type: "kv", - Options: map[string]string{"version": "2"}, + Type: "kv-v2", }) if err != nil { t.Fatal(err) @@ -926,3 +927,94 @@ func TestHandler_Patch_NotFound(t *testing.T) { t.Fatalf("expected PATCH request to fail with %d status code - err: %#v, resp: %#v\n", http.StatusNotFound, responseError, resp) } } + +func TestHandler_Patch_Audit(t *testing.T) { + coreConfig := &vault.CoreConfig{ + LogicalBackends: map[string]logical.Factory{ + "kv": kv.VersionedKVFactory, + }, + AuditBackends: map[string]audit.Factory{ + "file": auditFile.Factory, + }, + } + + cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{ + HandlerFunc: Handler, + }) + + cluster.Start() + defer cluster.Cleanup() + + cores := cluster.Cores + + core := cores[0].Core + c := cluster.Cores[0].Client + vault.TestWaitActive(t, core) + + if err := c.Sys().Mount("kv/", &api.MountInput{ + Type: "kv-v2", + }); err != nil { + t.Fatalf("kv-v2 mount attempt failed - err: %#v\n", err) + } + + auditLogFile, err := ioutil.TempFile("", "httppatch") + if err != nil { + t.Fatal(err) + } + + err = c.Sys().EnableAuditWithOptions("file", &api.EnableAuditOptions{ + Type: "file", + Options: map[string]string{ + "file_path": auditLogFile.Name(), + }, + }) + + writeData := map[string]interface{}{ + "data": map[string]interface{}{ + "bar": "a", + }, + } + + resp, err := c.Logical().Write("kv/data/foo", writeData) + if err != nil { + t.Fatalf("write request failed, err: %#v, resp: %#v\n", err, resp) + } + + patchData := map[string]interface{}{ + "data": map[string]interface{}{ + "baz": "b", + }, + } + + resp, err = c.Logical().JSONMergePatch("kv/data/foo", patchData) + if err != nil { + t.Fatalf("patch request failed, err: %#v, resp: %#v\n", err, resp) + } + + patchRequestLogCount := 0 + patchResponseLogCount := 0 + decoder := json.NewDecoder(auditLogFile) + + var auditRecord map[string]interface{} + for decoder.Decode(&auditRecord) == nil { + auditRequest := map[string]interface{}{} + + if req, ok := auditRecord["request"]; ok { + auditRequest = req.(map[string]interface{}) + } + + if auditRequest["operation"] == "patch" && auditRecord["type"] == "request" { + patchRequestLogCount += 1 + } else if auditRequest["operation"] == "patch" && auditRecord["type"] == "response" { + patchResponseLogCount += 1 + } + } + + if patchRequestLogCount != 1 { + t.Fatalf("expected 1 patch request audit log record, saw %d\n", patchRequestLogCount) + } + + if patchResponseLogCount != 1 { + t.Fatalf("expected 1 patch response audit log record, saw %d\n", patchResponseLogCount) + } +} From 4f9b112fe950d2f26c047489471d1b8a49c47171 Mon Sep 17 00:00:00 2001 From: Josh Black Date: Tue, 28 Sep 2021 13:59:36 -0700 Subject: [PATCH 14/30] patch CLI changes --- command/kv_patch.go | 130 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 117 insertions(+), 13 deletions(-) diff --git a/command/kv_patch.go b/command/kv_patch.go index 73a5e42a627c6..cf127b62a9098 100644 --- a/command/kv_patch.go +++ b/command/kv_patch.go @@ -6,6 +6,7 @@ import ( "os" "strings" + "github.com/hashicorp/vault/api" "github.com/mitchellh/cli" "github.com/posener/complete" ) @@ -18,7 +19,9 @@ var ( type KVPatchCommand struct { *BaseCommand - testStdin io.Reader // for tests + flagCAS int + flagMethod string + testStdin io.Reader // for tests } func (c *KVPatchCommand) Synopsis() string { @@ -45,6 +48,25 @@ Usage: vault kv patch [options] KEY [DATA] $ echo "abcd1234" | vault kv patch secret/foo bar=- + To perform a Check-And-Set operation, specify the -cas flag with the + appropriate version number corresponding to the key you want to perform + the CAS operation on: + + $ vault kv patch -cas=1 secret/foo bar=baz + + By default, this operation will attempt an HTTP PATCH operation. If your + policy does not allow that, it will fall back to a read/local update/write approach. + If you wish to specify which method this command should use, you may do so + with the -method flag. When -method=patch is specified, only an HTTP PATCH + operation will be tried. If it fails, the entire command will fail. + + $ vault kv patch -method=patch secret/foo bar=baz + + When -method=rw is specified, only a read/local update/write approach will be tried. + This was the default behavior previous to Vault 1.9. + + $ vault kv patch -method=rw secret/foo bar=baz + Additional flags and more advanced use cases are detailed below. ` + c.Flags().Help() @@ -54,6 +76,26 @@ Usage: vault kv patch [options] KEY [DATA] func (c *KVPatchCommand) Flags() *FlagSets { set := c.flagSet(FlagSetHTTP | FlagSetOutputField | FlagSetOutputFormat) + // Patch specific options + f := set.NewFlagSet("Common Options") + + f.IntVar(&IntVar{ + Name: "cas", + Target: &c.flagCAS, + Default: 0, + Usage: `Specifies to use a Check-And-Set operation. If set to 0 or not + set, the patch will be allowed. If the index is non-zero the patch will + only be allowed if the key’s current version matches the version + specified in the cas parameter.`, + }) + + f.StringVar(&StringVar{ + Name: "method", + Target: &c.flagMethod, + Default: "patch", + Usage: ``, + }) + return set } @@ -121,6 +163,28 @@ func (c *KVPatchCommand) Run(args []string) int { return 2 } + // Check the method and behave accordingly + var secret *api.Secret + var code int + + switch c.flagMethod { + case "rw": + secret, code = c.readThenWrite(client, path, newData) + case "patch": + secret, code = c.actualPatch(client, path, newData) + default: + c.UI.Error(fmt.Sprintf("Unsupported method provided to -method flag: %s", c.flagMethod)) + return 2 + } + + if code != 0 { + return code + } + + return OutputSecret(c.UI, secret) +} + +func (c *KVPatchCommand) readThenWrite(client *api.Client, path string, newData map[string]interface{}) (*api.Secret, int) { // First, do a read. // Note that we don't want to see curl output for the read request. curOutputCurl := client.OutputCurlString() @@ -129,45 +193,45 @@ func (c *KVPatchCommand) Run(args []string) int { client.SetOutputCurlString(curOutputCurl) if err != nil { c.UI.Error(fmt.Sprintf("Error doing pre-read at %s: %s", path, err)) - return 2 + return nil, 2 } // Make sure a value already exists if secret == nil || secret.Data == nil { c.UI.Error(fmt.Sprintf("No value found at %s", path)) - return 2 + return nil, 2 } // Verify metadata found rawMeta, ok := secret.Data["metadata"] if !ok || rawMeta == nil { c.UI.Error(fmt.Sprintf("No metadata found at %s; patch only works on existing data", path)) - return 2 + return nil, 2 } meta, ok := rawMeta.(map[string]interface{}) if !ok { c.UI.Error(fmt.Sprintf("Metadata found at %s is not the expected type (JSON object)", path)) - return 2 + return nil, 2 } if meta == nil { c.UI.Error(fmt.Sprintf("No metadata found at %s; patch only works on existing data", path)) - return 2 + return nil, 2 } // Verify old data found rawData, ok := secret.Data["data"] if !ok || rawData == nil { c.UI.Error(fmt.Sprintf("No data found at %s; patch only works on existing data", path)) - return 2 + return nil, 2 } data, ok := rawData.(map[string]interface{}) if !ok { c.UI.Error(fmt.Sprintf("Data found at %s is not the expected type (JSON object)", path)) - return 2 + return nil, 2 } if data == nil { c.UI.Error(fmt.Sprintf("No data found at %s; patch only works on existing data", path)) - return 2 + return nil, 2 } // Copy new data over @@ -183,19 +247,59 @@ func (c *KVPatchCommand) Run(args []string) int { }) if err != nil { c.UI.Error(fmt.Sprintf("Error writing data to %s: %s", path, err)) - return 2 + return nil, 2 } + if secret == nil { // Don't output anything unless using the "table" format if Format(c.UI) == "table" { c.UI.Info(fmt.Sprintf("Success! Data written to: %s", path)) } - return 0 + return nil, 0 } if c.flagField != "" { - return PrintRawField(c.UI, secret, c.flagField) + return secret, PrintRawField(c.UI, secret, c.flagField) } - return OutputSecret(c.UI, secret) + return secret, 0 +} + +func (c *KVPatchCommand) actualPatch(client *api.Client, path string, newData map[string]interface{}) (*api.Secret, int) { + data := map[string]interface{}{ + "data": newData, + "options": map[string]interface{}{}, + } + + if c.flagCAS > 0 { + data["options"].(map[string]interface{})["cas"] = c.flagCAS + } + + secret, err := client.Logical().JSONMergePatch(path, data) + if err != nil { + // If it's a 403, that probably means they don't have the patch capability in their policy. Fall back to + // the old way of doing it. + if re, ok := err.(*api.ResponseError); ok { + if re.StatusCode == 403 { + fmt.Println("detected a 403, trying the old way") + return c.readThenWrite(client, path, newData) + } + } + + c.UI.Error(fmt.Sprintf("Error writing data to %s: %s", path, err)) + if secret != nil { + OutputSecret(c.UI, secret) + } + return nil, 2 + } + + if secret == nil { + // Don't output anything unless using the "table" format + if Format(c.UI) == "table" { + c.UI.Info(fmt.Sprintf("Success! Data written to: %s", path)) + } + return nil, 0 + } + + return secret, 0 } From aeeff0e2f4746b6d1f3f87ff445f52d12ab15982 Mon Sep 17 00:00:00 2001 From: Josh Black Date: Wed, 29 Sep 2021 15:27:37 -0700 Subject: [PATCH 15/30] add patch CLI tests --- command/kv_patch.go | 7 +- command/kv_test.go | 206 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 207 insertions(+), 6 deletions(-) diff --git a/command/kv_patch.go b/command/kv_patch.go index cf127b62a9098..91932e47684d5 100644 --- a/command/kv_patch.go +++ b/command/kv_patch.go @@ -93,7 +93,10 @@ func (c *KVPatchCommand) Flags() *FlagSets { Name: "method", Target: &c.flagMethod, Default: "patch", - Usage: ``, + Usage: `Specifies which method of patching to use. If set to "patch", then + an HTTP PATCH request will be issued. This is the default. If set to "rw", + then a read will be performed, then a local update, followed by a remote + update.`, }) return set @@ -281,7 +284,7 @@ func (c *KVPatchCommand) actualPatch(client *api.Client, path string, newData ma // the old way of doing it. if re, ok := err.(*api.ResponseError); ok { if re.StatusCode == 403 { - fmt.Println("detected a 403, trying the old way") + c.UI.Warn(fmt.Sprintf("Data was written to %s but we recommend that you add the \"patch\" capability to your ACL policy in order to use HTTP PATCH in the future.", path)) return c.readThenWrite(client, path, newData) } } diff --git a/command/kv_test.go b/command/kv_test.go index 03d0ff219425d..b9920d70a9c01 100644 --- a/command/kv_test.go +++ b/command/kv_test.go @@ -816,6 +816,208 @@ func TestKVPatchCommand_RWMethodSucceeds(t *testing.T) { } } +func TestKVPatchCommand_CAS(t *testing.T) { + cases := []struct { + name string + args []string + expected string + out string + code int + }{ + { + "right version", + []string{"-cas", "1", "kv/foo", "bar=quux"}, + "quux", + "", + 0, + }, + { + "wrong version", + []string{"-cas", "2", "kv/foo", "bar=wibble"}, + "baz", + "check-and-set parameter did not match the current version", + 2, + }, + } + + for _, tc := range cases { + tc := tc + + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + client, closer := testVaultServer(t) + defer closer() + + if err := client.Sys().Mount("kv/", &api.MountInput{ + Type: "kv-v2", + }); err != nil { + t.Fatalf("kv-v2 mount attempt failed - err: %#v\n", err) + } + + // create a policy with patch capability + policy := `path "kv/*" { capabilities = ["create", "update", "read", "patch"] }` + secretAuth, err := createTokenForPolicy(t, client, policy) + if err != nil { + t.Fatalf("policy/token creation failed for policy %s, err: %#v\n", policy, err) + } + + kvClient, err := client.Clone() + if err != nil { + t.Fatal(err) + } + + kvClient.SetToken(secretAuth.ClientToken) + + _, err = kvClient.Logical().Write("kv/data/foo", map[string]interface{}{"data": map[string]interface{}{"bar": "baz"}}) + if err != nil { + t.Fatal(err) + } + + ui, cmd := testKVPatchCommand(t) + cmd.client = kvClient + + code := cmd.Run(tc.args) + + if code != tc.code { + t.Fatalf("expected code to be %d but was %d", tc.code, code) + } + + if tc.out != "" { + combined := ui.OutputWriter.String() + ui.ErrorWriter.String() + if !strings.Contains(combined, tc.out) { + t.Errorf("expected %q to contain %q", combined, tc.out) + } + } + + secret, err := kvClient.Logical().Read("kv/data/foo") + bar := secret.Data["data"].(map[string]interface{})["bar"] + if bar != tc.expected { + t.Fatalf("expected bar to be %q but it was %q", tc.expected, bar) + } + }) + } +} + +func TestKVPatchCommand_Methods(t *testing.T) { + cases := []struct { + name string + args []string + expected string + code int + }{ + { + "rw", + []string{"-method", "rw", "kv/foo", "bar=quux"}, + "quux", + 0, + }, + { + "patch", + []string{"-method", "patch", "kv/foo", "bar=wibble"}, + "wibble", + 0, + }, + } + + for _, tc := range cases { + tc := tc + + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + client, closer := testVaultServer(t) + defer closer() + + if err := client.Sys().Mount("kv/", &api.MountInput{ + Type: "kv-v2", + }); err != nil { + t.Fatalf("kv-v2 mount attempt failed - err: %#v\n", err) + } + + // create a policy with patch capability + policy := `path "kv/*" { capabilities = ["create", "update", "read", "patch"] }` + secretAuth, err := createTokenForPolicy(t, client, policy) + if err != nil { + t.Fatalf("policy/token creation failed for policy %s, err: %#v\n", policy, err) + } + + kvClient, err := client.Clone() + if err != nil { + t.Fatal(err) + } + + kvClient.SetToken(secretAuth.ClientToken) + + _, err = kvClient.Logical().Write("kv/data/foo", map[string]interface{}{"data": map[string]interface{}{"bar": "baz"}}) + if err != nil { + t.Fatal(err) + } + + _, cmd := testKVPatchCommand(t) + cmd.client = kvClient + + code := cmd.Run(tc.args) + + if code != tc.code { + t.Fatalf("expected code to be %d but was %d", tc.code, code) + } + + secret, err := kvClient.Logical().Read("kv/data/foo") + bar := secret.Data["data"].(map[string]interface{})["bar"] + if bar != tc.expected { + t.Fatalf("expected bar to be %q but it was %q", tc.expected, bar) + } + }) + } +} + +func TestKVPatchCommand_403Fallback(t *testing.T) { + t.Parallel() + client, closer := testVaultServer(t) + defer closer() + + if err := client.Sys().Mount("kv/", &api.MountInput{ + Type: "kv-v2", + }); err != nil { + t.Fatalf("kv-v2 mount attempt failed - err: %#v\n", err) + } + + // create a policy without patch capability + policy := `path "kv/*" { capabilities = ["create", "update", "read"] }` + secretAuth, err := createTokenForPolicy(t, client, policy) + if err != nil { + t.Fatalf("policy/token creation failed for policy %s, err: %#v\n", policy, err) + } + + kvClient, err := client.Clone() + if err != nil { + t.Fatal(err) + } + + kvClient.SetToken(secretAuth.ClientToken) + + // Write a value then attempt to patch it. It should fail, then fall back to the old method + _, err = kvClient.Logical().Write("kv/data/foo", map[string]interface{}{"data": map[string]interface{}{"bar": "baz"}}) + if err != nil { + t.Fatal(err) + } + + ui, cmd := testKVPatchCommand(t) + cmd.client = kvClient + code := cmd.Run([]string{"kv/foo", "bar=quux"}) + + if code != 0 { + t.Fatalf("expected code to be 0 but was %d", code) + } + + combined := ui.OutputWriter.String() + ui.ErrorWriter.String() + expected := `add the "patch" capability to your ACL policy` + if !strings.Contains(combined, expected) { + t.Errorf("expected %q to contain %q", combined, expected) + } +} + func createTokenForPolicy(t *testing.T, client *api.Client, policy string) (*api.SecretAuth, error) { t.Helper() @@ -827,7 +1029,6 @@ func createTokenForPolicy(t *testing.T, client *api.Client, policy string) (*api Policies: []string{"policy"}, TTL: "30m", }) - if err != nil { return nil, err } @@ -853,7 +1054,6 @@ func TestKVPatchCommand_RWMethodNoRead(t *testing.T) { // policy that does not have it in order to force command to fail policy := `path "kv/data/patch/foo" { capabilities = ["create", "update"] }` secretAuth, err := createTokenForPolicy(t, client, policy) - if err != nil { t.Fatalf("policy/token creation failed for policy %s, err: %#v\n", policy, err) } @@ -885,7 +1085,6 @@ func TestKVPatchCommand_RWMethodNoRead(t *testing.T) { expectedOutputSubstr := "Error doing pre-read" if !strings.Contains(combined, expectedOutputSubstr) { t.Fatalf("expected output %q to contain %q for cmd %#v with args %#v\n", combined, expectedOutputSubstr, cmd, args) - } } @@ -903,7 +1102,6 @@ func TestKVPatchCommand_RWMethodNoUpdate(t *testing.T) { // policy that does not have it in order to force command to fail policy := `path "kv/data/patch/foo" { capabilities = ["create", "read"] }` secretAuth, err := createTokenForPolicy(t, client, policy) - if err != nil { t.Fatalf("policy/token creation failed for policy %s, err: %#v\n", policy, err) } From af0e962d7ad6d5b36ba47ca11a1528ec725d4857 Mon Sep 17 00:00:00 2001 From: Chris Capurso Date: Thu, 30 Sep 2021 14:13:52 -0400 Subject: [PATCH 16/30] change JSONMergePatch func to accept a ctx --- api/logical.go | 18 +++++++++++------- command/kv_patch.go | 3 ++- http/handler_test.go | 18 ++++++++---------- 3 files changed, 21 insertions(+), 18 deletions(-) diff --git a/api/logical.go b/api/logical.go index 0e28844037dc9..9d3d6f4666a20 100644 --- a/api/logical.go +++ b/api/logical.go @@ -131,15 +131,18 @@ func (c *Logical) List(path string) (*Secret, error) { } func (c *Logical) Write(path string, data map[string]interface{}) (*Secret, error) { + ctx, cancelFunc := context.WithCancel(context.Background()) + defer cancelFunc() + r := c.c.NewRequest("PUT", "/v1/"+path) if err := r.SetJSONBody(data); err != nil { return nil, err } - return c.write(path, r) + return c.write(ctx, path, r) } -func (c *Logical) JSONMergePatch(path string, data map[string]interface{}) (*Secret, error) { +func (c *Logical) JSONMergePatch(ctx context.Context, path string, data map[string]interface{}) (*Secret, error) { r := c.c.NewRequest("PATCH", "/v1/"+path) r.Headers = http.Header{ "Content-Type": []string{"application/merge-patch+json"}, @@ -148,19 +151,20 @@ func (c *Logical) JSONMergePatch(path string, data map[string]interface{}) (*Sec return nil, err } - return c.write(path, r) + return c.write(ctx, path, r) } func (c *Logical) WriteBytes(path string, data []byte) (*Secret, error) { + ctx, cancelFunc := context.WithCancel(context.Background()) + defer cancelFunc() + r := c.c.NewRequest("PUT", "/v1/"+path) r.BodyBytes = data - return c.write(path, r) + return c.write(ctx, path, r) } -func (c *Logical) write(path string, request *Request) (*Secret, error) { - ctx, cancelFunc := context.WithCancel(context.Background()) - defer cancelFunc() +func (c *Logical) write(ctx context.Context, path string, request *Request) (*Secret, error) { resp, err := c.c.RawRequestWithContext(ctx, request) if resp != nil { defer resp.Body.Close() diff --git a/command/kv_patch.go b/command/kv_patch.go index 91932e47684d5..6a36a9a37e6e1 100644 --- a/command/kv_patch.go +++ b/command/kv_patch.go @@ -1,6 +1,7 @@ package command import ( + "context" "fmt" "io" "os" @@ -278,7 +279,7 @@ func (c *KVPatchCommand) actualPatch(client *api.Client, path string, newData ma data["options"].(map[string]interface{})["cas"] = c.flagCAS } - secret, err := client.Logical().JSONMergePatch(path, data) + secret, err := client.Logical().JSONMergePatch(context.Background(), path, data) if err != nil { // If it's a 403, that probably means they don't have the patch capability in their policy. Fall back to // the old way of doing it. diff --git a/http/handler_test.go b/http/handler_test.go index 30994e60ac06f..3352873a9a448 100644 --- a/http/handler_test.go +++ b/http/handler_test.go @@ -5,10 +5,16 @@ import ( "crypto/tls" "encoding/json" "errors" + "github.com/go-test/deep" + "github.com/hashicorp/go-cleanhttp" kv "github.com/hashicorp/vault-plugin-secrets-kv" "github.com/hashicorp/vault/api" "github.com/hashicorp/vault/audit" auditFile "github.com/hashicorp/vault/builtin/audit/file" + "github.com/hashicorp/vault/helper/namespace" + "github.com/hashicorp/vault/sdk/helper/consts" + "github.com/hashicorp/vault/sdk/logical" + "github.com/hashicorp/vault/vault" "io/ioutil" "net/http" "net/http/httptest" @@ -17,14 +23,6 @@ import ( "reflect" "strings" "testing" - "time" - - "github.com/go-test/deep" - "github.com/hashicorp/go-cleanhttp" - "github.com/hashicorp/vault/helper/namespace" - "github.com/hashicorp/vault/sdk/helper/consts" - "github.com/hashicorp/vault/sdk/logical" - "github.com/hashicorp/vault/vault" ) func TestHandler_parseMFAHandler(t *testing.T) { @@ -917,7 +915,7 @@ func TestHandler_Patch_NotFound(t *testing.T) { }, } - resp, err := c.Logical().JSONMergePatch("kv/data/foo", kvData) + resp, err := c.Logical().JSONMergePatch(context.Background(),"kv/data/foo", kvData) if err == nil { t.Fatalf("expected PATCH request to fail, resp: %#v\n", resp) } @@ -986,7 +984,7 @@ func TestHandler_Patch_Audit(t *testing.T) { }, } - resp, err = c.Logical().JSONMergePatch("kv/data/foo", patchData) + resp, err = c.Logical().JSONMergePatch(context.Background(),"kv/data/foo", patchData) if err != nil { t.Fatalf("patch request failed, err: %#v, resp: %#v\n", err, resp) } From 07ae4ea6f758be2152d610739b3bd544963ef785 Mon Sep 17 00:00:00 2001 From: Chris Capurso Date: Thu, 30 Sep 2021 14:14:29 -0400 Subject: [PATCH 17/30] fix TestKVPatchCommand_RWMethodNotExists and TestKVPatchCommand_RWMethodSucceeds to specify -method flag --- command/kv_test.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/command/kv_test.go b/command/kv_test.go index b9920d70a9c01..0cf96d1db8f16 100644 --- a/command/kv_test.go +++ b/command/kv_test.go @@ -752,7 +752,7 @@ func TestKVPatchCommand_RWMethodNotExists(t *testing.T) { ui, cmd := testKVPatchCommand(t) cmd.client = client - args := []string{"kv/patch/foo", "foo=a"} + args := []string{"-method", "rw", "kv/patch/foo", "foo=a"} code := cmd.Run(args) if code != 2 { t.Fatalf("expected code to be 2 but was %d for cmd %#v with args %#v\n", code, cmd, args) @@ -789,7 +789,7 @@ func TestKVPatchCommand_RWMethodSucceeds(t *testing.T) { cmd.client = client // Test single value - args := []string{"kv/patch/foo", "foo=aa"} + args := []string{"-method", "rw", "kv/patch/foo", "foo=aa"} code := cmd.Run(args) if code != 0 { t.Fatalf("expected code to be 0 but was %d for cmd %#v with args %#v\n", code, cmd, args) @@ -803,7 +803,10 @@ func TestKVPatchCommand_RWMethodSucceeds(t *testing.T) { } // Test multi value - args = []string{"kv/patch/foo", "foo=aaa bar=bbb"} + ui, cmd = testKVPatchCommand(t) + cmd.client = client + + args = []string{"-method", "rw", "kv/patch/foo", "foo=aaa", "bar=bbb"} code = cmd.Run(args) if code != 0 { t.Fatalf("expected code to be 0 but was %d for cmd %#v with args %#v\n", code, cmd, args) From d18f9e3ea5072bd4af4dcf141b22d1433d21c07d Mon Sep 17 00:00:00 2001 From: Chris Capurso Date: Thu, 30 Sep 2021 14:15:58 -0400 Subject: [PATCH 18/30] go fmt --- http/handler_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/http/handler_test.go b/http/handler_test.go index 3352873a9a448..48dcf8b6021c5 100644 --- a/http/handler_test.go +++ b/http/handler_test.go @@ -915,7 +915,7 @@ func TestHandler_Patch_NotFound(t *testing.T) { }, } - resp, err := c.Logical().JSONMergePatch(context.Background(),"kv/data/foo", kvData) + resp, err := c.Logical().JSONMergePatch(context.Background(), "kv/data/foo", kvData) if err == nil { t.Fatalf("expected PATCH request to fail, resp: %#v\n", resp) } @@ -984,7 +984,7 @@ func TestHandler_Patch_Audit(t *testing.T) { }, } - resp, err = c.Logical().JSONMergePatch(context.Background(),"kv/data/foo", patchData) + resp, err = c.Logical().JSONMergePatch(context.Background(), "kv/data/foo", patchData) if err != nil { t.Fatalf("patch request failed, err: %#v, resp: %#v\n", err, resp) } From 1cd8bf442aa542ac60f958100705cec9e190dc9a Mon Sep 17 00:00:00 2001 From: Josh Black Date: Fri, 1 Oct 2021 12:13:10 -0700 Subject: [PATCH 19/30] add a test to verify patching works by default with the root token --- vault/external_tests/kv/kv_patch_test.go | 58 +++++++++++++++++++ .../{misc => kv}/kvv2_upgrade_test.go | 2 +- 2 files changed, 59 insertions(+), 1 deletion(-) create mode 100644 vault/external_tests/kv/kv_patch_test.go rename vault/external_tests/{misc => kv}/kvv2_upgrade_test.go (99%) diff --git a/vault/external_tests/kv/kv_patch_test.go b/vault/external_tests/kv/kv_patch_test.go new file mode 100644 index 0000000000000..b2f71c19ae477 --- /dev/null +++ b/vault/external_tests/kv/kv_patch_test.go @@ -0,0 +1,58 @@ +package kv + +import ( + "context" + "testing" + + logicalKv "github.com/hashicorp/vault-plugin-secrets-kv" + "github.com/hashicorp/vault/api" + vaulthttp "github.com/hashicorp/vault/http" + "github.com/hashicorp/vault/sdk/logical" + "github.com/hashicorp/vault/vault" +) + +// Verifies that patching works by default with the root token +func TestKV_Patch_RootToken(t *testing.T) { + coreConfig := &vault.CoreConfig{ + LogicalBackends: map[string]logical.Factory{ + "kv": logicalKv.Factory, + }, + } + cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{ + HandlerFunc: vaulthttp.Handler, + }) + cluster.Start() + defer cluster.Cleanup() + + core := cluster.Cores[0] + vault.TestWaitActive(t, core.Core) + client := core.Client + + // make sure this client is using the root token + client.SetToken(cluster.RootToken) + + // Enable KVv2 + err := client.Sys().Mount("kv", &api.MountInput{ + Type: "kv-v2", + }) + if err != nil { + t.Fatal(err) + } + + // Write a kv value and patch it + _, err = client.Logical().Write("kv/data/foo", map[string]interface{}{"data": map[string]interface{}{"bar": "baz"}}) + if err != nil { + t.Fatal(err) + } + + _, err = client.Logical().JSONMergePatch(context.Background(), "kv/data/foo", map[string]interface{}{"data": map[string]interface{}{"bar": "quux"}}) + if err != nil { + t.Fatal(err) + } + + secret, err := client.Logical().Read("kv/data/foo") + bar := secret.Data["data"].(map[string]interface{})["bar"] + if bar != "quux" { + t.Fatalf("expected bar to be quux but it was %q", bar) + } +} diff --git a/vault/external_tests/misc/kvv2_upgrade_test.go b/vault/external_tests/kv/kvv2_upgrade_test.go similarity index 99% rename from vault/external_tests/misc/kvv2_upgrade_test.go rename to vault/external_tests/kv/kvv2_upgrade_test.go index 39d747e8ab53e..3d3eb486f2071 100644 --- a/vault/external_tests/misc/kvv2_upgrade_test.go +++ b/vault/external_tests/kv/kvv2_upgrade_test.go @@ -1,4 +1,4 @@ -package misc +package kv import ( "bytes" From a8c7e135dd6aa6baf241d36ac8edf459b2a3d273 Mon Sep 17 00:00:00 2001 From: Chris Capurso Date: Mon, 4 Oct 2021 13:15:40 -0400 Subject: [PATCH 20/30] add changelog entry --- changelog/12687.txt | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/12687.txt diff --git a/changelog/12687.txt b/changelog/12687.txt new file mode 100644 index 0000000000000..f5998deeda706 --- /dev/null +++ b/changelog/12687.txt @@ -0,0 +1,5 @@ +```release-note:feature +**KV patch**: Add partial update support the for the `//data/:path` kv-v2 +endpoint through HTTP `PATCH`. A new `patch` ACL capability has been added and +is required to make such requests. +``` From 723fb6baa6ae07c15f40362676dfbd2036b33d5b Mon Sep 17 00:00:00 2001 From: Chris Capurso Date: Thu, 7 Oct 2021 13:24:47 -0400 Subject: [PATCH 21/30] get vault-plugin-secrets-kv@add-patch-support --- go.mod | 6 ++---- go.sum | 2 ++ 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/go.mod b/go.mod index 0e6249154560e..bddd53866d0bd 100644 --- a/go.mod +++ b/go.mod @@ -6,8 +6,6 @@ replace github.com/hashicorp/vault/api => ./api replace github.com/hashicorp/vault/sdk => ./sdk -replace github.com/hashicorp/vault-plugin-secrets-kv => ../vault-plugin-secrets-kv - require ( cloud.google.com/go v0.56.0 cloud.google.com/go/spanner v1.5.1 @@ -112,13 +110,13 @@ require ( github.com/hashicorp/vault-plugin-secrets-azure v0.10.0 github.com/hashicorp/vault-plugin-secrets-gcp v0.10.2 github.com/hashicorp/vault-plugin-secrets-gcpkms v0.9.0 - github.com/hashicorp/vault-plugin-secrets-kv v0.5.7-0.20210811133805-e060c2307b24 + github.com/hashicorp/vault-plugin-secrets-kv v0.5.7-0.20211007143158-2d15a6fec12b github.com/hashicorp/vault-plugin-secrets-mongodbatlas v0.4.0 github.com/hashicorp/vault-plugin-secrets-openldap v0.5.1 github.com/hashicorp/vault-plugin-secrets-terraform v0.1.1-0.20210715043003-e02ca8f6408e github.com/hashicorp/vault-testing-stepwise v0.1.1 github.com/hashicorp/vault/api v1.1.1 - github.com/hashicorp/vault/sdk v0.2.1 + github.com/hashicorp/vault/sdk v0.2.2-0.20211004171540-a8c7e135dd6a github.com/influxdata/influxdb v0.0.0-20190411212539-d24b7ba8c4c4 github.com/jcmturner/gokrb5/v8 v8.0.0 github.com/jefferai/isbadcipher v0.0.0-20190226160619-51d2077c035f diff --git a/go.sum b/go.sum index 5744376461b41..751e80ce81575 100644 --- a/go.sum +++ b/go.sum @@ -765,6 +765,8 @@ github.com/hashicorp/vault-plugin-secrets-gcpkms v0.9.0 h1:7a0iWuFA/YNinQ1xXogyZ github.com/hashicorp/vault-plugin-secrets-gcpkms v0.9.0/go.mod h1:hhwps56f2ATeC4Smgghrc5JH9dXR31b4ehSf1HblP5Q= github.com/hashicorp/vault-plugin-secrets-kv v0.5.7-0.20210811133805-e060c2307b24 h1:uqPKQzkmO5vybOqk2aOdviXXi5088bcl2MrE0D1MhjM= github.com/hashicorp/vault-plugin-secrets-kv v0.5.7-0.20210811133805-e060c2307b24/go.mod h1:4j2pZrSynPuUAAYrZQVgSSHD0A9xj7GK9Ji1sWtnO4s= +github.com/hashicorp/vault-plugin-secrets-kv v0.5.7-0.20211007143158-2d15a6fec12b h1:1GJj7AjgI0Td95haW8EK5on3Usuox78wmzLj+J9vcm4= +github.com/hashicorp/vault-plugin-secrets-kv v0.5.7-0.20211007143158-2d15a6fec12b/go.mod h1:iEKCVaKBQzzYxzb778O6VGLdd+8gA40ZI14bo+8tQjs= github.com/hashicorp/vault-plugin-secrets-mongodbatlas v0.4.0 h1:6ve+7hZmGn7OpML81iZUxYj2AaJptwys323S5XsvVas= github.com/hashicorp/vault-plugin-secrets-mongodbatlas v0.4.0/go.mod h1:4mdgPqlkO+vfFX1cFAWcxkeqz6JAtZgKxL/67q/58Oo= github.com/hashicorp/vault-plugin-secrets-openldap v0.5.1 h1:iUJU3D/sA5qNBZnhXI5jFdwoWXMhgb6jeABDLYw631Y= From f309ea8f220d1dbcbb3c8d9d393e1eabaa2997b5 Mon Sep 17 00:00:00 2001 From: Josh Black Date: Thu, 7 Oct 2021 10:34:52 -0700 Subject: [PATCH 22/30] PR feedback --- command/kv_patch.go | 15 +++++---------- command/kv_test.go | 6 +++++- vault/external_tests/kv/kv_patch_test.go | 1 - 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/command/kv_patch.go b/command/kv_patch.go index 6a36a9a37e6e1..1b8e6cd7dcd73 100644 --- a/command/kv_patch.go +++ b/command/kv_patch.go @@ -175,7 +175,7 @@ func (c *KVPatchCommand) Run(args []string) int { case "rw": secret, code = c.readThenWrite(client, path, newData) case "patch": - secret, code = c.actualPatch(client, path, newData) + secret, code = c.mergePatch(client, path, newData) default: c.UI.Error(fmt.Sprintf("Unsupported method provided to -method flag: %s", c.flagMethod)) return 2 @@ -269,7 +269,7 @@ func (c *KVPatchCommand) readThenWrite(client *api.Client, path string, newData return secret, 0 } -func (c *KVPatchCommand) actualPatch(client *api.Client, path string, newData map[string]interface{}) (*api.Secret, int) { +func (c *KVPatchCommand) mergePatch(client *api.Client, path string, newData map[string]interface{}) (*api.Secret, int) { data := map[string]interface{}{ "data": newData, "options": map[string]interface{}{}, @@ -283,17 +283,12 @@ func (c *KVPatchCommand) actualPatch(client *api.Client, path string, newData ma if err != nil { // If it's a 403, that probably means they don't have the patch capability in their policy. Fall back to // the old way of doing it. - if re, ok := err.(*api.ResponseError); ok { - if re.StatusCode == 403 { - c.UI.Warn(fmt.Sprintf("Data was written to %s but we recommend that you add the \"patch\" capability to your ACL policy in order to use HTTP PATCH in the future.", path)) - return c.readThenWrite(client, path, newData) - } + if re, ok := err.(*api.ResponseError); ok && re.StatusCode == 403 { + c.UI.Warn(fmt.Sprintf("Data was written to %s but we recommend that you add the \"patch\" capability to your ACL policy in order to use HTTP PATCH in the future.", path)) + return c.readThenWrite(client, path, newData) } c.UI.Error(fmt.Sprintf("Error writing data to %s: %s", path, err)) - if secret != nil { - OutputSecret(c.UI, secret) - } return nil, 2 } diff --git a/command/kv_test.go b/command/kv_test.go index 0cf96d1db8f16..c8c9e5c514bca 100644 --- a/command/kv_test.go +++ b/command/kv_test.go @@ -676,8 +676,12 @@ func TestKvPatchCommand_StdinFull(t *testing.T) { } secretData := secretDataRaw.(map[string]interface{}) + foo, ok := secretData["foo"].(string) + if !ok { + t.Fatal("expected foo to be a string but it wasn't") + } - if exp, act := "bar", secretData["foo"].(string); exp != act { + if exp, act := "bar", foo; exp != act { t.Fatalf("expected %q to be %q, data: %#v\n", act, exp, secret.Data) } } diff --git a/vault/external_tests/kv/kv_patch_test.go b/vault/external_tests/kv/kv_patch_test.go index b2f71c19ae477..33d36f4f0b10d 100644 --- a/vault/external_tests/kv/kv_patch_test.go +++ b/vault/external_tests/kv/kv_patch_test.go @@ -25,7 +25,6 @@ func TestKV_Patch_RootToken(t *testing.T) { defer cluster.Cleanup() core := cluster.Cores[0] - vault.TestWaitActive(t, core.Core) client := core.Client // make sure this client is using the root token From de33fd6260ab6b5612937a1586bac92d1fc694de Mon Sep 17 00:00:00 2001 From: Chris Capurso Date: Thu, 7 Oct 2021 14:19:23 -0400 Subject: [PATCH 23/30] reorder some imports; go fmt --- http/handler_test.go | 17 +++++++++-------- sdk/framework/backend.go | 5 ++--- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/http/handler_test.go b/http/handler_test.go index 48dcf8b6021c5..87a79d3143394 100644 --- a/http/handler_test.go +++ b/http/handler_test.go @@ -5,6 +5,15 @@ import ( "crypto/tls" "encoding/json" "errors" + "io/ioutil" + "net/http" + "net/http/httptest" + "net/textproto" + "net/url" + "reflect" + "strings" + "testing" + "github.com/go-test/deep" "github.com/hashicorp/go-cleanhttp" kv "github.com/hashicorp/vault-plugin-secrets-kv" @@ -15,14 +24,6 @@ import ( "github.com/hashicorp/vault/sdk/helper/consts" "github.com/hashicorp/vault/sdk/logical" "github.com/hashicorp/vault/vault" - "io/ioutil" - "net/http" - "net/http/httptest" - "net/textproto" - "net/url" - "reflect" - "strings" - "testing" ) func TestHandler_parseMFAHandler(t *testing.T) { diff --git a/sdk/framework/backend.go b/sdk/framework/backend.go index ef8ab07b2d483..643c291191abf 100644 --- a/sdk/framework/backend.go +++ b/sdk/framework/backend.go @@ -5,7 +5,6 @@ import ( "crypto/rand" "encoding/json" "fmt" - jsonpatch "github.com/evanphx/json-patch" "io" "io/ioutil" "net/http" @@ -15,6 +14,7 @@ import ( "sync" "time" + jsonpatch "github.com/evanphx/json-patch" "github.com/hashicorp/errwrap" log "github.com/hashicorp/go-hclog" "github.com/hashicorp/go-kms-wrapping/entropy" @@ -280,7 +280,6 @@ func (b *Backend) HandleRequest(ctx context.Context, req *logical.Request) (*log return callback(ctx, req, &fd) } - func HandlePatchOperation(input *FieldData, resource map[string]interface{}, preprocessor PatchPreprocessorFunc) ([]byte, error) { var err error @@ -300,7 +299,7 @@ func HandlePatchOperation(input *FieldData, resource map[string]interface{}, pre } } - if preprocessor!= nil { + if preprocessor != nil { inputMap, err = preprocessor(inputMap) if err != nil { return nil, err From 2fe0402b25e49e0eb177662a30cf3533df1a3a70 Mon Sep 17 00:00:00 2001 From: Chris Capurso Date: Thu, 7 Oct 2021 15:02:12 -0400 Subject: [PATCH 24/30] add doc comment for HandlePatchOperation --- sdk/framework/backend.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/sdk/framework/backend.go b/sdk/framework/backend.go index 643c291191abf..65a72eb240a94 100644 --- a/sdk/framework/backend.go +++ b/sdk/framework/backend.go @@ -280,6 +280,13 @@ func (b *Backend) HandleRequest(ctx context.Context, req *logical.Request) (*log return callback(ctx, req, &fd) } +// HandlePatchOperation acts as an abstraction for performing JSON merge patch +// operations (see https://datatracker.ietf.org/doc/html/rfc7396) for HTTP +// PATCH requests. It is responsible for properly processing and marshalling +// the input and existing resource prior to performing the JSON merge operation +// using the MergePatch function from the json-patch library. The preprocessor +// is an arbitrary func that can be provided to further process the input. The +// MergePatch function accepts and returns byte arrays. func HandlePatchOperation(input *FieldData, resource map[string]interface{}, preprocessor PatchPreprocessorFunc) ([]byte, error) { var err error From 90c5725c4c6d2ef68cd3eedff4dbae028d8a75bb Mon Sep 17 00:00:00 2001 From: Chris Capurso Date: Thu, 7 Oct 2021 15:27:37 -0400 Subject: [PATCH 25/30] add json-patch@v5.5.0 to go.mod --- go.mod | 1 + 1 file changed, 1 insertion(+) diff --git a/go.mod b/go.mod index bddd53866d0bd..2ee5cec2e6472 100644 --- a/go.mod +++ b/go.mod @@ -42,6 +42,7 @@ require ( github.com/dsnet/compress v0.0.1 // indirect github.com/duosecurity/duo_api_golang v0.0.0-20190308151101-6c680f768e74 github.com/dustin/go-humanize v1.0.0 + github.com/evanphx/json-patch/v5 v5.5.0 // indirect github.com/fatih/color v1.11.0 github.com/fatih/structs v1.1.0 github.com/ghodss/yaml v1.0.1-0.20190212211648-25d852aebe32 From 5d04fff64f5e534a4cf5e092b142d2612e52885e Mon Sep 17 00:00:00 2001 From: Chris Capurso Date: Thu, 7 Oct 2021 15:43:02 -0400 Subject: [PATCH 26/30] remove unnecessary cancelFunc for WriteBytes --- api/logical.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/api/logical.go b/api/logical.go index 9d3d6f4666a20..f8f8bc5376612 100644 --- a/api/logical.go +++ b/api/logical.go @@ -155,13 +155,10 @@ func (c *Logical) JSONMergePatch(ctx context.Context, path string, data map[stri } func (c *Logical) WriteBytes(path string, data []byte) (*Secret, error) { - ctx, cancelFunc := context.WithCancel(context.Background()) - defer cancelFunc() - r := c.c.NewRequest("PUT", "/v1/"+path) r.BodyBytes = data - return c.write(ctx, path, r) + return c.write(context.Background(), path, r) } func (c *Logical) write(ctx context.Context, path string, request *Request) (*Secret, error) { From 29b9d6964b2d59fa33070840367154de442a72cf Mon Sep 17 00:00:00 2001 From: Josh Black Date: Thu, 7 Oct 2021 14:22:53 -0700 Subject: [PATCH 27/30] remove default for -method --- command/kv_patch.go | 15 +++---- command/kv_test.go | 95 +++++++++++++++++++++++++++++---------------- 2 files changed, 69 insertions(+), 41 deletions(-) diff --git a/command/kv_patch.go b/command/kv_patch.go index 1b8e6cd7dcd73..a6de937db75cc 100644 --- a/command/kv_patch.go +++ b/command/kv_patch.go @@ -91,9 +91,8 @@ func (c *KVPatchCommand) Flags() *FlagSets { }) f.StringVar(&StringVar{ - Name: "method", - Target: &c.flagMethod, - Default: "patch", + Name: "method", + Target: &c.flagMethod, Usage: `Specifies which method of patching to use. If set to "patch", then an HTTP PATCH request will be issued. This is the default. If set to "rw", then a read will be performed, then a local update, followed by a remote @@ -175,7 +174,9 @@ func (c *KVPatchCommand) Run(args []string) int { case "rw": secret, code = c.readThenWrite(client, path, newData) case "patch": - secret, code = c.mergePatch(client, path, newData) + secret, code = c.mergePatch(client, path, newData, false) + case "": + secret, code = c.mergePatch(client, path, newData, true) default: c.UI.Error(fmt.Sprintf("Unsupported method provided to -method flag: %s", c.flagMethod)) return 2 @@ -269,7 +270,7 @@ func (c *KVPatchCommand) readThenWrite(client *api.Client, path string, newData return secret, 0 } -func (c *KVPatchCommand) mergePatch(client *api.Client, path string, newData map[string]interface{}) (*api.Secret, int) { +func (c *KVPatchCommand) mergePatch(client *api.Client, path string, newData map[string]interface{}, rwFallback bool) (*api.Secret, int) { data := map[string]interface{}{ "data": newData, "options": map[string]interface{}{}, @@ -282,8 +283,8 @@ func (c *KVPatchCommand) mergePatch(client *api.Client, path string, newData map secret, err := client.Logical().JSONMergePatch(context.Background(), path, data) if err != nil { // If it's a 403, that probably means they don't have the patch capability in their policy. Fall back to - // the old way of doing it. - if re, ok := err.(*api.ResponseError); ok && re.StatusCode == 403 { + // the old way of doing it if the user didn't specify a -method. If they did, and it was "patch", then just error. + if re, ok := err.(*api.ResponseError); ok && re.StatusCode == 403 && rwFallback { c.UI.Warn(fmt.Sprintf("Data was written to %s but we recommend that you add the \"patch\" capability to your ACL policy in order to use HTTP PATCH in the future.", path)) return c.readThenWrite(client, path, newData) } diff --git a/command/kv_test.go b/command/kv_test.go index c8c9e5c514bca..945b478b0cb9a 100644 --- a/command/kv_test.go +++ b/command/kv_test.go @@ -980,48 +980,75 @@ func TestKVPatchCommand_Methods(t *testing.T) { } func TestKVPatchCommand_403Fallback(t *testing.T) { - t.Parallel() - client, closer := testVaultServer(t) - defer closer() - - if err := client.Sys().Mount("kv/", &api.MountInput{ - Type: "kv-v2", - }); err != nil { - t.Fatalf("kv-v2 mount attempt failed - err: %#v\n", err) + cases := []struct { + name string + args []string + expected string + code int + }{ + // if no -method is specified, and patch fails, it should fall back to rw and succeed + { + "unspecified", + []string{"kv/foo", "bar=quux"}, + `add the "patch" capability to your ACL policy`, + 0, + }, + // if -method=patch is specified, and patch fails, it should not fall back, and just error + { + "specifying patch", + []string{"-method", "patch", "kv/foo", "bar=quux"}, + "permission denied", + 2, + }, } - // create a policy without patch capability - policy := `path "kv/*" { capabilities = ["create", "update", "read"] }` - secretAuth, err := createTokenForPolicy(t, client, policy) - if err != nil { - t.Fatalf("policy/token creation failed for policy %s, err: %#v\n", policy, err) - } + for _, tc := range cases { + tc := tc - kvClient, err := client.Clone() - if err != nil { - t.Fatal(err) - } + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + client, closer := testVaultServer(t) + defer closer() + + if err := client.Sys().Mount("kv/", &api.MountInput{ + Type: "kv-v2", + }); err != nil { + t.Fatalf("kv-v2 mount attempt failed - err: %#v\n", err) + } - kvClient.SetToken(secretAuth.ClientToken) + // create a policy without patch capability + policy := `path "kv/*" { capabilities = ["create", "update", "read"] }` + secretAuth, err := createTokenForPolicy(t, client, policy) + if err != nil { + t.Fatalf("policy/token creation failed for policy %s, err: %#v\n", policy, err) + } - // Write a value then attempt to patch it. It should fail, then fall back to the old method - _, err = kvClient.Logical().Write("kv/data/foo", map[string]interface{}{"data": map[string]interface{}{"bar": "baz"}}) - if err != nil { - t.Fatal(err) - } + kvClient, err := client.Clone() + if err != nil { + t.Fatal(err) + } - ui, cmd := testKVPatchCommand(t) - cmd.client = kvClient - code := cmd.Run([]string{"kv/foo", "bar=quux"}) + kvClient.SetToken(secretAuth.ClientToken) - if code != 0 { - t.Fatalf("expected code to be 0 but was %d", code) - } + // Write a value then attempt to patch it + _, err = kvClient.Logical().Write("kv/data/foo", map[string]interface{}{"data": map[string]interface{}{"bar": "baz"}}) + if err != nil { + t.Fatal(err) + } - combined := ui.OutputWriter.String() + ui.ErrorWriter.String() - expected := `add the "patch" capability to your ACL policy` - if !strings.Contains(combined, expected) { - t.Errorf("expected %q to contain %q", combined, expected) + ui, cmd := testKVPatchCommand(t) + cmd.client = kvClient + code := cmd.Run(tc.args) + + if code != tc.code { + t.Fatalf("expected code to be %d but was %d", tc.code, code) + } + + combined := ui.OutputWriter.String() + ui.ErrorWriter.String() + if !strings.Contains(combined, tc.expected) { + t.Errorf("expected %q to contain %q", combined, tc.expected) + } + }) } } From 16135ef0191679b373fb7ade47fd72b9b6722ce8 Mon Sep 17 00:00:00 2001 From: Chris Capurso Date: Thu, 7 Oct 2021 17:25:10 -0400 Subject: [PATCH 28/30] use stable version of json-patch; go mod tidy --- go.mod | 2 +- go.sum | 8 ++------ 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/go.mod b/go.mod index 2ee5cec2e6472..18a933c6b9fb1 100644 --- a/go.mod +++ b/go.mod @@ -42,7 +42,6 @@ require ( github.com/dsnet/compress v0.0.1 // indirect github.com/duosecurity/duo_api_golang v0.0.0-20190308151101-6c680f768e74 github.com/dustin/go-humanize v1.0.0 - github.com/evanphx/json-patch/v5 v5.5.0 // indirect github.com/fatih/color v1.11.0 github.com/fatih/structs v1.1.0 github.com/ghodss/yaml v1.0.1-0.20190212211648-25d852aebe32 @@ -189,6 +188,7 @@ require ( google.golang.org/api v0.29.0 google.golang.org/grpc v1.29.1 google.golang.org/protobuf v1.27.1 + gopkg.in/evanphx/json-patch.v4 v4.11.0 // indirect gopkg.in/mgo.v2 v2.0.0-20180705113604-9856a29383ce gopkg.in/ory-am/dockertest.v3 v3.3.4 gopkg.in/square/go-jose.v2 v2.5.1 diff --git a/go.sum b/go.sum index 751e80ce81575..224447def1e31 100644 --- a/go.sum +++ b/go.sum @@ -360,8 +360,6 @@ github.com/evanphx/json-patch v0.0.0-20190203023257-5858425f7550/go.mod h1:50XU6 github.com/evanphx/json-patch v0.5.2/go.mod h1:ZWS5hhDbVDyob71nXKNL0+PWn6ToqBHMikGIFbs31qQ= github.com/evanphx/json-patch v4.2.0+incompatible h1:fUDGZCv/7iAN7u0puUVhvKCcsR6vRfwrJatElLBEf0I= github.com/evanphx/json-patch v4.2.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk= -github.com/evanphx/json-patch/v5 v5.5.0 h1:bAmFiUJ+o0o2B4OiTFeE3MqCOtyo+jjPP9iZ0VRxYUc= -github.com/evanphx/json-patch/v5 v5.5.0/go.mod h1:G79N1coSVB93tBe7j6PhzjmR3/2VvlbKOFpnXhI9Bw4= github.com/fatih/color v1.7.0/go.mod h1:Zm6kSWBoL9eyXnKyktHP6abPY2pDugNf5KwzbycvMj4= github.com/fatih/color v1.9.0/go.mod h1:eQcE1qtQxscV5RaZvpXrrb8Drkc3/DdQ+uUYCNjL+zU= github.com/fatih/color v1.11.0 h1:l4iX0RqNnx/pU7rY2DB/I+znuYY0K3x6Ywac6EIr0PA= @@ -685,8 +683,6 @@ github.com/hashicorp/go-version v1.0.0/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09 github.com/hashicorp/go-version v1.2.0/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA= github.com/hashicorp/go-version v1.2.1 h1:zEfKbn2+PDgroKdiOzqiE8rsmLqU2uwi5PB5pBJ3TkI= github.com/hashicorp/go-version v1.2.1/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA= -github.com/hashicorp/go-version v1.3.0 h1:McDWVJIU/y+u1BRV06dPaLfLCaT7fUTJLp5r04x7iNw= -github.com/hashicorp/go-version v1.3.0/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA= github.com/hashicorp/go.net v0.0.1/go.mod h1:hjKkEWcCURg++eb33jQU7oqQcI9XDCnUzHA0oac0k90= github.com/hashicorp/golang-lru v0.5.0/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8= github.com/hashicorp/golang-lru v0.5.1/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8= @@ -763,8 +759,6 @@ github.com/hashicorp/vault-plugin-secrets-gcp v0.10.2 h1:+DtlYJTsrFRInQpAo09KkYN github.com/hashicorp/vault-plugin-secrets-gcp v0.10.2/go.mod h1:psRQ/dm5XatoUKLDUeWrpP9icMJNtu/jmscUr37YGK4= github.com/hashicorp/vault-plugin-secrets-gcpkms v0.9.0 h1:7a0iWuFA/YNinQ1xXogyZHStolxMVtLV+sy1LpEHaZs= github.com/hashicorp/vault-plugin-secrets-gcpkms v0.9.0/go.mod h1:hhwps56f2ATeC4Smgghrc5JH9dXR31b4ehSf1HblP5Q= -github.com/hashicorp/vault-plugin-secrets-kv v0.5.7-0.20210811133805-e060c2307b24 h1:uqPKQzkmO5vybOqk2aOdviXXi5088bcl2MrE0D1MhjM= -github.com/hashicorp/vault-plugin-secrets-kv v0.5.7-0.20210811133805-e060c2307b24/go.mod h1:4j2pZrSynPuUAAYrZQVgSSHD0A9xj7GK9Ji1sWtnO4s= github.com/hashicorp/vault-plugin-secrets-kv v0.5.7-0.20211007143158-2d15a6fec12b h1:1GJj7AjgI0Td95haW8EK5on3Usuox78wmzLj+J9vcm4= github.com/hashicorp/vault-plugin-secrets-kv v0.5.7-0.20211007143158-2d15a6fec12b/go.mod h1:iEKCVaKBQzzYxzb778O6VGLdd+8gA40ZI14bo+8tQjs= github.com/hashicorp/vault-plugin-secrets-mongodbatlas v0.4.0 h1:6ve+7hZmGn7OpML81iZUxYj2AaJptwys323S5XsvVas= @@ -1703,6 +1697,8 @@ gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f/go.mod h1:Co6ibVJAznAaIkqp8 gopkg.in/cheggaaa/pb.v1 v1.0.25/go.mod h1:V/YB90LKu/1FcN3WVnfiiE5oMCibMjukxqG/qStrOgw= gopkg.in/errgo.v2 v2.1.0 h1:0vLT13EuvQ0hNvakwLuFZ/jYrLp5F3kcWHXdRggjCE8= gopkg.in/errgo.v2 v2.1.0/go.mod h1:hNsd1EY+bozCKY1Ytp96fpM3vjJbqLJn88ws8XvfDNI= +gopkg.in/evanphx/json-patch.v4 v4.11.0 h1:+kbwxm5IBGIiNYVhss+hM3Nv4ck+HnPSNscCNbD1cT0= +gopkg.in/evanphx/json-patch.v4 v4.11.0/go.mod h1:p8EYWUEYMpynmqDbY58zCKCFZw8pRWMG4EsWvDvM72M= gopkg.in/fsnotify.v1 v1.4.7 h1:xOHLXZwVvI9hhs+cLKq5+I5onOuwQLhQwiu63xxlHs4= gopkg.in/fsnotify.v1 v1.4.7/go.mod h1:Tz8NjZHkW78fSQdbUxIjBTcgA1z1m8ZHf0WmKUhAMys= gopkg.in/gcfg.v1 v1.2.3/go.mod h1:yesOnuUOFQAhST5vPY4nbZsb/huCgGGXlipJsBn0b3o= From 6fcc723d7399b5650654888639b2ca7fbcf1c66d Mon Sep 17 00:00:00 2001 From: Josh Black Date: Fri, 8 Oct 2021 10:04:57 -0700 Subject: [PATCH 29/30] more PR feedback --- command/kv_patch.go | 11 ++-- command/kv_test.go | 149 ++++++++++++++++++++------------------------ 2 files changed, 75 insertions(+), 85 deletions(-) diff --git a/command/kv_patch.go b/command/kv_patch.go index a6de937db75cc..908c9f14d53f6 100644 --- a/command/kv_patch.go +++ b/command/kv_patch.go @@ -94,9 +94,8 @@ func (c *KVPatchCommand) Flags() *FlagSets { Name: "method", Target: &c.flagMethod, Usage: `Specifies which method of patching to use. If set to "patch", then - an HTTP PATCH request will be issued. This is the default. If set to "rw", - then a read will be performed, then a local update, followed by a remote - update.`, + an HTTP PATCH request will be issued. If set to "rw", then a read will be + performed, then a local update, followed by a remote update.`, }) return set @@ -264,7 +263,7 @@ func (c *KVPatchCommand) readThenWrite(client *api.Client, path string, newData } if c.flagField != "" { - return secret, PrintRawField(c.UI, secret, c.flagField) + return nil, PrintRawField(c.UI, secret, c.flagField) } return secret, 0 @@ -301,5 +300,9 @@ func (c *KVPatchCommand) mergePatch(client *api.Client, path string, newData map return nil, 0 } + if c.flagField != "" { + return nil, PrintRawField(c.UI, secret, c.flagField) + } + return secret, 0 } diff --git a/command/kv_test.go b/command/kv_test.go index 945b478b0cb9a..4653f1f004419 100644 --- a/command/kv_test.go +++ b/command/kv_test.go @@ -1074,98 +1074,85 @@ func createTokenForPolicy(t *testing.T, client *api.Client, policy string) (*api return secret.Auth, err } -func TestKVPatchCommand_RWMethodNoRead(t *testing.T) { - client, closer := testVaultServer(t) - defer closer() - - if err := client.Sys().Mount("kv/", &api.MountInput{ - Type: "kv-v2", - }); err != nil { - t.Fatalf("kv-v2 mount attempt failed - err: %#v\n", err) - } - - // The rw patch method requires the read capability - create a - // policy that does not have it in order to force command to fail - policy := `path "kv/data/patch/foo" { capabilities = ["create", "update"] }` - secretAuth, err := createTokenForPolicy(t, client, policy) - if err != nil { - t.Fatalf("policy/token creation failed for policy %s, err: %#v\n", policy, err) - } - - client.SetToken(secretAuth.ClientToken) - - if _, err := client.Logical().Write("kv/data/patch/foo", map[string]interface{}{ - "data": map[string]interface{}{ - "foo": "a", - "bar": "b", +func TestKVPatchCommand_RWMethodPolicyVariations(t *testing.T) { + cases := []struct { + name string + args []string + policy string + expected string + code int + }{ + // if the policy doesn't have read capability and -method=rw is specified, it fails + { + "no read", + []string{"-method", "rw", "kv/foo", "bar=quux"}, + `path "kv/*" { capabilities = ["create", "update"] }`, + "permission denied", + 2, + }, + // if the policy doesn't have update capability and -method=rw is specified, it fails + { + "no update", + []string{"-method", "rw", "kv/foo", "bar=quux"}, + `path "kv/*" { capabilities = ["create", "read"] }`, + "permission denied", + 2, + }, + // if the policy has both read and update and -method=rw is specified, it succeeds + { + "read and update", + []string{"-method", "rw", "kv/foo", "bar=quux"}, + `path "kv/*" { capabilities = ["create", "read", "update"] }`, + "", + 0, }, - }); err != nil { - t.Fatalf("write failed, err: %#v\n", err) - } - - ui, cmd := testKVPatchCommand(t) - cmd.client = client - - args := []string{"kv/patch/foo", "foo=aa"} - code := cmd.Run(args) - expectedCode := 2 - - if code != expectedCode { - t.Fatalf("expected code to be %d but was %d for cmd %#v with args %#v\n", expectedCode, code, cmd, args) } - combined := ui.OutputWriter.String() + ui.ErrorWriter.String() - - expectedOutputSubstr := "Error doing pre-read" - if !strings.Contains(combined, expectedOutputSubstr) { - t.Fatalf("expected output %q to contain %q for cmd %#v with args %#v\n", combined, expectedOutputSubstr, cmd, args) - } -} + for _, tc := range cases { + tc := tc -func TestKVPatchCommand_RWMethodNoUpdate(t *testing.T) { - client, closer := testVaultServer(t) - defer closer() + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + client, closer := testVaultServer(t) + defer closer() - if err := client.Sys().Mount("kv/", &api.MountInput{ - Type: "kv-v2", - }); err != nil { - t.Fatalf("kv-v2 mount attempt failed - err: %#v\n", err) - } + if err := client.Sys().Mount("kv/", &api.MountInput{ + Type: "kv-v2", + }); err != nil { + t.Fatalf("kv-v2 mount attempt failed - err: %#v\n", err) + } - // The rw patch method requires the update capability - create a - // policy that does not have it in order to force command to fail - policy := `path "kv/data/patch/foo" { capabilities = ["create", "read"] }` - secretAuth, err := createTokenForPolicy(t, client, policy) - if err != nil { - t.Fatalf("policy/token creation failed for policy %s, err: %#v\n", policy, err) - } + secretAuth, err := createTokenForPolicy(t, client, tc.policy) + if err != nil { + t.Fatalf("policy/token creation failed for policy %s, err: %#v\n", tc.policy, err) + } - client.SetToken(secretAuth.ClientToken) + client.SetToken(secretAuth.ClientToken) - if _, err := client.Logical().Write("kv/data/patch/foo", map[string]interface{}{ - "data": map[string]interface{}{ - "foo": "a", - "bar": "b", - }, - }); err != nil { - t.Fatalf("write failed, err: %#v\n", err) - } - - ui, cmd := testKVPatchCommand(t) - cmd.client = client + if _, err := client.Logical().Write("kv/data/foo", map[string]interface{}{ + "data": map[string]interface{}{ + "foo": "bar", + "bar": "baz", + }, + }); err != nil { + t.Fatalf("write failed, err: %#v\n", err) + } - args := []string{"kv/patch/foo", "foo=aa"} - code := cmd.Run(args) - expectedCode := 2 + ui, cmd := testKVPatchCommand(t) + cmd.client = client - if code != expectedCode { - t.Fatalf("expected code to be %d but was %d for cmd %#v with args %#v\n", expectedCode, code, cmd, args) - } + code := cmd.Run(tc.args) - combined := ui.OutputWriter.String() + ui.ErrorWriter.String() + if code != tc.code { + t.Fatalf("expected code to be %d but was %d for cmd %#v with args %#v\n", tc.code, code, cmd, tc.args) + } - expectedOutputSubstr := "Error writing data" - if !strings.Contains(combined, expectedOutputSubstr) { - t.Fatalf("expected output %q to contain %q for cmd %#v with args %#v\n", combined, expectedOutputSubstr, cmd, args) + if code != 0 { + combined := ui.OutputWriter.String() + ui.ErrorWriter.String() + if !strings.Contains(combined, tc.expected) { + t.Fatalf("expected output %q to contain %q for cmd %#v with args %#v\n", combined, tc.expected, cmd, tc.args) + } + } + }) } } From aee77e3a3931bbb719e16d6d9f6216db98caad24 Mon Sep 17 00:00:00 2001 From: Chris Capurso Date: Wed, 13 Oct 2021 11:47:11 -0400 Subject: [PATCH 30/30] temp go get vault-plugin-secrets-kv@master until official release --- go.mod | 2 +- go.sum | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/go.mod b/go.mod index 0c47abcd17cb0..7d7af6efba935 100644 --- a/go.mod +++ b/go.mod @@ -113,7 +113,7 @@ require ( github.com/hashicorp/vault-plugin-secrets-azure v0.6.3-0.20210924190759-58a034528e35 github.com/hashicorp/vault-plugin-secrets-gcp v0.10.2 github.com/hashicorp/vault-plugin-secrets-gcpkms v0.9.0 - github.com/hashicorp/vault-plugin-secrets-kv v0.5.7-0.20211007143158-2d15a6fec12b + github.com/hashicorp/vault-plugin-secrets-kv v0.5.7-0.20211013154503-eec8a1c892fb github.com/hashicorp/vault-plugin-secrets-mongodbatlas v0.4.0 github.com/hashicorp/vault-plugin-secrets-openldap v0.4.1-0.20210921171411-e86105e4986d github.com/hashicorp/vault-plugin-secrets-terraform v0.1.1-0.20210715043003-e02ca8f6408e diff --git a/go.sum b/go.sum index 62304b7285929..ef1e39186b8d5 100644 --- a/go.sum +++ b/go.sum @@ -748,6 +748,8 @@ github.com/hashicorp/vault-plugin-secrets-gcpkms v0.9.0 h1:7a0iWuFA/YNinQ1xXogyZ github.com/hashicorp/vault-plugin-secrets-gcpkms v0.9.0/go.mod h1:hhwps56f2ATeC4Smgghrc5JH9dXR31b4ehSf1HblP5Q= github.com/hashicorp/vault-plugin-secrets-kv v0.5.7-0.20211007143158-2d15a6fec12b h1:1GJj7AjgI0Td95haW8EK5on3Usuox78wmzLj+J9vcm4= github.com/hashicorp/vault-plugin-secrets-kv v0.5.7-0.20211007143158-2d15a6fec12b/go.mod h1:iEKCVaKBQzzYxzb778O6VGLdd+8gA40ZI14bo+8tQjs= +github.com/hashicorp/vault-plugin-secrets-kv v0.5.7-0.20211013154503-eec8a1c892fb h1:nZ2a4a1G0ALLAzKOWQbLzD5oljKo+pjMarbq3BwU0pM= +github.com/hashicorp/vault-plugin-secrets-kv v0.5.7-0.20211013154503-eec8a1c892fb/go.mod h1:D/FQJ7zU5pD6FNJVUwaVtxr75ZsxIIqaG/Nh6RHt/xo= github.com/hashicorp/vault-plugin-secrets-mongodbatlas v0.4.0 h1:6ve+7hZmGn7OpML81iZUxYj2AaJptwys323S5XsvVas= github.com/hashicorp/vault-plugin-secrets-mongodbatlas v0.4.0/go.mod h1:4mdgPqlkO+vfFX1cFAWcxkeqz6JAtZgKxL/67q/58Oo= github.com/hashicorp/vault-plugin-secrets-openldap v0.4.1-0.20210921171411-e86105e4986d h1:o5Z9B1FztTYSnTQNzFr+iZJHPM8ZD23uV5A8gMxm2g0=