Fix fallback cert handling and improve cert testing infrastructure.
Review Request #15188 — Created July 21, 2026 and updated
I recently landed a change for HTTPS handling, but it was an older
version of the branch that was missing fixes for legacy cert handling
and testing.The backwards-compatibility logic for migrating old PEM data to modern
cert handling had some flaws. It attempted to build a standalone SSL
context, which wasn't safe or compatible with the way that
CertificateVerificationHTTPSHandlerworked, causing managed
certificates to be omitted.Instead of trying to shoe-horn in a custom context, that logic now
builds extra arguments for controlling hostname checks and extra CA data
for the managed context. The handler class takes these and ensures
they're factored in at the right stage.Hostnames and ports are no longer passed to
CertificateVerificationHTTPSHandler. These weren't available to the
constructor in my prior change, yet were being passed in as part of the
call due to the branch mismatch. These need to be inferred from the URL
in order to get the correct result. This broke several unit tests.Testing infrastructure has several key fixes:
Cert storage is now cleared between tests. This uses the path state
for the cert manager instead of manually building paths, like before.A new
CaptureSSLMixinclass is added for cert-related test classes
that set up the appropriate spies needed to perform SSL context
captures and to do so in a way that handles redirects. It also allows
for control over the expected response.Unit tests no longer hard-code stored cert paths, but instead use the
cert manager's computed state.
Unit tests passed.
Tested with a repository with a legacy self-signed cert. Connected to
my server and saw it worked. The certificate store had the migrated
cert.
| Summary | ID |
|---|---|
| c292dad6cd4d0e7f1f7199c1f0d055568ce4e29d |
| Description | From | Last Updated |
|---|---|---|
|
Please add a new instance variable annotation for this. |
|
|
|
Can you put this in the doc comment next to the definition? It makes more sense there. |
|
|
|
HTTPSHandler.__init__() sets self._context = None, clobbering the context you saved above. https_open() resets it too. If we really want to … |
|
|
|
This applies check_hostname=False and the legacy cadata to all hosts, even ones reached via a redirect. This isn't a new … |
|
|
|
Can we catch SSLError here and reraise a CertificateVerificationError? |
|
|
|
CaptureSSLMixin calls a bunch of kgb stuff, which will flag if its just typed as TestCase. Can we do this … |
|
|
|
Needs a trailing comma |
|
|
|
This (and the associated url attribute) never actually work. AbstractHTTPHandler.do_open() overwrites the response URL, so any URL passed to this … |
|
|
|
We should add a note in this docstring that users of the mixin will need to also mix in kgb.SpyAgency … |
|
|
|
Typo: certificats -> certificates |
|
|
|
This reads a little odd. Can we say "The mock HTTP responses to serve, in order"? |
|
|
|
This is currently not customizable. It gets set in setUp() and then immediately passed to SpyOpReturnInOrder, which creates an internal … |
|
|
|
This constructs a fresh CertificateManager, while stuff in test_hosting_service_http_request.py and test_hosting_service_auth_form.py do from reviewboard.certs.manager import cert_manager. The tests mutate state … |
|
|
|
Copy/pasto |
|
|
|
The TestCase instance will get discarded after teardown, so it seems like this is all just a noop? |
|
|
|
The docstring for this function needs to be updated. |
|
|
|
It's kind of weird to reuse cert_data as a success value. Can we create a separate migrated flag that gets … |
|
|
|
This is now unused. And once you get rid of this declaration, I think the ssl import will also be … |
|
|
|
These seem unused now. |
|
|
|
This seems unused now. |
|
|
|
This docstring was incorrect before, and is also incorrect now. |
|
-
-
-
-
HTTPSHandler.__init__()setsself._context = None, clobbering the context you saved above.https_open()resets it too.If we really want to be able to override this, we should store it separately and apply it (like I tried to do in /r/15185/). That said, the only use of the context parameter was removed in this change (hostingsvcs/base/http.py line 582), so we don't use it anymore. I'd say we just get rid of the parameter.
-
This applies
check_hostname=Falseand the legacy cadata to all hosts, even ones reached via a redirect. This isn't a new regression (the old single-context approach was essentially the same thing), but it's not too ugly to tighten up.In
HostingServiceHTTPRequest.open(), you were previously passing in the request URL and got rid of that in this change. I say let's bring it back for a different job:urlopen_handlers.append(CertificateVerificationHTTPHandler( local_site=hosting_account.local_site, legacy_cert_hostname=urlparse(url).hostname, **ssl_kwargs, ))Then the constructor above can save it, and in here:
if hostname: context = self._cert_manager.build_ssl_context(...) if hostname == self._legacy_cert_hostname: context.check_hostname = self._rb_check_hostname if extra_cert_data := self._extra_cert_data: context.load_verify_locations(cadata=extra_cert_data)test_with_http_to_https_redirect_without_certsis already testing a redirect. We can use that as a prototype for a new test that does something similar with a handler that creates the handler withlegacy_cert_hostname='example.com'and then check that the resulting SSL contexts include two items, and that the first one hascheck_hostname=Falseand the secondcheck_hostname=True. -
-
CaptureSSLMixin calls a bunch of kgb stuff, which will flag if its just typed as
TestCase. Can we do this instead?class TestCaseMixinBase(kgb.SpyAgency, TestCase): pass -
-
This (and the associated
urlattribute) never actually work.AbstractHTTPHandler.do_open()overwrites the response URL, so any URL passed to this will always get overwritten with the actual response URL.That makes the assignment in
test_open_with_legacy_ssl_cert_hostname_mismatcha no-op. It happens to pass because the mocked URL is the same as the response URL, but that's passing for the wrong reason.Probably the best thing is to just get rid of
urlandgeturl()here. -
We should add a note in this docstring that users of the mixin will need to also mix in
kgb.SpyAgencyahead of this. -
-
-
This is currently not customizable. It gets set in
setUp()and then immediately passed toSpyOpReturnInOrder, which creates an internal copy of the list. All the test cases which want to manipulate this are unspying/respying. -
This constructs a fresh
CertificateManager, while stuff in test_hosting_service_http_request.py and test_hosting_service_auth_form.py dofrom reviewboard.certs.manager import cert_manager. The tests mutate state through the global but then read paths off the instance. It works because they're both resolving to the same storage path, but it seems fragile.Can we have this mixin just use the singleton?
-
-
-
-
It's kind of weird to reuse
cert_dataas a success value.Can we create a separate
migratedflag that gets set toTruehere, and then below instead ofif cert_datawe can doif not migrated:? -
This is now unused. And once you get rid of this declaration, I think the ssl import will also be unused.
-
-
-