Do not crash if an empty YAML file is encrypted

[felixfontein]

An empty YAML file results in no tree branch, which trips over encrypt since it assumes there's at least one.

I'm not sure this is the correct way to fix this, since the encrypted result will be empty as well (zero tree branches).

On the other hand, if you encrypt a YAML file with {} or --- in it (and nothing else), there will be at least one tree branch. Decrypting the resulting file yields {}. Decrypting an empty file yields an sops metadata not found error.

[hiddeco]

Wondering if it wouldn't be better to abort the operation if len(branches) <= 0, instead of continuing to execute a variety of code paths.

In addition, wondering if this is the right place to address this issue. As it appears to be YAML specific; what do the other stores do when they run into an empty document? If they e.g. error out, could this behavior then be replicated within the YAML store?

[felixfontein]

In addition, wondering if this is the right place to address this issue. As it appears to be YAML specific; what do the other stores do when they run into an empty document? If they e.g. error out, could this behavior then be replicated within the YAML store?

YAML is different from other file formats such JSON, INI etc. in that the other file formats always contain exactly one document, so an empty JSON/INI/... results in one empty branch.

But an empty YAML file contains no document (https://yaml.org/spec/1.2.2/#92-streams), so you get zero branches. This is different from a YAML file which contains a single empty document. The following are examples of a YAML file with a single empty document:

---

or

{}

So we could either simply not support zero-document YAML files, or we convert them to single empty document YAML files (which is kind of wrong), or we allow to 'encrypt' them (which is what the PR right now does - the result cannot be decrypted though, though the decryption code could also be adjusted to convert an empty YAML document back to an empty YAML document).

[hiddeco]

Think having the encrypt and decrypt command handle empty documents gracefully is the best option to go with, as I think the behavior most users will be after is running encryption (or decryption) on a range of files expecting success (or an error, but not because the document is empty).

In addition, I think that changing it to {} would yield an unexpected outcome during "decryption", as you would end up with a different file than you started with (in terms of emptiness).

[felixfontein]

Hmm, while looking at how to implement that, I found some more problems:

1. The json, binary, and env output stores will crash if no tree branch is there.
2. The ini output store treats zero branches the same as one empty branch.
3. Adjusting the YAML store load an empty "encrypted" file is not so simple. Also LoadEncryptedFile is expected to return the metadata as well.
4. (Not really related: if there are two or more YAML documents, all stores except YAML will silently ignore all except the first one when emitting. Not sure whether this is a problem, but maybe emitting a warning woudn't be bad.)

For the first three points, it might be easier to disallow empty inputs completely. What do you prefer (given the above points)?

[hiddeco]

With "disallow", you are aiming at erroring out?

[felixfontein]

Yes. Which would be similar to the current behavior, except that the message is a nice text and not a stacktrace :-)

[felixfontein]

(It would still be possible to change this at some point later in time, as a new feature. But at least the crash would go away for now.)

[hiddeco]

This sounds good to me, we can ask the community for more feedback and use-cases afterwards.

[felixfontein]

I completely changed the implementation. There's now a new error which is used when a completely empty document is provided for encryption.

[felixfontein]

@hiddeco thanks for reviewing this one as well!