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

src: set SSL_OP_ALLOW_CLIENT_RENEGOTIATION #38753

Closed
wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented May 21, 2021

This commit sets SSL_OP_ALLOW_CLIENT_RENEGOTIATION for OpenSSL 3.0 as
this option is not set by default as it was in previous versions.

Without this option set there are a few tests that fail when linked
against OpenSSl 3.0.0-alpha-17, for example test-https-client-renegotiation-limit.js.

I'm not sure we should be setting this for OpenSSL 3.0 or not, but I'll take
a closer look at the implications. If nothing else this would allow
for us to update to alpha-17 in the mean time.

This commit sets SSL_OP_ALLOW_CLIENT_RENEGOTIATION for OpenSSL 3.0 as
this option is not set by default as it was in  previous versions.

Without this option set there are a few tests that fail when linked
against OpenSSl 3.0.0-alpha-17, for example
test-https-client-renegotiation-limit.js.

I'm not sure we should be setting this for OpenSSL 3.0 or not, but
I'll take a closer look at the implications but if nothing else this
would allow for us to update to alpha-17 in the mean time.
@github-actions github-actions bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels May 21, 2021
@danbev danbev marked this pull request as draft May 21, 2021 08:34
@jasnell
Copy link
Member

jasnell commented May 24, 2021

I will say that for http2 and quic, having renegotiation disabled by default does make the most sense... so we might want to revisit this

danbev added a commit to danbev/build that referenced this pull request Jul 16, 2021
This commit updates the version of quictls/openssl to 3.0.0-beta1. This
change will cause a test failure so it need to be coordinated with
nodejs/node#38753.
danbev added a commit to danbev/build that referenced this pull request Jul 16, 2021
This commit updates the version of quictls/openssl to 3.0.0-beta1. This
change will cause a test failure so it needs to be coordinated with
nodejs/node#38753.
richardlau pushed a commit to nodejs/build that referenced this pull request Jul 16, 2021
This commit updates the version of quictls/openssl to 3.0.0-beta1. This
change will cause a test failure so it needs to be coordinated with
nodejs/node#38753.
@richardlau richardlau marked this pull request as ready for review July 16, 2021 11:25
@richardlau
Copy link
Member

richardlau commented Jul 16, 2021

I need to restart the containers after they rebuild (in progress) and then I'll start CI for this.
Refs: nodejs/build#2712

@nodejs-github-bot
Copy link
Collaborator

@danbev
Copy link
Contributor Author

danbev commented Jul 16, 2021

I need to restart the containers after they rebuild (in progress) and then I'll start CI for this.

Thanks!

@richardlau richardlau added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 16, 2021
@nodejs-github-bot
Copy link
Collaborator

@danbev
Copy link
Contributor Author

danbev commented Jul 16, 2021

Re-run of failing node-test-commit-osx-arm ✔️

@richardlau richardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 16, 2021
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 16, 2021
@github-actions
Copy link
Contributor

Landed in 1efa8fe...44e3822

@github-actions github-actions bot closed this Jul 16, 2021
nodejs-github-bot pushed a commit that referenced this pull request Jul 16, 2021
This commit sets SSL_OP_ALLOW_CLIENT_RENEGOTIATION for OpenSSL 3.0 as
this option is not set by default as it was in  previous versions.

Without this option set there are a few tests that fail when linked
against OpenSSl 3.0.0-alpha-17, for example
test-https-client-renegotiation-limit.js.

I'm not sure we should be setting this for OpenSSL 3.0 or not, but
I'll take a closer look at the implications but if nothing else this
would allow for us to update to alpha-17 in the mean time.

PR-URL: #38753
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@danbev danbev deleted the reneg-openssl-master-issue branch July 16, 2021 13:46
targos pushed a commit that referenced this pull request Jul 17, 2021
This commit sets SSL_OP_ALLOW_CLIENT_RENEGOTIATION for OpenSSL 3.0 as
this option is not set by default as it was in  previous versions.

Without this option set there are a few tests that fail when linked
against OpenSSl 3.0.0-alpha-17, for example
test-https-client-renegotiation-limit.js.

I'm not sure we should be setting this for OpenSSL 3.0 or not, but
I'll take a closer look at the implications but if nothing else this
would allow for us to update to alpha-17 in the mean time.

PR-URL: #38753
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Jul 26, 2021
BethGriggs pushed a commit that referenced this pull request Jul 29, 2021
This commit sets SSL_OP_ALLOW_CLIENT_RENEGOTIATION for OpenSSL 3.0 as
this option is not set by default as it was in  previous versions.

Without this option set there are a few tests that fail when linked
against OpenSSl 3.0.0-alpha-17, for example
test-https-client-renegotiation-limit.js.

I'm not sure we should be setting this for OpenSSL 3.0 or not, but
I'll take a closer look at the implications but if nothing else this
would allow for us to update to alpha-17 in the mean time.

PR-URL: #38753
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants