buildkite-agent: secrecy improvements: non-store, non-Nix-source provisioning of secrets #31979
Conversation
deepfire
changed the title
|
cc @domenkozar |
|
This is one of efforts for #24288 |
| @@ -21,9 +16,11 @@ in | |||
| enable = mkEnableOption "buildkite-agent"; | |||
|
|
|||
| token = mkOption { | |||
| type = types.str; | |||
| type = types.either types.str types.path; | |||
There was a problem hiding this comment.
I think we should break backwards compatibility here, since the old method was insecure.
We should just allow types.path.
| @@ -3,16 +3,11 @@ | |||
| with lib; | |||
|
|
|||
| let | |||
| ## isPath :: String -> Bool | |||
| isPath = x: !(isAttrs x || isList x || isFunction x || isString x || isInt x || isBool x || isNull x) | |||
There was a problem hiding this comment.
Once we use just path, this is redundant (already checked by types.path)
There was a problem hiding this comment.
@deepfire needs to be removed
|
|
||
| echo "${cfg.openssh.publicKey}" > /var/lib/buildkite-agent/.ssh/id_rsa.pub | ||
| ${pkgs.coreutils}/bin/chmod 600 /var/lib/buildkite-agent/.ssh/id_rsa.pub | ||
| cat > "/var/lib/buildkite-agent/buildkite-agent.cfg" <<EOF |
There was a problem hiding this comment.
Missing a comment why this is done this way, to prevent issues in the future
|
|
||
| cat > "/var/lib/buildkite-agent/buildkite-agent.cfg" <<EOF | ||
| token="${catOrLiteral cfg.token}" | ||
| token="$(cat ${toString cfg.tokenPath})" |
There was a problem hiding this comment.
One big caveat here is that tokenPath needs to be on the machine running this build, so not something NixOS itself managed (but NixOps does).
I think we should clarify this in option descriptions.
| @@ -15,12 +15,12 @@ in | |||
| services.buildkite-agent = { | |||
| enable = mkEnableOption "buildkite-agent"; | |||
|
|
|||
| token = mkOption { | |||
| type = types.either types.str types.path; | |||
| tokenPath = mkOption { | |||
There was a problem hiding this comment.
The rename can only be done on master, for 17.09 we'll have to keep backwards compatibility :(
For master use mkRenamedOptionModule for renaming
…umentation # Conflicts: # nixos/modules/services/continuous-integration/buildkite-agent.nix
deepfire
force-pushed
the
buildkite-secrecy
branch
from
21f3eed
to
c6f94c9
Compare
|
@domenkozar, dropped the option renaming from this PR -- full version also submitted to master in #32009 |
|
@zimbatm, done! |
There was a problem hiding this comment.
LGTM. @domenkozar and last words? @deepfire please ping me again in a few days if this PR isn't merged
Motivation for this change
Things done
build-use-sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)