From ad732ef0dfbe348fd4550749541c995681a16837 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Tue, 12 Sep 2023 20:51:02 -0400 Subject: [PATCH 1/5] verify_cert: slim down test_too_many_path_calls The `test_too_many_path_calls` unit test artificially inflates the budget for signature validation operations so that it can easily force quadratic path building runtime, up to the default `build_chain_calls` budget. As a result this test takes a long time relative to our other unit tests: it completes in ~3s while the entire project test suite without this test and the bettertls tests in 0.6s. This commit adjusts the test to use a lower-than-default `build_chain_calls` budget. This lets us test the mechanics of the budget are working as intended, without having to expend a lot of runtime to run up the budget to the large default. --- src/verify_cert.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 988d3f00..c4240ef0 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -555,7 +555,7 @@ mod tests { #[test] #[cfg(feature = "alloc")] - fn test_too_many_path_calls() { + fn test_too_many_path_calls_mini() { assert!(matches!( build_degenerate_chain( 10, @@ -565,6 +565,10 @@ mod tests { // first expending the signature checks budget is tricky, so we artificially // inflate the signature limit to make this test easier to write. signatures: usize::MAX, + // We don't use the default build chain budget here. Doing so makes this test + // run slowly (due to testing quadratic runtime up to the limit) without adding + // much value from a testing perspective over using a smaller non-default limit. + build_chain_calls: 100, ..Budget::default() }) ), From 6a9e3184694da32e59cac12d9e89ce98876dc0a2 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Tue, 12 Sep 2023 21:03:31 -0400 Subject: [PATCH 2/5] bettertls: make test suite opt-in The Netflix bettertls test suite for pathbuilding and name constraint validation take an outsized amount of runtime compared to our other tests. These tests can take ~8s locally and the entire project test suite with these tests ignored can be run in 0.6s. This commit adds an `#[ignore]` flag so it won't be run by default. We will opt-in to running this test in CI w/ `cargo test -- --ignored`. --- tests/better_tls.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/better_tls.rs b/tests/better_tls.rs index 098bc471..84a0247e 100644 --- a/tests/better_tls.rs +++ b/tests/better_tls.rs @@ -12,6 +12,7 @@ use serde::Deserialize; use webpki::types::{CertificateDer, TrustAnchor}; use webpki::{extract_trust_anchor, KeyUsage, SubjectNameRef}; +#[ignore] // Runs slower than other unit tests - opt-in with `cargo test -- --ignored` #[test] fn better_tls() { let better_tls = testdata(); @@ -30,6 +31,7 @@ fn better_tls() { ); } +#[ignore] // Runs slower than other unit tests - opt-in with `cargo test -- --ignored` #[test] fn name_constraints() { let better_tls = testdata(); From 52ba3edee3526d965870681123d837d8139d4427 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Tue, 12 Sep 2023 21:35:01 -0400 Subject: [PATCH 3/5] tests: rename better_tls test to path_building The `better_tls` unit test in the `better_tls.rs` integration test module was named before we added the `name_constraints` unit test. This commit renames the `better_tls` unit test to `path_building` to reflect that it runs the path building testsuite from bettertls. --- tests/better_tls.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/better_tls.rs b/tests/better_tls.rs index 84a0247e..616d93e0 100644 --- a/tests/better_tls.rs +++ b/tests/better_tls.rs @@ -14,7 +14,7 @@ use webpki::{extract_trust_anchor, KeyUsage, SubjectNameRef}; #[ignore] // Runs slower than other unit tests - opt-in with `cargo test -- --ignored` #[test] -fn better_tls() { +fn path_building() { let better_tls = testdata(); let root_der = &better_tls.root_der(); let root_der = CertificateDer::from(root_der.as_slice()); From ac03e851150e97e62eb6b4eb2ff8d68c807980ab Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Wed, 13 Sep 2023 10:44:11 -0400 Subject: [PATCH 4/5] ci: run ignored tests in CI We ignore the bettertls path building and name constraint test suites because they take longer than the rest of the webpki test suite, bogging down local development. In the context of CI we don't mind that extra runtime (on the order of ~15..30s) so this commit updates the CI configuration to always run ignored tests alongside the rest of the test suite. --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2e987d12..43d7e7b4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -191,7 +191,7 @@ jobs: toolchain: ${{ matrix.rust_channel }} - name: cargo test (${{ matrix.mode }}, ${{ matrix.features }}) - run: cargo test -vv ${{ matrix.features }} ${{ matrix.mode }} + run: cargo test -vv ${{ matrix.features }} ${{ matrix.mode }} -- --ignored env: RUSTFLAGS: "-D warnings" From dc8f5d85f49411713d95051d8a715a70f41c23d6 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Wed, 13 Sep 2023 10:45:59 -0400 Subject: [PATCH 5/5] ci: add cargo hack feature powerset test This commit updates CI to use `cargo hack` to test the feature powerset, ensuring that we can catch feature interplay related breakages early. Unlike w/ the main Rustls repo the webpki powerset is small and the test completes in <30s, so it's fair to include in the default CI instead of needing to be separated into a separate daily tests workflow. --- .github/workflows/ci.yml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 43d7e7b4..b33b6775 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -292,3 +292,21 @@ jobs: - name: check no-std mode run: cargo check --target $NOSTD_TARGET ${{ matrix.features }} + + feature-powerset: + name: Feature Powerset + runs-on: ubuntu-20.04 + steps: + - name: Checkout sources + uses: actions/checkout@v4 + with: + persist-credentials: false + + - name: Install stable toolchain + uses: dtolnay/rust-toolchain@stable + + - name: Install cargo hack + uses: taiki-e/install-action@cargo-hack + + - name: Check feature powerset + run: cargo hack check --feature-powerset --no-dev-deps