Jens-G commented on PR #3818: URL: https://github.com/apache/thrift/pull/3818#issuecomment-5627806418
@slachiewicz — drop it as the default and keep it as an opt-in. Please go ahead with the follow-up. Why: - A client checks the server's certificate against the name it meant to reach. A server has no such reference for its clients: the source address is not something the client asserts, and NAT, proxies, load balancers and container networking rewrite it routinely, so a client certificate often cannot carry the address the server will see. - Python is the only binding that does this by default. C++ and D install their default access manager on client sockets only, and the other bindings with a TLS server leave client-certificate policy to the TLS configuration or to a callback the application supplies. - THRIFT-3599 added the check in 0.10.0 so that a server would not accept just any certificate its CA had signed. That is a fair concern, but which certificates may connect is the application's policy — a `validate_callback` that looks at the subject or the SAN, or a CA that issues only to the clients meant to connect — and the peer address is a poor stand-in for either. - It only concerns servers that request client certificates, since `cert_reqs` defaults to `CERT_NONE`. For those, master as it stands means 0.25.0 starts refusing clients on Python 3.12 and later whose certificates do not list the address the server sees. I would rather settle this before the release than ship that and take it back in the next one. For the follow-up: - A JIRA ticket of its own, as you suggested. - Only `TSSLServerSocket`'s default changes. `TSSLSocket` and `sslcompat._match_hostname` stay as they are, since the client path still relies on them. - `thrift.transport.sslcompat.match_peer_ipaddress` stays, as the documented way to opt back in with `validate_callback=match_peer_ipaddress`. That is also why #3818 is still worth having: a server that opts in should see `::ffff:127.0.0.1` and `127.0.0.1` as the same address. - The `_match_has_ipaddress` check that raises `ValueError` in `TSSLServerSocket.__init__` only makes sense while the matcher is the default. - `lib/py/README.md`, Breaking Changes for 0.25.0: the paragraph on the default `validate_callback` ends by saying that `TSSLServerSocket` validates a client certificate against the address the connection arrived from. That needs rewording, together with a note for servers on Python 3.11 or earlier that relied on the check: passing the callback brings it back for addresses listed as IP SANs, but not the commonName fallback that `ssl.match_hostname` also applied. - `test/keys/README.md` says the same about the Python server and needs the same update. - Tests that run. `TSSLSocketTest`, which holds `test_client_cert`, still carries the unconditional `@unittest.skip`, so no unit test that runs goes through `TSSLServerSocket` at all. New cases belong in a class that runs, like `TSSLSocketHostnameTest`, and should go through `TSSLServerSocket.accept()` rather than the matcher alone: `client.crt`, trusted by the server but carrying no IP SAN, is accepted by default and refused with `validate_callback=match_peer_ipaddress`, and `client_v3.crt` is accepted with it. Neither certificate needs regenerating. *This comment was drafted with AI assistance.* -- 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]
