test_https_bearer_token_authorization fails against latest Twisted

[exarkun]

[ERROR]
Traceback (most recent call last):
Failure: testtools.testresult.real._StringException: Traceback (most recent call last):
  File "/home/exarkun/Work/LeastAuthority/txkube/src/txkube/test/test_authentication.py", line 221, in test_https_bearer_token_authorization
    [connection] = reactor.sslClients
ValueError: need more than 0 values to unpack

txkube.test.test_authentication.AuthenticateWithServiceAccountTests.test_https_bearer_token_authorization

Against Twisted 17.9.0 (and probably some earlier versions)

[exarkun]

It also fails with Twisted 17.1 and 17.5. Cannot test txkube master@HEAD against Twisted 16.6.0 because of other incompatibilities that have been introduced but txkube 080a406f19a922f4142f7480af0f63d367b58754 (which is mostly the same in the ways that matter) passes this test against Twisted 16.6 so I guess the problem was introduced between 16.6 and 17.1.

[exarkun]

The test fails against Twisted 63dc0387ae49134f68305873a822a76a3367d897 and passes against the parent of that revision. That revision changes Agent from using SSL4ClientEndpoint for https scheme resources to using wrapClientTLS. The former uses IReactorSSL.connectSSL and the latter uses the in-memory TLS implementation on top of IReactorTCP.connectTCP.

[exarkun]

Okay, test_https_bearer_token_authorization as currently written has some shortcomings.

• The proximate cause of this failure, it assumes that an https scheme resource will be retrieved by establishing a TLS connection using IReactorSSL.connectSSL. Superficially, this doesn't seem like a terrible assumption but the reality is that this is the assumption that has failed us. Agent no longer behaves this way. Looking at the history of TLS in Twisted, it's not terribly surprising either. And it's hard to quantify the "publicness" of this attribute with respect to the Twisted backwards compatibility policy.
• Though the test seems intent on verifying the behavior for https scheme resources, it does almost nothing in the way of validating the TLS-specific behaviors. It completely short-circuits all TLS negotiation so it doesn't actually verify any of the TLS-specific implementation: the careful server certificate selection and TLS context factory creation are executed but nothing asserts anything about the consequences. So basically this test does nothing beyond the http scheme test except encode the above mentioned assumption (now invalid) about how https scheme resources are accessed.
• It is verbose to the point of tedium and duplicates much from _authorized_request.

A better test (or group of tests) would remove the now invalid assumption and verify that the correct server certificate is selected and the TLS context factory is configured to require that certificate. There would also still be validation that the correct Authorization header is sent (or the implementation would be refactored to remove the duplication this validation, already performed once in test_http_bearer_token_authorization).

[exarkun]

The server certificate selection logic and the TLS context factory creation are buried in the middle of authenticate_with_serviceaccount making it difficult to test them independently of the rest (and vice versa). Factoring that logic into a separate function may make testing the different pieces simpler.