Add subkeys endpoint #59

ccapurso · 2022-01-28T20:27:04Z

Overview

KVv2 added patch support in #49 which allows users to update a subset of a secret entries subkeys without the need to first read the read the data. The knowledge of a secret entry's structure (i.e. its subkeys) will allow users to more confidently perform patch operations while still not having to retrieve the underlying secret data. This PR adds a new subkeys/:path endpoint to achieve this. Its response structure resembles that of the data/:path endpoint except the underlying values of each leaf subkey is replaced with nil.

The subkeys endpoint remains consistent with the data endpoint in multiple ways. Like the data/:path endpoint, the current version of the secret entry stored at :path will be read. A specific version can be read by using the version parameter. If the requested version has been deleted or destroyed, a nil Logical.Response is returned which ultimately leads to a status of 404 Not Found. Both provide the same metadata structure in the response.

The depth parameter can be used to limit the depth of the structure. If the depth is not provided or is equal to 0, the full structure is returned. A depth of 1, for example, will provide only the top-level subkeys.

Example secret entry:

{
  "foo": "abc",
  "bar": {
    "baz": "def"
  }
}

Example subkeys response:

{
  "subkeys": {
    "foo": null,
    "bar": {
      "baz": null
      },
  },
  "metadata": {
    "created_time": "2021-12-14T20:28:00.773477Z",
    "custom_metadata": null,
    "deletion_time": "",
    "destroyed": false,
    "version": 1
  }
}

Design of Change

The pathSubkeysRead method has been added to handle ReadOperation requests for the subkeys/:path endpoint. The underlying logic closely resembles that of the pathDataRead method which handles ReadOperation requests for the data/:path endpoint. The secret data fetched from storage is run through removeValues which walks the map obtained from unmarshaling the JSON entry and replaces any values for leaf keys with nil. The map is modified in place.

Leaf keys are defined as those whose type is not a map[string]interface{} or is a map[string]interface{} but has no underlying keys. The depth parameter will also artificially treat keys as leaves if they reside at the associated level within the map.

Related Issues/Pull Requests

vault API docs PR #13893

HridoyRoy

Looking really good, just a few questions/comments!

HridoyRoy · 2022-02-08T21:39:49Z

helpers_test.go

@@ -0,0 +1,53 @@
+package kv


I think these helpers should be moved to a file that doesn't end with test.go? As far as I remember, only files containing tests are named *_test.go.

Ah, whoops. You're right, thanks! I can move these to a non _test.go file in a separate test helpers package to make it obvious and import it directly

Actually, after a bit of research this might not be true. Forgive me, since I'm new to Go. It seems that a file ending in _test.go indicates that the file should be packaged up when running go test. I don't think it necessarily dictates that the file must contain tests. Creating a file that doesn't end in _test.go would mean that it would be packaged into the binary which I don't think we want. Thoughts?

All that you're saying is true. Nonetheless, we typically put helpers in non-_test.go files, because otherwise they can't be imported by other packages.

The Go linker is pretty smart, I don't think it'll include the helper code in the prod binary unless prod code invokes it.

Of course if the helpers aren't public anyway, it's up to you.

That makes sense. I would like to follow our general practices so I will move things. Thanks for the feedback everyone.

So now looking at this more, having a separate package will result in cyclic imports as the getBackend function requires the VersionedKVFactory. I didn't like that the getBackend function was defined in a path_data_test.go and instead wanted it in a separate file. Though it's all the same test package so maybe I'm just complicating things and might just revert all that and go back to way things were prior to this PR.

HridoyRoy · 2022-02-08T22:47:28Z

helpers_test.go

+	}
+
+	// Wait for the upgrade to finish
+	time.Sleep(time.Second)


Can we check to make sure the upgrade finishes instead of using the sleep statement here?

The getBackend method existed before this PR. It is used by the unit tests to instantiate a KV backend. I moved it from one file to another. That said, I agree that it would be good to ensure that the upgrade finishes rather than a single hopeful sleep.

The backend itself has a flag that states that the upgrade is pending. I think it would be cleaner to add polling with a timeout as you said. That's what we do for the kv CLI tests in the vault repo. I can try to get something like that in place here.

I added a ticker/timeout which ticks per second. Each tick, a read request will be submitted to the config endpoint to ensure that it doesn't result in an error.

HridoyRoy · 2022-02-08T22:49:06Z

helpers_test.go

+}
+
+// getKeySet will produce a set of the keys that exist in m
+func getKeySet(m map[string]interface{}) map[string]struct{} {


Why are we changing the tests to check the keySet now? Shouldn't we check the nested structure as well?

This was a pre-existing function that I added in the last release cycle to verify the structure of response bodies. I had it defined in a specific test file and moved it to a more central location. It is used to check the top-level keys of the subkeys response (subkeys and metadata) and then the keys underneath metadata See:

vault-plugin-secrets-kv/path_subkeys_test.go

Lines 82 to 84 in 9f2cb0e

if diff := deep.Equal(getKeySet(resp.Data), expectedRespKeys); len(diff) > 0 {

t.Fatalf("metadata map keys mismatch, diff: %#v", diff)

}

vault-plugin-secrets-kv/path_subkeys_test.go

Lines 86 to 89 in 9f2cb0e

metadata, ok := resp.Data["metadata"].(map[string]interface{})

if !ok {

t.Fatalf("expected metadata to be map, actual: %#v", metadata)

}

I just noticed, however, that the error message for the top-level keys check is incorrect and I'll have to change that.

An example of a check for the full nested structure can be found:

vault-plugin-secrets-kv/path_subkeys_test.go

Lines 95 to 114 in 9f2cb0e

expectedSubkeys := map[string]interface{}{

"foo": nil,

"bar": map[string]interface{}{

"a": map[string]interface{}{

"c": map[string]interface{}{

"d": nil,

},

},

"b": nil,

},

"baz": map[string]interface{}{

"e": nil,

},

"quux": nil,

"quuz": nil,

}

if diff := deep.Equal(resp.Data["subkeys"], expectedSubkeys); len(diff) > 0 {

t.Fatalf("resp and expected data mismatch, diff: %#v", diff)

}

HridoyRoy · 2022-02-08T22:52:21Z

path_subkeys.go

+			},
+		},
+		Callbacks: map[logical.Operation]framework.OperationFunc{
+			logical.ReadOperation: b.upgradeCheck(b.pathSubkeysRead()),


What's the significance of b.upgradeCheck here?

Every endpoint is wrapped with b.upgradeCheck. This is to ensure an error is returned if the backend is being upgraded from v1 (non-versioned entries) to v2 (versioned entries). In other words, it ensures the backend is inaccessible while the backend's storage is updated to fit the new structure of v2. This is outlined in the Vault docs here. Does that answer your question?

HridoyRoy · 2022-02-08T22:53:33Z

path_subkeys.go

+	var walk func(interface{}, int)
+
+	walk = func(in interface{}, depth int) {
+		val := reflect.ValueOf(in)


Do we need to check IsValid on this?

Ah, yes! Good catch.

Added IsValid checks prior to any method calls on any reflect.Value vars. How does that look?

HridoyRoy · 2022-02-08T23:03:19Z

path_subkeys.go

+		}
+
+		versionNum := meta.CurrentVersion
+		versionParam := data.Get("version").(int)


I could be wrong on this, but if memory serves we need to do GetOk for parameters that are not required?

I think that GetOk will actually result in a false value for the ok return val if the field is not provided. Additional logic would then be required to default it appropriately. The Get method will call GetOk under the hood ultimately defaulting to the default or zero value for the field. The version field does not have a default defined thus the zero value (i.e. 0) will be used. A 0 for this field denotes "use the current version." Note that on line 105 versionNum is initialized as meta.CurrentVersion. It is only overridden on line 109 if versionParam > 0.

HridoyRoy · 2022-02-08T23:05:59Z

path_subkeys.go

+			}
+
+			if deletionTime.Before(time.Now()) {
+				return logical.RespondWithStatusCode(resp, req, http.StatusNotFound)


Do we want to return the response here? What about returning logical.ErrorResponse instead?

I believe that returning a logical.ErrorResponse would result in a 400 status code, correct? The desire is to return a 404. I believe the helper function logical.RespondWithStatusCode is required to achieve that. It takes a logical.Response, the incoming logical.Request and an HTTP status code. It will add HTTPStatusCode to the Data field in the logical.Response. Vault's response handling logic will pull this out and use it as the response status. This is a means to ensure that a 404 response is provided if the secret entry has been deleted (we also check if it is destroyed). This check is consistent with some of the other KV endpoints. See the read for data/:path:

vault-plugin-secrets-kv/path_data.go

Lines 126 to 143 in 9f2cb0e

// If the version has been deleted return metadata with a 404

if vm.DeletionTime != nil {

deletionTime, err := ptypes.Timestamp(vm.DeletionTime)

if err != nil {

return nil, err

}

if deletionTime.Before(time.Now()) {

return logical.RespondWithStatusCode(resp, req, http.StatusNotFound)

}

}

// If the version has been destroyed return metadata with a 404

if vm.Destroyed {

return logical.RespondWithStatusCode(resp, req, http.StatusNotFound)

}

ncabatoff

Nice work!

ncabatoff · 2022-02-09T15:55:52Z

path_subkeys.go

+// provide all subkeys with nesting fully intact. The modifications are made
+// to the input in-place. maxDepth will denote how deep to traverse. A maxDepth
+// of 0 is the equivalent of no limit.
+func removeValues(input map[string]interface{}, maxDepth int) {


You might like https://github.com/mitchellh/reflectwalk. (I'm not asking you to use it, just pointing out it exists. For your use case it might be simpler to do as you're doing.)

I did look at that a bit, actually! It's a very intriguing library. I was hesitant to use it though. My thought was that the added boilerplate might outweigh the overall benefit based on the fairly narrow problem scope.

path_subkeys_test.go

swayne275

looks good!

swayne275 · 2022-02-09T19:52:34Z

path_subkeys_test.go

+		"metadata": {},
+	}
+
+	if diff := deep.Equal(getKeySet(resp.Data), expectedRespKeys); len(diff) > 0 {


does this not check the values in the map? like is it only checking that the keys are present and the same?

Yup, this is just checking top-level keys to verify that the response structure matches our expectations. The metadata keys (but not the associated values) are then checked below on line 91. The actual structure of the subkeys map is then checked on line 112. All these checks are done separately to verify the overall structure of the response body.

ccapurso added 9 commits January 28, 2022 15:15

move some test helper funcs to helper file

90343af

add subkeys endpoint and associated ReadOperation handler

6f1e9ec

fix max depth handling to prevent leaking data

f7339a1

move getBackend test helper to common test helpers

987d99d

adding more subkeys tests

27d1fcf

go fmt

2db238c

return metadata in subkeys resp if secret is destroyed

09f389c

adding more unit tests

0ec2e88

add depth param to subkeys endpoint

e63713a

ccapurso changed the title ~~Vault 4302 subkeys endpoint~~ Add subkeys endpoint Feb 3, 2022

update subkeys help description

7542624

ccapurso mentioned this pull request Feb 3, 2022

add API docs for KVv2 subkeys endpoint hashicorp/vault#13893

Merged

cleaning up some comments

9f2cb0e

ccapurso marked this pull request as ready for review February 4, 2022 17:54

ccapurso requested a review from a team February 4, 2022 17:54

ccapurso mentioned this pull request Feb 7, 2022

vault cli to support only fields output hashicorp/vault#13701

Closed

HridoyRoy reviewed Feb 8, 2022

View reviewed changes

ccapurso added 3 commits February 9, 2022 08:46

add IsValid checks to prevent panics

541e2f1

fix test error message

003e718

change getBackend test helper to wait for upgrade to finish

033d430

ncabatoff approved these changes Feb 9, 2022

View reviewed changes

ccapurso added 2 commits February 9, 2022 13:02

move test helper functions back

bf62710

gofmt

ec17248

swayne275 approved these changes Feb 9, 2022

View reviewed changes

ccapurso added 2 commits February 9, 2022 17:25

fix test error logging so it cannot panic

222e893

fix typo

2100e7b

ccapurso merged commit a1df167 into master Feb 14, 2022

ccapurso deleted the vault-4302-subkeys-endpoint branch February 14, 2022 15:18

r3nic1e mentioned this pull request Jul 15, 2022

Add support for KVv2 subkeys hashicorp/vault#16305

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add subkeys endpoint #59

Add subkeys endpoint #59

ccapurso commented Jan 28, 2022 •

edited

HridoyRoy left a comment

HridoyRoy Feb 8, 2022

ccapurso Feb 9, 2022

ccapurso Feb 9, 2022

ncabatoff Feb 9, 2022

ncabatoff Feb 9, 2022

ccapurso Feb 9, 2022

ccapurso Feb 9, 2022

HridoyRoy Feb 8, 2022

ccapurso Feb 9, 2022

ccapurso Feb 9, 2022

HridoyRoy Feb 8, 2022

ccapurso Feb 9, 2022

HridoyRoy Feb 8, 2022

ccapurso Feb 9, 2022

HridoyRoy Feb 8, 2022

ccapurso Feb 9, 2022

ccapurso Feb 9, 2022

HridoyRoy Feb 8, 2022

ccapurso Feb 9, 2022

HridoyRoy Feb 8, 2022

ccapurso Feb 9, 2022 •

edited

ncabatoff left a comment

ncabatoff Feb 9, 2022

ccapurso Feb 10, 2022

swayne275 left a comment

swayne275 Feb 9, 2022

ccapurso Feb 10, 2022

	if diff := deep.Equal(getKeySet(resp.Data), expectedRespKeys); len(diff) > 0 {
	t.Fatalf("metadata map keys mismatch, diff: %#v", diff)
	}

	metadata, ok := resp.Data["metadata"].(map[string]interface{})
	if !ok {
	t.Fatalf("expected metadata to be map, actual: %#v", metadata)
	}

	expectedSubkeys := map[string]interface{}{
	"foo": nil,
	"bar": map[string]interface{}{
	"a": map[string]interface{}{
	"c": map[string]interface{}{
	"d": nil,
	},
	},
	"b": nil,
	},
	"baz": map[string]interface{}{
	"e": nil,
	},
	"quux": nil,
	"quuz": nil,
	}

	if diff := deep.Equal(resp.Data["subkeys"], expectedSubkeys); len(diff) > 0 {
	t.Fatalf("resp and expected data mismatch, diff: %#v", diff)
	}

	// If the version has been deleted return metadata with a 404
	if vm.DeletionTime != nil {
	deletionTime, err := ptypes.Timestamp(vm.DeletionTime)
	if err != nil {
	return nil, err
	}

	if deletionTime.Before(time.Now()) {
	return logical.RespondWithStatusCode(resp, req, http.StatusNotFound)

	}
	}

	// If the version has been destroyed return metadata with a 404
	if vm.Destroyed {
	return logical.RespondWithStatusCode(resp, req, http.StatusNotFound)

	}

Add subkeys endpoint #59

Add subkeys endpoint #59

Conversation

ccapurso commented Jan 28, 2022 • edited

Overview

Design of Change

Related Issues/Pull Requests

HridoyRoy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccapurso Feb 9, 2022 • edited

Choose a reason for hiding this comment

ncabatoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swayne275 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccapurso commented Jan 28, 2022 •

edited

ccapurso Feb 9, 2022 •

edited