Skip to content

testSNISupport no longer verifies SNI #1077

Description

@schlessera

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:

  1. 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.
  2. 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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions