Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tools: log and check shasum of archive file #48088

Merged
merged 1 commit into from
May 25, 2023

Conversation

fasenderos
Copy link
Contributor

@fasenderos fasenderos commented May 20, 2023

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

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label May 20, 2023
@fasenderos fasenderos marked this pull request as ready for review May 20, 2023 15:57
@fasenderos fasenderos marked this pull request as draft May 21, 2023 15:34
@marco-ippolito
Copy link
Member

marco-ippolito commented May 22, 2023

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

Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fasenderos
Copy link
Contributor Author

fasenderos commented May 23, 2023

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.

@fasenderos fasenderos marked this pull request as ready for review May 23, 2023 12:38
@tniessen
Copy link
Member

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.

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

tools/dep_updaters/utils.sh Outdated Show resolved Hide resolved
@@ -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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change? It doesnt seem related to the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 -
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

marco-ippolito commented May 25, 2023

pr for histogram dep_updater : #48171

@tniessen 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
@nodejs-github-bot 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 nodejs-github-bot merged commit 847b9e0 into nodejs:main May 25, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 847b9e0

@fasenderos fasenderos deleted the log-shasum branch May 26, 2023 07:50
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request May 26, 2023
PR-URL: nodejs#48088
Refs: nodejs/security-wg#973
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
@fasenderos fasenderos restored the log-shasum branch May 27, 2023 09:55
@fasenderos fasenderos deleted the log-shasum branch May 27, 2023 10:30
targos pushed a commit that referenced this pull request May 30, 2023
PR-URL: #48088
Refs: nodejs/security-wg#973
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
@targos targos mentioned this pull request Jun 4, 2023
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
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
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
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
PR-URL: nodejs#48088
Refs: nodejs/security-wg#973
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants