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.

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

tools: fix release URL computation in update-root-certs.mjs #56843

Merged
merged 1 commit into from
Feb 14, 2025
Merged

tools: fix release URL computation in update-root-certs.mjs #56843

merged 1 commit into from
Feb 14, 2025

Conversation

joyeecheung
Copy link
Member

Previously this would compute the release tag to be something like FIREFOX_134_0.2_RELEASE which would not lead to a valid URL, failing to pull the latest NSS updates from the Firefox release. It should replace all the dots with underscores to compute something like FIREFOX_134_0_2_RELEASE instead.

Before when I ran it locally:

Fetching Firefox release data from https://nucleus.mozilla.org/rna/all-releases.json.
Fetching NSS tag from https://hg.mozilla.org/releases/mozilla-release/raw-file/FIREFOX_134_0.2_RELEASE/security/nss/TAG-INFO.
Failed to fetch https://hg.mozilla.org/releases/mozilla-release/raw-file/FIREFOX_134_0.2_RELEASE/security/nss/TAG-INFO: 404: Not Found

After:

Fetching Firefox release data from https://nucleus.mozilla.org/rna/all-releases.json.
Fetching NSS tag from https://hg.mozilla.org/releases/mozilla-release/raw-file/FIREFOX_134_0_2_RELEASE/security/nss/TAG-INFO.
Found tag NSS_3_107_RTM.
Updating to NSS version 3.107
Fetching https://raw.githubusercontent.com/nss-dev/nss/refs/tags/NSS_3_107_RTM/lib/ckfw/builtins/certdata.txt

# logs from actually parsing and computing the update

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/security-wg

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Jan 31, 2025
@richardlau
Copy link
Member

FWIW the failing GHA workflow is #56063 (comment).

@joyeecheung
Copy link
Member Author

By the way I wonder what we think about migrating away from tools/mk-ca-bundle.pl, I am thinking about updating it to output the certificate data in octal literals in #56832 to skip the unnecessary serdes cost, but then if we are changing it substantially, we might as well just rewrite it in JavaScript instead of invoking a Perl script from JavaScript (and the Perl script already has some modifications from our side, like omitting TrustCor CAs)

@richardlau
Copy link
Member

By the way I wonder what we think about migrating away from tools/mk-ca-bundle.pl, I am thinking about updating it to output the certificate data in octal literals in #56832 to skip the unnecessary serdes cost, but then if we are changing it substantially, we might as well just rewrite it in JavaScript instead of invoking a Perl script from JavaScript (and the Perl script already has some modifications from our side, like omitting TrustCor CAs)

I think if we're not planning to resync to upstream curl's version of the tool at any point in the future (I think it was tried once and abandoned) then rewriting in something other than Perl would be a plus.

@richardlau
Copy link
Member

By the way I wonder what we think about migrating away from tools/mk-ca-bundle.pl, I am thinking about updating it to output the certificate data in octal literals in #56832 to skip the unnecessary serdes cost, but then if we are changing it substantially, we might as well just rewrite it in JavaScript instead of invoking a Perl script from JavaScript (and the Perl script already has some modifications from our side, like omitting TrustCor CAs)

I think if we're not planning to resync to upstream curl's version of the tool at any point in the future (I think it was tried once and abandoned) then rewriting in something other than Perl would be a plus.

Maybe this discussion should be an issue to itself. FWIW https://blog.mozilla.org/security/2021/05/10/beware-of-applications-misusing-root-stores/ recommends https://www.ccadb.org/resources rather than parsing certdata.txt.

@joyeecheung
Copy link
Member Author

joyeecheung commented Feb 7, 2025 •
edited
Loading

There is an old issue about the storing as DER idea #45768 - I added a comment to reference the conversations here.

@joyeecheung joyeecheung mentioned this pull request Feb 7, 2025
Open
@bnoordhuis
Copy link
Member

The actual download is https://ccadb.my.salesforce-sites.com/mozilla/IncludedRootsPEMTxt?TrustBitsInclude=Websites which is a domain name that doesn't exactly instill a warm fuzzy sense of security.

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 12, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 12, 2025
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65165/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65177/

@joyeecheung
Copy link
Member Author

The Linux GH action is failing in a c++ compliation error, which I think requires a rebase? Not sure if the checkout action does it..

@joyeecheung
tools: fix release URL computation in update-root-certs.mjs
a0f48fa 
Previously this would compute the release tag to be something
like FIREFOX_134_0.2_RELEASE which would not lead to a valid
URL, failing to pull the latest NSS updates from the Firefox
release. It should replace all the dots with underscores to
compute something like FIREFOX_134_0_2_RELEASE instead.
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 13, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 13, 2025
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65185/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65200/

@richardlau richardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 14, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 14, 2025
@nodejs-github-bot nodejs-github-bot merged commit 1e6a656 into nodejs:main Feb 14, 2025
54 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 1e6a656

targos pushed a commit that referenced this pull request Feb 17, 2025
@joyeecheung @targos
tools: fix release URL computation in update-root-certs.mjs
1efd74b 
Previously this would compute the release tag to be something
like FIREFOX_134_0.2_RELEASE which would not lead to a valid
URL, failing to pull the latest NSS updates from the Firefox
release. It should replace all the dots with underscores to
compute something like FIREFOX_134_0_2_RELEASE instead.

PR-URL: #56843
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ulises Gascón <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@UlisesGascon UlisesGascon UlisesGascon approved these changes

@richardlau richardlau richardlau approved these changes

Assignees
No one assigned
Labels
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.
7 participants
@joyeecheung @nodejs-github-bot @richardlau @bnoordhuis @jasnell @lpinca @UlisesGascon