bpo-36076: Add SNI support to ssl.get_server_certificate.

[juhovh]

Many servers in the cloud environment require SNI to be used during the
SSL/TLS handshake, therefore it is not possible to fetch their certificates
using the ssl.get_server_certificate interface.

This change adds an additional optional hostname argument that can be used to
set the SNI. Note that it is intentionally a separate argument instead of
using the host part of the addr tuple, because one might want to explicitly
fetch the default certificate or fetch a certificate from a specific IP
address with the specified SNI hostname. A separate argument also works better
for backwards compatibility.

https://bugs.python.org/issue36076

Automerge-Triggered-By: GH:tiran

[tiran]

Thanks for the PR.

It's not necessary to add a hostname argument to the function. The addr argument is always a hostname, port pair. You can simply grab the hostname from the first argument.

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

[juhovh]

@tiran Thank you for your comments, I tried to explain in the description why I wanted to make it an additional argument, main reasons being:

• backwards compatibility
• ability to fetch the default certificate without an SNI extension (which might be different from all the certificates served with SNI)
• ability to connect to a specific IP address and fetch a certificate for a specific hostname, without using the system DNS resolver (which will randomly connect to one of the IPs)

Please let me know if this reasoning has any flaws. Simply using the hostname from the addr argument would not have worked for my particular use case (because of the aforementioned reasons), and I am therefore not very interested in that solution. Basically think of e.g. the following code:

ssl.get_server_certificate(("13.225.146.42", 443), hostname="fiba3x3.com")

It is impossible to guarantee connection to that specific IP address of the multiple IP addresses for that domain without having hostname as a separate argument.

[juhovh]

The Azure Pipelines PR tests failing is a real issue, turns out IPv6 tests didn't run locally for me so I will fix those and request for re-review, including review for the above comment.

[tiran]

I'm interested to have sane and sensible default behavior. The sensible default behavior is to reuse the hostname from the addr field to fetch the certificate for the hostname. That is also the behavior that a common user is going to expect.

• ability to fetch the default certificate without an SNI extension (which might be different from all the certificates served with SNI)

That's an advanced feature. You can do if you pass an IP address instead of a hostname as the first argument. The ssl module does not send a SNI when the hostname is an IP address.

• ability to connect to a specific IP address and fetch a certificate for a specific hostname, without using the system DNS resolver (which will randomly connect to one of the IPs)

This is a very advanced feature. IMHO it s fine to not support this use case in Python's stdlib. The standard library does not have to solve all problems. The get_server_certificate function is short and trivial. You can easily build our own advanced implementation with just a couple of lines of code.

[juhovh]

I'm interested to have sane and sensible default behavior. The sensible default behavior is to reuse the hostname from the addr field to fetch the certificate for the hostname. That is also the behavior that a common user is going to expect.

I agree, most users would probably expect this behaviour.

That's an advanced feature. You can do if you pass an IP address instead of a hostname as the first argument. The ssl module does not send a SNI when the hostname is an IP address.

I was actually not aware that there is an additional check for an IP address, but that is good since TLS spec explicitly disallows IP addresses in the SNI extension. Makes things a lot easier.

• ability to connect to a specific IP address and fetch a certificate for a specific hostname, without using the system DNS resolver (which will randomly connect to one of the IPs)

This is a very advanced feature. IMHO it s fine to not support this use case in Python's stdlib. The standard library does not have to solve all problems. The get_server_certificate function is short and trivial. You can easily build our own advanced implementation with just a couple of lines of code.

I'm not going to lie, I'm a little bit disappointed because this happened to be my very advanced use case, and removing it makes this change more or less useless for me. 😄 But as you said, it's just a couple of lines. Anyway, since I have already spent some time on this, and more importantly spent your time on the topic, I have made the requested changes; please review again

[bedevere-bot]

Thanks for making the requested changes!

@tiran: please review the changes made to this pull request.

[tiran]

I'm sorry that it took so long. I like to land this PR before 3.10 feature freeze in 3 weeks. Could you please rebase the PR on latest master?

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[juhovh]

Rebased on the latest master, didn't modify the date on the NEWS.d file, since there were no effective changes to the functionality, let me know if any changes are necessary. I have made the requested changes; please review again

[bedevere-bot]

Thanks for making the requested changes!

@tiran: please review the changes made to this pull request.

[tiran]

Thanks!

[miss-islington]

@juhovh: Status check is done, and it's a success ✅ .

[Vorsku]

Just wanted to express my gratitude for this commit, confirmed this works in 3.10 and it's saved my project from being a non-starter - thanks @juhovh and @tiran