Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add additional test cases sourced from x509test (experimental) #255

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ctz
Copy link
Contributor

@ctz ctz commented Feb 27, 2022

This PR adds 90 additional test cases generated from the google/x509test test corpus. The process for that conversion is over on ctz/webpki-x509test.

This PR doesn't include test cases that unexpectedly failed, I will file an issue to discuss those.

Briefly, the structure of x509test is:

  • there are some test cases that are expected to pass (prefixed ok-) and some expected to fail (prefixed xf-).
  • in the x509test sources these are split up into directories whether they test a 2-deep or 3-deep certificate chain.

This naming is maintained into the generated code, so doing cargo test test_xf_v1_extensions runs this. The test inputs in x509test have the RFC clause they relate to embedded in the certificate subject. These test names get mapped to expected webpki errors over here -- in this case we don't support v1 so we expect that test case to produce Error::UnsupportedCertVersion.

There's a lot of commonality to the test cases; currently the generated rust code is very longwinded. I won't do much more polishing on this though until/unless this seems to be a good avenue for improving testing. This might be a good basis for #248 perhaps.

@cpu
Copy link
Contributor

cpu commented May 26, 2023

I started looking at porting this coverage over to the rustls/webpki fork but decided to abandon the effort for a few reasons. I thought recording the challenges here would be helpful history:

  1. The upstream google/x509test project has been archived by the owners. There's no note about why, but it seems clear they're not going to be maintaining it anymore.
  2. The x509test generation code is pretty hairy Python without any typing or unit tests, and it's specific to Python2. I fixed the immediate Python3 errors, but I wouldn't want to maintain this code myself in perpetuity.
  3. After fixing the upstream x509test stuff, there was some further work to do in ctz/webpki-x509test to account for some API drift in Webpki. This was easy enough, but:
  4. Once I had the testcases generated and the x509test.rs driver building without error every one of the test cases are failing with an unexpected SignatureAlgorithmMismatch error. Peeking at one of the DER files I'm seeing the signature algorithm encoded as an unknown OID.

I'm sure with more effort I could get the signature algorithm problem resolved but taking a step back and looking at the overall state of this approach I think it's going to be difficult to maintain and so i'm tabling the work for now.

I've also been getting input from @djc that there's not a lot of appetite for adding further Python code to the webpki test suite. I think keeping the test generation code in a separate repo like webpki-x509test might be an acceptable compromise on that front, but it will promote the kind of bitrot we found here from API drift. Folding it into the webpki repo will mean more Python to maintain and also requires submodules for x509test and der2ascii and I personally find git submodules to be a very frustrating developer UX.

TLDR: I vote that we take a more tactical approach to this coverage and create our own testcases on a more targetted basis (ideally in Rust, but failing that using pyca cryptography).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants