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.

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

deps: update openssl to OpenSSL 3.0.2 #42356

Closed

Conversation

hassaanp
Copy link
Contributor

Updated openssl dep to openssl-3.0.2+quic using the maintenance guide.

Refs: https://mta.openssl.org/pipermail/openssl-announce/2022-March/000217.html

hassaanp and others added 3 commits March 16, 2022 04:49
This updates all sources in deps/openssl/openssl by:
    $ git clone git@github.com:quictls/openssl.git
    $ cd openssl
    $ cd ../node/deps/openssl
    $ rm -rf openssl
    $ cp -R ../openssl openssl
    $ rm -rf openssl/.git* openssl/.travis*
    $ git add --all openssl
    $ git commit openssl
After an OpenSSL source update, all the config files need to be
regenerated and committed by:
    $ make -C deps/openssl/config
    $ git add deps/openssl/config/archs
    $ git add deps/openssl/openssl
    $ git commit
Last OpenSSL 3 update changes behaviour back to be
closer to that of OpenSSL 1.1.1. Remove some instances
where we expected different errors from OpenSSL 3 versus
OpenSSL 1.1.1.

Signed-off-by: Michael Dawson <midawson@redhat.com>
@nodejs-github-bot nodejs-github-bot added dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. openssl Issues and PRs related to the OpenSSL dependency. labels Mar 16, 2022
@hassaanp
Copy link
Contributor Author

@mhdawson @richardlau
As per your suggestions, I have opened a PR against master and also cherry-picked the fix for the tests.
I hope this looks good.

@RaisinTen
Copy link
Contributor

1.1.1n -> 3.0.2 in the PR title?

@hassaanp hassaanp changed the title deps: update openssl to OpenSSL 1.1.1n deps: update openssl to OpenSSL 3.0.2 Mar 16, 2022
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 16, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 16, 2022
@nodejs-github-bot

This comment was marked as outdated.

@richardlau richardlau added fast-track PRs that do not need to wait for 48 hours to land. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Mar 16, 2022
@github-actions
Copy link
Contributor

Fast-track has been requested by @richardlau. Please 👍 to approve.

@aduh95
Copy link
Contributor

aduh95 commented Mar 16, 2022

All commits should be self-contained, meaning every commit should pass all
tests. This makes it much easier when bisecting to find a breaking change.

Looking at d37dceb, it looks like tests would not be passing for e06c733 and/or 3361921. Should we add a first commit to disable the failing test (before the OpenSSL update) and re-enable it in a follow up commit?

@richardlau
Copy link
Member

All commits should be self-contained, meaning every commit should pass all
tests. This makes it much easier when bisecting to find a breaking change.

Looking at d37dceb, it looks like tests would not be passing for e06c733 and/or 3361921. Should we add a first commit to disable the failing test (before the OpenSSL update) and re-enable it in a follow up commit?

This is a tricky one for OpenSSL because as long as I can remember we've always split the OpenSSL updates into two commits, one to update the sources and a second to regen the config files, and the first commit isn't buildable without the second. I've no preference either way regarding the test in the third commit.

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM

@richardlau
Copy link
Member

richardlau commented Mar 16, 2022

I'm currently refreshing the CI machines to update the sharedlibs containers to OpenSSL 3.0.2 to fix the failing linked-openssl300 build.

Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mhdawson
Copy link
Member

This is a tricky one for OpenSSL because as long as I can remember we've always split the OpenSSL updates into two commits, one to update the sources and a second to regen the config files, and the first commit isn't buildable without the second. I've no preference either way regarding the test in the third commit.

I think we might want to adjust the text saying all commits must pass all tests, for something like an OpenSSL update I see value in them being separate in terms of understanding/being able to recreate. A PR to update the text would be a good place to have that discussion and see if we want to update the OpenSSL patch generation process but that should not be part of getting the security releases out.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 17, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 17, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in f4b7f6d...f1b6d87

nodejs-github-bot pushed a commit that referenced this pull request Mar 17, 2022
This updates all sources in deps/openssl/openssl by:
    $ git clone git@github.com:quictls/openssl.git
    $ cd openssl
    $ cd ../node/deps/openssl
    $ rm -rf openssl
    $ cp -R ../openssl openssl
    $ rm -rf openssl/.git* openssl/.travis*
    $ git add --all openssl
    $ git commit openssl

PR-URL: #42356
Refs: https://mta.openssl.org/pipermail/openssl-announce/2022-March/000217.html
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <midawson@redhat.com>
nodejs-github-bot pushed a commit that referenced this pull request Mar 17, 2022
After an OpenSSL source update, all the config files need to be
regenerated and committed by:
    $ make -C deps/openssl/config
    $ git add deps/openssl/config/archs
    $ git add deps/openssl/openssl
    $ git commit

PR-URL: #42356
Refs: https://mta.openssl.org/pipermail/openssl-announce/2022-March/000217.html
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <midawson@redhat.com>
nodejs-github-bot pushed a commit that referenced this pull request Mar 17, 2022
Last OpenSSL 3 update changes behaviour back to be
closer to that of OpenSSL 1.1.1. Remove some instances
where we expected different errors from OpenSSL 3 versus
OpenSSL 1.1.1.

Signed-off-by: Michael Dawson <midawson@redhat.com>

PR-URL: #42356
Refs: https://mta.openssl.org/pipermail/openssl-announce/2022-March/000217.html
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <midawson@redhat.com>
richardlau pushed a commit that referenced this pull request Mar 17, 2022
This updates all sources in deps/openssl/openssl by:
    $ git clone git@github.com:quictls/openssl.git
    $ cd openssl
    $ cd ../node/deps/openssl
    $ rm -rf openssl
    $ cp -R ../openssl openssl
    $ rm -rf openssl/.git* openssl/.travis*
    $ git add --all openssl
    $ git commit openssl

PR-URL: #42356
Refs: https://mta.openssl.org/pipermail/openssl-announce/2022-March/000217.html
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <midawson@redhat.com>
richardlau pushed a commit that referenced this pull request Mar 17, 2022
After an OpenSSL source update, all the config files need to be
regenerated and committed by:
    $ make -C deps/openssl/config
    $ git add deps/openssl/config/archs
    $ git add deps/openssl/openssl
    $ git commit

PR-URL: #42356
Refs: https://mta.openssl.org/pipermail/openssl-announce/2022-March/000217.html
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <midawson@redhat.com>
richardlau pushed a commit that referenced this pull request Mar 17, 2022
Last OpenSSL 3 update changes behaviour back to be
closer to that of OpenSSL 1.1.1. Remove some instances
where we expected different errors from OpenSSL 3 versus
OpenSSL 1.1.1.

Signed-off-by: Michael Dawson <midawson@redhat.com>

PR-URL: #42356
Refs: https://mta.openssl.org/pipermail/openssl-announce/2022-March/000217.html
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@richardlau richardlau mentioned this pull request Mar 17, 2022
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
This updates all sources in deps/openssl/openssl by:
    $ git clone git@github.com:quictls/openssl.git
    $ cd openssl
    $ cd ../node/deps/openssl
    $ rm -rf openssl
    $ cp -R ../openssl openssl
    $ rm -rf openssl/.git* openssl/.travis*
    $ git add --all openssl
    $ git commit openssl

PR-URL: nodejs#42356
Refs: https://mta.openssl.org/pipermail/openssl-announce/2022-March/000217.html
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <midawson@redhat.com>
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
After an OpenSSL source update, all the config files need to be
regenerated and committed by:
    $ make -C deps/openssl/config
    $ git add deps/openssl/config/archs
    $ git add deps/openssl/openssl
    $ git commit

PR-URL: nodejs#42356
Refs: https://mta.openssl.org/pipermail/openssl-announce/2022-March/000217.html
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <midawson@redhat.com>
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
Last OpenSSL 3 update changes behaviour back to be
closer to that of OpenSSL 1.1.1. Remove some instances
where we expected different errors from OpenSSL 3 versus
OpenSSL 1.1.1.

Signed-off-by: Michael Dawson <midawson@redhat.com>

PR-URL: nodejs#42356
Refs: https://mta.openssl.org/pipermail/openssl-announce/2022-March/000217.html
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. dependencies Pull requests that update a dependency file. fast-track PRs that do not need to wait for 48 hours to land. needs-ci PRs that need a full CI run. openssl Issues and PRs related to the OpenSSL dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants