GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — tools: log and check shasum of archive file by fasenderos · Pull Request #48088 · nodejs/node

Conversation

@fasenderos

@fasenderos fasenderos commented May 20, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
marco-ippolito reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@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

marco-ippolito commented May 22, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
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!

@nodejs/security-wg
fasenderos reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@fasenderos
fasenderos force-pushed the log-shasum branch from 5f07023 to a876334 Compare May 22, 2023 11:56
@marco-ippolito
marco-ippolito requested a review from tniessen May 22, 2023 12:05
RafaelGSS
RafaelGSS approved these changes May 22, 2023
View reviewed changes

@RafaelGSS RafaelGSS left a comment

Copy link
Copy Markdown
Member

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.

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@fasenderos

fasenderos commented May 23, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
marco-ippolito
marco-ippolito approved these changes May 23, 2023
View reviewed changes
@fasenderos
fasenderos marked this pull request as ready for review May 23, 2023 12:38
@tniessen

tniessen commented May 23, 2023

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tniessen
tniessen reviewed May 24, 2023
View reviewed changes

@tniessen tniessen left a comment

Copy link
Copy Markdown
Member

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.

Nice work!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread tools/dep_updaters/utils.sh Resolved

Uh oh!

There was an error while loading. Please reload this page.
marco-ippolito
marco-ippolito reviewed May 24, 2023
View reviewed changes
Comment thread tools/dep_updaters/update-ada.sh Outdated

@marco-ippolito marco-ippolito May 24, 2023

Copy link
Copy Markdown
Member

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.

why this change? It doesnt seem related to the PR

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@fasenderos fasenderos May 24, 2023

Copy link
Copy Markdown
Contributor Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tniessen reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
marco-ippolito
marco-ippolito approved these changes May 24, 2023
View reviewed changes
tniessen
tniessen reviewed May 24, 2023
View reviewed changes
Comment thread tools/dep_updaters/update-openssl.sh Outdated

@tniessen tniessen May 24, 2023

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@richardlau richardlau May 24, 2023

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@tniessen tniessen May 24, 2023

Copy link
Copy Markdown
Member

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 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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@fasenderos
fasenderos force-pushed the log-shasum branch from ea37d4c to a7b3b8d Compare May 25, 2023 07:13
@fasenderos
tools: log and verify sha256sum
31fda6a
@fasenderos
fasenderos force-pushed the log-shasum branch from a7b3b8d to 31fda6a Compare May 25, 2023 07:21
@marco-ippolito

marco-ippolito commented May 25, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

pr for histogram dep_updater : #48171
fasenderos reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
UlisesGascon
UlisesGascon approved these changes May 25, 2023
View reviewed changes
@tniessen tniessen added author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue PRs queued for automated landing through the Commit Queue. labels May 25, 2023
@tniessen tniessen mentioned this pull request May 25, 2023
tools: automate histogram update #48171
Merged
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label May 25, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 847b9e0 into nodejs:main May 25, 2023
@nodejs-github-bot

nodejs-github-bot commented May 25, 2023

Copy link
Copy Markdown
Collaborator

Landed in 847b9e0
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@fasenderos
fasenderos deleted the log-shasum branch May 26, 2023 07:50
@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
@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]>
@targos 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
@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]>
@danielleadams 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
@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 free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tniessen tniessen tniessen left review comments
@richardlau richardlau richardlau left review comments
@UlisesGascon UlisesGascon UlisesGascon approved these changes
@RafaelGSS RafaelGSS RafaelGSS approved these changes
@marco-ippolito marco-ippolito marco-ippolito approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. tools Issues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@fasenderos @marco-ippolito @tniessen @nodejs-github-bot @UlisesGascon @richardlau @RafaelGSS
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.