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

crypto: use SSL_get_SSL_CTX. #8995

Closed
wants to merge 1 commit into from
Closed

crypto: use SSL_get_SSL_CTX. #8995

wants to merge 1 commit into from

Conversation

agl
Copy link
Contributor

@agl agl commented Oct 9, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto

Description of change

SSL_get_SSL_CTX returns the SSL_CTX for an SSL. Previously the code
accessed ssl->ctx directly, but that's no longer possible with OpenSSL
1.1.0.

SSL_get_SSL_CTX exists all the way back to (at least) OpenSSL 0.9.8 and
so this change should be fully compatible.

@agl
crypto: use SSL_get_SSL_CTX.
a1a6605 
SSL_get_SSL_CTX returns the SSL_CTX for an SSL. Previously the code
accessed |ssl->ctx| directly, but that's no longer possible with OpenSSL
1.1.0.

SSL_get_SSL_CTX exists all the way back to (at least) OpenSSL 0.9.8 and
so this change should be fully compatible.
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 9, 2016
@mscdex mscdex added crypto Issues and PRs related to the crypto subsystem. tls Issues and PRs related to the tls subsystem. and removed crypto Issues and PRs related to the crypto subsystem. labels Oct 9, 2016
@mscdex
Copy link
Contributor

mscdex commented Oct 9, 2016

/cc @nodejs/crypto

bnoordhuis
bnoordhuis approved these changes Oct 10, 2016
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM. CI seems to be unreachable at the moment.

indutny
indutny approved these changes Oct 10, 2016
View reviewed changes
Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

LGTM

@shigeki
Copy link
Contributor

shigeki commented Oct 12, 2016

CI is running on https://ci.nodejs.org/job/node-test-pull-request/4485/.

shigeki
shigeki approved these changes Oct 12, 2016
View reviewed changes
Copy link
Contributor

@shigeki shigeki left a comment

Choose a reason for hiding this comment

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

CI icons show they are still in progress on Linux platforms but I confirmed all tests were finished and green. FreeBSD failures were due to npm install errors so they are nothing to do with this. LTGM

shigeki pushed a commit that referenced this pull request Oct 12, 2016
@agl @shigeki
crypto: use SSL_get_SSL_CTX.
499d058 
SSL_get_SSL_CTX returns the SSL_CTX for an SSL. Previously the code
accessed |ssl->ctx| directly, but that's no longer possible with OpenSSL
1.1.0.

SSL_get_SSL_CTX exists all the way back to (at least) OpenSSL 0.9.8 and
so this change should be fully compatible.

PR-URL: #8995
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
@shigeki
Copy link
Contributor

shigeki commented Oct 12, 2016

Landed in 499d058.
@agl Thanks. Please let us know to what version do you want this to be backported.

@shigeki shigeki closed this Oct 12, 2016
jasnell pushed a commit that referenced this pull request Oct 12, 2016
@agl @jasnell
crypto: use SSL_get_SSL_CTX.
89643b6 
SSL_get_SSL_CTX returns the SSL_CTX for an SSL. Previously the code
accessed |ssl->ctx| directly, but that's no longer possible with OpenSSL
1.1.0.

SSL_get_SSL_CTX exists all the way back to (at least) OpenSSL 0.9.8 and
so this change should be fully compatible.

PR-URL: #8995
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
@MylesBorins
Copy link
Contributor

@shigeki / @jasnell should this be backported? It lands cleanly on v6 but not on v4. My gut is to say just land it cleanly on v6 but avoid v4... just in case of future security changes we can limit the delta in the implementations.

Thoughts?

@indutny
Copy link
Member

indutny commented Nov 11, 2016

@thealphanerd this isn't necessary for v4, I'd go with v6, though.

MylesBorins pushed a commit that referenced this pull request Nov 11, 2016
@agl
crypto: use SSL_get_SSL_CTX.
9cfa91b 
SSL_get_SSL_CTX returns the SSL_CTX for an SSL. Previously the code
accessed |ssl->ctx| directly, but that's no longer possible with OpenSSL
1.1.0.

SSL_get_SSL_CTX exists all the way back to (at least) OpenSSL 0.9.8 and
so this change should be fully compatible.

PR-URL: #8995
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
@MylesBorins MylesBorins mentioned this pull request Nov 22, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shigeki shigeki shigeki approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@indutny indutny indutny approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@agl @mscdex @shigeki @MylesBorins @indutny @bnoordhuis @nodejs-github-bot