slachiewicz commented on PR #3818: URL: https://github.com/apache/thrift/pull/3818#issuecomment-5582456680
@Jens-G — flagging this for you, since it lands on [d6782a87](https://github.com/apache/thrift/commit/d6782a879c9bd5961e106ad8211d91d40d9efa55), and there is something in it worth knowing beyond this PR. Master has been red since that commit: [run 34190474984](https://github.com/apache/thrift/actions/runs/34190474984) fails where [the run before it](https://github.com/apache/thrift/actions/runs/34168141419) passed. All 183 failures are `ip-ssl` with a Python server — py-py, py-cpp, py-rb, py-rs, py-nodejs and py-kotlin — and every other job in the workflow is green. The cause is the bug this PR fixes plus one it does not. Your change swapped the 3.12 branch of the shim, which was `def match(cert, hostname): return True`, for a real check, so a comparison that had never actually run in CI started running. Two things then stop it passing: 1. The peer arrives IPv4-mapped as `::ffff:127.0.0.1` while certificates carry `127.0.0.1`, and `ipaddress` calls those different addresses. That is [THRIFT-6201](https://issues.apache.org/jira/browse/THRIFT-6201), fixed here. 2. [`test/keys/client.crt`](https://github.com/apache/thrift/blob/master/test/keys/client.crt) — which `client.p12` wraps and the cross-test clients present — carries no extensions at all, so there is no `subjectAltName` for the matcher to read. That is deliberate: [make-serverkey.sh:103](https://github.com/apache/thrift/blob/master/test/keys/keygen/make-serverkey.sh#L103) signs it without `-extensions v3_req`, with `client_v3.crt` as the variant that has them. So this PR alone will not turn CI green, and the second half is yours to steer rather than mine to guess at. The question underneath it is whether `TSSLServerSocket` should match a client certificate against the address the connection arrived from at all: a client behind NAT or a proxy can never satisfy that, and on Python 3.11 and earlier the same call would have rejected `client.crt` too, so the fixture and the check have disagreed for a long time without anyone noticing. 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. Say which way you want it and I will write the follow-up. *This comment was created 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]
