Add GPG signature verification to install script #7944

jdolitsky · 2020-04-17T18:19:25Z

The script fetches the KEYS file from GitHub, as well as the .asc files on the release and verifies the
release artifacts are signed by a valid key.

Added new boolean config options in the install script which allow for fine-grained control over verification and output:

DEBUG: sets -x in the bash script (default: false)
VERIFY_CHECKSUM: verifies checksum (default: true)
VERIFY_SIGNATURE: verifies signature (default: true)

Also reduced check for curl/wget to only one time.

Resolves #7943.
Resolves #7838.

The script fetches the KEYS file from GitHub, as well as the .asc files on the release and verifies the release artifacts are signed by a valid key. Added new boolean config options in the install script which allow for fine-grained control over verification and output: - DEBUG: sets -x in the bash script (default: false) - VERIFY_CHECKSUM: verifies checksum (default: true) - VERIFY_SIGNATURE: verifies signature (default: true) Also reduced check for curl/wget to only one time. Resolves helm#7943. Resolves helm#7838. Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com>

jdolitsky · 2020-04-17T18:28:44Z

I'm wondering if I should push the GPG output into /dev/null? Turns the output of the script from just 3 lines to ~30 and displays all the maintainers emails ... maybe only if DEBUG is enabled, show GPG output?

scripts/get-helm-3

Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com>

jdolitsky · 2020-04-17T19:12:28Z

I've silenced the GPG output unless DEBUG=true, now the output looks like this:

$ VERIFY_SIGNATURES=true scripts/get-helm-3
Downloading https://get.helm.sh/helm-v3.1.2-darwin-amd64.tar.gz
Verifying checksum... Done.
Verifying signatures... Done.
Preparing to install helm into /usr/local/bin
helm installed into /usr/local/bin/helm

Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com>

scripts/get-helm-3

Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com>

jdolitsky · 2020-04-22T18:08:01Z

Modified the script. No longer auto-imports KEYS file, just expects you've done this prior to running the script.

mattfarina · 2020-04-22T18:14:40Z

scripts/get-helm-3

+  if [ "${DEBUG}" == "true" ]; then
+    gpg_stderr_device="/dev/stderr"
+  fi
+  gpg --import "${HELM_TMP_ROOT}/${keys_filename}" 2> "${gpg_stderr_device}"


This is going to add the keys to the users active GPG keyring. The gpg verification below will use these keys plus all the other keys in the keyring to look for a good signature.

The gpg keys from the Helm project should not be imported into the users keyring. Instead, a keyring only containing the Helm maintainers should be created, used for verification, and deleted afterwards as part of cleanup. This script should not try to use keys in the users existing keyring and should not alter their environment.

The linux kernel script does this. You can look at it for ideas... https://git.kernel.org/pub/scm/linux/kernel/git/mricon/korg-helpers.git/tree/get-verified-tarball

This script should not try to use keys in the users existing keyring and should not alter their environment.

Alright. Thanks for the example script.

So then, should we modify this to take a filepath to a .asc file? Or auto-fetch KEYS file?

I think what you're saying is at odds with @bacongobbler 's suggestion?

yes and no. I wasn't aware that the process was to create a separate keyring as opposed to using your existing keyring, but I fully agree that these keys shouldn't be added to the user's keyring using either approach.

If that's the recommendation at the linux kernel, then that should work for us.

It's possible to have a separate keyring used for gpg verification calls. It's a couple steps to set it up. We should make it so the only keys in the keyring are those from the maintainers. This way the good verification can only come from that set of keys and not any key in the users personal keyring.

bacongobbler

One more nitpick, then this looks good!

><> export VERIFY_SIGNATURES=true
><> ./get-helm-3 
Helm v3.2.0 is available. Changing from version v3.1.2.
Downloading https://get.helm.sh/helm-v3.2.0-darwin-amd64.tar.gz
Verifying checksum... Done.
Verifying signatures... Done.
Preparing to install helm into /usr/local/bin
Password: ^C
Failed to install helm
	For support, go to https://github.com/helm/helm.

scripts/get-helm-3

Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com>

…ature

Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com>

jdolitsky · 2020-05-05T17:22:02Z

@bacongobbler @mattfarina this should be good to go now.

Running into an issue on Mac, which I will file once this is closed:

gpg: can't connect to the agent: IPC connect call failed

Appears to be fixed if the temp gpg home is somewhere under the users home? The ownership on the temp dir seem find.. Found some conversation about it here: https://sourceforge.net/p/macgpg/mailman/message/18850673/

So for now, I've put a check in place which requires Linux if using VERIFY_SIGNATURES=true (false by default)

For proof this isnt messing with the user's active keyring:

$ gpg --list-keys | grep ^pub | wc -l
0

$ curl -s https://keybase.io/jdolitsky/pgp_keys.asc | gpg --import 2>/dev/null

$ gpg --list-keys | grep ^pub | wc -l
1

$ VERIFY_SIGNATURES=true scripts/get-helm-3
Downloading https://get.helm.sh/helm-v3.2.0-linux-amd64.tar.gz
Verifying checksum... Done.
Verifying signatures... Done.
Preparing to install helm into /usr/local/bin
helm installed into /usr/local/bin/helm

$ gpg --list-keys | grep ^pub | wc -l
1

jdolitsky · 2020-05-05T17:23:43Z

scripts/get-helm-3

+  local num_goodlines_sha=$(gpg --verify --keyring="${gpg_keyring}" --status-fd=1 "${HELM_TMP_ROOT}/helm-${TAG}-${OS}-${ARCH}.tar.gz.sha256.asc" 2> "${gpg_stderr_device}" | grep -c -E '^\[GNUPG:\] (GOODSIG|VALIDSIG)')
+  if [[ ${num_goodlines_sha} -lt 2 ]]; then
+    echo "Unable to verify the signature of helm-${TAG}-${OS}-${ARCH}.tar.gz.sha256!"
+    echo -e "${error_text}"
+    exit 1
+  fi
+  local num_goodlines_tar=$(gpg --verify --keyring="${gpg_keyring}" --status-fd=1 "${HELM_TMP_ROOT}/helm-${TAG}-${OS}-${ARCH}.tar.gz.asc" 2> "${gpg_stderr_device}" | grep -c -E '^\[GNUPG:\] (GOODSIG|VALIDSIG)')
+  if [[ ${num_goodlines_tar} -lt 2 ]]; then
+    echo "Unable to verify the signature of helm-${TAG}-${OS}-${ARCH}.tar.gz!"
+    echo -e "${error_text}"
+    exit 1
+  fi
+  echo "Done."


this strange grep + line count check was taken directly from the linux kernel script https://git.kernel.org/pub/scm/linux/kernel/git/mricon/korg-helpers.git/tree/get-verified-tarball#n164

jdolitsky · 2020-06-27T16:17:59Z

@mattfarina @bacongobbler want to try to get this into the install script for 3.3?

bacongobbler · 2020-07-07T16:04:50Z

scripts/get-helm-3

+      exit 1
+    fi
+    if [ "${OS}" != "linux" ]; then
+      echo "Signature verification is currently only supported on Linux."


why only linux support when there's native gpg support for MacOS? Can you please elaborate what needs to change to add MacOS support?

bacongobbler

does not work. Please test your changes before asking for a review.

><> docker run -it ubuntu bash
root@c5a6147940e8:/# apt update && apt install wget git openssl gpg
root@c5a6147940e8:/# wget https://raw.githubusercontent.com/jdolitsky/helm/validate-signature/scripts/get-helm-3
root@c5a6147940e8:/# chmod 755 get-helm-3
root@c5a6147940e8:/# VERIFY_SIGNATURES=true ./get-helm-3 
Downloading https://get.helm.sh/helm-v3.2.4-linux-amd64.tar.gz
Verifying checksum... Done.
Verifying signatures... https://raw.githubusercontent.com/helm/helm/master/KEYS: No such file or directory
Failed to install helm
	For support, go to https://github.com/helm/helm.

Signed-off-by: jdolitsky <393494+jdolitsky@users.noreply.github.com>

jdolitsky · 2020-07-07T18:11:22Z

@bacongobbler - apologies. Was testing on a Linux system with curl for the 2nd revision and didn't catch the bad syntax with wget commands. I just updated and tested it on system with just curl, and system with just wget

Regarding Mac support - it's not clear to me what the issue is. Here is the output when using DEBUG=true:

...
+ gpg --batch --quiet --homedir=/var/folders/lj/xzzflpn14bg1vd8f2c5q_v500000gn/T/helm-installer-XXXXXX.JjFTM6Vw/gnupg --import /var/folders/lj/xzzflpn14bg1vd8f2c5q_v500000gn/T/helm-installer-XXXXXX.JjFTM6Vw/KEYS
gpg: can't connect to the agent: IPC connect call failed
gpg: can't connect to the agent: IPC connect call failed
gpg: can't connect to the agent: IPC connect call failed
gpg: can't connect to the agent: IPC connect call failed
gpg: can't connect to the agent: IPC connect call failed
gpg: can't connect to the agent: IPC connect call failed
+ fail_trap
+ result=2
+ '[' 2 '!=' 0 ']'
+ [[ -n '' ]]
+ echo 'Failed to install helm'
Failed to install helm
...

It appears to hang on can't connect to the agent. Was unable to successfully find something on the internet to resolve this without launching a GPG agent (which I don't think the script should do).

If I modify HELM_TMP_ROOT to be instead ${HOME}/helm-install (vs. $(mktemp -dt helm-installer-XXXXXX)) on line 141, things seem to work just fine:

...
+ gpg --batch --quiet --homedir=/Users/jdolitsky/helm-install/gnupg --import /Users/jdolitsky/helm-install/KEYS
+ gpg --batch --no-default-keyring --keyring /Users/jdolitsky/helm-install/gnupg/pubring.kbx --export
+ local github_release_url=https://github.com/helm/helm/releases/download/v3.2.4
+ '[' true == true ']'
+ curl -SsL https://github.com/helm/helm/releases/download/v3.2.4/helm-v3.2.4-darwin-amd64.tar.gz.sha256.asc -o /Users/jdolitsky/helm-install/helm-v3.2.4-darwin-amd64.tar.gz.sha256.asc
...

So it's something related to using mktemp. When I inspect these temp directories, they appear to be fully owned by my user:

+ HELM_TMP_ROOT=/var/folders/lj/xzzflpn14bg1vd8f2c5q_v500000gn/T/helm-installer-XXXXXX.hGNuwU8u
+ ls -la /var/folders/lj/xzzflpn14bg1vd8f2c5q_v500000gn/T/helm-installer-XXXXXX.hGNuwU8u
total 0
drwx------    2 jdolitsky  staff    64 Jul  7 13:08 .
drwx------@ 136 jdolitsky  staff  4352 Jul  7 13:08 ..

I could definitely use some help figuring this out. But I do think this PR still provides value for Linux users, especially in the context of CI etc.

bacongobbler

re-tested using the same instructions. Works now. I think this is in a workable state to move forward. We can address the MacOS issue in a follow-up.

* Add GPG signature verification to install script The script fetches the KEYS file from GitHub, as well as the .asc files on the release and verifies the release artifacts are signed by a valid key. Added new boolean config options in the install script which allow for fine-grained control over verification and output: - DEBUG: sets -x in the bash script (default: false) - VERIFY_CHECKSUM: verifies checksum (default: true) - VERIFY_SIGNATURE: verifies signature (default: true) Also reduced check for curl/wget to only one time. Resolves helm#7943. Resolves helm#7838. Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com> * disable signature verification by default Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com> * remove repeated line Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com> * fix typo Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com> * do not auto-import GPG keys Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com> * silence errors about missing commands Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com> * use a temporary gpg keyring Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com> * Fix wget commands for VERIFY_SIGNATURES=true Signed-off-by: jdolitsky <393494+jdolitsky@users.noreply.github.com>

jdolitsky requested a review from mattfarina April 17, 2020 18:19

helm-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 17, 2020

bacongobbler reviewed Apr 17, 2020

View reviewed changes

scripts/get-helm-3 Outdated Show resolved Hide resolved

jdolitsky added 2 commits April 17, 2020 14:07

Merge branch 'master' into validate-signature

d79752b

disable signature verification by default

5f721c9

Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com>

jdolitsky force-pushed the validate-signature branch from f146f9c to 5f721c9 Compare April 17, 2020 19:09

jdolitsky added 2 commits April 22, 2020 10:25

remove repeated line

88dbe15

Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com>

Merge branch 'master' of github.com:helm/helm into validate-signature

ff562e1

bacongobbler requested changes Apr 22, 2020

View reviewed changes

scripts/get-helm-3 Outdated Show resolved Hide resolved

scripts/get-helm-3 Outdated Show resolved Hide resolved

jdolitsky added 2 commits April 22, 2020 12:58

fix typo

2e45800

Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com>

do not auto-import GPG keys

420da12

Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com>

mattfarina requested changes Apr 22, 2020

View reviewed changes

bacongobbler requested changes Apr 22, 2020

View reviewed changes

scripts/get-helm-3 Outdated Show resolved Hide resolved

jdolitsky added 3 commits April 22, 2020 13:22

silence errors about missing commands

f8356f3

Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com>

Merge branch 'master' of github.com:jdolitsky/helm into validate-sign…

21ba0c9

…ature

use a temporary gpg keyring

ff32bba

Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com>

jdolitsky commented May 5, 2020

View reviewed changes

bacongobbler reviewed Jul 7, 2020

View reviewed changes

bacongobbler requested changes Jul 7, 2020

View reviewed changes

jdolitsky added 2 commits July 7, 2020 12:48

Merge branch 'master' of github.com:helm/helm into validate-signature

2e6d865

Fix wget commands for VERIFY_SIGNATURES=true

3cb208f

Signed-off-by: jdolitsky <393494+jdolitsky@users.noreply.github.com>

bacongobbler approved these changes Sep 4, 2020

View reviewed changes

bacongobbler mentioned this pull request Sep 4, 2020

github: add Asset Transparency action for GitHub releases #8697

Merged

3 tasks

jdolitsky merged commit 6898ad1 into helm:master Sep 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GPG signature verification to install script #7944

Add GPG signature verification to install script #7944

jdolitsky commented Apr 17, 2020

jdolitsky commented Apr 17, 2020

jdolitsky commented Apr 17, 2020

jdolitsky commented Apr 22, 2020

mattfarina Apr 22, 2020

jdolitsky Apr 22, 2020

jdolitsky Apr 22, 2020

bacongobbler Apr 22, 2020 •

edited

mattfarina Apr 22, 2020

bacongobbler left a comment •

edited

jdolitsky commented May 5, 2020

jdolitsky May 5, 2020

jdolitsky commented Jun 27, 2020

bacongobbler Jul 7, 2020

bacongobbler left a comment •

edited

jdolitsky commented Jul 7, 2020

bacongobbler left a comment

Add GPG signature verification to install script #7944

Add GPG signature verification to install script #7944

Conversation

jdolitsky commented Apr 17, 2020

jdolitsky commented Apr 17, 2020

jdolitsky commented Apr 17, 2020

jdolitsky commented Apr 22, 2020

mattfarina Apr 22, 2020

Choose a reason for hiding this comment

jdolitsky Apr 22, 2020

Choose a reason for hiding this comment

jdolitsky Apr 22, 2020

Choose a reason for hiding this comment

bacongobbler Apr 22, 2020 • edited

Choose a reason for hiding this comment

mattfarina Apr 22, 2020

Choose a reason for hiding this comment

bacongobbler left a comment • edited

Choose a reason for hiding this comment

jdolitsky commented May 5, 2020

jdolitsky May 5, 2020

Choose a reason for hiding this comment

jdolitsky commented Jun 27, 2020

bacongobbler Jul 7, 2020

Choose a reason for hiding this comment

bacongobbler left a comment • edited

Choose a reason for hiding this comment

jdolitsky commented Jul 7, 2020

bacongobbler left a comment

Choose a reason for hiding this comment

bacongobbler Apr 22, 2020 •

edited

bacongobbler left a comment •

edited

bacongobbler left a comment •

edited