tools: log and check shasum of archive file #48088

fasenderos · 2023-05-20T15:48:18Z

As discussed here, there is the need to log the sha256sum for the external dependencies. With this PR in addition to log the checksum, we also check it's validity and eventually exit with an error. If the check is out of scope, of course I can just leave the log.

I started with nghttp2 and when everything is ok I continue with the other deps

Refs: nodejs/security-wg#973

marco-ippolito · 2023-05-22T08:47:32Z

Although it doesn't reduce the risk of downloading a tampered package to 0 (if someone changed the archive can also change the checksum to match) I see it a good practice!

@nodejs/security-wg

RafaelGSS

LGTM

fasenderos · 2023-05-23T12:34:13Z

Unfortunately GitHub doesn’t have built-in support for checksums in Releases (here and here), so is responsibility of the author include this information in the release notes. Currently only nghttp2 and icu-small (which already checks the checksum) have this information.

tniessen · 2023-05-23T12:56:49Z

Although it doesn't reduce the risk of downloading a tampered package to 0 (if someone changed the archive can also change the checksum to match) I see it a good practice!

To be clear, when I suggested logging checksums, it was not meant as a security mechanism. I only want them logged so that we can better audit our buggy update scripts. Checking is optional IMHO, sha256sum --tag "$FILE" is all I want.

tniessen

Nice work!

tools/dep_updaters/utils.sh

marco-ippolito · 2023-05-24T09:04:22Z

tools/dep_updaters/update-ada.sh

@@ -35,13 +38,14 @@ cleanup () {
 trap cleanup INT TERM EXIT

 ADA_REF="v$NEW_VERSION"
-ADA_ZIP="ada-$NEW_VERSION.zip"
+ADA_ZIP="ada-$ADA_REF.zip"


why this change? It doesnt seem related to the PR

For consistency with the other deps that logs the checksum in the format SHA256 (packagename-v1.0.0.zip) = abc123 for e.g. SHA256 (ada-v2.4.2.zip) = 1c1d96f27307b6f7bd21af87d10d528207f44e904f77a550837037e9def06607

tniessen · 2023-05-24T12:20:06Z

tools/dep_updaters/update-openssl.sh

+  OPENSSL_TARBALL="openssl-v$OPENSSL_VERSION.tar.gz"
+  curl -sL -o "$OPENSSL_TARBALL" "https://api.github.com/repos/quictls/openssl/tarball/openssl-$OPENSSL_VERSION"
+  log_and_verify_sha256sum "openssl" "$OPENSSL_TARBALL"
+  gzip -dc "$OPENSSL_TARBALL" | tar xf -


Out of curiosity, why the explicit gzip processes instead of tar z?

It's more portable. Some non-GNU versions of tar (e.g. on AIX) do not support -z.

I see. I assumed these would only have to work under ubuntu-latest compatible systems, but if we want to aim for portability with other systems, that makes sense. FWIW, the command I suggested (sha256sum --tag) also won't work with some non-GNU systems (yet I prefer it over the more portable output format).

marco-ippolito · 2023-05-25T09:29:17Z

pr for histogram dep_updater : #48171

nodejs-github-bot · 2023-05-25T19:46:09Z

Landed in 847b9e0

PR-URL: nodejs#48088 Refs: nodejs/security-wg#973 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

PR-URL: #48088 Refs: nodejs/security-wg#973 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

PR-URL: nodejs#48088 Refs: nodejs/security-wg#973 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label May 20, 2023

fasenderos marked this pull request as ready for review May 20, 2023 15:57

fasenderos marked this pull request as draft May 21, 2023 15:34

fasenderos force-pushed the log-shasum branch from 5f07023 to a876334 Compare May 22, 2023 11:56

marco-ippolito requested a review from tniessen May 22, 2023 12:05

RafaelGSS approved these changes May 22, 2023

View reviewed changes

marco-ippolito approved these changes May 23, 2023

View reviewed changes

fasenderos marked this pull request as ready for review May 23, 2023 12:38

tniessen reviewed May 24, 2023

View reviewed changes

tools/dep_updaters/utils.sh Outdated Show resolved Hide resolved

marco-ippolito reviewed May 24, 2023

View reviewed changes

marco-ippolito approved these changes May 24, 2023

View reviewed changes

tniessen reviewed May 24, 2023

View reviewed changes

fasenderos force-pushed the log-shasum branch from ea37d4c to a7b3b8d Compare May 25, 2023 07:13

tools: log and verify sha256sum

31fda6a

fasenderos force-pushed the log-shasum branch from a7b3b8d to 31fda6a Compare May 25, 2023 07:21

UlisesGascon approved these changes May 25, 2023

View reviewed changes

tniessen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels May 25, 2023

tniessen mentioned this pull request May 25, 2023

tools: automate histogram update #48171

Merged

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 25, 2023

nodejs-github-bot merged commit 847b9e0 into nodejs:main May 25, 2023

fasenderos deleted the log-shasum branch May 26, 2023 07:50

fasenderos restored the log-shasum branch May 27, 2023 09:55

fasenderos deleted the log-shasum branch May 27, 2023 10:30

targos pushed a commit that referenced this pull request May 30, 2023

tools: log and verify sha256sum

801573b

PR-URL: #48088 Refs: nodejs/security-wg#973 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

targos mentioned this pull request Jun 4, 2023

v20.3.0 release proposal #48332

Merged

danielleadams pushed a commit that referenced this pull request Jul 6, 2023

tools: log and verify sha256sum

5767844

PR-URL: #48088 Refs: nodejs/security-wg#973 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023

tools: log and verify sha256sum

24abe07

PR-URL: nodejs#48088 Refs: nodejs/security-wg#973 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

danielleadams mentioned this pull request Jul 10, 2023

v18.17.0 release proposal #48694

Merged

Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023

tools: log and verify sha256sum

600a0c0

PR-URL: nodejs#48088 Refs: nodejs/security-wg#973 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023

tools: log and verify sha256sum

baa9eb6

PR-URL: nodejs#48088 Refs: nodejs/security-wg#973 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: log and check shasum of archive file #48088

tools: log and check shasum of archive file #48088

fasenderos commented May 20, 2023 •

edited

Loading

marco-ippolito commented May 22, 2023 •

edited

Loading

RafaelGSS left a comment

fasenderos commented May 23, 2023 •

edited

Loading

tniessen commented May 23, 2023

tniessen left a comment

marco-ippolito May 24, 2023

fasenderos May 24, 2023

tniessen May 24, 2023

richardlau May 24, 2023

tniessen May 24, 2023

marco-ippolito commented May 25, 2023 •

edited

Loading

nodejs-github-bot commented May 25, 2023

tools: log and check shasum of archive file #48088

tools: log and check shasum of archive file #48088

Conversation

fasenderos commented May 20, 2023 • edited Loading

marco-ippolito commented May 22, 2023 • edited Loading

RafaelGSS left a comment

Choose a reason for hiding this comment

fasenderos commented May 23, 2023 • edited Loading

tniessen commented May 23, 2023

tniessen left a comment

Choose a reason for hiding this comment

marco-ippolito May 24, 2023

Choose a reason for hiding this comment

fasenderos May 24, 2023

Choose a reason for hiding this comment

tniessen May 24, 2023

Choose a reason for hiding this comment

richardlau May 24, 2023

Choose a reason for hiding this comment

tniessen May 24, 2023

Choose a reason for hiding this comment

marco-ippolito commented May 25, 2023 • edited Loading

nodejs-github-bot commented May 25, 2023

fasenderos commented May 20, 2023 •

edited

Loading

marco-ippolito commented May 22, 2023 •

edited

Loading

fasenderos commented May 23, 2023 •

edited

Loading

marco-ippolito commented May 25, 2023 •

edited

Loading