Panic when encrypting yaml file with document separator followed by comment

[mccutchen]

I've found a panic when encrypting a yaml document that begins with a document separator (i.e. ---) followed by a comment.

Demonstration

$ cat testcase-panic.yml
---
# comment
key: value

sops-3.0.2.darwin --encrypt testcase-panic.yml
panic: expected end of document event but got 9 [recovered]
	panic: expected end of document event but got 9

goroutine 1 [running]:
go.mozilla.org/sops/vendor/github.com/mozilla-services/yaml.handleErr(0xc420162738)
	/home/travis/gopath/src/go.mozilla.org/sops/vendor/github.com/mozilla-services/yaml/yaml.go:192 +0x9f
panic(0x1622580, 0xc420049bc0)
	/home/travis/.gimme/versions/go1.9.linux.amd64/src/runtime/panic.go:491 +0x283
go.mozilla.org/sops/vendor/github.com/mozilla-services/yaml.(*parser).document(0xc420152900, 0x1028689)
	/home/travis/gopath/src/go.mozilla.org/sops/vendor/github.com/mozilla-services/yaml/decode.go:144 +0x2d1
go.mozilla.org/sops/vendor/github.com/mozilla-services/yaml.(*parser).parse(0xc420152900, 0x174af90)
	/home/travis/gopath/src/go.mozilla.org/sops/vendor/github.com/mozilla-services/yaml/decode.go:117 +0x6b
go.mozilla.org/sops/vendor/github.com/mozilla-services/yaml.CommentUnmarshaler.Unmarshal(0xc4201e0000, 0x19, 0x219, 0x16016e0, 0xc420149b60, 0x0, 0x0)
	/home/travis/gopath/src/go.mozilla.org/sops/vendor/github.com/mozilla-services/yaml/yaml.go:53 +0x161
go.mozilla.org/sops/stores/yaml.Store.Unmarshal(0xc4201e0000, 0x19, 0x219, 0x1010b88, 0xa0, 0x16bd660, 0x1, 0xc4201be460)
	/home/travis/gopath/src/go.mozilla.org/sops/stores/yaml/store.go:39 +0x88
go.mozilla.org/sops/stores/yaml.(*Store).Unmarshal(0x1b18cf8, 0xc4201e0000, 0x19, 0x219, 0x219, 0x0, 0x0, 0x1c45b48, 0x0)
	<autogenerated>:1 +0x5a
main.encrypt(0x1aad660, 0xc4201add10, 0x1ab1060, 0x1b18cf8, 0x1ab1060, 0x1b18cf8, 0x7fff5fbffb7c, 0x12, 0xc420049b60, 0x1, ...)
	/home/travis/gopath/src/go.mozilla.org/sops/cmd/sops/encrypt.go:32 +0x202
main.main.func5(0xc4200b4c60, 0x0, 0x0)
	/home/travis/gopath/src/go.mozilla.org/sops/cmd/sops/main.go:380 +0x27d7
go.mozilla.org/sops/vendor/gopkg.in/urfave/cli%2ev1.HandleAction(0x1633000, 0x174b5c8, 0xc4200b4c60, 0x0, 0x0)
	/home/travis/gopath/src/go.mozilla.org/sops/vendor/gopkg.in/urfave/cli.v1/app.go:490 +0xd2
go.mozilla.org/sops/vendor/gopkg.in/urfave/cli%2ev1.(*App).Run(0xc4201871e0, 0xc42000e0c0, 0x3, 0x3, 0x0, 0x0)
	/home/travis/gopath/src/go.mozilla.org/sops/vendor/gopkg.in/urfave/cli.v1/app.go:264 +0x635
main.main()
	/home/travis/gopath/src/go.mozilla.org/sops/cmd/sops/main.go:519 +0x201b

Interestingly, there is no panic if either the opening document separator or the opening comment is dropped:

$ cat testcase-ok-01.yml
# comment
key: value

$ cat testcase-ok-02.yml
---
key: value

$ sops-3.0.2.darwin --encrypt testcase-ok-01.yml &>/dev/null ; echo $?
0
$ sops-3.0.2.darwin --encrypt testcase-ok-02.yml &>/dev/null ; echo $?
0

(Output elided above to avoid leaking AWS account details.)

Details

• Panic seen in version 3.0.0 on Linux and macOS and 3.0.2 on macOS
• Both using prebuilt binaries downloaded from GitHub

Please let me know if I can provide any more info!

[autrilla]

The error is caused because of our YAML parser. Because of the internal representation of the YAML structure, a YAML document can only contain one structure at the top level (e.g. a YAML list, or a YAML map). Comments are then stored as nodes in the internal YAML parser representation (just like lists, maps, etc), so that means there can not be a comment and then a map at the top level. Please be aware that for this reason SOPS will not preserve comments on the top of the YAML file, even if this panic was fixed. The panic can definitely be fixed, but the comment is not going to be preserved any time soon as it'd require heavy modification to the parser.

[mccutchen]

Thanks for the explanation! I've noticed that when sops encrypts the file, the top-level comment is lost and I'm glad to understand why.

Please be aware that for this reason SOPS will not preserve comments on the top of the YAML file, even if this panic was fixed. The panic can definitely be fixed, but the comment is not going to be preserved any time soon as it'd require heavy modification to the parser.

That's totally fine for our purposes — we're wrapping sops in some tooling and want to present users an "annotated" example set of secrets.

FWIW, this bug is not a blocker for us (we're simply drop the opening --- document separator, which was added mostly out of habit), but it seemed odd and worth reporting since it was so easily reproducible and surprising.

[mccutchen]

❤️ ❤️ ❤️ thanks, y'all!