Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add (simple) support for quoted multiline variables in .env files #1748

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MidasLamb
Copy link

@MidasLamb MidasLamb commented Feb 4, 2025

This re-uses almost all previously existing logic, it only changes what constitutes a "line" when parsing the file.

This is not a fully fledged parser (e.g. escaping quotes doesn't work) but it should handle some additional cases now.

Fixes #965

Signed-off-by: Midas Lambrichts <midaslamb@gmail.com>
@felixfontein
Copy link
Contributor

Thanks for your contribution!

Please note that this is a breaking change and cannot be merged. There has been an earlier attempt at this (#622) that had to be reverted because of this (#706).

Ref: #1435

@MidasLamb
Copy link
Author

MidasLamb commented Feb 5, 2025

Thanks for the quick response @felixfontein !

I quickly went through the issue, and I'd like to point out that this PR does not add a full dotenv parser.
On the encryption side, it just looks make sure that it considers an entire key-value pair, not just lines, but how to value is encrypted has not changed.
On the decryption side there is maybe a small inconsistency, that shouldn't be an actual issue, where it might turn a "\n" combination into and actual newline.
i.e.

FOO="bar\nbaz"

becomes

FOO="bar
baz"

after an encryption->decryption cycle. But semantically, these values are actually the same, so this shouldn't be a breaking change.

This PR is just meant as a small incremental step to allow for multiline strings, without changing anything else really. In my reasoning the changes introduced here just expand the amount of allowed values, without doing any actual internal changes.

The main issue seemed to be with this change: https://github.com/getsops/sops/pull/622/files#diff-c9aa3c7e333fa3bc6fa314822b8da1e449389f6bb14b68608e47d048c4997a08R104, where it adds single quotes unconditionally to values.

If you want some additional tests/proof about this addition not being a breaking change in the way the earlier code was (which seems more like a proper dotenv parser, while this is just a slight expansion upon the currently working code.)

@felixfontein
Copy link
Contributor

While it's definitely true that your implementation comes very close to not breaking backwards compatibility, there are some cases that are broken. One of them you yourself listed. Your claim on "semantically equivalence" is problematic, since there is no well-defined .env file format. There are some assumptions that are true for many of these .env formats, but SOPS' implementation always was somewhat different. And since there is no formal specification to follow, every change is breaking.

Next to the change you mentioned above, is that if someone already encrypted a=foo\nbar" (something not compatible with many .env file format definitions, but with SOPS' definition), then your change will result in this being decrypted to

a=foo
bar"

Right now I don't see a way of adjusting the .env format (and the .ini format) without some versioning/file format identifier. I'll add some more thoughts on that to #1435 later today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SOPS cannot handle multi-line .env files
2 participants