Closed cpu closed 3 months ago
I'm starting to wonder if instead of these gymnastics we should just back out the time
argument being fed through to the CertificateVerifier
until we work out a broader solution to https://github.com/rustls/rustls-platform-verifier/issues/59. That would let the early checkValidity
call surface the better error early on.
I'm starting to wonder if instead of these gymnastics we should just back out the time argument being fed through to the CertificateVerifier until we work out a broader solution to https://github.com/rustls/rustls-platform-verifier/issues/59.
@complexspaces WDYT :point_up: vs this branch? In a perfect world we'd fix #59 instead of doing either but I don't think I'll personally be able to find time to work on that solution and it would be nice to have CI report the correct failure before it happens on April 26th (and every 90d afterwards).
WDYT ☝️ vs this branch?
I'm not pushed either way, honestly. If we backed the time
parameter out we should keep the root error "digging" because I think no matter the route, we should try and return a clear expiry error as the main priority. What do you think about just merging this to keep CI nicer and maintain the "good" errors most end users will see?
What do you think about just merging this to keep CI nicer and maintain the "good" errors most end users will see?
That works for me :-) If you have a chance to put a review on this branch I'll merge when ready.
Thanks!
Recently CI started failing the real world verification suite on Android (https://github.com/rustls/rustls-platform-verifier/issues/68). The root cause was the vendored Let's Encrypt end entity certificate expiring. Figuring this out was hard for two main reasons:
CI was eating the Android logs we needed to see the error message. This has since been fixed (https://github.com/rustls/rustls-platform-verifier/pull/72).
The Android verifier was surfacing the expiration as an unknown issuer error with a generic "Chain validation failed" message.
This branch attempts to fix item 2 by surfacing a more relevant error message.
Unfortunately doing so is a little bit annoying. The
checkServerTrusted
call throws an exception with ~3 layers of wrapping before you get at theCertificateExpiredException
that's the root cause.Since non-test configurations should never have an expired exception thrown at this stage (recall we check this explicitly earlier in processing), we gate the more complicated exception "digging" based on
BuildConfig.TEST
.This should be a good balance of:
Before this fix running the verification test suite back at 6cd02326f1be869b4a6732f262bb04c4958ecf3b before 15d487a9c6e4bf23bc54ac2450d074c10f63eb69 landed would produce output like:
Afterwards it logs output like: