Enhance sys/raw to read and write values that cannot be encoded in json #13537

shwuandwing · 2021-12-31T02:59:27Z

This enhances sys/raw so it can return / write values that cannot be encoded in json (like gzip data for core/mounts, or protobuf in KV v2 mounts)

Addresses / fixes #12746, #13536

…sed in json

ncabatoff · 2022-01-06T18:19:35Z

Hi @shwuandwing,

I'm excited to see you working on this, as this limitation is something I'd love to see addressed.

I don't think your current solution actually solves #12746; I would expect to still see the same error as was reported in that issue after your changes, since we're still looking at the error from compressutil.Decompress.

What do you think about adding an optional compressed boolean option? handleRawRead would maintain its current behaviour when the option isn't provided or when compressed=true; if compressed=false is given, it would not attempt to call Decompress. handleRawWrite would compress the provided value before storing it if compressed=true was provided. I prefer that we not rely on the user to do the compression of the value they want written, since that seems error prone - I'm sure plenty of users would try to compress the data without using our compressutil.

This doesn't obviate the need for base64. I don't think it's strictly required - I just did some tests with uncompressed binary storage entries and I was able to read/write the value just fine. But it's not ideal to have binary unencoded data go to the user's terminal. From that perspective, your solution of returning both the original value plus a value_base64 isn't ideal, in that it still allows for that to happen. How about instead we just retain the single value (no value_base64), and add an encoding=base64 option for both reads and writes?

…her to compress/decompress, whether value is base64 encoded

vault/logical_raw.go

ncabatoff · 2022-01-17T19:39:12Z

vault/logical_raw.go

+
+	encoding := data.Get("encoding").(string)
+	if encoding != "" && encoding != "base64" {
+		return logical.ErrorResponse("invalid encoding '%s'", encoding), logical.ErrInvalidRequest


This is fine, but FYI more commonly we omit the second return (error) value when we're using logical.ErrorResponse. logical.ErrInvalidRequest translates to http.StatusBadRequest, which is the default for when we return an error with a non-nil response, as is the case for logical.ErrorResponse.

vault/logical_raw.go

ncabatoff · 2022-01-17T19:50:48Z

vault/logical_raw.go

+		}
+	}
+
+	if compressed {


If the user doesn't provide compressed=true, we're not making use of the information we have in the existing storage entry, and we'll go ahead and replace an existing compressed entry with an uncompressed one. Can you reorder things so that if this is an update (instead of a create), we use the information in the existing storage entry to determine whether and how to compress? And in that case maybe we should return an error if they did specify those options and they don't match the existing storage entry?

Just clarifying. To differentiate between update vs create, backends generally need to implement "ExistenceCheck" -- https://github.com/hashicorp/vault/blob/main/sdk/framework/path.go#L92.

Are you saying you want this diff to implement "ExistenceCheck" for sys/raw and use that in "handleRawWrite", or skip ExistenceCheck and have "handleRawWrite" do something like

entry, err := b.barrier.Get(ctx, path)
_, compressionType, _, _ := compressutil.DecompressWithCanary(entry.Value)
if compressionType != "" && !compressed {
return err // this is compressed data but compression was not specified
}

One problem with this approach is what if users want to replace an existing compressed entry with an uncompressed one (that's actually what happens today if you use sys/raw to update any json-compressed entries like core/audit or core/mounts).

Okay, I implemented an existence check -- and updates now will use the existing compression type (or no compression) -- and check + error out if the new compression type is different the existing compression type.

I did make one small exception, users can force an entry to be non-compressed even if it was compressed before. This ensures a users can workaround unknown issues with compressutil .

sdk/helper/compressutil/compress.go

…sion and the type compression from the existing value.

ncabatoff · 2022-01-19T20:53:15Z

sdk/helper/compressutil/compress_test.go

+		}
+
+		if compressionType != test.compressionConfig.Type {
+			t.Fatalf("bad compressiontType value;\nexpected: %q\nactional: %q", test.compressionConfig.Type, compressionType)


Suggested change

t.Fatalf("bad compressiontType value;\nexpected: %q\nactional: %q", test.compressionConfig.Type, compressionType)

t.Fatalf("bad compressionType value;\nexpected: %q\nactual: %q", test.compressionConfig.Type, compressionType)

vault/logical_raw.go

ncabatoff · 2022-01-19T22:15:13Z

vault/logical_system_test.go

+		if err != nil {
+			t.Fatalf("err: %v", err)
+		}
+		if !strings.HasPrefix(resp.Data["value"].(string), "{\"type\":\"mounts\"") {


Suggested change

if !strings.HasPrefix(resp.Data["value"].(string), "{\"type\":\"mounts\"") {

if !strings.HasPrefix(resp.Data["value"].(string), `{"type":"mounts"`) {

vault/logical_system_test.go

ncabatoff · 2022-01-19T22:26:22Z

Looking really good, ty for the comprehensive tests.

ncabatoff · 2022-01-20T12:53:04Z

Nice work, thanks @shwuandwing !

Enhance sys/raw so it can read and write values that cannot be expres…

86793a8

…sed in json

shwuandwing requested a review from taoism4504 as a code owner December 31, 2021 02:59

vercel bot temporarily deployed to Preview – vault-storybook December 31, 2021 02:59 Inactive

Add changelog file

948a314

vercel bot temporarily deployed to Preview – vault-storybook December 31, 2021 03:02 Inactive

heatherezell added the enhancement label Jan 3, 2022

shwuandwing added 2 commits January 9, 2022 06:35

Merge remote-tracking branch 'upstream/main'

ad5504e

Address feedback. Use 'compressed','encoding' field to determine whet…

02c4b76

…her to compress/decompress, whether value is base64 encoded

vercel bot temporarily deployed to Preview – vault-storybook January 9, 2022 18:30 Inactive

Update documentation formatting

92f2e35

vercel bot temporarily deployed to Preview – vault-storybook January 9, 2022 18:39 Inactive

ncabatoff reviewed Jan 10, 2022

View reviewed changes

vault/logical_raw.go Show resolved Hide resolved

vault/logical_raw.go Outdated Show resolved Hide resolved

shwuandwing added 2 commits January 10, 2022 10:44

Infer compression type if possible

ec83553

enforce encoding should be empty or 'base64'

adb8a06

vercel bot temporarily deployed to Preview – vault-storybook January 10, 2022 18:58 Inactive

shwuandwing requested a review from ncabatoff January 10, 2022 19:18

fix typo in var name in compressutil

29fcf53

vercel bot temporarily deployed to Preview – vault-storybook January 10, 2022 19:32 Inactive

ncabatoff reviewed Jan 17, 2022

View reviewed changes

Address additional feedback. Updates now infer whether to use compres…

2c47657

…sion and the type compression from the existing value.

vercel bot temporarily deployed to Preview – vault-storybook January 19, 2022 08:56 Inactive

update docs to remove compressed parameter from create/update raw

2423d90

vercel bot temporarily deployed to Preview – vault-storybook January 19, 2022 09:00 Inactive

shwuandwing requested a review from ncabatoff January 19, 2022 18:22

ncabatoff reviewed Jan 19, 2022

View reviewed changes

vault/logical_raw.go Outdated Show resolved Hide resolved

ncabatoff reviewed Jan 19, 2022

View reviewed changes

vault/logical_raw.go Show resolved Hide resolved

ncabatoff reviewed Jan 19, 2022

View reviewed changes

vault/logical_system_test.go Outdated Show resolved Hide resolved

address additional feedback on tests, comments, logical.updateoperation

fa52e36

vercel bot temporarily deployed to Preview – vault-storybook January 19, 2022 23:57 Inactive

address comment on checking types in tests

54bb6e9

vercel bot temporarily deployed to Preview – vault-storybook January 20, 2022 00:17 Inactive

shwuandwing requested a review from ncabatoff January 20, 2022 00:19

ncabatoff approved these changes Jan 20, 2022

View reviewed changes

ncabatoff merged commit 94d921f into hashicorp:main Jan 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance sys/raw to read and write values that cannot be encoded in json #13537

Enhance sys/raw to read and write values that cannot be encoded in json #13537

shwuandwing commented Dec 31, 2021

ncabatoff commented Jan 6, 2022

ncabatoff Jan 17, 2022

ncabatoff Jan 17, 2022

shwuandwing Jan 18, 2022

shwuandwing Jan 19, 2022

ncabatoff Jan 19, 2022

ncabatoff Jan 19, 2022

ncabatoff commented Jan 19, 2022

ncabatoff commented Jan 20, 2022

	t.Fatalf("bad compressiontType value;\nexpected: %q\nactional: %q", test.compressionConfig.Type, compressionType)
	t.Fatalf("bad compressionType value;\nexpected: %q\nactual: %q", test.compressionConfig.Type, compressionType)

	if !strings.HasPrefix(resp.Data["value"].(string), "{\"type\":\"mounts\"") {
	if !strings.HasPrefix(resp.Data["value"].(string), `{"type":"mounts"`) {

Enhance sys/raw to read and write values that cannot be encoded in json #13537

Enhance sys/raw to read and write values that cannot be encoded in json #13537

Conversation

shwuandwing commented Dec 31, 2021

ncabatoff commented Jan 6, 2022

ncabatoff Jan 17, 2022

Choose a reason for hiding this comment

ncabatoff Jan 17, 2022

Choose a reason for hiding this comment

shwuandwing Jan 18, 2022

Choose a reason for hiding this comment

shwuandwing Jan 19, 2022

Choose a reason for hiding this comment

ncabatoff Jan 19, 2022

Choose a reason for hiding this comment

ncabatoff Jan 19, 2022

Choose a reason for hiding this comment

ncabatoff commented Jan 19, 2022

ncabatoff commented Jan 20, 2022