1172615 - opportunistic encryption potentially perturbing certificate verification telemetry

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

SSL_CERT_VERIFICATION_ERRORS is reporting a huge amount of SEC_ERROR_UNKNOWN_ISSUER failures. Strangely, these numbers are not reflected in a similar measure, SSL_CERT_ERROR_OVERRIDES. After looking at the code flow, it appears we attempt to verify a certificate, note a failure (SSL_CERT_VERIFICATION_ERRORS), check if we're supposed to bypass authentication on that socket, and then gather information about overrides (SSL_CERT_ERROR_OVERRIDES). It could be the case that the number of connections using opportunistic encryption is severely throwing off our error telemetry gathering. Furthermore, it seems unnecessary to try to verify a certificate if we're going to allow it anyway after encountering an error. We should probably move that check to AuthCertificateHook.

Comment 1

[Patrick McManus [:mcmanus]]

this is interesting.. but I would be surprised if there was anywhere enough usage of OE to have this impact at this point. but hey, I live to be surprised.

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch

Yeah, now that I think about it it seems unlikely this would account for all of the discrepancy, but it's at least a place to start. Also, this should make OE faster, since we'll skip a bunch of unnecessary operations, which seems like a win regardless.

Comment 3

[Patrick McManus [:mcmanus]]

this patch seems directly at odds with https://bugzilla.mozilla.org/show_bug.cgi?id=964466#c0 - which is what I used as info to implement it with.. am I misreading it? (some function names have changed in the interim, so I'm not 100% sure).. is it not the right thing todo?

obviously we could always just suppress the telemetry bits, but if the optimization can be had safely that would be good too.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler]]

I think the motivation behind that comment is to provide functionality that is performance-wise as close as possible to a successful verification while not actually requiring that certificates verify correctly (so tools can use it to show performance characteristics of sites in development or something). As long as HandshakeCallback gets called (and it's my understanding that it will always be called when the handshake completes), all of the relevant bookkeeping should still happen. In short, I believe this optimization is safe.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Thanks for the review. Here's a try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2075a2da8bcf

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48a77f828d59

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/48a77f828d59