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

OpenSSL 1.1.0 compatibility #2

Closed
wants to merge 3 commits into from

Conversation

laomaiweng
Copy link
Contributor

OpenSSL 1.1.0, especially on Gentoo (where deprecated functions are disabled by
default), has a lot of API changes. This PR restores compatibility between
pshs's SSL module and OpenSSL 1.1.0, hopefully without breaking anything for
older versions of OpenSSL.

Note that RSA_generate_key() has been replaced by RSA_generate_key_ex() from
OpenSLS 1.0.0 onwards, since it was deprecated even at that time.

Also, the PR includes a commit that fixes incorrect refcounting on the RSA key
object allocated in the SSL module. I do not know if this affects OpenSSL
1.0.x, but it looks like it could. If it turns out not to, it should be guarded
in an #if block.

This has been tested to work against OpenSSL 1.1.0g. I have not tested (or even
compile-tested) against previous versions of OpenSSL.

Stop using deprecated/removed APIs when compiling against OpenSSL >= 1.1.0.
Switch to RSA_generate_key_ex() when compiling against OpenSSL >= 1.0.0.
EVP_PKEY_assign_RSA() does not increment the refcount on the RSA key,
so it gets really freed when the unique pointer goes out of scope.
Use EVP_PKEY_set1_RSA() which does increment the refcount.
src/ssl.cxx Outdated
static void key_progress_cb(int p, int n, void* arg)
# else
static int key_progress_cb(int p, int n, BN_GENCB *cb)
Copy link
Member

Choose a reason for hiding this comment

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

Please respect the original code style, in particular use of spaces etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, sorry.
Do you mean the # else bit (should I use a tab? or no space at all?), or the BN_GENCB *cb one? Or both?

Copy link
Member

Choose a reason for hiding this comment

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

Both/all of them. I'm not sure what are the conventions in the surrounding code but I think no space would be better here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are tabs used in # include statements inside a #if block at the top of the file, but that didn't feel very readable inside a code block. I'll go with no space as you suggest.

src/ssl.cxx Outdated

std::unique_ptr<EVP_PKEY, std::function<void(EVP_PKEY*)>>
pkey{EVP_PKEY_new(), EVP_PKEY_free};
std::unique_ptr<X509, std::function<void(X509*)>>
x509{X509_new(), X509_free};
/* XXX: settable params */
std::unique_ptr<RSA, std::function<void(RSA*)>>
# if OPENSSL_VERSION_NUMBER < 0x10000000L
rsa{RSA_generate_key(2048, RSA_F4, key_progress_cb, 0), RSA_free};

if (!pkey || !x509 || !rsa)
Copy link
Member

Choose a reason for hiding this comment

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

Please don't mix part of one statement with followup statements in #if block, this makes it really hard to read. I.e. start the #if line earlier and include whole std::unique_ptr in it.

Copy link
Member

@mgorny mgorny left a comment

Choose a reason for hiding this comment

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

Thanks. I'll squash and merge it.

@mgorny mgorny closed this in 5589edb Feb 26, 2018
@@ -31,8 +32,15 @@
#ifdef HAVE_LIBSSL
static std::unique_ptr<SSL_CTX, std::function<void(SSL_CTX*)>> ssl;

#if OPENSSL_VERSION_NUMBER < 0x10000000L
Copy link
Member

Choose a reason for hiding this comment

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

FYI, I've had to change this to 0x10100000L since it apparently does not work with 1.0.2n.

@laomaiweng
Copy link
Contributor Author

Thank you! :)

@laomaiweng laomaiweng deleted the openssl-1.1.0-compat branch February 26, 2018 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants