Opened 3 years ago

Closed 12 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

TicketTypeStatusOwnerSummary
#19499defectclosedyawningCompile time warnings with OpenSSL 1.1.0-pre6-dev (aka master).
#19981defectclosedMake sure we build with OpenSSL 1.1.0 with all deprecated APIs removed
#25353defectclosednickmConfigure fails with some OpenSSL 1.1.0 built with no-deprecated.

Change History (25)

comment:1 Changed 3 years ago by cypherpunks

FYI, Tor 0.2.9.3-alpha doesn't compile with OpenSSL 1.1.0b with all deprecated features disabled.

comment:2 Changed 2 years ago by nickm

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 2 years ago by nickm

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 2 years ago by nickm

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 2 years ago by nickm

Milestone: Tor: 0.3.0.x-finalTor: 0.3.???

comment:6 Changed 2 years ago by nickm

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 2 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:8 Changed 2 years ago by dgoulet

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 22 months ago by nickm

Keywords: openssl refactor future-proof added

comment:10 Changed 14 months ago by laomaiweng

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 14 months ago by laomaiweng

Cc: quentin@… added

comment:12 Changed 12 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.4.x-final
Owner: changed from yawning to nickm
Status: newaccepted

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 12 months ago by nickm

Status: acceptedneeds_review

comment:14 Changed 12 months ago by dgoulet

Reviewer: catalyst

comment:15 Changed 12 months ago by catalyst

#19981 is still listed as a child ticket; does it need a separate patch or does this patch cover that ticket as well?

comment:16 Changed 12 months ago by nickm

This patch also covers that ticket.

comment:17 Changed 12 months ago by catalyst

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 12 months ago by catalyst

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 12 months ago by catalyst

Status: needs_reviewneeds_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 12 months ago by catalyst

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 12 months ago by nickm

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:22 Changed 12 months ago by nickm

okay. Tests pass for me now.

comment:23 Changed 12 months ago by nickm

Status: needs_revisionneeds_review

comment:24 Changed 12 months ago by catalyst

Status: needs_reviewmerge_ready

Thanks! Works for me now on 1.1.0 and 1.1.0-nodep.

comment:25 Changed 12 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merging!

Note: See TracTickets for help on using tickets.