fetchgit: add git signature verification #330457
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| { | ||
| lib, | ||
| runCommand, | ||
| writeShellApplication, | ||
| writeText, | ||
| git, | ||
| openssh, | ||
| gnupg, | ||
| }: | ||
| ( | ||
| { | ||
| name, | ||
| revWithTag, | ||
| verifyCommit, | ||
| verifyTag, | ||
| publicKeys, | ||
| leaveDotGit, | ||
| fetchresult, | ||
| }: | ||
| let | ||
| # split gpg keys from ssh keys | ||
| keysPartitioned = lib.partition (k: k.type == "gpg") publicKeys; | ||
| gpgKeys = lib.catAttrs "key" keysPartitioned.right; | ||
| sshKeys = keysPartitioned.wrong; | ||
|
Comment on lines
+23
to
+24
Member
There was a problem hiding this comment. I think these are confusingly inconsistent:
To fix this, I think (a) there should be different names for values that are expected to be paths versus strings, e.g. use |
||
| # create a keyring containing gpgKeys | ||
| gpgKeyring = runCommand "gpgKeyring" { buildInputs = [ gnupg ]; } '' | ||
| gpg --homedir /build --no-default-keyring --keyring $out --fingerprint # create empty keyring at $out | ||
| for KEY in ${lib.concatStringsSep " " gpgKeys} | ||
|
Member
There was a problem hiding this comment.
|
||
| do | ||
| gpg --homedir /build --no-default-keyring --keyring $out --import $KEY # import $KEY | ||
| done | ||
| ''; | ||
| gitOnRepo = writeShellApplication { | ||
| name = "gitOnRepo"; | ||
| runtimeInputs = [ git ]; | ||
| text = '' | ||
| git -C "${fetchresult}" -c safe.directory='*' "$@" | ||
|
Member
There was a problem hiding this comment. Nit: you're quoting a store path here, but not in other bits of shell code. I don't particularly mind which you do (personally, I'd either pass things in as environment variables then quote the variable, or use |
||
| ''; | ||
| }; | ||
| # wrap gpg to use gpgKeyring | ||
| gpgWithKeys = writeShellApplication { | ||
| name = "gpgWithKeys"; | ||
| runtimeInputs = [ | ||
| gnupg | ||
| gitOnRepo | ||
| ]; | ||
| text = '' | ||
| committerTime="$(gitOnRepo -c core.pager=cat log --format="%cd" --date=raw -n 1 ${revWithTag})" | ||
|
Member
There was a problem hiding this comment.
|
||
| gpg --faked-system-time "$committerTime" --homedir /build --no-default-keyring --always-trust --keyring ${gpgKeyring} "$@" | ||
| ''; | ||
| }; | ||
| # create "allowed signers" file for ssh key verification: https://man.openbsd.org/ssh-keygen.1#ALLOWED_SIGNERS | ||
| allowedSignersFile = writeText "allowed signers" ( | ||
| lib.concatMapStrings (k: "* ${k.type} ${k.key}\n") sshKeys | ||
| ); | ||
| in | ||
| runCommand name | ||
| { | ||
| buildInputs = [ | ||
| gitOnRepo | ||
| openssh | ||
| gpgWithKeys | ||
| ]; | ||
| inherit verifyCommit verifyTag leaveDotGit; | ||
| } | ||
| '' | ||
| gpgWithKeys -k | ||
| if test "$verifyCommit" == 1; then | ||
|
Member
There was a problem hiding this comment.
I'd much prefer |
||
| gitOnRepo \ | ||
| -c gpg.ssh.allowedSignersFile="${allowedSignersFile}" \ | ||
| -c gpg.program="gpgWithKeys" \ | ||
| verify-commit ${revWithTag} | ||
|
Member
There was a problem hiding this comment. I think |
||
| fi | ||
|
|
||
| if test "$verifyTag" == 1; then | ||
| gitOnRepo \ | ||
| -c gpg.ssh.allowedSignersFile="${allowedSignersFile}" \ | ||
| -c gpg.program="gpgWithKeys" \ | ||
| verify-tag ${revWithTag} | ||
| fi | ||
|
|
||
| if test "$leaveDotGit" != 1; then | ||
| cp -r --no-preserve=all "${fetchresult}" $out | ||
| rm -rf "$out"/.git | ||
| else | ||
| ln "${fetchresult}" $out | ||
| fi | ||
| '' | ||
| ) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there should be documentation, probably here but definitely somewhere in these changes, to explain the need for the two-step verification. Something like the explanation at #330457 (comment) (notwithstanding my overall review comment) needs to be included in the repository itself, rather than needing someone to go looking for it in the PR discussions.