Skip to content

bpo-30008: OpenSSL 1.1 compatibility without using deprecated API #3943

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

Closed
wants to merge 4 commits into from

Conversation

markwright
Copy link
Contributor

@markwright markwright commented Oct 10, 2017

Note: RAND_pseudo_bytes() is deprecated so RAND_bytes() is used when pseudo is requested.

https://bugs.python.org/issue30008

Note: RAND_pseudo_bytes() is deprecated so RAND_bytes() is used when pseudo is requested.
@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

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

Thanks,

you also need to sign the contributor agreement and add a news file with blurb.

@@ -1012,7 +1012,7 @@ PyInit__hashlib(void)
{
PyObject *m, *openssl_md_meth_names;

#ifndef OPENSSL_VERSION_1_1
#if (OPENSSL_VERSION_NUMBER < 0x10100000L) || defined(LIBRESSL_VERSION_NUMBER)
Copy link
Member

Choose a reason for hiding this comment

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

Good catch, but this is an unrelated fix and should be tracked in a separate issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, however it was the compiler that told me I needed to make this change
to correct this compiler error that occurs building Python 3.7.0a2 with openssl
1.1.0g built with disable-deprecated:

/var/tmp/portage/dev-lang/python-3.7.0_alpha2/work/Python-3.7.0a2/Modules/_hashopenssl.c: In function 'PyInit__hashlib':
/var/tmp/portage/dev-lang/python-3.7.0_alpha2/work/Python-3.7.0a2/Modules/_hashopenssl.c:1017:5: error: implicit declaration of function 'OPENSSL_add_all_algorithms_noconf' [-Werror=implicit-function-declaration]
     OPENSSL_add_all_algorithms_noconf();
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/var/tmp/portage/dev-lang/python-3.7.0_alpha2/work/Python-3.7.0a2/Modules/_hashopenssl.c:1018:5: error: implicit declaration of function 'ERR_load_crypto_strings'; did you mean 'ERR_load_ERR_strings'? [-Werror=implicit-function-declaration]
     ERR_load_crypto_strings();
     ^~~~~~~~~~~~~~~~~~~~~~~
     ERR_load_ERR_strings

@@ -63,6 +63,7 @@ static PySocketModule_APIObject PySocketModule;
#include "openssl/err.h"
#include "openssl/rand.h"
#include "openssl/bio.h"
#include "openssl/dh.h"
Copy link
Member

Choose a reason for hiding this comment

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

Why is dh.h required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To fix thie compiler error that occurs building Python 3.7.0a2 with openssl 1.1.0g built with disable-deprecated:

/var/tmp/portage/dev-lang/python-3.7.0_alpha2/work/Python-3.7.0a2/Modules/_ssl.c: In function '_ssl__SSLContext_load_dh_params':
/var/tmp/portage/dev-lang/python-3.7.0_alpha2/work/Python-3.7.0a2/Modules/_ssl.c:3757:5: error: implicit declaration of function 'DH_free'; did you mean 'bt_free'? [-Werror=implicit-function-declaration]
     DH_free(dh);
     ^~~~~~~
     bt_free

Modules/_ssl.c Outdated
@@ -5264,7 +5277,9 @@ PyInit__ssl(void)
return NULL;
PySocketModule = *socket_api;

#ifndef OPENSSL_VERSION_1_1
#ifdef OPENSSL_VERSION_1_1
OPENSSL_init_ssl(0, NULL);
Copy link
Member

Choose a reason for hiding this comment

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

No need to call this, OpenSSL calls this function internally and automatically.

https://www.openssl.org/docs/man1.1.0/ssl/OPENSSL_init_ssl.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, reverted in a separate commit. I can squash all the commits into 1 later if you like.
PS: I didn't expect the Spanish Inquisition!

Modules/_ssl.c Outdated
@@ -4625,7 +4634,11 @@ PySSL_RAND(int len, int pseudo)
if (bytes == NULL)
return NULL;
if (pseudo) {
#ifdef OPENSSL_VERSION_1_1
ok = RAND_bytes((unsigned char*)PyBytes_AS_STRING(bytes), len);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it is safe to replace RAND_pseudo_bytes with RAND_bytes. A RAND engine may implement bytes and pseudobytes differently, e.g. bytes may not fill the buffer to the end in case of an error.

The function https://docs.python.org/3/library/ssl.html#ssl.RAND_pseudo_bytes has been deprecated. We can drop when OpenSSL 1.2.0 removes the feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I replaced it with:

        ok = (_PyOS_URandom((unsigned char*)PyBytes_AS_STRING(bytes), len) == 0 ? 1 : 0);

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I didn't expect the Spanish Inquisition!. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@ivvolnov-radargeo
Copy link

This patch is not working for me:

*** WARNING: renaming "_ssl" since importing it failed: build/lib.linux-x86_64-3.6/_ssl.cpython-36m-x86_64-linux-gnu.so: undefined symbol: TLSv1_method

@markwright
Copy link
Contributor Author

@ivvolnov-radargeo The patch was split into 2 pull requests, you need both, see:
https://bugs.python.org/issue30008#msg304028

@markwright
Copy link
Contributor Author

I didn't expect the Spanish Inquisition!

@bedevere-bot
Copy link

Nobody expects the Spanish Inquisition!

@tiran: please review the changes made to this pull request.

@devurandom
Copy link

@tiran What's the status of this?

cotequeiroz added a commit to cotequeiroz/cpython that referenced this pull request Jun 5, 2018
Backport of PRs python#3934 and python#3943

Signed-off-by: Eneas U de Queiroz <[email protected]>
@neheb
Copy link

neheb commented Nov 3, 2019

This PR rebased to Python 3.8.0: https://raw.githubusercontent.com/neheb/packages/1acf047c7f5c9d807ad49b9787aaaad543734be8/lang/python/python3/patches/020-ssl-module-emulate-tls-methods.patch

Additional fixes:

--- a/Modules/_ssl.c
+++ b/Modules/_ssl.c
@@ -47,6 +47,7 @@ static PySocketModule_APIObject PySocketModule;
 
 /* Include OpenSSL header files */
 #include "openssl/rsa.h"
+#include "openssl/dh.h"
 #include "openssl/crypto.h"
 #include "openssl/x509.h"
 #include "openssl/x509v3.h"
@@ -128,13 +129,13 @@ static void _PySSLFixErrno(void) {
 #include "_ssl_data.h"
 
 #if (OPENSSL_VERSION_NUMBER >= 0x10100000L) && !defined(LIBRESSL_VERSION_NUMBER)
-#  define OPENSSL_VERSION_1_1 1
-#  define PY_OPENSSL_1_1_API 1
+# define OPENSSL_VERSION_1_1 1
+# define PY_OPENSSL_1_1_API 1
 #endif
 
 /* LibreSSL 2.7.0 provides necessary OpenSSL 1.1.0 APIs */
 #if defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER >= 0x2070000fL
-#  define PY_OPENSSL_1_1_API 1
+# define PY_OPENSSL_1_1_API 1
 #endif
 
 /* Openssl comes with TLSv1.1 and TLSv1.2 between 1.0.0h and 1.0.1
@@ -201,6 +202,11 @@ static void _PySSLFixErrno(void) {
 #define TLS_method SSLv23_method
 #define TLS_client_method SSLv23_client_method
 #define TLS_server_method SSLv23_server_method
+#define X509_get0_notBefore X509_get_notBefore
+#define X509_get0_notAfter X509_get_notAfter
+#define OpenSSL_version_num SSLeay
+#define OpenSSL_version SSLeay_version
+#define OPENSSL_VERSION SSLEAY_VERSION
 
 static int X509_NAME_ENTRY_set(const X509_NAME_ENTRY *ne)
 {
@@ -1615,7 +1621,7 @@ _decode_certificate(X509 *certificate) {
     ASN1_INTEGER *serialNumber;
     char buf[2048];
     int len, result;
-    ASN1_TIME *notBefore, *notAfter;
+    const ASN1_TIME *notBefore, *notAfter;
     PyObject *pnotBefore, *pnotAfter;
 
     retval = PyDict_New();
@@ -1677,7 +1683,7 @@ _decode_certificate(X509 *certificate) {
     Py_DECREF(sn_obj);
 
     (void) BIO_reset(biobuf);
-    notBefore = X509_get_notBefore(certificate);
+    notBefore = X509_get0_notBefore(certificate);
     ASN1_TIME_print(biobuf, notBefore);
     len = BIO_gets(biobuf, buf, sizeof(buf)-1);
     if (len < 0) {
@@ -1694,7 +1700,7 @@ _decode_certificate(X509 *certificate) {
     Py_DECREF(pnotBefore);
 
     (void) BIO_reset(biobuf);
-    notAfter = X509_get_notAfter(certificate);
+    notAfter = X509_get0_notAfter(certificate);
     ASN1_TIME_print(biobuf, notAfter);
     len = BIO_gets(biobuf, buf, sizeof(buf)-1);
     if (len < 0) {
@@ -3235,7 +3241,7 @@ _ssl__SSLContext_impl(PyTypeObject *type, int proto_version)
        conservative and assume it wasn't fixed until release. We do this check
        at runtime to avoid problems from the dynamic linker.
        See #25672 for more on this. */
-    libver = SSLeay();
+    libver = OpenSSL_version_num();
     if (!(libver >= 0x10001000UL && libver < 0x1000108fUL) &&
         !(libver >= 0x10000000UL && libver < 0x100000dfUL)) {
         SSL_CTX_set_mode(self->ctx, SSL_MODE_RELEASE_BUFFERS);
@@ -6403,10 +6409,10 @@ PyInit__ssl(void)
         return NULL;
 
     /* OpenSSL version */
-    /* SSLeay() gives us the version of the library linked against,
+    /* OpenSSL_version_num() gives us the version of the library linked against,
        which could be different from the headers version.
     */
-    libver = SSLeay();
+    libver = OpenSSL_version_num();
     r = PyLong_FromUnsignedLong(libver);
     if (r == NULL)
         return NULL;
@@ -6416,7 +6422,7 @@ PyInit__ssl(void)
     r = Py_BuildValue("IIIII", major, minor, fix, patch, status);
     if (r == NULL || PyModule_AddObject(m, "OPENSSL_VERSION_INFO", r))
         return NULL;
-    r = PyUnicode_FromString(SSLeay_version(SSLEAY_VERSION));
+    r = PyUnicode_FromString(OpenSSL_version(OPENSSL_VERSION));
     if (r == NULL || PyModule_AddObject(m, "OPENSSL_VERSION", r))
         return NULL;
 

@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Feb 25, 2022
@neheb
Copy link

neheb commented Feb 25, 2022

I believe Python 3 compiles without deprecated OpenSSL 1.1 APIs now. 3.0 is a much different story.

@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Feb 27, 2022
@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Aug 13, 2022
@pquentin
Copy link

This pull request can be closed, per this comment:

PR is partly based on and supersedes #3943

Originally posted by @tiran in #20397 (comment)

@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label May 11, 2023
@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Jun 10, 2023
@arhadthedev
Copy link
Member

Closing after #3943 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting change review stale Stale PR or inactive for long period of time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants