Fix #611 - Dont break when config file exists but CreationRules are empty

[Kamahl19]

This fixes couple of similar issues, one of them a real use-case.

Currently SOPS breaks if .sops.yaml:

• exists but is empty
• contains creation_rules: with no defined rules
• contains only DestinationRules but no CreationRules
• contains no matching CreationRules
• contains invalid CreationRules

It should only break in 4th and 5th case. First 3 cases should be treated as if no .sops.yaml was provided. It's best demonstrated by 3rd case when we want to provide KMS/GPG via CLI argument but we want to have .sops.yaml file with DestionationRule to publish secrets to Vault. This is currently not possible and is fixed in this PR.

I added tests for all 4 cases. 5th already has a proper test.

Closes #611

[autrilla]

I think we should do this a different way -- parseCreationRuleForFile should return an error (as it does now) when no config is found. The main underlying issue is that we're using the same function to load two different kinds of config. I think we should split the loading into two and handle each case appropriately. If wanting to create a new file and no creation rule is found, we can then output a good error message for that. For publishing, the fact that no creation rule was found is irrelevant, and we don't need to look at creation rules at all.

The tests look great by the way!

[Kamahl19]

@autrilla does it mean you agree with reasoning and tests, but you want to achieve it with bigger refactoring? I am willing to see this through however it's my first golang try so it would be great if you could navigate me a bit :)

parseCreationRuleForFile is only used in LoadForFile which is used in Main and `UpdateKeys. Are those the "same function to load two different kinds of config" you mentioned?

What "loading" method would you split into 2? Do you mean LoadForFile? What would be those 2 cases?

Does this only concern creating a new file?

Thanks for the feedback!

[autrilla]

No problem at all. I agree with the reasoning, kind of, but I think it has some unintended side effects and is not particularly clear. But yes, I think we should fix this with a bigger refactor.

parseCreationRuleForFile is only used in LoadForFile which is used in Main and `UpdateKeys. Are those the "same function to load two different kinds of config" you mentioned?

I was referring to LoadForFile specifically. This function is used to load both creation rules as well as destination rules, which are completely distinct and used in different contexts. I would indeed split LoadForFile, as you guess, into two, one function for loading creation rules, and one for loading destination rules. Then, we can make sure we only load creation rules for creating new files, since it's the only thing they're relevant for. And we can keep the loading of destionation rules isolated to sops publish as well. How does that sound?

Let me know if you have any questions, I'm more than happy to help you with this.

[Kamahl19]

Actually there already is a LoadDestinationRuleForFile used only for publishing.

The problem I am fixing here happens during the creation. When I use config only for DestinationRules then during creation it tries to load CreationRules using LoadForFile and it crashes because 1.) it finds the config file 2.) it does not find any CreationRules which results in error. https://github.com/mozilla/sops/blob/4261f1bb816212fba04e0f970cf08b17beab417b/config/config.go#L298 If it doesnt find the config file, it just continues the creation process and uses inputs from CLI. https://github.com/mozilla/sops/blob/afd073a5be0fe2232d7cd345b9b30edc70ccb962/cmd/sops/main.go#L972

So I duplicated this behaviour of "assuming config does not exist during creation if configPath is not found" for the case when "config is found but no CreationRules are found".

[autrilla]

Ah, you're right! Thanks for your patch, this is good as it is. We should probably rename LoadConfigForFile to LoadCreationRuleForFile or something along those lines, but let's do that in another PR.

[autrilla]

Thanks for the patch :)

[Kamahl19]

Thanks! Glad we figured it out :)