Opened 3 years ago
Closed 20 months ago
#19429 closed enhancement (implemented)
Clean up our OpenSSL 1.1 support.
Reported by: | yawning | Owned by: | nickm |
---|---|---|---|
Priority: | Medium | Milestone: | Tor: 0.3.4.x-final |
Component: | Core Tor/Tor | Version: | Tor: 0.2.8.4-rc |
Severity: | Normal | Keywords: | tor-crypto openssl refactor future-proof |
Cc: | quentin@… | Actual Points: | |
Parent ID: | Points: | 3 | |
Reviewer: | catalyst | Sponsor: |
Description
In an ideal world, we should support OpenSSL 1.1 built with the deprecated APIs entirely disabled. Additionally while the code that uses the new opaque wrappers for the RSA stuff is functional and maintainable, it would be cleaner if we provided our own wrappers for a more unified codepath.
Neither of these are high priority as the current code works, the changes suggested would just make things better.
Child Tickets
Change History (25)
comment:1 Changed 3 years ago by
comment:2 Changed 3 years ago by
These are the deprecated functions we use:
CRYPTO_cleanup_all_ex_data ENGINE_cleanup ERR_free_strings ERR_load_crypto_strings EVP_CIPHER_CTX_cleanup EVP_cleanup OpenSSL_add_all_algorithms SSL_library_init SSL_load_error_strings X509_get_notAfter X509_get_notBefore
comment:3 Changed 3 years ago by
These functions are replaced by no-ops in OpenSSL 1.1:
CRYPTO_cleanup_all_ex_data ENGINE_cleanup ERR_free_strings EVP_cleanup
These are replaced with variants of OPENSSL_init_crypto()
:
ERR_load_crypto_strings OpenSSL_add_all_algorithms
These are replaced with variants of OPENSSL_init_ssl()
:
SSL_library_init SSL_load_error_strings
These have new names:
EVP_CIPHER_CTX_cleanup()
becomes EVP_CIPHER_CTX_reset()
.
X509_get_notAfter()
becomes X509_get0_notAfter
or X509_getm_notAfter
.
X509_get_notBefore()
becomes X509_get0_notBefore
or X509_getm_notBefore
.
comment:4 Changed 3 years ago by
Of the new functions listed above, none was available in OpenSSL 1.0.2. I believe we should defer this ticket until we no longer want to support OpenSSL 1.0.2.
comment:5 Changed 3 years ago by
Milestone: | Tor: 0.3.0.x-final → Tor: 0.3.??? |
---|
comment:6 Changed 3 years ago by
Keywords: | tor-03-unspecified-201612 added |
---|---|
Milestone: | Tor: 0.3.??? → Tor: unspecified |
Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.
comment:7 Changed 3 years ago by
Keywords: | tor-03-unspecified-201612 removed |
---|
Remove an old triaging keyword.
comment:8 Changed 3 years ago by
Keywords: | tor-core removed |
---|
The tor-core keyword doesn't really make sense now that we have "Core Tor/Tor" for component.
comment:9 Changed 2 years ago by
Keywords: | openssl refactor future-proof added |
---|
comment:10 Changed 22 months ago by
FWIW, Tor 0.3.3.1_alpha fails to build against Gentoo's OpenSSL 1.1.0g (masked, only enabled at user's own risk) because these deprecated APIs are disabled. :(
comment:11 Changed 22 months ago by
Cc: | quentin@… added |
---|
comment:12 Changed 20 months ago by
Milestone: | Tor: unspecified → Tor: 0.3.4.x-final |
---|---|
Owner: | changed from yawning to nickm |
Status: | new → accepted |
I've adapted the patch on https://bugs.gentoo.org/630380 to support this, along with the patch from #25353. See branch ticket19429_034
. Since this is a new feature, I suggest no backport. Pull request at https://github.com/torproject/tor/pull/55
comment:13 Changed 20 months ago by
Status: | accepted → needs_review |
---|
comment:14 Changed 20 months ago by
Reviewer: | → catalyst |
---|
comment:15 Changed 20 months ago by
#19981 is still listed as a child ticket; does it need a separate patch or does this patch cover that ticket as well?
comment:17 Changed 20 months ago by
Discovered some possible coverage indeterminacy shown by coveralls. See #25908. All the coverage decreases seem independent of the changes.
Changes look good by visual inspection. I still would like to try building with differently configured openssl versions to confirm the intended functionality.
comment:18 Changed 20 months ago by
I get
src/common/tortls.c:64:0: error: "X509_get_notBefore" redefined [-Werror] #define X509_get_notBefore(cert) \ ^ In file included from /home/tlyu/ssl/include/openssl/ssl.h:50:0, from src/common/tortls.c:41: /home/tlyu/ssl/include/openssl/x509.h:635:0: note: this is the location of the previous definition # define X509_get_notBefore X509_getm_notBefore ^ src/common/tortls.c:66:0: error: "X509_get_notAfter" redefined [-Werror] #define X509_get_notAfter(cert) \ ^ In file included from /home/tlyu/ssl/include/openssl/ssl.h:50:0, from src/common/tortls.c:41: /home/tlyu/ssl/include/openssl/x509.h:636:0: note: this is the location of the previous definition # define X509_get_notAfter X509_getm_notAfter ^
with a freshly built openssl-1.1.0h and default options (except for --prefix
). I'll check against a --no-deprecated
build to see if there's any difference.
comment:19 Changed 20 months ago by
Status: | needs_review → needs_revision |
---|
Looks like it works with openssl-1.1.0h built with no-deprecated
, so it's 1.1.0h with deprecated APIs that's the problem.
comment:20 Changed 20 months ago by
Confirmed that without --enable-fatal-warnings
, the building using the default build of openssl-1.1.0h seems to only have warnings in that one file. All of these are built on Ubuntu 16.04.04 amd64.
comment:21 Changed 20 months ago by
whoops. Should have tested with my regular openssl after testing with 1.1.1.
Revising, and testing with 1.1.0, 1.1.1, and 1.1.1-nodep.
comment:23 Changed 20 months ago by
Status: | needs_revision → needs_review |
---|
comment:24 Changed 20 months ago by
Status: | needs_review → merge_ready |
---|
Thanks! Works for me now on 1.1.0 and 1.1.0-nodep.
FYI, Tor 0.2.9.3-alpha doesn't compile with OpenSSL 1.1.0b with all deprecated features disabled.