Summary

Add wider support and validation for valid SHA1/256 fingerprints.

Review Request #14925 — Created March 17, 2026 and submitted April 8, 2026, 1:17 a.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-7.1.x

Bugs

Depends On

Blocks

14926

Reviewers

Groups

reviewboard

People

Description

CertificateFingerprints.from_string() now supports plain SHA1 and
SHA256 strings (without the semicolons used in fingerprints). These will
be converted into the correct format.

The values are also now validated against regexes. This will key off
the pattern and information needed to optionally normalize the string
and then set it, based on the length of the string provided. If there
isn't a suitable match, None will be returned.

Testing Done

Unit tests pass.

Commits

Summary	ID
Add wider support and validation for valid SHA1/256 fingerprints. `CertificateFingerprints.from_string()` now supports plain SHA1 and SHA256 strings (without the semicolons used in fingerprints). These will be converted into the correct format. The values are also now validated against regexes. This will key off the pattern and information needed to optionally normalize the string and then set it, based on the length of the string provided. If there isn't a suitable match, ``None`` will be returned.	db7091e37d7b46b63a1a7ba68b3628c7ac5af3a3

Summary

Add wider support and validation for valid SHA1/256 fingerprints.

`CertificateFingerprints.from_string()` now supports plain SHA1 and SHA256 strings (without the semicolons used in fingerprints). These will be converted into the correct format. The values are also now validated against regexes. This will key off the pattern and information needed to optionally normalize the string and then set it, based on the length of the string provided. If there isn't a suitable match, ``None`` will be returned.

db7091e37d7b46b63a1a7ba68b3628c7ac5af3a3

Issues

Description	From	Last Updated
A SHA1 hash is 20 bytes, but that corresponds to 40 hex characters. Your test didn't catch this because the …	david	April 2, 2026, 2:05 a.m.
Can we add a type hint here?	david	April 8, 2026, 1:16 a.m.
Leftover debug code.	maubin	April 2, 2026, 1:09 a.m.
It might be nice to switch to re.fullmatch() for these, to make it less fragile for future changes. I also …	david	April 2, 2026, 1:54 a.m.
Might be nicer to do length == X and cls.SHA... here instead of nested if statements? Same with the elifs …	maubin	April 2, 2026, 8:06 p.m.

flake8 passed.

JSHint passed.

reviewboard/certs/cert.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Leftover debug code.
```

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

The issue has been dropped. Show all issues

Might be nicer to do length == X and cls.SHA... here instead of nested if statements? Same with the elifs below.

chipx86

April 2, 2026, 1:23 a.m.

This was done to short-circuit quickly if it matches the length but not the pattern. Say we get a string that's 95 chars, but not a valid SHA256 fingerprint. Combining the conditionals means we'd then check every other elif until we inevitably return None. I wanted to take the approach for this code that fails fast.

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

The issue has been resolved. Show all issues

A SHA1 hash is 20 bytes, but that corresponds to 40 hex characters. Your test didn't catch this because the invalid character happened to be within the first half of the hash.

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

The issue has been resolved. Show all issues

It might be nice to switch to re.fullmatch() for these, to make it less fragile for future changes.

I also agree with Michelle's comment about combining the conditionals--as long as length is listed first, it won't check the regex match.

It might also be nice to order these to keep sha1 and sha256 together, instead of going in reverse order by length.

chipx86

April 2, 2026, 1:23 a.m.

I mentioned this in the comment to Michelle, but I want to short-circuit this as soon as we know the length is a match but the pattern is not.

Thinking of another approach.

Change Summary:


Reworked from_string()'s length checks to index into a dictionary of information used to validate/normalize/set the fingerprint. This keeps from having to check conditions for each possible input value, and centralizes the SHA-to-fingerprint logic.
Updated unit tests to inject an invalid character only at the end of the string.
Updated SHA1_RE to have the right length, and switched to re.fullmatch().
Updated versions to RB 8.

Description:

		`CertificateFingerprints.from_string()` now supports plain SHA1 and
		SHA256 strings (without the semicolons used in fingerprints). These will
		be converted into the correct format.

~		The values are also now validated against regexes. This will first check
~		the line lengths and then follow up with a pattern match.
	~	The values are also now validated against regexes. This will key off
	~	the pattern and information needed to optionally normalize the string
	+	and then set it, based on the length of the string provided. If there
	+	isn't a suitable match, `None` will be returned.

Commits:

	Summary	ID
	Add wider support and validation for valid SHA1/256 fingerprints. `CertificateFingerprints.from_string()` now supports plain SHA1 and SHA256 strings (without the semicolons used in fingerprints). These will be converted into the correct format. The values are also now validated against regexes. This will first check the line lengths and then follow up with a pattern match.	3591bcfc48229e9dc2e6d64c8267028e03d0b358
	Add wider support and validation for valid SHA1/256 fingerprints. `CertificateFingerprints.from_string()` now supports plain SHA1 and SHA256 strings (without the semicolons used in fingerprints). These will be converted into the correct format. The values are also now validated against regexes. This will key off the pattern and information needed to optionally normalize the string and then set it, based on the length of the string provided. If there isn't a suitable match, ``None`` will be returned.	db7091e37d7b46b63a1a7ba68b3628c7ac5af3a3

Diff:

Revision 2 (+268 -16)

Show changes

	reviewboard/certs/cert.py
	reviewboard/certs/tests/test_certificate_fingerprints.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

reviewboard/certs/cert.py (Diff revisions 1 - 2)
The issue has been resolved. Show all issues
```
Can we add a type hint here?
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-7.1.x (7968667)