stores: test for duplicate keys, reserve keyword (yaml only now) #1203

holiman · 2023-04-21T08:24:57Z

I started looking into sops, which I find to be a really nice idea and implementation. Kudos!
However, when testing some edgecase scenarios, I found some rough corners.

Specifically, it was possible to, when editing the file, add a sops key. This was then encrypted, and when trying to open it later on, I got

go run ./cmd/sops/ mytestfile2.yaml 
Error unmarshalling input yaml: yaml: unmarshal errors:
  line 12: mapping key "sops" already defined at line 7

This is a bit so-so UX wise, since the only way to recover is to remove the sops-line, and then reopen the file with the --ignore-mac enabled, and then save it again.

In general, it seems to me that sops should be strict about validation. In that sense, this PR:

Applies a check for disallowing duplicate keys in YAML
Tests for, but does not disallow, duplicate keys in json.
Applies a check for disallowing reserved keywords in YAML (only sops now).
Remaining small changes come from go fmt.

If you think this is a good idea, the reserved keyword should be applied for other formats too, and I can go ahead and make that change too.

holiman · 2023-04-21T08:30:47Z

Looking at #596 (comment), it seems that perhaps data should be a reserved key too (?)

felixfontein · 2023-04-21T11:01:57Z

This should fix #851.

felixfontein · 2023-09-16T12:21:49Z

Looking at #596 (comment), it seems that perhaps data should be a reserved key too (?)

The README explicitly shows using data for other things in quite a few examples, so I guess it's not a good idea to disallow it globally.

I think #596 should probably be handled differently, like only treating it as binary if there is no other key than data and its value is a string. (I'm not sure which mechanism is used right now. Maybe detection is only based on the file name.)

felixfontein · 2023-09-16T12:34:48Z

I did look a bit into detection; the detection is done purely by filename, not by anything else.

Something that definitely can be improved is error handling in BinaryStore.EmitPlainFile, if data's value is not a string. That would produce a better readable error message instead of a panic (and that error message could mention --output-type so that users qiuckly get an idea what to do). (Edit: #1289 does that.)

felixfontein · 2023-09-16T13:28:25Z

stores/json/store_test.go

+	if err != nil {
+		t.Errorf("did not expect error, got %v", err)
+	}


Suggested change

if err != nil {

t.Errorf("did not expect error, got %v", err)

}

assert.Nil(t, err)

felixfontein · 2023-09-16T13:29:07Z

stores/json/store.go

@@ -211,7 +211,9 @@ func (store Store) jsonFromTreeBranch(branch sops.TreeBranch) ([]byte, error) {

 func (store Store) treeBranchFromJSON(in []byte) (sops.TreeBranch, error) {
 	dec := json.NewDecoder(bytes.NewReader(in))
-	dec.Token()
+	if _, err := dec.Token(); err != nil {
+		return nil, err


Could you please document what exactly this is doing? I guess this is "Tests for, but does not disallow, duplicate keys in json.", but it is totally unclear to me how this code achieves that.

This change is (as far as I can tell, been a while) not required, it's just a drive-by change that I made: Token() does return an error, and checking+returning it is correct.
Later on, the store.treeBranchFromJSONDecoder(dec) call will internally do the same thing

func (store Store) treeItemFromJSONDecoder(dec *json.Decoder) (sops.TreeItem, error) { var item sops.TreeItem key, err := dec.Token() if err != nil && err != io.EOF { return item, err }

So I guess I figured that we should do it on the very first instance too

Except that this one fails when err == io.EOF. Which I guess is OK. I would add a comment explaining that.

Happy to add a comment, but I'm not sure what to write: I don't quite see why we need to document why we handle an error. Usually, motivation/explanation is required as for why one omits to handle an error.

After dwelling in sources of the JSON module, I now understand why this change is correct.

The main problem I have with this change is that it takes a long time to figure out what it does, and since it is totally unrelated to the commit message it appears in it is very confusing to figure out where it comes from.

It would be a lot better to move this to a separate commit (or even a separate PR) with a clear commit message (something like "Make sure errors returned when extracting the first token are handled").

Also there is another potential error lurking here: the first return value also needs to be checked whether it's a {. Otherwise LoadPlainFile produces "strange" error messages that aren't very helpful:

$ echo '[]' | sops --encrypt --input-type json /dev/stdin Error unmarshalling file: Could not unmarshal input data: Expected JSON object key, got ] of type json.Delim instead $ echo '[}' | sops --encrypt --input-type json /dev/stdin Error unmarshalling file: Could not unmarshal input data: invalid character '}' looking for beginning of value $ echo '["foo","bar"]' | sops --encrypt --input-type json /dev/stdin Error unmarshalling file: Could not unmarshal input data: invalid character '}' after array element $ echo '["foo":"bar"}' | sops --encrypt --input-type json /dev/stdin Error unmarshalling file: Could not unmarshal input data: invalid character ':' after array element

I don't see what difference this makes. As far as I can tell this PR does not change the behaviour against master

This PR:

$ echo '[]' | go run ./cmd/sops/ --encrypt --input-type json /dev/stdin Error unmarshalling file: Could not unmarshal input data: Expected JSON object key, got ] of type json.Delim instead $ echo '[}' | go run ./cmd/sops/ --encrypt --input-type json /dev/stdin Error unmarshalling file: Could not unmarshal input data: invalid character '}' looking for beginning of value $ echo '["foo","bar"]' | go run ./cmd/sops/ --encrypt --input-type json /dev/stdin Error unmarshalling file: Could not unmarshal input data: Expected JSON object key, got ] of type json.Delim instead $ echo '["foo":"bar"}' | go run ./cmd/sops/ --encrypt --input-type json /dev/stdin Error unmarshalling file: Could not unmarshal input data: invalid character ':' after array element

Master:

$ echo '[]' | go run ./cmd/sops/ --encrypt --input-type json /dev/stdin Error unmarshalling file: Could not unmarshal input data: Expected JSON object key, got ] of type json.Delim instead $ echo '[}' | go run ./cmd/sops/ --encrypt --input-type json /dev/stdin Error unmarshalling file: Could not unmarshal input data: invalid character '}' looking for beginning of value $ echo '["foo","bar"]' | go run ./cmd/sops/ --encrypt --input-type json /dev/stdin Error unmarshalling file: Could not unmarshal input data: Expected JSON object key, got ] of type json.Delim instead $ echo '["foo":"bar"}' | go run ./cmd/sops/ --encrypt --input-type json /dev/stdin Error unmarshalling file: Could not unmarshal input data: invalid character ':' after array element

But sure I can remove this change if you want

Also there is another potential error lurking here: the first return value also needs to be checked whether it's a {.

It should not literally check that it's a {, since whitespace is also accepted. I think the error messages are pretty good.

Consider ["foo":"bar"}. If we say error: it must start with {, then I think we are mixing different layers, with different concerns.

First of all, the input needs to be valid json. The value ["foo":"bar"} is not valid json, because a json list cannot contain key-value pairs, so the parse fails. The user will have to fix up his json, there are many tools to validate / lint json.

Secondly, there are app-specific constraints on what is required of the json object: such as that it is a dict, not a list. But those come later, IMO.

If this app starts validating specific characters in the json stream, like "checked whether it's a {.", then it overreaches. And before you know it, you are basically taking on the maintenance of an in-house json parser.

It should not literally check that it's a {, since whitespace is also accepted. I think the error messages are pretty good.

Yes, it should for a literal {, since whitespace has already been processed by the decoder.

First of all, the input needs to be valid json. The value ["foo":"bar"} is not valid json, because a json list cannot contain key-value pairs, so the parse fails. The user will have to fix up his json, there are many tools to validate / lint json.

True, but did you also consider the [] example? Or tried out something like [{}]? The error messages are not really helpful, because the code already mixes the two layers: it parses valid JSON, and at the same time only parses JSON objects.

Secondly, there are app-specific constraints on what is required of the json object: such as that it is a dict, not a list. But those come later, IMO.

The function is called treeBranchFromJSON. It cannot process anything else than a JSON object.

In any case, this discusison IMO shows why this should not part of a totally unrelated commit. This has nothing to do with (not) checking for duplicate keys.

holiman · 2023-09-19T12:12:35Z

Rebased on main, a few tests seem to be failing, not quite sure why

hiddeco · 2023-09-19T12:39:49Z

You did not sign-off your commits, which is required to accept our DCO. Please refer to https://github.com/getsops/sops/pull/1203/checks?check_run_id=16924447836 for more information.

holiman · 2023-09-19T13:14:10Z

You did not sign-off your commits

Fixed (on my branch, seems to be taking github some time to propagate the changes here)

felixfontein · 2023-09-20T19:50:58Z

stores/json/store.go

@@ -211,7 +211,9 @@ func (store Store) jsonFromTreeBranch(branch sops.TreeBranch) ([]byte, error) {

 func (store Store) treeBranchFromJSON(in []byte) (sops.TreeBranch, error) {
 	dec := json.NewDecoder(bytes.NewReader(in))
-	dec.Token()
+	if _, err := dec.Token(); err != nil {
+		return nil, err


Except that this one fails when err == io.EOF. Which I guess is OK. I would add a comment explaining that.

stores/yaml/store_test.go

felixfontein · 2023-09-27T20:32:41Z

Do you want to fix the error handling in a separate PR, or should I do that?

Also you need to sign-off the commit (DCO check). Finally, it's probably better to squash b44f024 and 4bad36e are merged into one commit.

stores/json: use assert stores/yaml: fix failing test (empty data) stores/yaml: use assert in tests unfix error handling and ignore error Signed-off-by: Martin Holst Swende <martin@swende.se>

holiman · 2023-09-28T06:59:03Z

I squashed everything now.

Do you want to fix the error handling in a separate PR, or should I do that?

I'd prefer to leave that to you, thanks

felixfontein · 2023-09-28T11:27:43Z

stores/json/store_test.go

+`
+	s := new(Store)
+	_, err := s.LoadPlainFile([]byte(data))
+	assert.Nil(t, err)


At some point, we should extend this test to verify that re-serializing yields the same result.

felixfontein · 2023-09-28T11:28:28Z

stores/yaml/store_test.go

+	assert.NotNil(t, err)
+	assert.Equal(t, `yaml: unmarshal errors:
+  line 3: mapping key "hello" already defined at line 2`, err.Error())
+}


We should also add a test which looks at multi-document YAML streams (with collisions in later documents).

hiddeco · 2023-09-28T13:18:02Z

This is looking good to me, but I think we should release a patch first before moving ahead with changes which force a new minor.

felixfontein · 2023-09-28T20:17:57Z

Do you want to fix the error handling in a separate PR, or should I do that?

I'd prefer to leave that to you, thanks

#1307.

felixfontein · 2023-11-05T13:59:58Z

@hiddeco should we merge this now?

enmanuelmoreira · 2024-01-23T14:56:16Z

Any updates about this? the problem stills happening at version 3.8.1

hiddeco

Needs a rebase to resolve the conflict, but other than this (and a tiny nit) it looks good to me.

Sorry for the wait @holiman, and thanks for your contribution 🍍

hiddeco · 2024-02-09T13:39:49Z

stores/yaml/store.go

@@ -309,6 +316,12 @@ func (store *Store) LoadPlainFile(in []byte) (sops.TreeBranches, error) {
 		if err != nil {
 			return nil, fmt.Errorf("Error unmarshaling input YAML: %s", err)
 		}
+		// Prevent use of reserved keywords
+		for _, item := range branch {
+			if item.Key == "sops" {


Nit: given the string is also matched against on L281. It may be wise to move it to a const.

There's already a constant for it: SopsMetadataKey (in stores/stores.go). That one should be used.

felixfontein · 2024-06-15T16:12:13Z

@holiman can you rebase and use the constant?

holiman changed the title ~~stores: test for duplicate keys, reseve keyword (yaml only now)~~ stores: test for duplicate keys, reserve keyword (yaml only now) Apr 21, 2023

felixfontein mentioned this pull request Apr 21, 2023

Version 3.7.0 and duplicated keys in a YAML file #851

Open

hiddeco added this to the v3.9.0 milestone Jul 3, 2023

hiddeco changed the base branch from master to main July 6, 2023 20:57

felixfontein reviewed Sep 16, 2023

View reviewed changes

holiman force-pushed the fixes branch from faa520b to 32f1320 Compare September 19, 2023 12:11

holiman force-pushed the fixes branch from 32f1320 to 377b6b4 Compare September 19, 2023 13:41

hiddeco self-requested a review September 20, 2023 11:46

felixfontein reviewed Sep 20, 2023

View reviewed changes

holiman force-pushed the fixes branch from ffd644a to 8055bae Compare September 21, 2023 07:10

stores: test for duplicate keys, reseve keyword (yaml only now)

627bb97

stores/json: use assert stores/yaml: fix failing test (empty data) stores/yaml: use assert in tests unfix error handling and ignore error Signed-off-by: Martin Holst Swende <martin@swende.se>

holiman force-pushed the fixes branch from b44f024 to 627bb97 Compare September 28, 2023 06:57

felixfontein approved these changes Sep 28, 2023

View reviewed changes

felixfontein mentioned this pull request Sep 28, 2023

Handle return values of dec.Token() to improve error messages #1307

Merged

hiddeco approved these changes Feb 9, 2024

View reviewed changes

felixfontein removed this from the v3.9.0 milestone Jun 26, 2024

felixfontein added this to the 3.10.0 milestone Jun 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stores: test for duplicate keys, reserve keyword (yaml only now) #1203

stores: test for duplicate keys, reserve keyword (yaml only now) #1203

holiman commented Apr 21, 2023

holiman commented Apr 21, 2023

felixfontein commented Apr 21, 2023

felixfontein commented Sep 16, 2023

felixfontein commented Sep 16, 2023 •

edited

Loading

felixfontein Sep 16, 2023

felixfontein Sep 16, 2023

holiman Sep 19, 2023

felixfontein Sep 20, 2023

holiman Sep 21, 2023

felixfontein Sep 21, 2023

holiman Sep 22, 2023

holiman Sep 22, 2023 •

edited

Loading

felixfontein Sep 22, 2023

holiman commented Sep 19, 2023

hiddeco commented Sep 19, 2023

holiman commented Sep 19, 2023

felixfontein Sep 20, 2023

felixfontein commented Sep 27, 2023

holiman commented Sep 28, 2023

felixfontein Sep 28, 2023

felixfontein Sep 28, 2023

hiddeco commented Sep 28, 2023

felixfontein commented Sep 28, 2023

felixfontein commented Nov 5, 2023

enmanuelmoreira commented Jan 23, 2024

hiddeco left a comment

hiddeco Feb 9, 2024

felixfontein Feb 10, 2024

felixfontein commented Jun 15, 2024

stores: test for duplicate keys, reserve keyword (yaml only now) #1203

Are you sure you want to change the base?

stores: test for duplicate keys, reserve keyword (yaml only now) #1203

Conversation

holiman commented Apr 21, 2023

holiman commented Apr 21, 2023

felixfontein commented Apr 21, 2023

felixfontein commented Sep 16, 2023

felixfontein commented Sep 16, 2023 • edited Loading

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

holiman Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holiman commented Sep 19, 2023

hiddeco commented Sep 19, 2023

holiman commented Sep 19, 2023

Choose a reason for hiding this comment

felixfontein commented Sep 27, 2023

holiman commented Sep 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiddeco commented Sep 28, 2023

felixfontein commented Sep 28, 2023

felixfontein commented Nov 5, 2023

enmanuelmoreira commented Jan 23, 2024

hiddeco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixfontein commented Jun 15, 2024

felixfontein commented Sep 16, 2023 •

edited

Loading

holiman Sep 22, 2023 •

edited

Loading