testSNISupport in tests/Transport/BaseTestCase.php is meant to check that our transports send Server Name Indication. It does not do that anymore, and has been marked as skipped until it can be made to do so again.
Why it does not test anything
The test works by requesting an HTTPS host and expecting a 200. That only proves something if the host serves a different certificate when no SNI is sent, so that a client which omits it fails. The docblock said as much:
humanmade.com (owned by Human Made and used with permission) points to CloudFront, and will fail if SNI isn't sent.
That is no longer true. Comparing the certificate served with and without SNI:
humanmade.com with SNI: hm-marketing-prod.altis.cloud without: identical
wordpress.org with SNI: wordpress.org without: identical
www.php.net with SNI: www.php.net without: identical
requests-test-server.onrender.com with SNI: *.onrender.com without: identical
None of them distinguishes the two cases, so a transport which silently stopped sending SNI would still pass the test. sni.velox.ch, which used to be the go-to host for this sort of check, no longer resolves at all.
This is unlikely to be fixable by picking a different public host: servers these days generally present a usable default certificate whether or not SNI is sent.
Why it is skipped rather than repointed
The host it currently uses answers 403 to every client, including a plain curl, so the test fails for reasons which have nothing to do with the library. Swapping in another host would make the suite green again, but it would also make the test look like it covers something it does not, so skipping it is the more honest state to leave it in.
Worth noting the domain has already been changed once for a similar reason: feelingrestful.com was decommissioned in 2019 (fcb184a), which is how we ended up on the current one.
Ways forward
These are not mutually exclusive:
- Restore real coverage with a local TLS endpoint serving two certificates on one port, so a request without SNI receives the wrong one and fails. mitmproxy is already a test dependency and can terminate TLS, so the pieces are largely there. This would also take a third party out of the test entirely.
- Rename the test to describe what it actually does and drop the SNI framing, so the name stops implying coverage we do not have.
Related
SNI_enabled, and its decoupling from the verifyname option in 213f7e7, currently has no test which would catch a regression.
testSNISupportintests/Transport/BaseTestCase.phpis meant to check that our transports send Server Name Indication. It does not do that anymore, and has been marked as skipped until it can be made to do so again.Why it does not test anything
The test works by requesting an HTTPS host and expecting a
200. That only proves something if the host serves a different certificate when no SNI is sent, so that a client which omits it fails. The docblock said as much:That is no longer true. Comparing the certificate served with and without SNI:
None of them distinguishes the two cases, so a transport which silently stopped sending SNI would still pass the test.
sni.velox.ch, which used to be the go-to host for this sort of check, no longer resolves at all.This is unlikely to be fixable by picking a different public host: servers these days generally present a usable default certificate whether or not SNI is sent.
Why it is skipped rather than repointed
The host it currently uses answers
403to every client, including a plaincurl, so the test fails for reasons which have nothing to do with the library. Swapping in another host would make the suite green again, but it would also make the test look like it covers something it does not, so skipping it is the more honest state to leave it in.Worth noting the domain has already been changed once for a similar reason:
feelingrestful.comwas decommissioned in 2019 (fcb184a), which is how we ended up on the current one.Ways forward
These are not mutually exclusive:
Related
SNI_enabled, and its decoupling from theverifynameoption in 213f7e7, currently has no test which would catch a regression.