THRIFT-6201: Reduce IPv4-mapped addresses before matching a peer certificate

[slachiewicz]

JIRA: THRIFT-6201
Client: py

match_peer_ipaddress compares the peer address to the certificate's subjectAltName entries without reducing IPv4-mapped IPv6 addresses. A dual-stack listener reports an IPv4 peer as ::ffff:127.0.0.1, a certificate normally carries IP Address:127.0.0.1, and ipaddress treats those as different addresses, so the peer is refused:

WARNING:thrift.transport.TSSLSocket:Failed to validate client certificate address: ::ffff:127.0.0.1
  File ".../thrift/transport/sslcompat.py", line 112, in match_peer_ipaddress
    raise TTransportException(
TTransportException: Peer address "::ffff:127.0.0.1" is not covered by the certificate it presented

They are the same address, so both sides are reduced with IPv6Address.ipv4_mapped before comparing.

The test drives the matcher directly, in both directions and with two negatives (a different IPv4 address, and ::1 against a certificate for 127.0.0.1).

Verified: test_peer_address_matcher_unmaps_ipv4 raises the message quoted above without the change and passes with it.

Scope

The SSL cross tests pass on master without this change, because 715f46c moved the cross-test clients to test/keys/client_v3.crt, which lists ::ffff:127.0.0.1 alongside 127.0.0.1. A certificate issued outside this repository normally carries only the IPv4 form, and that is the case this fixes.

Whether TSSLServerSocket should match a client certificate against the address the connection arrived from by default is THRIFT-6233, #3839. The matcher stays as the documented opt-in there, so a server that opts in sees both forms as the same address.

The path only began running on Python 3.12 and later in 0.25.0; before that the shim on those versions was a function returning True, so nothing reached the comparison.

This change was created with AI assistance.

[slachiewicz]

@Jens-G, master has been red since d6782a87, and the cause is worth your attention beyond this PR.

Run 34190474984 fails where the run before it passed. All 183 failures use the ip-ssl transport with a Python server (py-py, py-cpp, py-rb, py-rs, py-nodejs, and py-kotlin). Every other job in the workflow is green.

Your change replaced the Python 3.12 branch in the sslcompat.py shim, which returned True unconditionally, with a real check. That comparison had never run in CI before. Two things now stop it from passing:

1. A peer arrives IPv4-mapped as ::ffff:127.0.0.1, certificates carry 127.0.0.1, and the ipaddress module treats those as different addresses. This PR fixes that, tracked as THRIFT-6201.
2. The test/keys/client.crt certificate, which the client.p12 keystore wraps and the cross-test clients present, carries no extensions, so the matcher finds no subjectAltName to read. That is deliberate: make-serverkey.sh:103 signs it without -extensions v3_req, and client_v3.crt is the variant that has them.

So this PR alone does not turn CI green, and I did not want to decide the second point for you. The open question is whether TSSLServerSocket should match a client certificate against the address the connection arrived from at all. A client behind NAT or a proxy cannot satisfy that check. On Python 3.11 and earlier, the same call would have rejected client.crt too, so the fixture has not satisfied the check for a long time and nothing surfaced it.

Keeping the check means giving client.crt a SAN, which changes what that fixture is for. Dropping it for client certificates is a small change. Tell me which you prefer, and I'll write the follow-up.

This comment was created with AI assistance.

[Jens-G]

@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 THRIFT-6201: Reduce IPv4-mapped addresses before matching a peer certificate #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.

[slachiewicz]

Rebased on master (703622c) to pick up the TNonblockingServerTest fix from THRIFT-6244 behind the AppVeyor failure. The diff is unchanged (same patch-id).