Add automation for updating `OpenSSL` dependency #45605

facutuesca · 2022-11-24T14:23:36Z

Add a Github Action that checks for new versions of the OpenSSL library, and creates a PR to update it if a newer version than the one present in the repo is found.

Refs: nodejs/security-wg#828

nodejs-github-bot · 2022-11-24T14:23:41Z

Review requested:

@nodejs/actions

facutuesca · 2022-11-24T14:23:59Z

cc @RafaelGSS @mhdawson

mhdawson · 2022-12-01T19:50:08Z

tools/dep_updaters/README.md

+1. Check the `CHANGES.md` file in the [repo](https://github.com/quictls/openssl)
+   for things that might require changes in Node.js.
+2. Check the diffs to ensure the changes are right. Even if there are no changes
+   in the source, `buildinf.h` files will be updated because they have timestamp


Not related to this PR, but we should add a task somwhere to see if we can address this. Having it be more repeatable would be good.

mhdawson · 2022-12-01T19:52:49Z

tools/dep_updaters/update-openssl.sh

+  exit 1
+fi
+
+command -v perl >/dev/null 2>&1 || { echo >&2 "Error: 'Perl' required but not installed."; exit 1; }


again likely a follow on, but I wonder if we need to pin/capture what versions of perl etc was used. The same might be asked about the environment/os level in the action but. Not a blocker for this PR as we don't specify/fix those when people are running manually today.

mhdawson

LGTM

RafaelGSS · 2022-12-02T01:09:04Z

I'm adding the labels to not backport it to versions that use openssl 1

RafaelGSS

I'll review it properly tomorrow, but I believe is better to have a workflow_dispatch instead. At least I think we'll have more control on those updates. Wdyt @mhdawson?

richardlau · 2022-12-02T01:15:34Z

This will only create one commit instead of two? I don't know if that will make the PR harder to review.

facutuesca · 2022-12-06T14:38:14Z

This will only create one commit instead of two? I don't know if that will make the PR harder to review.

Are the changes manually reviewed? Since the update consists a copy-paste of an entire repo followed by regenerating config files programatically, the resulting PRs are pretty big. For example, the latest update in this PR #45309 has changes in 800 files, with +15,840-5,825 lines changed.

I ask because the current CI workflow for creating PRs to update dependencies assume that all dependencies will be updated using a single commit. If there's value in having two commits for OpenSSL we can do it, but it will require modifying that workflow to have a special case for OpenSSL separate from the rest of the dependencies.

RafaelGSS

If there's value in having two commits for OpenSSL we can do it, but it will require modifying that workflow to have a special case for OpenSSL separate from the rest of the dependencies.

There's a value in two commits. The second commit is arch build and normally some errors are caught in this phase (for example, nasm build issue - mentioned in the maintaining guide) so you just need to re-do the second one.

mhdawson · 2022-12-16T19:45:49Z

I'll review it properly tomorrow, but I believe is better to have a workflow_dispatch instead. At least I think we'll have more control on those updates. Wdyt @mhdawson?

@RafaelGSS do you mean we should have workflow_dispatch and then trigger it manually, or just have that as another option for triggering and update?

RafaelGSS · 2022-12-16T19:50:15Z

I'll review it properly tomorrow, but I believe is better to have a workflow_dispatch instead. At least I think we'll have more control on those updates. Wdyt @mhdawson?

@RafaelGSS do you mean we should have workflow_dispatch and then trigger it manually, or just have that as another option for triggering and update?

Yes, the first option. Triggering it manually through workflow_dispatch.

mhdawson · 2022-12-16T19:52:54Z

I think having the ability to trigger it manually would be good, but having a PR auto generated and available when there is a new release seems useful, provided it does not require manual work most of the time. I think for OpenSSL we probably want to try to keep as up to date as possible in the main branch.

RafaelGSS

LGTM. I think we are good to go. As discussed previously in a SecurityWG meeting, adding the support for two commits will be a bit out-of-scope.

Therefore, I'm fine moving forward with it, and if needed, we include support for multiple commits.

mhdawson · 2022-12-16T21:11:48Z

The current documentation does say to land as 2 commits so unless its hard/lots of work to do that I'm thinking we should do that now versus later. @richardlau what's your take?

richardlau · 2022-12-16T21:21:02Z

The current documentation does say to land as 2 commits so unless its hard/lots of work to do that I'm thinking we should do that now versus later. @richardlau what's your take?

My take is that the conservative option is the status quo -- two commits. On one hand that allows people to validate the first commit -- you should be able to get the exact same files. Keeping that property may be helpful in the future in terms of potential work around supply chain/verifying our dependencies.

The second commit has timestamps and local directories written into it which makes it harder to compare/validate.

On the other hand the two commit approach has always been the exception to the

All commits should be self-contained, meaning every commit should pass all tests.

guidance in our collaborator guide.

mhdawson · 2022-12-16T22:04:59Z

My take is that the conservative option is the status quo -- two commits. On one hand that allows people to validate the first commit -- you should be able to get the exact same files. Keeping that property may be helpful in the future in terms of potential work around supply chain/verifying our dependencies.

@facutuesca I think it would be best to try to implement the 2 commits unless its going to take a larger effort.

RafaelGSS · 2022-12-17T22:34:10Z

It will require including that support in our current tooling (dep_updaters)

facutuesca · 2022-12-19T18:44:27Z

@mhdawson @RafaelGSS @richardlau

I changed it to create 2 commits instead of one. I re-requested reviews since the new approach is different enough that it should be reviewed as a new PR. The things to keep in mind are:

The GH workflow for updating OpenSSL is now separate from the one used for the other dependencies. The OpenSSL steps (having multiple commits, each after running a different command) is different enough from the other dependencies that it can't be generalised to a single workflow without adding too much complexity
The bash script to update OpenSSL was also updated. Now it has two sub-commands (download and regenerate), corresponding to the two commits. A full update needs both sub-commands to be run, and the README was updated to reflect that

(edit: for some reason I cannot correctly re-request reviews, maybe a permissions issue since I also don't have permissions to add reviewers)

RafaelGSS · 2022-12-22T23:29:36Z

.github/workflows/update-openssl.yml

+          persist-credentials: false
+      - name: Check if update branch already exists
+        run: |
+          BRANCH_EXISTS=$(git ls-remote --heads origin actions/tools-update-openssl)


I think this block the automatic backport. We won't be able to update v14 and v16 at same time considering the race condition of the branch creation.

So, I'll include the labels dont-land-on-vX and after landing you will need to manually backport and change the branch name to actions/tools-update-openssl-vX.

Does that make sense to you?

This is the same logic used for the other deps. See this line from the current dependency update action:

node/.github/workflows/tools.yml

Line 153 in 9b1ed04

branch: actions/tools-update-${{ matrix.id }} # Custom branch *just* for this Action.

As far as I understand, this would be already an issue for corepack, eslint, etc right? Since the branches created for them are called tools-update-corepack and tools-update-eslint without mentioning the Node version.

Do you know how those are handled currently?

I don't think we have different versions of eslint/corepack in each release line. Could you check if the bot has ever opened PR targeting other branches instead of main?

The bot will not because the automatic scheduled runs only happen on the default branch:
https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#schedule

Scheduled workflows run on the latest commit on the default or base branch.

RafaelGSS · 2022-12-22T23:31:19Z

.github/workflows/update-openssl.yml

+        # Only run rest of the workflow if the update branch does not yet exist
+        if: ${{ env.BRANCH_EXISTS == '' }}
+        run: |
+          NEW_VERSION=$(gh api repos/quictls/openssl/releases -q '.[].tag_name|select(contains("openssl-3"))|ltrimstr("openssl-")' | head -n1)


this also blocks the automatic backport to v14.x. We use the original repo on v14.x

Does v14.x have any existing action for dependency updates? I could not find any

Currently there is no automation via Actions for dependency updates for any version of Node.js outside of the main branch.

.github/workflows/update-openssl.yml

Add a Github Action that checks for new versions of the `OpenSSL` library, and creates a PR to update it if a newer version than the one present in the repo is found. Refs: nodejs/security-wg#828

RafaelGSS · 2023-02-27T13:05:01Z

ping @mhdawson

mhdawson

LGTM

Add a Github Action that checks for new versions of the `OpenSSL` library, and creates a PR to update it if a newer version than the one present in the repo is found. Refs: nodejs/security-wg#828 PR-URL: #45605 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>

mhdawson · 2023-03-02T20:48:51Z

Landed in c4103c1

Add a Github Action that checks for new versions of the `OpenSSL` library, and creates a PR to update it if a newer version than the one present in the repo is found. Refs: nodejs/security-wg#828 PR-URL: #45605 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>

nodejs-github-bot added meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Nov 24, 2022

mhdawson reviewed Dec 1, 2022

View reviewed changes

mhdawson approved these changes Dec 1, 2022

View reviewed changes

RafaelGSS added dont-land-on-v14.x labels Dec 2, 2022

RafaelGSS reviewed Dec 2, 2022

View reviewed changes

RafaelGSS reviewed Dec 8, 2022

View reviewed changes

facutuesca mentioned this pull request Dec 8, 2022

Automate updates of all dependencies nodejs/security-wg#828

Closed

23 tasks

RafaelGSS approved these changes Dec 16, 2022

View reviewed changes

facutuesca requested review from mhdawson and RafaelGSS and removed request for mhdawson December 19, 2022 18:37

facutuesca requested review from mhdawson and removed request for RafaelGSS and mhdawson December 19, 2022 19:13

facutuesca requested review from RafaelGSS and mhdawson and removed request for RafaelGSS December 19, 2022 19:13

mhdawson requested a review from RafaelGSS December 19, 2022 20:14

RafaelGSS reviewed Dec 22, 2022

View reviewed changes

RafaelGSS added the dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. label Dec 22, 2022

marco-ippolito approved these changes Feb 24, 2023

View reviewed changes

facutuesca and others added 2 commits February 24, 2023 18:58

tools: add automation for updating OpenSSL dependency

a30e72c

Add a Github Action that checks for new versions of the `OpenSSL` library, and creates a PR to update it if a newer version than the one present in the repo is found. Refs: nodejs/security-wg#828

tools: add openssl version to commit

1e1cd4b

RafaelGSS approved these changes Feb 27, 2023

View reviewed changes

mhdawson approved these changes Mar 2, 2023

View reviewed changes

mhdawson closed this Mar 2, 2023

marco-ippolito mentioned this pull request Mar 5, 2023

deps: update OpenSSL to 3.0.8-quic1 #46957

Closed

targos mentioned this pull request Mar 14, 2023

v19.8.0 release proposal #47087

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add automation for updating `OpenSSL` dependency #45605

Add automation for updating `OpenSSL` dependency #45605

facutuesca commented Nov 24, 2022

nodejs-github-bot commented Nov 24, 2022

facutuesca commented Nov 24, 2022 •

edited

Loading

mhdawson Dec 1, 2022

mhdawson Dec 1, 2022

mhdawson left a comment

RafaelGSS commented Dec 2, 2022 •

edited

Loading

RafaelGSS left a comment

richardlau commented Dec 2, 2022

facutuesca commented Dec 6, 2022

RafaelGSS left a comment

mhdawson commented Dec 16, 2022

RafaelGSS commented Dec 16, 2022

mhdawson commented Dec 16, 2022

RafaelGSS left a comment

mhdawson commented Dec 16, 2022

richardlau commented Dec 16, 2022

mhdawson commented Dec 16, 2022

RafaelGSS commented Dec 17, 2022

facutuesca commented Dec 19, 2022 •

edited

Loading

RafaelGSS Dec 22, 2022

facutuesca Jan 4, 2023

RafaelGSS Jan 4, 2023

richardlau Jan 4, 2023

RafaelGSS Dec 22, 2022

facutuesca Jan 4, 2023

richardlau Jan 4, 2023

RafaelGSS commented Feb 27, 2023

mhdawson left a comment

mhdawson commented Mar 2, 2023

Add automation for updating OpenSSL dependency #45605

Add automation for updating OpenSSL dependency #45605

Conversation

facutuesca commented Nov 24, 2022

nodejs-github-bot commented Nov 24, 2022

facutuesca commented Nov 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

RafaelGSS commented Dec 2, 2022 • edited Loading

RafaelGSS left a comment

Choose a reason for hiding this comment

richardlau commented Dec 2, 2022

facutuesca commented Dec 6, 2022

RafaelGSS left a comment

Choose a reason for hiding this comment

mhdawson commented Dec 16, 2022

RafaelGSS commented Dec 16, 2022

mhdawson commented Dec 16, 2022

RafaelGSS left a comment

Choose a reason for hiding this comment

mhdawson commented Dec 16, 2022

richardlau commented Dec 16, 2022

mhdawson commented Dec 16, 2022

RafaelGSS commented Dec 17, 2022

facutuesca commented Dec 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RafaelGSS commented Feb 27, 2023

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Mar 2, 2023

Add automation for updating `OpenSSL` dependency #45605

Add automation for updating `OpenSSL` dependency #45605

facutuesca commented Nov 24, 2022 •

edited

Loading

RafaelGSS commented Dec 2, 2022 •

edited

Loading

facutuesca commented Dec 19, 2022 •

edited

Loading