security-wg icon indicating copy to clipboard operation
security-wg copied to clipboard

Automate icu data updates?

Open mhdawson opened this issue 3 years ago • 3 comments

PR's like this are really hard to validate and should probably be done through automation.

https://github.com/nodejs/node/pull/44283

@RafaelGSS,@facutuesca is that something you could add to your do list?

mhdawson avatar Aug 26 '22 19:08 mhdawson

FYI if you are not aware there is a PR to add a workflow for updating the timezone information: https://github.com/nodejs/node/pull/43988.

Updating ICU itself is sometimes done together with V8 updates if V8 has bumped ICU versions.

richardlau avatar Aug 26 '22 20:08 richardlau

@richardlau thanks for pointing that out.

@RafaelGSS, @facutuesca maybe what we should focus on is looking at all of the dependencies, if upates are automated and if not identify which ones should be, and prioritize which ones we'd want to automate. (For example I think we had some discusssions around openSSL, but I think we should track/work on them as an overall program to ensure progress).

If that makes sense to you two I might update the title of this issue to be more about doing it in general for all of the dependencies.

mhdawson avatar Aug 31 '22 17:08 mhdawson

That makes sense to me.

RafaelGSS avatar Aug 31 '22 19:08 RafaelGSS

@mhdawson I'll start working on this (looking at all dependencies and see which ones we could update with a script). Should we change the title of the issue to match that?

facutuesca avatar Oct 27 '22 09:10 facutuesca

@facutuesca thanks, I've updated the title.

mhdawson avatar Oct 27 '22 20:10 mhdawson

I updated the first part of the issue to have the list of deps along with checkboxes. We can track progress there in terms of which ones we have automated versus not so far.

mhdawson avatar Oct 27 '22 20:10 mhdawson

@BethGriggs if you have any insight/suggestions of what we might need/want to include in the automation based on what you have leared about SALSA, that info would be good to factor into how we do the automation.

mhdawson avatar Oct 27 '22 20:10 mhdawson

@mhdawson

The following dependencies are already updated automatically via a Github action:

The following have a script + docs on how to update them (but no GH Action):

The following have only docs on how to update them:

Finally, the following don't have any docs/scripts/etc:

  • acorn
  • brotli
  • googletest
  • histogram
  • uv

facutuesca avatar Nov 02 '22 10:11 facutuesca

I'd go with the ones that are often updated, such as OpenSSL / ICU / zlib

RafaelGSS avatar Nov 02 '22 12:11 RafaelGSS

ICU doesn't update very often (about once a year), although we do now have an automated workflow for updating the timezone information (which updates more often) in the ICU data file.

I believe the npm team have their own automation to push npm releases into Node.js core.

richardlau avatar Nov 02 '22 13:11 richardlau

I'd agree that starting with the ones we update most often would be good, and in particular OpenSSL. I think we have a few starting points. I'd written up https://github.com/nodejs/node/issues/42395 and I think that @RafaelGSS had also done some work on that front as well.

I also think that working on the list for which we have no instructions is also a priority as I see not having that documented as a risk we might get it wrong if we do have to do an update.

mhdawson avatar Nov 02 '22 13:11 mhdawson

@facutuesca and thanks for the good categorization, it's good to be able to look at the overall list like that.

mhdawson avatar Nov 02 '22 13:11 mhdawson

Root certificates is another thing we could add to the list: https://github.com/nodejs/node/issues/45477 Update process: https://github.com/nodejs/node/blob/main/doc/contributing/maintaining-root-certs.md

FWIW Adoptium have some automation for something similar for Termurin Java builds (thanks @sxa for the pointer 🙇): https://github.com/adoptium/temurin-build/blob/master/.github/workflows/ca-cert-updater.yml I believe their version of mk-ca-bundle.pl differs from ours and they also have less information in their commit messages regarding the certificates removed/added or NSS version the update is based on.

richardlau avatar Nov 15 '22 18:11 richardlau

@richardlau thanks for pointing that out. I think starting with automating the root cert updates would be a good thing to start with.

mhdawson avatar Nov 15 '22 18:11 mhdawson

So far we have added:

  • acorn: https://github.com/nodejs/node/pull/45357
  • base64: https://github.com/nodejs/node/pull/45300
  • libuv: https://github.com/nodejs/node/pull/45362

Currently on review:

  • OpenSSL: https://github.com/nodejs/node/pull/45605

cc @RafaelGSS

facutuesca avatar Dec 08 '22 15:12 facutuesca

@facutuesca thanks for progressing this and the update.

mhdawson avatar Dec 16 '22 19:12 mhdawson

we can update the list since nghttp2 is now updated with action with this pr https://github.com/nodejs/node/pull/46700

marco-ippolito avatar Feb 21 '23 17:02 marco-ippolito

@marco-ippolito I've selected llhttp as completed, but IIRC we use a different major of llhttp on v16/v14, so this automation wouldn't solve all the lines.

Maybe bring this discussion up in the next security-wg call? We might find some ideas.

Note, the case of llhttp happens to OpenSSL and I assume to other libraries as well.

RafaelGSS avatar Feb 27 '23 12:02 RafaelGSS

Let's discuss about it in the next security-wg I'm happy to work on it

marco-ippolito avatar Feb 27 '23 15:02 marco-ippolito

feel free to edit if I made a mistake update on dependecy status:

The following dependencies are already updated automatically via a Github action:

  • eslint
  • undici
  • cares
  • corepack
  • llhttp
  • nghttp2
  • npm
  • base64
  • ada
  • brotli
  • uv
  • acorn
  • ngtcp2
  • OpenSSL
  • zlib
  • cjs-module-lexer
  • googletest
  • uvwasi
  • V8 (patch)

The following have only docs on how to update them:

  • icu-small

Finally, the following don't have any docs/scripts/etc:

  • histogram (version header file missing I've opened a PR to include it)

marco-ippolito avatar Mar 21 '23 18:03 marco-ippolito

Root certificates is another thing we could add to the list: nodejs/node#45477 Update process: https://github.com/nodejs/node/blob/main/doc/contributing/maintaining-root-certs.md

Opened a PR for updating the root certificates: https://github.com/nodejs/node/pull/47425

richardlau avatar Apr 05 '23 19:04 richardlau

@richardlau thanks! Great to see that automated.

mhdawson avatar Apr 05 '23 19:04 mhdawson

Hi everyone! As @marco-ippolito suggested, I'm currently working into uvwasi automation

Ranieri93 avatar Apr 06 '23 15:04 Ranieri93

UPDATE from #945

  • [x] Create a maintaining-overview.md

RafaelGSS avatar Apr 13 '23 14:04 RafaelGSS

histogram (version header file missing I've opened a PR to include it)

FWIW, the version header file is not necessary. You can take a look at my approach from https://github.com/nodejs/node/pull/47482, which uses some git magic to figure out if anything changed between the latest upstream version and what's in the node repository. It doesn't actually matter what version is in the node repo. The only difference to https://github.com/nodejs/node/pull/47482 is that for googletest, we follow HEAD, whereas for HdrHistogram_c we probably want to use the latest published release.

tniessen avatar Apr 19 '23 14:04 tniessen

Updated by github action:

  • acorn
  • ada
  • base64
  • brotli
  • c-ares
  • cjs-module-lexer
  • corepack
  • googletest
  • icu-small
  • llhttp
  • minimatch
  • nghttp2
  • ngtcp2
  • npm
  • openssl
  • postject
  • simdutf
  • undici
  • uv
  • uvwasi
  • V8
  • zlib
  • icu-small (wip: https://github.com/nodejs/node/pull/47727)

Finally, the following don't have any docs/scripts/etc:

  • histogram (version header file missing I've opened a PR to include it)

PR for dependencies overview: https://github.com/nodejs/node/pull/47589

One left to go 🥳

marco-ippolito avatar Apr 26 '23 14:04 marco-ippolito

There appears to be an issue with nghttp3 updates, see https://github.com/nodejs/node/pull/47576#issuecomment-1537382159.

tniessen avatar May 12 '23 09:05 tniessen

Last dependency update automation https://github.com/nodejs/node/pull/48171

marco-ippolito avatar May 25 '23 09:05 marco-ippolito

and it's merged 🎉🎉🎉🎉🎉🎉 all the dependencies are automated 🥳

marco-ippolito avatar May 30 '23 08:05 marco-ippolito

Bravo!!

UlisesGascon avatar May 30 '23 09:05 UlisesGascon