Closed Bug 959026 Opened 7 years ago Closed 7 years ago

Add telemetry for OCSP fetching

Categories

(Core :: Security: PSM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox27 --- fixed
firefox28 --- fixed
firefox29 --- fixed

People

(Reporter: briansmith, Assigned: briansmith)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

Attached patch ocsp-may-fetch-telementry.patch (obsolete) — Splinter Review
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: 7 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.