928142 - Add option to control whether OCSP GET is used

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: 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

[Kaspar]

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.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: 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.)

Comment 3

[Robert Relyea]

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

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: add-option-to-disable-OCSP-GET.patch

Bob, sorry about that. I attached the wrong version of the patch. Here is the fixed version.

Comment 5

[Wan-Teh Chang]

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" :-)

Comment 6

[Robert Relyea]

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

r+ rrelyea

Comment 7

[Robert Relyea]

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

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: 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.

Comment 9

[Robert Relyea]

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

r+ Thanks brian.

bob

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 11

[Kai Engert (:KaiE:)]

changing target milestone to 3.15.4