Add option to control whether OCSP GET is used

RESOLVED FIXED in 3.15.4

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

3.15.3
3.15.4

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Created attachment 818739 [details] [diff] [review]
WIP: Add an option to control whether OCSP GET is used.

In order to minimize risk from the change to have NSS use OCSP GET by default in Gecko, we need an option that will enable us to go back to the old behavior (only POST). I believe this patch is correct, but I haven't tested it. It would be best if we added this to NSS before we import the current NSS trunk into mozilla-central.

Comment 1

4 years ago
Comment on attachment 818739 [details] [diff] [review]
WIP: Add an option to control whether OCSP GET is used.

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

::: lib/certhigh/ocsp.h
@@ +172,5 @@
>  CERT_DisableOCSPDefaultResponder(CERTCertDBHandle *handle);
>  
> +/* When enabled, OCSP requests will be sent using the HTTP GET method, with
> + * a fallback to POST when we fail to receive a response and/or when we
> + * receive an uncacheable response like "Unknown." When disabled, OCSP

Note that "Unknown" responses received for a POST request are currently treated as cacheable, see bug 745747 comment 23. I consider the current implementation of ocspMode_FailureIsNotAVerificationFailure flawed for this reason, and would at least recommend to make sure that GET and POST requests use the same caching strategy.
Created attachment 822008 [details] [diff] [review]
Add an option to control whether OCSP GET is used

I tested this manually in Firefox and verified that Firefox will use GET and cache "Good" responses when the option is at its default value, and that Firefox will send a POST request when the option is disabled, even if there is a cached response to a previous "GET." So, everything I tested seems to be working correctly. However, I didn't test the edge cases ("Unknown" or unsigned responses.)
Assignee: nobody → brian
Attachment #818739 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #822008 - Flags: review?(rrelyea)
Priority: -- → P1
Target Milestone: --- → 3.15.3
See Also: → bug 871954, bug 898431

Comment 3

4 years ago
Brian, why the second sanity check for OCSP_Global.monitor being set up. at line 5272? You've already checked at the beginning of the function.

bob
Created attachment 822070 [details] [diff] [review]
add-option-to-disable-OCSP-GET.patch

Bob, sorry about that. I attached the wrong version of the patch. Here is the fixed version.
Attachment #822008 - Attachment is obsolete: true
Attachment #822008 - Flags: review?(rrelyea)
Attachment #822070 - Flags: review?(rrelyea)

Comment 5

4 years ago
Comment on attachment 822070 [details] [diff] [review]
add-option-to-disable-OCSP-GET.patch

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

r=wtc.

This patch is fine by me. If the Mozilla team ever finds the new OCSP GET
code unusable in Firefox, then we should simply make an NSS patch release
to turn it off and then fix the problem. This patch shows it is a simple
change to turn off OCSP GET in NSS. We should try to avoid adding another
configuration function to NSS.

I leave this decision to you, as long as OCSP GET is enabled by default.

::: lib/certhigh/ocsp.c
@@ +5205,1 @@
>      PRBool retry = PR_FALSE;

Nit: did you mean to add this blank line? It is uncommon to have a blank line in the middle of variable declarations.

::: lib/certhigh/ocsp.h
@@ +177,5 @@
> + * requests will only be sent using the HTTP POST method.
> + *
> + * The default is to enable OCSP using HTTP GET.
> + */
> +extern SECStatus CERT_SetOCSPGetEnabled(PRBool enabled);

Can we name this function
  SECStatus CERT_EnableOCSPGet(PRBool enabled);
or
  SECStatus CERT_DisableOCSPGet(void);
?

It is a little confusing to see both "Set" and "Get" :-)
Attachment #822070 - Flags: review+

Comment 6

4 years ago
Comment on attachment 822070 [details] [diff] [review]
add-option-to-disable-OCSP-GET.patch

r+ rrelyea
Attachment #822070 - Flags: review?(rrelyea) → review+

Comment 7

4 years ago
I just thought of something... Kai made changes to both the traditional and libpkix calls. I think the selection of get versus post is pushed up to that level, so you may be missing turning off libpkix.

bob
Blocks: 932176
Created attachment 823821 [details] [diff] [review]
add-libpkix-option-to-disable-OCSP-GET.patch

Thanks Bob. Here's the libpkix option. Since libpkix isn't supposed to rely on global shared state, I added this as a flag like the other revocation options. I also tested this manually in Firefox with the patch for bug 932176.
Attachment #823821 - Flags: review?(rrelyea)

Comment 9

4 years ago
Comment on attachment 823821 [details] [diff] [review]
add-libpkix-option-to-disable-OCSP-GET.patch

r+ Thanks brian.

bob
Attachment #823821 - Flags: review?(rrelyea) → review+
http://hg.mozilla.org/projects/nss/rev/b45c9b1101aa
http://hg.mozilla.org/projects/nss/rev/9cdaf745a4ec

(In reply to Wan-Teh Chang from comment #5)
> I leave this decision to you, as long as OCSP GET is enabled by default.

It is important for us to have an option that we can control with a pref, because some of our emergency regression response/chemspill plans are predicated on having prefs. Usually we can remove such prefs after a release or two

> Nit: did you mean to add this blank line? It is uncommon to have a blank
> line in the middle of variable declarations.

Fixed.

> Can we name this function
>   SECStatus CERT_EnableOCSPGet(PRBool enabled);
> or
>   SECStatus CERT_DisableOCSPGet(void);
> ?
> 
> It is a little confusing to see both "Set" and "Get" :-)

I named it CERT_ForcePostMethodForOCSP to match the libpkix option. I also inverted the sense. I tested this with an updated patch for bug 932176 to make sure it was correct.

Thank you for the reviews.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Comment 11

4 years ago
changing target milestone to 3.15.4
Target Milestone: 3.15.3 → 3.15.4
You need to log in before you can comment on or make changes to this bug.