Jens-G opened a new pull request, #3876: URL: https://github.com/apache/thrift/pull/3876
JIRA: [THRIFT-6282](https://issues.apache.org/jira/browse/THRIFT-6282) Client: erl The second point of THRIFT-6268, split out as THRIFT-6282: https for the HTTP transport. **Stacked on #3875 (THRIFT-6283).** Both change `thrift_http_transport:request/2`, so this branch is built on that one. Until #3875 is merged, this PR also shows its commit `3f1320881`. Only the top commit, `f702b70b3`, belongs here. `thrift_http_transport` built its URL as `"http://"; ++ Host ++ Path`, so it could not reach a server that only speaks TLS. ### Change - **Scheme.** `new/3` takes a `{scheme, http | https}` option; `http` stays the default. Other values are refused as `invalid_option`. - **TLS defaults.** For https, httpc receives `{ssl, Defaults ++ CallerTlsOptions}`, where `CallerTlsOptions` is the caller's own `{ssl, _}` from `http_options`. - The defaults are what `httpc:ssl_verify_host_options(true)` gives: `{verify, verify_peer}`, the system's trusted roots, and https host name matching (wildcards included). - httpc applies these itself only from OTP 26 on, and only when it is given no TLS options at all. OTP 25 checks nothing by default. - **The caller's options win.** httpc passes the list to `ssl:connect/3` unchanged, and that function honours the last occurrence of a duplicated option. So each option the caller gives replaces the matching default: `{verify, verify_none}` still works, and so does a `server_name_indication`. - **Roots, handled as in `thrift_sslsocket_transport` (THRIFT-6184):** - The system's roots are left out when the caller names roots with `cacerts` or `cacertfile`. Otherwise a system `{cacerts, _}` would win over the caller's `{cacertfile, _}`. - They are also left out when the host has no trust store (`public_key:cacerts_get/0` raises). The request is then refused by ssl, instead of crashing or going ahead unchecked. **This goes one step further than the ticket's suggestion.** The ticket applies the defaults only when the caller gives no TLS options. Here they are also applied underneath options the caller does give. Otherwise a caller who passes, say, only a client certificate would get no check on OTP 25, and a refused connection on OTP 26 and later (`verify_peer` without roots). The socket transport already works this way. The ticket's measured claims held: OTP 25 connects to an untrusted server when no options are given, and OTP 26+ refuses it. No correction was needed. ### Tests An https test server uses certificates that `public_key:pkix_test_data/1` makes for each test: EC keys, signed with SHA-256, since TLS 1.3 does not accept the SHA-1 default. Its roots are in no system store. The server reads requests with the same code as the plain test servers, which now accepts TLS sockets too. - A server whose root the caller trusts is reached. - **Without TLS options:** the certificate is refused with `unknown_ca`, and no request reaches the server. - **Wrong host name, trusted root:** refused. The alert is `handshake_failure` on OTP 25/26 and `bad_certificate` on 27/28, so the test checks the description for `hostname_check_failed`. This also holds when the caller names only the roots. - **Wildcard certificate:** matched as https matches it. The name to check is given as `server_name_indication`, because the test server can only be reached as `localhost`. On OTP 25–29, the default match without the https match function refuses such a certificate (checked separately). - **Caller options:** - `{verify, verify_none}` replaces the default check. - A root given as `cacertfile` is not shadowed by the system's. - **No trust store** (`public_key:cacerts_get/0` mocked to raise): the request is refused, and the transport does not crash. - **Scheme option:** an explicit `{scheme, http}` works, and invalid schemes are refused. Results: - **On the #3875 code:** 9 of the 12 new tests fail. The three that pass are the invalid-scheme checks, since `scheme` is an unknown option there. - **`rebar3 eunit`:** 409 tests, 0 failures on OTP 25.3.2.21, 25.3.2.9 (rebar3 3.18.0), 26.2.5.21, 27.3.4.17 and 28.5.0.6. Repeated runs on the final code: 4 on 27 and on 25.3.2.21, 3 each on 26, 28 and 25.3.2.9. - **Mutations**, each detected: - Always using `http://` fails all 7 https tests that expect to reach the server or to be refused for a certificate reason. - Dropping the defaults fails, on OTP 25: - the default check; - the host name check with caller roots; - the no-trust-store test. - On OTP 28, the same mutation fails only the wildcard and no-trust-store tests, because httpc's own OTP 26+ defaults cover the others. - Adding the system roots despite the caller's is caught by the `cacertfile` test. - Putting the defaults after the caller's options is caught by the `verify_none` test. - Dropping the https match function is caught by the wildcard test, on 25 and 28. - Letting `cacerts_get/0` raise is caught by the no-trust-store test. That test also catches the missing defaults, for a different reason: without them, httpc's own default on OTP 26+ raises. - **Dialyzer:** no warnings. - **erlfmt:** `rebar3 fmt -c` reports nothing for the changed files. ### Threat model Checked against `doc/thrift-threat-model.md`. The change adds a TLS client path. It checks the server's certificate by default on every OTP release, the same way the Erlang socket transport does since THRIFT-6184. The document is not changed. The change was written with AI assistance (Claude Opus 5). 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
