Skip to content

tools: log and check shasum of archive file#48088

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.

Sign up for GitHub

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

Jump to bottom
Merged
nodejs-github-bot merged 1 commit into from
May 25, 2023
Merged

tools: log and check shasum of archive file #48088

nodejs-github-bot merged 1 commit into from
May 25, 2023

Conversation

@fasenderos
Copy link
Contributor

@fasenderosfasenderos commented May 20, 2023•
edited
Loading

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-botnodejs-github-bot added the tools Issues and PRs related to the tools directory. label May 20, 2023
@fasenderosfasenderos marked this pull request as ready for review May 20, 2023 15:57
@fasenderosfasenderos marked this pull request as draft May 21, 2023 15:34
@marco-ippolito
Copy link
Member

marco-ippolito commented May 22, 2023•
edited
Loading

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
RafaelGSS approved these changes May 22, 2023
View reviewed changes
Copy link
Member

@RafaelGSSRafaelGSS 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
ContributorAuthor

fasenderos commented May 23, 2023•
edited
Loading

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.

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

tniessen commented May 23, 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!

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
tniessen reviewed May 24, 2023
View reviewed changes
Copy link
Member

@tniessentniessen 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!

marco-ippolito
marco-ippolito reviewed May 24, 2023
View reviewed changes
tools/dep_updaters/update-ada.sh

ADA_REF="v$NEW_VERSION"
ADA_ZIP="ada-$NEW_VERSION.zip"
ADA_ZIP="ada-$ADA_REF.zip"
Copy link
Member

@marco-ippolitomarco-ippolitoMay 24, 2023

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
ContributorAuthor

@fasenderosfasenderosMay 24, 2023

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

tniessen
tniessen reviewed May 24, 2023
View reviewed changes
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 -
Copy link
Member

@tniessentniessenMay 24, 2023

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

@richardlaurichardlauMay 24, 2023

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

@tniessentniessenMay 24, 2023

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•
edited
Loading

pr for histogram dep_updater : #48171

@tniessentniessen 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
@tniessentniessen mentioned this pull request May 25, 2023
Merged
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 25, 2023
@nodejs-github-botnodejs-github-bot merged commit 847b9e0 into nodejs:mainMay 25, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 25, 2023

Landed in 847b9e0

@fasenderosfasenderos 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
@fasenderos@marco-ippolito
tools: log and verify sha256sum
163922e
PR-URL: nodejs#48088 Refs: nodejs/security-wg#973 Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
@fasenderosfasenderos restored the log-shasum branch May 27, 2023 09:55
@fasenderosfasenderos deleted the log-shasum branch May 27, 2023 10:30
targos pushed a commit that referenced this pull request May 30, 2023
@fasenderos@targos
tools: log and verify sha256sum
801573b
PR-URL: #48088 Refs: nodejs/security-wg#973 Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
@targostargos mentioned this pull request Jun 4, 2023
Merged
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@fasenderos@danielleadams
tools: log and verify sha256sum
5767844
PR-URL: #48088 Refs: nodejs/security-wg#973 Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
@fasenderos@MoLow
tools: log and verify sha256sum
24abe07
PR-URL: nodejs#48088 Refs: nodejs/security-wg#973 Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
@danielleadamsdanielleadams mentioned this pull request Jul 10, 2023
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@fasenderos@Ceres6
tools: log and verify sha256sum
600a0c0
PR-URL: nodejs#48088 Refs: nodejs/security-wg#973 Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@fasenderos@Ceres6
tools: log and verify sha256sum
baa9eb6
PR-URL: nodejs#48088 Refs: nodejs/security-wg#973 Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tniessentniessentniessen left review comments

@richardlaurichardlaurichardlau left review comments

@UlisesGasconUlisesGasconUlisesGascon approved these changes

@RafaelGSSRafaelGSSRafaelGSS approved these changes

@marco-ippolitomarco-ippolitomarco-ippolito approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.toolsIssues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@fasenderos@marco-ippolito@tniessen@nodejs-github-bot@UlisesGascon@richardlau@RafaelGSS