String context lost in identity path

LEXUGE commented 1 year ago

Hi, I embed a non-confidential key file when I build a ISO

age.identityPaths = [ ../../secrets/raw/img_key_ed25519 ];

This decrypts "secrets" in LiveCD correctly before commit https://github.com/ryantm/agenix/commit/37c729795605ec52638a7bdab1ec52e15eb3ff3a. However, after the commit was introduced, it no longer works.

It seems like ../../secrets/raw/img_key_ed25519 is added to store as /nix/store/<hash>-img_key_ed25519. However, the code at https://github.com/ryantm/agenix/blob/37c729795605ec52638a7bdab1ec52e15eb3ff3a/modules/age.nix#L75

is coerced as

/nix/store/<hash>-sources/secrets/raw/img_key_ed25519

which is non-existent.

n8henrie commented 1 year ago

Thanks for the bug report! I'm responsible for that commit, sorry for the trouble -- should be able to look at this today.

LEXUGE commented 1 year ago

No worries!

n8henrie commented 1 year ago

Trying to make sure I understand.

You're building an ISO
../../secrets/raw/img_key_ed25519 is available at build time
Inside the running live image, it expects to find /nix/store/<hash>-sources/secrets/raw/img_key_ed25519 but finds nothing because the key is actually at /nix/store/<hash>-img_key_ed25519

Is that right?

LEXUGE commented 1 year ago

Yes. And I also found the path of identity at testIdentities is correctly finalized as /nix/store/<hash>-img_key_ed25519, https://github.com/ryantm/agenix/blob/37c729795605ec52638a7bdab1ec52e15eb3ff3a/modules/age.nix#L102

n8henrie commented 1 year ago

Hmmm. So this seems like an unusual use case, since the whole point of this module is to keep secrets out of the store, but you're intentionally putting them into the store. Do I have that right?

age.identityPaths is [ ../../secrets/raw/img_key_ed25519 ] at build time, right? What is it at run time?

Can you provide a little more context for your code? A link is fine if hosted somewhere, if it's private, can you at minimum include:

ISO build-time age configuration (age.secrets, age, identityPaths, etc.)
live image run-time age configuration (age.secrets, age, identityPaths, etc.)
what are the paths to $RULES / secrets
what does your secrets.nix look like?

Please obscure / redact anything that seems too private of course.

I could probably guess some of this, but having you provide it will help me get to the point faster and possibly build a test case.

LEXUGE commented 1 year ago

Hi, the entire config is fully available at https://github.com/LEXUGE/flake. You can build the image using nix build .#img-x1c7.

The use case is a little unusual. And it's not indeed containing any secret.

The runtime path of ../../secrets/raw/img_key_ed25519 I think is just /nix/store/<hash>-img_key_ed25519 as that is what made into the store of image.

LEXUGE commented 1 year ago

You can also test out the known-defect image at https://github.com/LEXUGE/flake/releases/download/build-20230224_0953/imgs.x1c7.iso

n8henrie commented 1 year ago

Hmmm, including a path in identityPaths instead of a string seems like a problem; I'm not sure if it working previously was a bug or a feature :) I'm guessing it worked because identities was evaluated at build time and the result was cached?

I'll continue to tinker with it. One good thing is that the PR in question also allows keys to be missing, so you can add additional keys (one that exists at build time, another that exists at run time) and it should work, though they would need to be strings.

$ nix build .#nixosConfigurations.img-x1c7.config.system.build.isoImage
[23.9 MiB DL] evaluating derivation 'git+file:///tmp/tmp.cLUUziznTI/flake#nixosConfigurations.img-x1c7.config.system.build.isoImage'
Segmentation fault (core dumped)

Gah, my kingdom to have nix rewritten in rust.

ryantm commented 1 year ago

If I understand correctly, I feel like the way it is now fixed a security vulnerability. I do not want the age module to be causing private keys to end up in the nix store.

LEXUGE commented 1 year ago

I agree private keys should not be in the nix store. However, this doesn't fix the vulnerability either as we can always make it into the store. It's better to make a predicate to see if the path starts with /nix/store and throw a warning instead of the current behavior.

The reason I want to have agenix in LiveCD is to minimize the amount of modules/config that I need to write seperately for LiveCD and my config. Being able to "bootstrap agenix" makes it possible.

LEXUGE commented 1 year ago

For my bizarre case, I think the following will do the trick:

age.identityPaths = [ (pkgs.writeText "img_key_ed25519" (builtins.readFile ../../secrets/raw/img_key_ed25519)) ];

I will close once I confirm the workaround.

n8henrie commented 1 year ago

If I understand correctly, I feel like the way it is now fixed a security vulnerability.

That's what I was thinking as well -- perhaps not exactly a vulnerability, but something that encouraged misconfiguration.

Perhaps relevant wording in the README could change -- from referring to paths (which I think of as ../example/id_rsa to strings ("/path/to/id_rsa").

n8henrie commented 1 year ago

FWIW, I think (?) I've recreated the issue with a failing test at https://github.com/n8henrie/agenix/tree/issue_165

ryantm commented 1 year ago

It's better to make a predicate to see if the path starts with /nix/store and throw a warning instead of the current behavior.

Good idea. I think I saw this pattern somewhere before, but don't remember where. If we close this issue, we should at open one to add this predicate.

ryantm / agenix

String context lost in identity path #165