Jens-G opened a new pull request, #3850:
URL: https://github.com/apache/thrift/pull/3850
One patch for OpenSSL 4.0, merging #3761, #3752 and #3736 together with the
two
patches that were posted as comments rather than PRs. It supersedes all
three.
Every enum value keeps working. `SSLv3_method()`, `TLSv1_method()`,
`TLSv1_1_method()` and `TLSv1_2_method()` are replaced by `TLS_method()`
with the
protocol window pinned through `SSL_CTX_set_min_proto_version()` /
`SSL_CTX_set_max_proto_version()`, which is OpenSSL's documented migration
and
reproduces the removed methods exactly -- `TLSv1_2_method()` meant TLS 1.2
only,
and `min == max == TLS1_2_VERSION` means the same. So `TLSv1_0`, `TLSv1_1`,
`TLSv1_2` and `LATEST` do not start throwing `"Unknown protocol"` on 4.0.
### Why the new path is gated at >= 1.1.0, not at >= 4.0
No CI configuration builds against OpenSSL 4.0. Gating the replacement at
`>= 0x40000000L` ships a code path that nothing in CI ever compiles, let
alone
runs -- the objection I raised on #3761. The replacement APIs all exist since
1.1.0, so selecting them from there on puts the new code under the existing
SSL
matrix tests on the OpenSSL 3.x that CI does have. Same reasoning for the
`const`
qualifiers, whose consumers have taken `const` since 1.1.0.
LibreSSL is excluded throughout: it reports `OPENSSL_VERSION_NUMBER` as
`0x20000000L` but keeps the pre-1.1.0 signatures, so it stays on exactly the
path
it compiles today.
`SSLTLS` is untouched. `SSLv23_method()` is *not* removed in 4.0 -- it is an
unconditional `#define SSLv23_method TLS_method` -- so that arm needed no
change,
and its protocol floor still comes from the existing `SSL_CTX_set_options()`
call.
`SSLv3` keeps `SSLv3_method()` below 4.0, the only release that removes it,
rather
than becoming a window like the others: `SSL_CTX_set_min_proto_version()`
consults
the security level and would refuse `SSL3_VERSION` at the default one, where
the
method function does not -- and the context is built before
`TSSLSocketFactory::ciphers()` lowers it. On 4.0 the protocol is gone
outright and
the arm is compiled out.
### Tests
- `SecurityTest` gains `explicit_protocol_version_window`, asserting the
min/max
window per enum value rather than just that a context came back -- the
latter
passes on a remapping that silently leaves the floor to the library
default.
Proven two-state: with the window not pinned it fails 8 assertions,
`LATEST`
among them, reporting floor `0`. It also asserts that an unavailable
`SSLv3` is
reported rather than quietly satisfied with some other version.
- Both matrix tests skip `SSLv3` on 4.0. `OPENSSL_NO_SSL3` is **not** defined
there (verified), so without this they try SSLv3 and time out -- exactly
what
@loqs reported on #3752.
- `testtransportsslsocket` gains the same two cases for c_glib.
- `TSSLSocketMatchNameTest` built its subject name through
`X509_get_subject_name()` and then mutated it; that is a hard compile
error on
4.0. **None of the three PRs covers this file, so none of them actually
yields
a tree that passes `make check -C lib/cpp` on 4.0.** It now builds a
separate
`X509_NAME`. While there I found it has no SIGPIPE handler although several
cases deliberately provoke rejected handshakes: it dies with status 141 in
6 of
20 runs on master today, and 11 of 20 with this change, since the
allocation
pattern shifts the race. It is ignored now, as `SecurityTest` already
does, and
25 of 25 runs pass.
### Verification
Built and ran `lib/cpp` and `lib/c_glib` against three libraries -- the
distribution **OpenSSL 3.0.2**, a from-source **4.0.2**, and a from-source
**3.0.2 configured with `enable-ssl3`** so that the `SSLv3` arm, which is
compiled out everywhere else, is covered too:
| | 3.0.2 | 4.0.2 | 3.0.2 +ssl3 |
|---|---|---|---|
| `SecurityTest` | 16/16 cells | 16/16 cells | 25/25 cells |
| `SecurityFromBufferTest` | 16/16 cells | 16/16 cells | -- |
| `TSSLSocketMatchNameTest` | pass, 25/25 stable | pass, 15/15 stable | -- |
| `testtransportsslsocket` | 7/7 | 7/7 | -- |
The matrix results are identical across versions and cell-for-cell identical
to
an unpatched-master baseline built against the same libraries. Unpatched
master
against 4.0.2 fails to compile with 7 errors in C++ and 5 in c_glib.
**Not verified here: LibreSSL.** The guard shapes are @brad0's unchanged,
and he
confirmed #3761 built on OpenBSD; a re-confirmation on this branch would be
welcome.
### Credit
- @sebastianas -- #3761, the original patch and the thread
- @jaipaulcheernam -- #3752, the `TLS_method()` + min/max approach and the
`const` work
- @brad0 -- #3736, the LibreSSL guards, kept on the outer `#if` as in his
force-push
- @loqs -- the `OPENSSL_NO_SSL3` test guard and the c_glib context test cases
- @FinnRG -- the switch-computes-version structure and the
`set_*_proto_version()` return-value check
### Deliberately out of scope
`lib/d` still will not link against libcrypto 4.0: `ERR_remove_state(0)` is
ungated at `transport/ssl.d:144` and `async/ssl.d:216`, and
`ASN1_STRING_data` at
`internal/ssl.d:134` (`transport/ssl.d:503` *is* correctly guarded). That is
the
same class of breakage as this ticket but a different `Client:`, so it wants
its
own ticket -- I can file one.
Three smaller observations, noted rather than changed: `SSL_set1_host` is
newly
deprecated in 4.0 at `thrift_ssl_socket.c:446` (a warning, not a removal);
`testtransportsslsocket` is listed in `Makefile.am` but not in
`lib/c_glib/test/CMakeLists.txt`, so `ctest` never runs it -- wiring it in
works
and all 7 cases pass, but that is a pre-existing build-system gap; and there
is no
c_glib CI job at all, while `cmake.yml` builds the compiler only
(`-DBUILD_LIBRARIES=OFF`), so nothing in CI exercises c_glib either way.
### Note on `make style`
Not run. `.clang-format` has drifted far from these files: on unmodified
master
it wants to rewrite 285 lines of `TSSLSocket.cpp` and 293 of
`SecurityTest.cpp`
(which is 4-space/Allman throughout while `.clang-format` is
2-space/attached),
and it mangles the commented-out `SSLv2` line in the enum. The change follows
each file's local convention instead; no hard tabs and no trailing whitespace
were introduced.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
