Closed Bug 928142 Opened 6 years ago Closed 6 years ago
Add 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 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.
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.)
Priority: -- → P1
Target Milestone: --- → 3.15.3
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
Bob, sorry about that. I attached the wrong version of the patch. Here is the fixed version.
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 on attachment 822070 [details] [diff] [review] add-option-to-disable-OCSP-GET.patch r+ rrelyea
Attachment #822070 - Flags: review?(rrelyea) → review+
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
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 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
Closed: 6 years ago
Resolution: --- → FIXED
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.