aciceri / agenix-shell

https://flake.parts/options/agenix-shell
31 stars 2 forks source link

The default `secretsPath` executes the name of the repository as a command #11

Open brokenpylons opened 2 months ago

brokenpylons commented 2 months ago

https://github.com/aciceri/agenix-shell/blob/884a3318aa661e237816e35b46dd014775498f19/flakeModules/agenix-shell.nix#L69

There is an $(...) around the cfg.flakeName.

aciceri commented 2 months ago

You made me worry for a moment but it's correct, or at least it's expected, probably flakeName should be renamed to something like flakeNameCommand. If you check its definition you see:

flakeName = mkOption {
  type = types.str;
  default = "git rev-parse --show-toplevel | xargs basename";
  description = "Command returning the name of the flake, used as part of the secrets path.";
};

So by default it assumes that the flake is a git repository (which is not true but covers 99% of my use cases) and uses the directory name as to build the temporary path. So if you want to use a custom name you would need to set it to flakeName = "echo my-custom-project-name".

Probably 2 things could be done here:

Feel free to send a PR or I will do it in some days/weeks.

Thank you for trying this :)

PS: in terms of security I don't think this is a big deal, when you run nix shell github:foo/bar you are already executing arbitrary commands with your user (and if you are a nix trusted user then it's directly root access, see here)

brokenpylons commented 2 months ago

I would prefer the first option.

What is the purpose of having secrets grouped by flakeName?

I'll open a pull request when I have the time, I also have a few other issues:

What do you think?

aciceri commented 1 month ago

I would prefer the first option.

They are not mutually exclusive, the first one has to be done for sure while the second one was more an idea since currently we use git rev-parse --show-toplevel | xargs basename as default while https://github.com/srid/flake-root should already provide something hopefully smarter. Feel free to open a PR addressing only the first thing for now :)

What is the purpose of having secrets grouped by flakeName?

No particular purpose, just for the sake of order (also makes the debugging easier). Are you suggesting to just use an UID? I accept suggestions/proposals.

$(id -u) in secrets path could be $UID which avoids running a command.

Agree

There is no /run/user/ on macOS. I think the fix is to just use /tmp, which has the same properties there.

Is /tmp deleted when you reboot on Mac? How does the agenix module on darwin behave? We should probably mimic it

The flakeName, secretsPath and installationScript use packages from the system instead of the nix store [...] Making secretsPath a package (writeShellApplication) and adding coreutils to the runtimeInputs would probably solve this.

Agree, IIRC there was an idiomatic way (with Nix) to make a script use only what we want ignoring the "global PATH", if it comes to mind I'll write it down here.

What do you think?

Go ahead and thanks! Whatever you do, even if partial compared to what we wrote here is well accepted.