Add telemetry for OCSP fetching

RESOLVED FIXED in Firefox 27

Status

()

enhancement
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

(Blocks 1 bug)

Trunk
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox27 fixed, firefox28 fixed, firefox29 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

The question I'm hoping to answer is "When there's no stapled response, approximately how often would we fail to do OCSP fetching because there is no valid OCSP URI in the cert?" I'm assuming that, long-term, the need for us to do fetching due to stapled expired responses will go to zero, so that shouldn't be counted.
Attachment #8359028 - Attachment is patch: true
Attachment #8359028 - Attachment mime type: text/x-patch → text/plain
Attachment #8359028 - Flags: review?(dkeeler)
Comment on attachment 8359028 [details] [diff] [review]
ocsp-may-fetch-telementry.patch

Review of attachment 8359028 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good - r=me.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +644,5 @@
>    NS_DECL_NSIRUNNABLE
>  
>    // Must be called only on the socket transport thread
>    SSLServerCertVerificationJob(const void * fdForLogging,
> +                               nsNSSSocketInfo * infoObject, 

nit: trailing space (it was there already, but might as well get rid of it)

@@ +928,5 @@
>      Telemetry::Accumulate(Telemetry::SSL_OCSP_STAPLING, 2);
> +
> +    uint32_t reasonsForNotFetching = 0;
> +
> +    char * ocspURI = CERT_GetOCSPAuthorityInfoAccessLocation(cert);

nit: I know this file is all over the place with where to put the "*" in declarations like this, but it looks like in this function, it's common to do "char *ocspURI". Your call, though.
Attachment #8359028 - Flags: review?(dkeeler) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/67e4b3c91c1d

I made the changes you suggested, except I follow the Mozilla coding style: "char* ocspURI". I will try to use the Mozilla coding style more consistently; please feel free to correct me if/when I diverge from it.
Target Milestone: --- → mozilla29
[Approval Request Comment]
Bug caused by (feature/regressing bug #): adding telemetry only.

User impact if declined: We need the telemetry from these probes in order to communicate important changes to how we do OCSP and revocation checking in general. These numbers are particularly important because they are likely to show that people are over-estimating how much revocation checking we're doing. Thus, the numbers should help motivate people to support us in implementing new revocation checking mechanisms.

Testing completed (on m-c, etc.): Just landed on inbound..

Risk to taking this patch (and alternatives if risky): Low, because it's just telemetry.

String or IDL/UUID changes made by this patch: I wouldn't dare.
Attachment #8359028 - Attachment is obsolete: true
Attachment #8359624 - Flags: review+
Attachment #8359624 - Flags: checkin+
Attachment #8359624 - Flags: approval-mozilla-beta?
Attachment #8359624 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/67e4b3c91c1d
Assignee: nobody → brian
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Attachment #8359624 - Flags: approval-mozilla-beta?
Attachment #8359624 - Flags: approval-mozilla-beta+
Attachment #8359624 - Flags: approval-mozilla-aurora?
Attachment #8359624 - Flags: approval-mozilla-aurora+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.