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

[holiman]

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]

Looking at https://github.com/mozilla/sops/issues/596#issuecomment-569066218, it seems that perhaps data should be a reserved key too (?)

[felixfontein]

This should fix #851.

[felixfontein]

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]

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.)

[holiman]

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

[hiddeco]

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]

You did not sign-off your commits

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

[felixfontein]

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 b44f024b0fca42d7c68905c085d3caac1dd74076 and 4bad36ea39d4e65feddb5ed895d66e07c08f89c8 are merged into one commit.

[holiman]

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

[hiddeco]

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]

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]

@hiddeco should we merge this now?

[enmanuelmoreira]

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

[felixfontein]

@holiman can you rebase and use the constant?