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

String context lost in identity path #165

Closed
LEXUGE opened this issue Feb 24, 2023 · 14 comments · Fixed by #168
Closed

String context lost in identity path #165

LEXUGE opened this issue Feb 24, 2023 · 14 comments · Fixed by #168

Comments

@LEXUGE
Copy link

LEXUGE commented Feb 24, 2023

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 37c7297. 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

for identity in ${toString cfg.identityPaths}; do

is coerced as

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

which is non-existent.

@n8henrie
Copy link
Collaborator

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

@LEXUGE
Copy link
Author

LEXUGE commented Feb 24, 2023

No worries!

@n8henrie
Copy link
Collaborator

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
Copy link
Author

LEXUGE commented Feb 24, 2023

Yes. And I also found the path of identity at testIdentities is correctly finalized as /nix/store/<hash>-img_key_ed25519,

test -f ${path} || echo '[agenix] WARNING: config.age.identityPaths entry ${path} not present!'

@n8henrie
Copy link
Collaborator

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
Copy link
Author

LEXUGE commented Feb 24, 2023

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
Copy link
Author

LEXUGE commented Feb 24, 2023

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

@n8henrie
Copy link
Collaborator

n8henrie commented Feb 24, 2023

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
Copy link
Owner

ryantm commented Feb 24, 2023

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
Copy link
Author

LEXUGE commented Feb 24, 2023

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
Copy link
Author

LEXUGE commented Feb 24, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

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

@ryantm
Copy link
Owner

ryantm commented Feb 24, 2023

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.

@LEXUGE LEXUGE closed this as not planned Won't fix, can't repro, duplicate, stale Feb 25, 2023
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 a pull request may close this issue.

3 participants