gh-118658: Return consistent types from `get_un/verified_chain` in `SSLObject` and `SSLSocket`

[matiuszka]

Fixses inconsistent return types between SSLObject and SSLSocket as described in gh-118656

• Issue: gh-118658

[matiuszka]

I restored my old branch from which I created the original PR https://github.com/python/cpython/pull/109113. There is one file missing about NEWS, I am not sure if that was removed on purpose or not.

[matiuszka]

Seems good to me, since these methods are now documented would it make sense to have a test case?

Why not, it will not hurt. I will add it.

[felixfontein]

@matiuszka do you plan to add the test cases soon? I would love to see this PR merged.

[matiuszka]

@matiuszka do you plan to add the test cases soon? I would love to see this PR merged.

Sorry, I forgot about this issue. I will be available next week, so probably next Monday I will have it ready.

[matiuszka]

@sethmlarson, @felixfontein I tried to write tests for it but I am not sure how to start. There are no tests for the SSL module, so there is nothing that I can use to start.

I would need a boilerplate code to see how to execute such a test.

[felixfontein]

I'm not familiar with how the ssl module is tested, but aren't there some tests in https://github.com/python/cpython/blob/main/Lib/test/test_ssl.py ? Maybe adding something to test_internal_chain_client would be a good idea (though that only tests one of the two set of functions).

[matiuszka]

Oh, I overlooked that file. I think I can figure this out from the already-created tests.

[felixfontein]

@matiuszka @sethmlarson hmm, I just saw that Python 3.13.0 RC1 got released, which probably doesn't contain this bugfix. Is it somehow possible to get this into 3.13.0, so that the API won't have a breaking change later on?

[sethmlarson]

This might have a harder time landing immediately now that the RC is here, but I agree that it's something we should get correct for 3.13 so that we don't have to do a silly "backwards incompatible" change to make the APIs match eachother.

@matiuszka This PR still needs a test, is that something you can provide? Once we're there we can signal this to the RM to review this one.

[matiuszka]

@sethmlarson @felixfontein I am just back from vacations. I will try to do it today.

[matiuszka]

@sethmlarson @felixfontein I've added a very simple test for now. It is not as good as I wanted but I have some trouble making it better:

• First of all, I have no idea how to load and parse certificates in raw Python to check if the chains contain the exact certificates that I expect - right now I am checking only the number of certificates and comparing verified vs unverified. Maybe that's enough I am not sure.
• The second problem I have is that in the test we are creating ssl.SSLSocket not ssl.SSLObject and I have no idea how to test the second one. When I was testing this change with my code I saw that when TLS was created in async context ssl.SSLObject was used but I am not familiar with internals.

[matiuszka]

I think I can write some util function that will load the file and extract certificates as bytes. So if that will be worth doing I can do that as well.

[felixfontein]

First of all, I have no idea how to load and parse certificates in raw Python to check if the chains contain the exact certificates that I expect - right now I am checking only the number of certificates and comparing verified vs unverified. Maybe that's enough I am not sure.

You don't have to parse the certificates for that, simply comparing them to the expected ones is enough. I don't know where the certificates come from (I don't really know the tests so I don't know how hard it is to get hold of the expected ones, but comparing should be trivial (you might need https://docs.python.org/3/library/ssl.html#ssl.PEM_cert_to_DER_cert if they are only availabe in PEM format).

The second problem I have is that in the test we are creating ssl.SSLSocket not ssl.SSLObject and I have no idea how to test the second one. When I was testing this change with my code I saw that when TLS was created in async context ssl.SSLObject was used but I am not familiar with internals.

I think for this PR the tests are fine since this PR is about ssl.SSLSocket. Generally having tests for ssl.SSLOBject would also be great. I would do that in a separate PR though so this one can get completed as soon as possible. (I would probably try to add it here, or similar to that place: https://github.com/python/cpython/blob/c8c8fcbed87f95fa4959e14ee2770a1be10de87d/Lib/test/test_ssl.py#L1780)

[matiuszka]

@felixfontein I applied your remarks.

[sethmlarson]

As far as backporting to 3.13, I'll tag in @Yhg1s. The gist is that this API solidified, but didn't get applied to both places where peer certificates can be fetched. Not backporting this will be confusing to other Python implementations which will start implementing the API now that it's public in 3.13 and potentially leave the APIs in a strange place wrt backwards compatibility.

There are only two projects which used these APIs that I'm aware of prior to their public availability and they both should be robust to a backport of this API.

[felixfontein]

I assume you mean https://github.com/sethmlarson/truststore/pull/137/files#diff-64ad03ae2eedc4414b98845398d17597aa584eccc854cabd4efe9f9445e6a9faR285 and https://github.com/ansible-collections/community.crypto/pull/784/files#diff-635a318cc493470aa9c8734ff6623a4ff998bf00cf457197469268f21bea52faR524 ?

[miss-islington-app[bot]]

Thanks @matiuszka for the PR, and @Yhg1s for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13. 🐍🍒⛏🤖

[bedevere-app[bot]]

GH-123082 is a backport of this pull request to the 3.13 branch.

[kanavin]

Sadly, I think this has to be reverted, as it broke certificate generation script, and doesn't contain any other way to generate cert3.pem: https://github.com/python/cpython/pull/107594#issuecomment-2376821053

Alternatively, you need to produce a fix to that script that includes correct regeneration of cert3.pem.

[felixfontein]

@kanavin how about simply disabling the broken test instead of reverting the whole PR? It would be pretty sad if the interface would be re-broken by reverting this.

[felixfontein]

I looked at the certs; cert3.pem is simply the last certificate in keycert3.pem, so extracting it from there should be easy.

[felixfontein]

#124598 should fix Lib/test/certdata/make_ssl_certs.py to extract cert3.pem from keycert3.pem.

[felixfontein]

~~#124599 is simpler and modifies cert3.pem (by adding the comment that's part of keycert3.pem). It shouldn't affect the test.~~ Unfortunately it doesn't work since the comment breaks PEM_cert_to_DER_cert...