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

Decryption: do not fail if no matching creation_rule is present in config file #1434

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

Conversation

felixfontein
Copy link
Contributor

Since the config file loading is only done for backwards compatibility (and the result isn't used), we can simply skip it when only decrypting.

(This is not a problem for new-style decryption - sops decrypt subcommand - since that doens't load the config for the file.)

Fixes #868.

Copy link
Contributor

@devstein devstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One readability comment, but otherwise looks good to me (code-wise)

Comment on lines +1536 to +1538
// Load configuration here for backwards compatibility (error out in case of bad config files),
// but only when not just decrypting (https://github.com/getsops/sops/issues/868)
needsCreationRule := isEncryptMode || isRotateMode || isSetMode || isEditMode
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, this would only be false if

isEncryptMode = false
isRotateMode = false
isSetMode = false
isDecryptMode = true

If this is correct, it might be easier to reason about if this is written as

Suggested change
// Load configuration here for backwards compatibility (error out in case of bad config files),
// but only when not just decrypting (https://github.com/getsops/sops/issues/868)
needsCreationRule := isEncryptMode || isRotateMode || isSetMode || isEditMode
// Load configuration here for backwards compatibility (error out in case of bad config files),
// but only when not just decrypting (https://github.com/getsops/sops/issues/868)
needsCreationRule := !(isEncryptMode || isRotateMode || isSetMode) && isDecryptMode

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, this would only be false if

isEncryptMode = false
isRotateMode = false
isSetMode = false
isDecryptMode = true

Yes, due to the way isEditMode works (it's true if no other mode is set).

If this is correct, it might be easier to reason about if this is written as

I find the new expression harder to understand, and the intended behavior - the creation rule needs to be checked here if one of the modes is active that needs it later - less clear.

Also your expression has a boolean error, it evaluates to true in case needsCreationRule should be false, so it would have to be needsCreationRule := isEncryptMode || isRotateMode || isSetMode || !isDecryptMode.

@devstein devstein requested a review from a team June 11, 2024 00:05
…ent in config file.

Signed-off-by: Felix Fontein <felix@fontein.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decryption fails if file is not in the creation_rules
2 participants