gh-96931: Fix incorrect results in ssl.SSLSocket.shared_ciphers by benfogle · Pull Request #96932 · python/cpython

FWIW, I would suggest you all just deprecate this method. I've never seen anything use this method across Google (other than urllib3, but that just re-exports it... nothing seems to use the re-exported version). Paging through Debian code search, I see only one user, which just uses it in some debug logs.

First, the meaning of this method seems also to have been lost over time. And this PR in particular seems to run into an ambiguous meaning of the word "shared". :-)

Client cipher list

Looking back, this seems to date to 4cb1781, which aimed to fix #67375. Interestingly, #67375 (comment) talks about seeing the complete client list, not the list intersected with the server list. I.e. SSL_get_client_ciphers.

It was implemented by reading ssl->session->ciphers. This was at a time when OpenSSL internal structures were exposed, and OpenSSL was particularly sloppy about what state it stored on what object (more on this later). And so it did indeed mostly implement what it claimed. I say mostly because if the cipher suite is unrecognized by OpenSSL, it would be omitted. OpenSSL, of course, does not have SSL_CIPHERs for cipher suites that it doesn't know about.

Configured cipher list

Then 598894f ported to OpenSSL 1.1.0. However, in doing so, it switched to SSL_get_ciphers instead of SSL_get_client_ciphers. This is now the configured list of ciphers, rather than the peer ciphers.

Intersected cipher list

Then we have this PR, which switches it to yet another interpretation, the intersection between client and server ciphers. I think the root mixup is "list of ciphers shared by the client" here did not mean "the ciphers which you and the client share". It meant "the list of ciphers that the client shared with you"!

What to do

So, which meaning do you take? Now, I think of the three behaviors this function has had over the years, the intersected is the most useless. I've done TLS things my whole career, and I cannot imagine a scenario where you'd want this, apart from cipher negotiation. But cipher negotiation is done in OpenSSL anyway. By the time Python gets a pop at it, only the final cipher matters.

You could switch to SSL_get_client_ciphers and restore the original intent, but I'd actually argue you should deprecate this. First, given this method was broken not two years after it was introduced, clearly no one actually cared about the original semantics. :-)

More importantly, having this method cements a memory cost to the whole open source ecosystem. The problem with returning either the client cipher list (as a server) or the intersected cipher list is that merely offering this API implies you retain that cipher list for the duration of a connection. Per-connection memory on a server with many, many connections is precious, and there's really no reason to retain this list after the handshake.

(Of course, whether you all use that function, OpenSSL retains the list in memory anyway, so it won't actually help you. But I don't think OpenSSL should retain the memory either, and removing the method at least removes Python from the list of reasons why OpenSSL needs to do this. 😄 And if you ever change your TLS strategy, this method will constrain you here.)

Context: I came across this because @Yhg1s pointed me at an incompatibility between this patch and BoringSSL. In BoringSSL, to reduce per-connection memory, we never added SSL_get_client_ciphers and don't retain the peer cipher list after the final cipher is chosen. This is the first we've ever seen someone use it. We decided to just patch this feature out of our CPython, as it appears to be unused. To be clear, I'm not requesting you to anything to help with that! We can just carry this patch locally. Per the postscript here, we don't like to burden the open source ecosystem with BoringSSL support. BoringSSL-only issues shouldn't be your problem. But the underlying memory constraints and pointlessness of the method are general, so I thought I'd pass this note along.