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 secrets change detection #132

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

erikarvstedt
Copy link
Contributor

@erikarvstedt erikarvstedt commented Jan 9, 2023

Previously, secrets were decrypted and recreated on each call to system/activate.
Besides the unneeded extra computation, this also added noisy log output on nixos-rebuild switch.

Now, secrets generations are only created when secrets have changed.

Strategy

For all reasonable use cases [1], the store path of a secret unambiguously represents its content.
So just use a hash of builtins.toJSON config.age.secrets as the generation name.

This yields a minimal implementation that might be even simpler than rolling generation numbers.

[1] One could create nondeterministic secrets with Nix derivations, but this would defeat agenix' purpose.

Todo

Store paths

This strategy only works when secrets paths are store paths (thus the option type change).
Is there any known use case for secrets with non-store paths?

Test

Are you interested in adding this? Then I'll create a test.

@cole-h
Copy link
Collaborator

cole-h commented Jan 9, 2023

Hmm... Why is this better than "rolling generation numbers"?

@cole-h cole-h requested a review from ryantm January 9, 2023 21:42
@erikarvstedt
Copy link
Contributor Author

erikarvstedt commented Jan 9, 2023

Edit: I've amended the initial paragraph.

This makes the following commit more readable.
Copy link

@Radvendii Radvendii left a comment

Choose a reason for hiding this comment

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

Shortcomings of this approach:

  • Course-grained (if one secret is changed, all of them will be updated)
  • Only checks encrypted contents (if secrets are re-keyed, they will be updated)

But it's better than what we currently have, so I say go for it? Not my call though.

Comment on lines +128 to +139
type = mkOptionType {
name = "nix-path";
descriptionClass = "noun";
check = builtins.isPath;
merge = mergeEqualOption;
};

Choose a reason for hiding this comment

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

Suggested change
type = mkOptionType {
name = "nix-path";
descriptionClass = "noun";
check = builtins.isPath;
merge = mergeEqualOption;
};
type = types.package;

Does this work? See https://github.com/NixOS/nixpkgs/blob/master/lib/types.nix#L464-L473

Copy link
Contributor Author

@erikarvstedt erikarvstedt Jan 10, 2023

Choose a reason for hiding this comment

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

No, because at the moment the value is typechecked, it's not yet a store path but a mere path (like /my/repo/secrets/foo.nix).

Choose a reason for hiding this comment

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

I see, yes. builtins.toString /my/repo/secrets/foo.nix yields "/my/repo/secrets/foo.nix" not the store path where it'll get stored.

Comment on lines +70 to +75
if (( _localstatus > 0 )); then
mv "${cfg.secretsMountPoint}/$_agenix_generation"{,-incomplete}
_agenix_generation+=-incomplete
fi

Choose a reason for hiding this comment

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

What is this about? Maybe worth a comment in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Choose a reason for hiding this comment

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

Excellent, makes sense. I still don't understand where _localstatus comes from or under what conditions it's valid, but maybe my google-fu isn't good enough to find the bash documentation for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

_localstatus is defined here.

@ryantm
Copy link
Owner

ryantm commented Jan 29, 2023

This seems cool. I've generally been worried about how noisy agenix is during activation and this idea could solve it mostly.

One downside I see is when someone is debugging agenix, they'd not know what their previous agenix generation was. This could be solved by one more layer of indirection though. That is, keep the rolling numbers but have them point to hashed names.

@erikarvstedt
Copy link
Contributor Author

erikarvstedt commented Jan 29, 2023

How exactly can rolling generation numbers help with debugging? Can you expand a bit on that?
Edit: Ah, you probably mean that when there are two generations with hashes as names, it's not obvious which generation is newer.

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.

4 participants