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

crypto: account for OpenSSL 1.1.0 function signature change. #14761

Closed
wants to merge 1 commit into from

Conversation

davidben
Copy link
Contributor

In OpenSSL 1.1.0, SSL_CTX_sess_set_get_cb's callback has a slightly
different function signature, see [1]. Account for that with an
OPENSSL_VERSION_NUMBER check. This gets a little closer to 1.1.0
compatibility.

[1] https://git.openssl.org/gitweb/?p=openssl.git;a=blob;f=include/openssl/ssl.h;h=41cb36e9438e1debf4f1abba47f2d8d273883ffa;hb=abd30777cc72029e8a44e4b67201cae8ed3d19c1#l618

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto

In OpenSSL 1.1.0, SSL_CTX_sess_set_get_cb's callback has a slightly
different function signature, see [1]. Account for that with an
OPENSSL_VERSION_NUMBER check. This gets a little closer to 1.1.0
compatibility.

[1] https://git.openssl.org/gitweb/?p=openssl.git;a=blob;f=include/openssl/ssl.h;h=41cb36e9438e1debf4f1abba47f2d8d273883ffa;hb=abd30777cc72029e8a44e4b67201cae8ed3d19c1#l618
@nodejs-github-bot nodejs-github-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. labels Aug 11, 2017
@TimothyGu TimothyGu requested review from sam-github and removed request for sam-github August 11, 2017 03:24
@mscdex
Copy link
Contributor

mscdex commented Aug 11, 2017

I think this is already taken into account in #8491.

@mscdex
Copy link
Contributor

mscdex commented Aug 11, 2017

Also, I think @shigeki will be working on a PR for OpenSSL 1.1.1 when it gets released...

@shigeki
Copy link
Contributor

shigeki commented Aug 11, 2017

We're also waiting for FIPS support of 1.1.x. They are now working on it as https://www.openssl.org/blog/blog/2017/07/25/fips/.
This patch is included in #8491 and to be included in the future upgrade. We will stay on OpenSSL-1.0.2 until the release of OpenSSL-1.1.1 or Node-v10 to meet EOLS of 1.0.2.

@davidben I guess you need this for binding Node with BoringSSL. Could you keep this as your floating patch until we upgrade? It is no harm but I just want to avoid any confusions of misunderstanding that Node supports OpenSSL-1.1.0.

@davidben
Copy link
Contributor Author

#8491 is out of date and has some mistakes in it, since fixed on master by #9409 and #9347. Is that PR still the plan for 1.1.0 compatibility? I had assumed it was abandoned. Were you not planning on adding 1.1.0 compatibility sooner than that? Things like distro packages probably aren't using the vendored OpenSSL and would probably appreciate fixes.

(We are indeed looking at advertising 1.1.0 rather than 1.0.2 in BoringSSL, which would need this patch, though not the rest of #8491. But if the patch isn't useful to you, we can certainly carry it or find some workaround. I am also happy to try updating #8491 if you all would prefer something less piecemeal.)

@davidben
Copy link
Contributor Author

Oh, I see you already have #11828 which updates it. Sorry, I'd missed that.

Anyway, I'll defer to you all, however you all would like to do this.

@davidben
Copy link
Contributor Author

Closing in favor of more complete 1.1.0 PR later.

@davidben davidben closed this Aug 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants