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

Merged
merged 1 commit into from
Jun 21, 2024

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 +1556
// 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
@felixfontein felixfontein force-pushed the decryption-creation-rules branch 2 times, most recently from 8b803bb to c145df8 Compare June 15, 2024 15:53
Copy link

@sabre1041 sabre1041 left a comment

Choose a reason for hiding this comment

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

LGTM

@felixfontein felixfontein merged commit 3458c35 into getsops:main Jun 21, 2024
9 checks passed
@felixfontein felixfontein deleted the decryption-creation-rules branch June 21, 2024 10:23
@felixfontein
Copy link
Contributor Author

@devstein @sabre1041 thanks for reviewing this!

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
3 participants