Summary

Add a sense of purpose to SSL/TLS certificates.

Review Request #14896 — Created March 11, 2026 and updated April 2, 2026, 8:02 a.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-7.1.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

The original design led to incorrect calls in build_ssl_context() and
therefore build_urlopen_kwargs(), meaning we didn't always have the
right context in place to handle the operation.

This change addresses this by:

Introducing a CertPurpose enum containing TRUST (for
cert trust/verification needs) and CLIENT (for client
authentication).
A purpose field in Certificate and all cert-related operations in
CertificateManager, and storage equivalents in the backends.

A "trust" cert may not have a private key, and a "client" cert must
(although technically that cert could embed the key, we're requiring two
separate files in our implementation for now).

This does change the signatures and expectations in the storage
backends, and we're not going through a deprecation cycle for that. The
reason being that, while we introduced all this in Review Board 6, we
never made actual use of it. Going through a deprecation cycle would
introduce a fair amount of complexity, and isn't worth it for something
that users haven't touched or would have had a need to build upon.

One of the backwards-incompatible changes is the location of files in
the file storage backend. Both kinds of certs used to live in certs/,
but now they live in certs/client/ and certs/trust/ instead.

The change is pretty straight-forward. The biggest change in behavior is
build_ssl_context(), which now passes the right kinds of certs to the
right functions.

The bulk of the change is actually unit tests and testdata file
movement. We now have trust and client purpose variations for each
relevant test.

As a note, there's some initial attempts at modernizing some of the
typing in this change, but it's purposefully incomplete. The rest will
be tackled separately.

Testing Done

All unit tests pass.

Commits

Summary	ID
Add a sense of purpose to SSL/TLS certificates. My original design for SSL/TLS certificate management combined two important purposes into one concept of a certificate. Depending on the call site and the presence of a private key, a `Certificate` could be used for verifying a server or authenticating the client with a server. These are very different purposes, and needed to be managed separately. The original design led to incorrect calls in `build_ssl_context()` and therefore `build_urlopen_kwargs()`, meaning we didn't always have the right context in place to handle the operation. This change addresses this by: 1. Introducing a `CertPurpose` enum containing `TRUST` (for cert trust/verification needs) and `CLIENT` (for client authentication). 2. A `purpose` field in `Certificate` and all cert-related operations in `CertificateManager`, and storage equivalents in the backends. A "trust" cert may not have a private key, and a "client" cert must (although technically that cert could embed the key, we're requiring two separate files in our implementation for now). This does change the signatures and expectations in the storage backends, and we're not going through a deprecation cycle for that. The reason being that, while we introduced all this in Review Board 6, we never made actual use of it. Going through a deprecation cycle would introduce a fair amount of complexity, and isn't worth it for something that users haven't touched or would have had a need to build upon. One of the backwards-incompatible changes is the location of files in the file storage backend. Both kinds of certs used to live in `certs/`, but now they live in `certs/client/` and `certs/purpose/` instead. The change is pretty straight-forward. The biggest change in behavior is `build_ssl_context()`, which now passes the right kinds of certs to the right functions. The bulk of the change is actually unit tests and testdata file movement. We now have trust and client purpose variations for each relevant test.	7853c5eed80c779e4d65c33a3dce5065ee43da0c

Summary

Add a sense of purpose to SSL/TLS certificates.

My original design for SSL/TLS certificate management combined two important purposes into one concept of a certificate. Depending on the call site and the presence of a private key, a `Certificate` could be used for verifying a server or authenticating the client with a server. These are very different purposes, and needed to be managed separately. The original design led to incorrect calls in `build_ssl_context()` and therefore `build_urlopen_kwargs()`, meaning we didn't always have the right context in place to handle the operation. This change addresses this by: 1. Introducing a `CertPurpose` enum containing `TRUST` (for cert trust/verification needs) and `CLIENT` (for client authentication). 2. A `purpose` field in `Certificate` and all cert-related operations in `CertificateManager`, and storage equivalents in the backends. A "trust" cert may not have a private key, and a "client" cert must (although technically that cert could embed the key, we're requiring two separate files in our implementation for now). This does change the signatures and expectations in the storage backends, and we're not going through a deprecation cycle for that. The reason being that, while we introduced all this in Review Board 6, we never made actual use of it. Going through a deprecation cycle would introduce a fair amount of complexity, and isn't worth it for something that users haven't touched or would have had a need to build upon. One of the backwards-incompatible changes is the location of files in the file storage backend. Both kinds of certs used to live in `certs/`, but now they live in `certs/client/` and `certs/purpose/` instead. The change is pretty straight-forward. The biggest change in behavior is `build_ssl_context()`, which now passes the right kinds of certs to the right functions. The bulk of the change is actually unit tests and testdata file movement. We now have trust and client purpose variations for each relevant test.

7853c5eed80c779e4d65c33a3dce5065ee43da0c

Issues

Description	From	Last Updated
Your description says certs/purpose/ where I think it should say certs/trust/	david	March 18, 2026, 11:15 p.m.
'typing.cast' imported but unused Column: 1 Error code: F401	reviewbot	March 11, 2026, 10:28 p.m.
too many blank lines (2) Column: 5 Error code: E303	reviewbot	March 11, 2026, 10:28 p.m.
local variable 'local_site1' is assigned to but never used Column: 9 Error code: F841	reviewbot	March 11, 2026, 10:28 p.m.
This should use typelets.runtime.raise_invalid_type() so type checkers don't flag this branch as unreachable.	david	March 18, 2026, 11:17 p.m.
It seems like this should happen before we validate key_path	david	March 18, 2026, 11:22 p.m.
This should use typelets.runtime.raise_invalid_type() so type checkers don't flag this branch as unreachable.	david	March 18, 2026, 11:23 p.m.
Since this class is already an ABC subclass, perhaps we should wrap this method in @abstractmethod, so that it's not …	david	March 18, 2026, 11:24 p.m.
This doesn't seem right. _build_fingerprints_file_path doesn't take a purpose argument, and should still return <hostname>__<port>.json .	david	March 18, 2026, 11:25 p.m.
This isn't a new issue with the change, but a bunch of the docstrings here say create_form_files instead of create_from_files	david	March 18, 2026, 11:26 p.m.
Mind fixing this while you're here? founD -> found	david	April 1, 2026, 9:43 p.m.
Can we log something if there's a client cert path but no key file?	david	April 1, 2026, 9:45 p.m.
Purpose is required, so we can get rid of "if specified" in here.	david	April 1, 2026, 9:45 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/certs/tests/test_certificate_manager.py (Diff revision 1)
The issue has been resolved. Show all issues
```
'typing.cast' imported but unused

Column: 1
Error code: F401
```
reviewboard/certs/tests/test_file_storage_backend.py (Diff revision 1)
The issue has been resolved. Show all issues
```
too many blank lines (2)

Column: 5
Error code: E303
```
reviewboard/certs/tests/test_file_storage_backend.py (Diff revision 1)
The issue has been resolved. Show all issues
```
local variable 'local_site1' is assigned to but never used

Column: 9
Error code: F841
```

Change Summary:

Removed an unused import, an unused variable, and a blank line.

Commits:

	Summary	ID
	Add a sense of purpose to SSL/TLS certificates. My original design for SSL/TLS certificate management combined two important purposes into one concept of a certificate. Depending on the call site and the presence of a private key, a `Certificate` could be used for verifying a server or authenticating the client with a server. These are very different purposes, and needed to be managed separately. The original design led to incorrect calls in `build_ssl_context()` and therefore `build_urlopen_kwargs()`, meaning we didn't always have the right context in place to handle the operation. This change addresses this by: 1. Introducing a `CertPurpose` enum containing `TRUST` (for cert trust/verification needs) and `CLIENT` (for client authentication). 2. A `purpose` field in `Certificate` and all cert-related operations in `CertificateManager`, and storage equivalents in the backends. A "trust" cert may not have a private key, and a "client" cert must (although technically that cert could embed the key, we're requiring two separate files in our implementation for now). This does change the signatures and expectations in the storage backends, and we're not going through a deprecation cycle for that. The reason being that, while we introduced all this in Review Board 6, we never made actual use of it. Going through a deprecation cycle would introduce a fair amount of complexity, and isn't worth it for something that users haven't touched or would have had a need to build upon. One of the backwards-incompatible changes is the location of files in the file storage backend. Both kinds of certs used to live in `certs/`, but now they live in `certs/client/` and `certs/purpose/` instead. The change is pretty straight-forward. The biggest change in behavior is `build_ssl_context()`, which now passes the right kinds of certs to the right functions. The bulk of the change is actually unit tests and testdata file movement. We now have trust and client purpose variations for each relevant test.	da5fa26364cb96ef5e3380c4769a5f3083287366
	Add a sense of purpose to SSL/TLS certificates. My original design for SSL/TLS certificate management combined two important purposes into one concept of a certificate. Depending on the call site and the presence of a private key, a `Certificate` could be used for verifying a server or authenticating the client with a server. These are very different purposes, and needed to be managed separately. The original design led to incorrect calls in `build_ssl_context()` and therefore `build_urlopen_kwargs()`, meaning we didn't always have the right context in place to handle the operation. This change addresses this by: 1. Introducing a `CertPurpose` enum containing `TRUST` (for cert trust/verification needs) and `CLIENT` (for client authentication). 2. A `purpose` field in `Certificate` and all cert-related operations in `CertificateManager`, and storage equivalents in the backends. A "trust" cert may not have a private key, and a "client" cert must (although technically that cert could embed the key, we're requiring two separate files in our implementation for now). This does change the signatures and expectations in the storage backends, and we're not going through a deprecation cycle for that. The reason being that, while we introduced all this in Review Board 6, we never made actual use of it. Going through a deprecation cycle would introduce a fair amount of complexity, and isn't worth it for something that users haven't touched or would have had a need to build upon. One of the backwards-incompatible changes is the location of files in the file storage backend. Both kinds of certs used to live in `certs/`, but now they live in `certs/client/` and `certs/purpose/` instead. The change is pretty straight-forward. The biggest change in behavior is `build_ssl_context()`, which now passes the right kinds of certs to the right functions. The bulk of the change is actually unit tests and testdata file movement. We now have trust and client purpose variations for each relevant test.	df8e4659369631fe00d7778df9f6890bd91781b3

Diff:

Revision 2 (+5444 -1576)

Show changes

	reviewboard/certs/cert.py
	reviewboard/certs/manager.py
	reviewboard/certs/storage/base.py
	reviewboard/certs/storage/file_storage.py
	reviewboard/certs/tests/test_certificate.py
	reviewboard/certs/tests/test_certificate_fingerprints.py
	reviewboard/certs/tests/test_certificate_manager.py
	reviewboard/certs/tests/test_file_storage_backend.py
	18 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Your description says certs/purpose/ where I think it should say certs/trust/

reviewboard/certs/cert.py (Diff revision 2)

The issue has been resolved. Show all issues

This should use typelets.runtime.raise_invalid_type() so type checkers don't flag this branch as unreachable.

reviewboard/certs/cert.py (Diff revision 2)

The issue has been dropped. Show all issues

It seems like this should happen before we validate key_path

chipx86 March 18, 2026, 11:28 p.m.

I have this organized as:

Argument combinations (it's not key_path, it's the combination of purpose + key_path, and if we ever support passwords for keys then we'd include that here too).
Cert and key path values (including existence).
Contents of the files we loaded.

reviewboard/certs/cert.py (Diff revision 2)

The issue has been resolved. Show all issues

This should use typelets.runtime.raise_invalid_type() so type checkers don't flag this branch as unreachable.

reviewboard/certs/storage/base.py (Diff revision 2)

The issue has been dropped. Show all issues

Since this class is already an ABC subclass, perhaps we should wrap this method in @abstractmethod, so that it's not even possible to define a subclass that doesn't implement this?

chipx86 March 18, 2026, 11:28 p.m.

The challenge with this (and something I want to address in housekeeping) is that we then break interfaces at import time rather than runtime, which completely alters the entire backwards-compatibility story. If this were, say, an SCMTool, then doing this would break any legacy tools right out of the box. I'd rather avoid this until we have a better solution we can use that preserves this.

reviewboard/certs/storage/file_storage.py (Diff revision 2)

The issue has been resolved. Show all issues

This doesn't seem right. _build_fingerprints_file_path doesn't take a purpose argument, and should still return <hostname>__<port>.json .

chipx86 March 18, 2026, 11:28 p.m.

Holdover from an earlier attempt at this work. All these are wrong. Fixing.

reviewboard/certs/tests/test_certificate.py (Diff revision 2)

The issue has been resolved. Show all issues

This isn't a new issue with the change, but a bunch of the docstrings here say create_form_files instead of create_from_files

Change Summary:


Made use of raise_invalid_type().
Fixed the file path references for the paths that ship with this change.
Fixed some typos in unit test docstrings.

Description:

		My original design for SSL/TLS certificate management combined two
		important purposes into one concept of a certificate. Depending on the
		call site and the presence of a private key, a `Certificate` could be
		used for verifying a server or authenticating the client with a server.
		These are very different purposes, and needed to be managed separately.

		The original design led to incorrect calls in `build_ssl_context()` and
		therefore `build_urlopen_kwargs()`, meaning we didn't always have the
		right context in place to handle the operation.

		This change addresses this by:

		Introducing a `CertPurpose` enum containing `TRUST` (for cert trust/verification needs) and `CLIENT` (for client authentication).
		A `purpose` field in `Certificate` and all cert-related operations in `CertificateManager`, and storage equivalents in the backends.

		A "trust" cert may not have a private key, and a "client" cert must
		(although technically that cert could embed the key, we're requiring two
		separate files in our implementation for now).

		This does change the signatures and expectations in the storage
		backends, and we're not going through a deprecation cycle for that. The
		reason being that, while we introduced all this in Review Board 6, we
		never made actual use of it. Going through a deprecation cycle would
		introduce a fair amount of complexity, and isn't worth it for something
		that users haven't touched or would have had a need to build upon.

		One of the backwards-incompatible changes is the location of files in
		the file storage backend. Both kinds of certs used to live in `certs/`,
~		but now they live in `certs/client/` and `certs/purpose/` instead.
	~	but now they live in `certs/client/` and `certs/trust/` instead.

		The change is pretty straight-forward. The biggest change in behavior is
		`build_ssl_context()`, which now passes the right kinds of certs to the
		right functions.

		The bulk of the change is actually unit tests and testdata file
		movement. We now have trust and client purpose variations for each
		relevant test.

		As a note, there's some initial attempts at modernizing some of the
		typing in this change, but it's purposefully incomplete. The rest will
		be tackled separately.

Commits:

	Summary	ID
	Add a sense of purpose to SSL/TLS certificates. My original design for SSL/TLS certificate management combined two important purposes into one concept of a certificate. Depending on the call site and the presence of a private key, a `Certificate` could be used for verifying a server or authenticating the client with a server. These are very different purposes, and needed to be managed separately. The original design led to incorrect calls in `build_ssl_context()` and therefore `build_urlopen_kwargs()`, meaning we didn't always have the right context in place to handle the operation. This change addresses this by: 1. Introducing a `CertPurpose` enum containing `TRUST` (for cert trust/verification needs) and `CLIENT` (for client authentication). 2. A `purpose` field in `Certificate` and all cert-related operations in `CertificateManager`, and storage equivalents in the backends. A "trust" cert may not have a private key, and a "client" cert must (although technically that cert could embed the key, we're requiring two separate files in our implementation for now). This does change the signatures and expectations in the storage backends, and we're not going through a deprecation cycle for that. The reason being that, while we introduced all this in Review Board 6, we never made actual use of it. Going through a deprecation cycle would introduce a fair amount of complexity, and isn't worth it for something that users haven't touched or would have had a need to build upon. One of the backwards-incompatible changes is the location of files in the file storage backend. Both kinds of certs used to live in `certs/`, but now they live in `certs/client/` and `certs/purpose/` instead. The change is pretty straight-forward. The biggest change in behavior is `build_ssl_context()`, which now passes the right kinds of certs to the right functions. The bulk of the change is actually unit tests and testdata file movement. We now have trust and client purpose variations for each relevant test.	df8e4659369631fe00d7778df9f6890bd91781b3
	Add a sense of purpose to SSL/TLS certificates. My original design for SSL/TLS certificate management combined two important purposes into one concept of a certificate. Depending on the call site and the presence of a private key, a `Certificate` could be used for verifying a server or authenticating the client with a server. These are very different purposes, and needed to be managed separately. The original design led to incorrect calls in `build_ssl_context()` and therefore `build_urlopen_kwargs()`, meaning we didn't always have the right context in place to handle the operation. This change addresses this by: 1. Introducing a `CertPurpose` enum containing `TRUST` (for cert trust/verification needs) and `CLIENT` (for client authentication). 2. A `purpose` field in `Certificate` and all cert-related operations in `CertificateManager`, and storage equivalents in the backends. A "trust" cert may not have a private key, and a "client" cert must (although technically that cert could embed the key, we're requiring two separate files in our implementation for now). This does change the signatures and expectations in the storage backends, and we're not going through a deprecation cycle for that. The reason being that, while we introduced all this in Review Board 6, we never made actual use of it. Going through a deprecation cycle would introduce a fair amount of complexity, and isn't worth it for something that users haven't touched or would have had a need to build upon. One of the backwards-incompatible changes is the location of files in the file storage backend. Both kinds of certs used to live in `certs/`, but now they live in `certs/client/` and `certs/purpose/` instead. The change is pretty straight-forward. The biggest change in behavior is `build_ssl_context()`, which now passes the right kinds of certs to the right functions. The bulk of the change is actually unit tests and testdata file movement. We now have trust and client purpose variations for each relevant test.	e03270282a569a1a8251bd2d57dc1f718cd1e828

Diff:

Revision 3 (+5456 -1578)

Show changes

	reviewboard/certs/cert.py
	reviewboard/certs/manager.py
	reviewboard/certs/storage/base.py
	reviewboard/certs/storage/file_storage.py
	reviewboard/certs/tests/test_certificate.py
	reviewboard/certs/tests/test_certificate_fingerprints.py
	reviewboard/certs/tests/test_certificate_manager.py
	reviewboard/certs/tests/test_file_storage_backend.py
	18 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/certs/cert.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Mind fixing this while you're here? founD -> found
```

reviewboard/certs/manager.py (Diff revision 3)

The issue has been resolved. Show all issues

Can we log something if there's a client cert path but no key file?

reviewboard/certs/storage/base.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Purpose is required, so we can get rid of "if specified" in here.
```

Change Summary:


Updated versions to RB 8.0.
Fixed some issues in docs.
Added logging if a client cert was found but a key was not.

Commits:

	Summary	ID
	Add a sense of purpose to SSL/TLS certificates. My original design for SSL/TLS certificate management combined two important purposes into one concept of a certificate. Depending on the call site and the presence of a private key, a `Certificate` could be used for verifying a server or authenticating the client with a server. These are very different purposes, and needed to be managed separately. The original design led to incorrect calls in `build_ssl_context()` and therefore `build_urlopen_kwargs()`, meaning we didn't always have the right context in place to handle the operation. This change addresses this by: 1. Introducing a `CertPurpose` enum containing `TRUST` (for cert trust/verification needs) and `CLIENT` (for client authentication). 2. A `purpose` field in `Certificate` and all cert-related operations in `CertificateManager`, and storage equivalents in the backends. A "trust" cert may not have a private key, and a "client" cert must (although technically that cert could embed the key, we're requiring two separate files in our implementation for now). This does change the signatures and expectations in the storage backends, and we're not going through a deprecation cycle for that. The reason being that, while we introduced all this in Review Board 6, we never made actual use of it. Going through a deprecation cycle would introduce a fair amount of complexity, and isn't worth it for something that users haven't touched or would have had a need to build upon. One of the backwards-incompatible changes is the location of files in the file storage backend. Both kinds of certs used to live in `certs/`, but now they live in `certs/client/` and `certs/purpose/` instead. The change is pretty straight-forward. The biggest change in behavior is `build_ssl_context()`, which now passes the right kinds of certs to the right functions. The bulk of the change is actually unit tests and testdata file movement. We now have trust and client purpose variations for each relevant test.	e03270282a569a1a8251bd2d57dc1f718cd1e828
	Add a sense of purpose to SSL/TLS certificates. My original design for SSL/TLS certificate management combined two important purposes into one concept of a certificate. Depending on the call site and the presence of a private key, a `Certificate` could be used for verifying a server or authenticating the client with a server. These are very different purposes, and needed to be managed separately. The original design led to incorrect calls in `build_ssl_context()` and therefore `build_urlopen_kwargs()`, meaning we didn't always have the right context in place to handle the operation. This change addresses this by: 1. Introducing a `CertPurpose` enum containing `TRUST` (for cert trust/verification needs) and `CLIENT` (for client authentication). 2. A `purpose` field in `Certificate` and all cert-related operations in `CertificateManager`, and storage equivalents in the backends. A "trust" cert may not have a private key, and a "client" cert must (although technically that cert could embed the key, we're requiring two separate files in our implementation for now). This does change the signatures and expectations in the storage backends, and we're not going through a deprecation cycle for that. The reason being that, while we introduced all this in Review Board 6, we never made actual use of it. Going through a deprecation cycle would introduce a fair amount of complexity, and isn't worth it for something that users haven't touched or would have had a need to build upon. One of the backwards-incompatible changes is the location of files in the file storage backend. Both kinds of certs used to live in `certs/`, but now they live in `certs/client/` and `certs/purpose/` instead. The change is pretty straight-forward. The biggest change in behavior is `build_ssl_context()`, which now passes the right kinds of certs to the right functions. The bulk of the change is actually unit tests and testdata file movement. We now have trust and client purpose variations for each relevant test.	7853c5eed80c779e4d65c33a3dce5065ee43da0c

Diff:

Revision 4 (+5466 -1580)

Show changes

	reviewboard/certs/cert.py
	reviewboard/certs/manager.py
	reviewboard/certs/storage/base.py
	reviewboard/certs/storage/file_storage.py
	reviewboard/certs/tests/test_certificate.py
	reviewboard/certs/tests/test_certificate_fingerprints.py
	reviewboard/certs/tests/test_certificate_manager.py
	reviewboard/certs/tests/test_file_storage_backend.py
	18 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```