test: add more testing for CRL revocation

[jsha]

In revocation_test.go, fetch all CRLs, and look for revoked certificates on both CRLs and OCSP.

Make s3-test-srv listen on all interfaces, so the CRL URLs in the CA config work.

Add IssuerNameIDs to the CRL URLs in ca.json, to match how those CRLs are uploaded to S3.

Make TestRevocation parallel. Speedup from ~60s to ~3s.

Increase ocsp-responder's allowed parallelism to account for parallel test. Also, add "maxInflightSignings" to config/ since it's in prod. "maxSigningWaiters" is not yet in prod, so don't move that field.

Add a mutex around running crl-updater, and decrease the log level so errors stand out more when they happen.

[github-actions]

@jsha, this PR appears to contain configuration and/or SQL schema changes. Please ensure that a corresponding deployment ticket has been filed with the new values.

[jsha]

Hi bot,

The configuration changes are only meaningful for testing:

• Fix the URLs for CRLs
• More parallelism for OCSP Updater
• Less logging for CRL Updater

[beautifulentropy]

This change looks great, just one small comment and a question.

[aarongable]

Embedding the resulting cert in the expectation and having a mutex around the list of expectations is very clever, but it took me a moment to realize what was going on, and I think that having the "actual" cert embedded inside an "expectation" struct is counterintuitive. You could just change the name of the struct, but consider this alternate approach:

1. Have each testcase have its own name (fmt.Sprintf("%s_%d_%s", kind, reason, method))
2. Have a map of names to expectations (var want map[string]expectation), which the innermost loop inserts into directly
3. Have a map of names to actual certs (var got map[string]*x509.Certificate), which the go func inserts into directly (I'm not sure if this has to be protected by a mutex -- we can guarantee that all of the keys will be unique, but the runtime might not trust us on that)
4. Have the final loop that checks the actual certs against our expectations simply access the same keys in both maps

[jsha]

I agree that making the cert an embedded field is too cute by half. Named the field revokedCert instead.

For the map-of-fields vs struct-with-fields approach, I still think the struct-with-fields is clearer. I added a comment to try and make it clearer what's going on.

Have a map of names to actual certs (var got map[string]*x509.Certificate), which the go func inserts into directly (I'm not sure if this has to be protected by a mutex -- we can guarantee that all of the keys will be unique, but the runtime might not trust us on that)

FWIW, concurrent writes to a map are not safe, even if the writes are to different keys. Intuition: any write may require rebucketing; even a write to an already existing field, if the map implementation inlines its values.

[jsha]

One other thought on this: instead of a struct, the expectations could be closures that take the resolved list of all CRLs. E.g.

var expectations []func(*testing.T, map[string][]*x509.RevocationList)

...
expectations = append(expectations, func(t *testing.T, allCRLs map[string][]*x509.RevocationList) {
  checkRevoked(t, revocations, cert, expectedReason)
}

Functionally the same - cert and expectedReason are carried as captured variables, rather than struct fields - but it puts the verification logic right next to the setup logic.

[jsha]

I tried this closure approach in the latest commit. I like it!

[aarongable]

LGTM. There was a slightly concerning //test/integration/revocation_test.go failure, but it was during certificate issuance and resolved itself with a single rerun.

[jprenken]

LGTM. There was a slightly concerning //test/integration/revocation_test.go failure, but it was during certificate issuance and resolved itself with a single rerun.

I think I saw a similar failure on my draft PR, so we may have a flaky test in main.

Fixed all the things!