verify_cert: correct handling of fatal errors.

[cpu]

Description

This change fell out of an observation I made while back-porting the budget fixes in main to the rel-0.100 release branch. In that branch the error handling for path building is simpler. Notably we don't carry forward a Error result from the first loop_while_non_fatal_error into the second, later ranking encountered errors to determine which to return.

In the rel-0.100 branch there was existing logic for terminating for a fatal error from the first loop without carrying anything forward:

webpki/src/verify_cert.rs

Lines 89 to 94 in c8b8214

	// If the error is not fatal, then keep going.
	match result {
	Ok(()) => return Ok(()),
	err @ Err(Error::MaximumSignatureChecksExceeded) => return err,
	_ => {}
	};

On main and in rel-0.101 we did not have that logic:

webpki/src/verify_cert.rs

Lines 206 to 209 in 932ad95

	let err = match result {
	Ok(()) => return Ok(()),
	Err(err) => err,
	};

All three branches should be terminating further path building when the first loop_while_non_fatal_error encounters a fatal err, but in practice only rel-0.100 handled this properly. This was demonstrated when that branch started failing the backported test_too_many_path_calls test, because the first fatal error encountered is from expending the signature checking budget. In main and rel-0.101 that error is ignored and we continue until the path building call budget is expended.

This branch fixes the fatal error handling, and requires adjusting the test_too_many_path_calls test to do what it was intending to do in the presence of proper fatal error handling. I slightly refactored the verify_chain helper to make this easier (and to let the existing name_constraint_budget test also benefit from the shared helper).

[codecov]

Codecov Report

Merging #168 (c69804e) into main (932ad95) will decrease coverage by 0.06%.
Report is 3 commits behind head on main.
The diff coverage is 98.83%.

@@            Coverage Diff             @@
##             main     #168      +/-   ##
==========================================
- Coverage   96.70%   96.64%   -0.06%     
==========================================
  Files          15       16       +1     
  Lines        4549     4532      -17     
==========================================
- Hits         4399     4380      -19     
- Misses        150      152       +2     

Files Changed	Coverage Δ
src/verify_cert.rs	97.67% <98.70%> (-0.46%)	⬇️
src/error.rs	59.01% <100.00%> (+3.24%)	⬆️

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

📢 Have feedback on the report? Share it here.

[djc]

Maybe split the extraction of the is_fatal() method from the core fix to make it more obvious what the problem was?

[cpu]

Maybe split the extraction of the is_fatal() method from the core fix to make it more obvious what the problem was?

Done. I also split the test helper refactoring into two parts.

[djc]

Sorry: if you add an is_fatal() helper it should happen in the same commit as starting to use it, so that we can verify that the logic remains the same, and then changing the logic should be a separate commit (either order could make sense).

[cpu]

@djc Ah! Ok, will rework 👍

[cpu]

if you add an is_fatal() helper it should happen in the same commit as starting to use it, so that we can verify that the logic remains the same, and then changing the logic should be a separate commit (either order could make sense).

Updated so the new helper is added in bcdd680 and immediately used to replace the existing fatal err handling in loop_while_non_fatal_error.

The change in error handling behaviour is now isolated to b5af2ce where the helper is added to the error handling between the two loop_while_non_fatal_error invocations.

Hopefully that better matches what you were looking for 🤞

[cpu]

I'd like to merge this to make progress on backports. If there's any late feedback I'm happy to iterate further!