Add new test case to integration tests #324

dwhjames · 2025-02-23T20:40:48Z

Test the Amazon certificate authorities
https://www.amazontrust.com/repository/
with and without CRLs.

codecov · 2025-02-23T20:44:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.53%. Comparing base (84b1463) to head (01633d5).
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #324      +/-   ##
==========================================
+ Coverage   97.46%   97.53%   +0.06%     
==========================================
  Files          20       20              
  Lines        4343     4343              
==========================================
+ Hits         4233     4236       +3     
+ Misses        110      107       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

djc · 2025-02-23T20:57:59Z

Why?

dwhjames · 2025-02-23T21:11:02Z

Why?

@djc, I initially did this privately as I was testing a use case that involves AWS and revocation checking. But I thought the extra test coverage may be of use to the project, so I’ve tidied it up to offer it back.

Due to dependency tree restrictions, I originally had to work against v0.101.7. And at that version it seemed the integration test examples for CRLs were limited in number as well as simple (single CRL); moreover, the types were more ‘awkward’. It seems like main is now ‘cleaner’ for handling webpki::CertRevocationList.

I’m not offended if this is overkill for the amount of test coverage that this might gain relative to amount of new test code: just trying to offer what might be of use (more coverage / more complex integration example) rather than keep it private.

Test the Amazon certificate authorities https://www.amazontrust.com/repository/ with and without CRLs.

djc · 2025-02-23T21:35:07Z

How much code would you have to duplicate to put these tests in a separate file? Would be interesting to look at where the extra coverage is coming from. I think it might be interesting to do this, but it also feels like overkill a bit.

How much runtime does it add in practice? Probably not a lot?

cpu · 2025-02-23T21:35:10Z

Thanks for sharing some tests 👍

more coverage

From codecov's perspective it doesn't seem like this branch added coverage outside of a 0.07% indirect coverage improvement in src/time.rs.

I buy that there could be interesting properties that differ between this test corpus and the existing coverage that are meaningful outside of naive coverage metrics, but I'd like to see those specific differences highlighted. I think it would be a stronger contribution if the new tests were minimized to focus on what's new vs existing.

dwhjames · 2025-02-23T21:46:26Z

tests/integration.rs

+            let path = cert
+                .verify_for_usage(
+                    webpki::ALL_VERIFICATION_ALGS,
+                    &all_anchors,
+                    &intermediates_legacy,
+                    time,
+                    KeyUsage::server_auth(),
+                    crls.map(|l| revocation_options_for_test(l)),
+                    None,
+                )
+                .unwrap();
+
+            // verify should find shortest path
+            assert!(anchors.contains(path.anchor()));


@cpu @djc
I’ve scanned the codebase and this is the only test usage of verify_for_usage where the computed path is checked, and specifically this check establishes that the shortest path is found. With the trust anchors and intermediates given here, there are two valid paths for every leaf cert tested.

specifically this check establishes that the shortest path is found. With the trust anchors and intermediates given here, there are two valid paths for every leaf cert tested.

I believe the better TLS test coverage in tests/better_tls.rs has a variety of test cases that exercise path building like this. Did you take a peek at those?

I didn’t, but now I’m looking

https://github.com/Netflix/bettertls/tree/master/test-suites/pathbuilding

I’m actually not seeing a test case that matches what is seen in figures 1 & 2 in

https://aws.amazon.com/blogs/security/acm-will-no-longer-cross-sign-certificates-with-starfield-class-2-starting-august-2024/

of course, there are a lot of complex/pathological cases in that bettertls suite.

Note the integration of this crate with better_tls also has no revocation checking.

webpki/tests/better_tls.rs

Lines 79 to 86 in 84b1463

let result = ee_cert

.verify_for_usage(

ALGS,

roots,

intermediates,

now,

KeyUsage::server_auth(),

None,

Note the integration of this crate with better_tls also has no revocation checking.

Fair point 👍

dwhjames · 2025-02-23T21:52:52Z

tests/integration.rs

+        for &crls in &[&intermediates_crls, &all_crls] {
+            assert!(
+                cert.verify_for_usage(
+                    webpki::ALL_VERIFICATION_ALGS,
+                    &anchors,
+                    &intermediates,
+                    time,
+                    KeyUsage::server_auth(),
+                    Some(revocation_options_for_test(crls)),
+                    None,
+                )
+                .is_err_and(|e| matches!(e, webpki::Error::CertRevoked))


@cpu @djc to the best of my ability, I couldn’t see any other test cases where revocation checking occurs,

with real-world CRLs (as opposed to those constructed by generate.py), i.e. production data and not trivially small data (hypothesis being that real data can be weird and thus useful for finding edge cases and regressions)

where the collection of CRLs are from multiple authorities

The assertion immediately above this one is complementary (shows the revoked certificates will verify if the crls from the intermediate CAs are not given).

dwhjames · 2025-02-23T21:56:15Z

tests/integration.rs

+        assert!(
+            cert.verify_for_usage(
+                webpki::ALL_VERIFICATION_ALGS,
+                &anchors,
+                &intermediates,
+                time,
+                KeyUsage::server_auth(),
+                None,
+                None,
+            )
+            .is_err_and(|e| matches!(e, webpki::Error::CertExpired { .. }))
+        );


@cpu @djc if you’re looking for code to trim, this is likely lower value; I included this for completeness (test everything that https://www.amazontrust.com/repository/ has to offer) but there are already tests for expiry.

dwhjames · 2025-02-23T21:59:27Z

How much runtime does it add in practice? Probably not a lot?

@djc, sub 1 second on my machine (2.3 GHz Intel Core i9)

$ cargo test --test integration --no-default-features --features alloc,std,aws-lc-rs -- --exact amazon
…
running 1 test
test amazon ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 14 filtered out; finished in 0.13s

dwhjames · 2025-02-23T22:09:10Z

Some additional coverage to note: these certificates cover RSA (2048, 4096) and EC (P-256, P-384) along with SHA-256 and SHA-384. I sampled around and saw some but not all of these (no RSA-4096 or EC P-384 from my admittedly incomplete sampling).

dwhjames · 2025-02-23T22:10:21Z

How much code would you have to duplicate to put these tests in a separate file?

@djc, almost nothing, I think.

dwhjames · 2025-02-23T23:19:03Z

Moving test code to a separate source file in 01633d5. I was interpreting this as your implied preference @djc .

dwhjames · 2025-02-24T00:07:37Z

@cpu is there something broken with the CI configuration?

All the integration tests appears to be filtered out, rather than running.

https://github.com/rustls/webpki/actions/runs/13487872127/job/37681489585?pr=324

and that seems to be the case on at least the previous PR

https://github.com/rustls/webpki/actions/runs/13458105815/job/37606542735?pr=323

ctz · 2025-02-24T11:51:28Z

@cpu is there something broken with the CI configuration?

Excellent spot! Addressing this in #325

dwhjames force-pushed the amazon-test branch from 16f8528 to 70147e6 Compare February 23, 2025 21:00

Add new test case to integration tests

cf8333b

Test the Amazon certificate authorities https://www.amazontrust.com/repository/ with and without CRLs.

dwhjames force-pushed the amazon-test branch from 70147e6 to cf8333b Compare February 23, 2025 21:30

dwhjames commented Feb 23, 2025

View reviewed changes

split amazon test case into separate source file

01633d5

dwhjames force-pushed the amazon-test branch from da11edf to 01633d5 Compare February 23, 2025 23:20

ctz approved these changes Feb 24, 2025

View reviewed changes

djc approved these changes Feb 24, 2025

View reviewed changes

ctz added this pull request to the merge queue Feb 24, 2025

Merged via the queue into rustls:main with commit 7cb0aa7 Feb 24, 2025
33 checks passed

dwhjames deleted the amazon-test branch February 24, 2025 22:04

	let result = ee_cert
	.verify_for_usage(
	ALGS,
	roots,
	intermediates,
	now,
	KeyUsage::server_auth(),
	None,

Add new test case to integration tests #324

Add new test case to integration tests #324

Uh oh!

Conversation

dwhjames commented Feb 23, 2025

Uh oh!

codecov bot commented Feb 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

djc commented Feb 23, 2025

Uh oh!

dwhjames commented Feb 23, 2025

Uh oh!

djc commented Feb 23, 2025

Uh oh!

cpu commented Feb 23, 2025

Uh oh!

dwhjames Feb 23, 2025

Choose a reason for hiding this comment

Uh oh!

cpu Feb 23, 2025

Choose a reason for hiding this comment

Uh oh!

dwhjames Feb 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpu Feb 23, 2025

Choose a reason for hiding this comment

Uh oh!

dwhjames Feb 23, 2025

Choose a reason for hiding this comment

Uh oh!

dwhjames Feb 23, 2025

Choose a reason for hiding this comment

Uh oh!

dwhjames Feb 23, 2025

Choose a reason for hiding this comment

Uh oh!

dwhjames commented Feb 23, 2025

Uh oh!

dwhjames commented Feb 23, 2025

Uh oh!

dwhjames commented Feb 23, 2025

Uh oh!

dwhjames commented Feb 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dwhjames commented Feb 24, 2025

Uh oh!

ctz commented Feb 24, 2025

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Feb 23, 2025 •

edited

Loading

dwhjames Feb 23, 2025 •

edited

Loading

dwhjames commented Feb 23, 2025 •

edited

Loading