PSM changes and tests to support OCSP stapling

RESOLVED FIXED in mozilla25

Status

()

Core
Security: PSM
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: kaie, Assigned: keeler)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

Trunk
mozilla25
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 17 obsolete attachments)

13.26 KB, patch
keeler
: review+
Details | Diff | Splinter Review
2.30 KB, patch
keeler
: review+
Details | Diff | Splinter Review
66.98 KB, patch
keeler
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
Created attachment 572850 [details] [diff] [review]
Patch v1

For testing the upcoming support for OCSP stapling in NSS, we need to be able to test it in PSM/Firefox.

This bug proposes to add a pref, off by default, which allows us to manually enable support for this new feature.

This depends on landing an NSS release in mozilla-central which support the new feature.
(Reporter)

Updated

6 years ago
Depends on: 360420
(Reporter)

Updated

6 years ago
Blocks: 700698
(Reporter)

Comment 1

6 years ago
We need changes in PSM to support OCSP stapling, in a way that is compatible with the recent SSL thread changes. Therefore I'm extending the scope of this bug to also impleent those PSM level changes.
Summary: Add pref to enable OCSP stapling in PSM (off by default) → PSM changes to support OCSP stapling (off by default)
(Reporter)

Comment 2

6 years ago
Created attachment 581512 [details] [diff] [review]
Patch v10
Assignee: nobody → kaie
Attachment #572850 - Attachment is obsolete: true
(In reply to Kai Engert (:kaie) from comment #2)
> Created attachment 581512 [details] [diff] [review]
> Patch v10

(1) Because certificate validation is happening on a thread that isn't the socket transport thread, and the file descriptor might be closed while we are doing certificate validation, we cannot call any functions on the file descriptor during certificate validation.

(2) I am working on the fix for bug 660749. In my patch for that, we will not have a file descriptor for an SSL socket to pass from the cache to SSLServerCertVerification; we will instead have to pass a non-PRFileDesc pointer as fdForLogging.

For these reasons, we should avoid adding any dependencies on fdForLogging being a file descriptor. This is why I made fdForLogging a const void*.
(Reporter)

Comment 4

6 years ago
Created attachment 581796 [details] [diff] [review]
Patch v11

I addressed your requests.
Attachment #581512 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Attachment #581796 - Flags: review?(bsmith)
Keywords: dev-doc-needed
Target Milestone: --- → mozilla12
Comment on attachment 581796 [details] [diff] [review]
Patch v11

I am clearing the review flag on this, because I am pretty sure you replace this patch with a new one after addressing the review comments in bug 360420.
Attachment #581796 - Flags: review?(bsmith)
(Reporter)

Comment 6

5 years ago
reassign bug owner.
mass-update-kaie-20120918
Assignee: kaie → nobody
(Reporter)

Comment 7

5 years ago
Created attachment 675574 [details] [diff] [review]
patch v29

matches patch p29 from bug 360420
Attachment #581796 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Target Milestone: mozilla12 → mozilla19
Blocks: 803582
Depends on: 714477
David is working on this now.
Assignee: nobody → dkeeler
Comment on attachment 675574 [details] [diff] [review]
patch v29

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

The main thing we need to do here is document what happens and test the various cases:

* No stapled response
* "Good" response for the given cert that is valid with respect to freshness.
* "tryLater" stapled response (which is unsigned)
* "Good" response for a different cert
* "Good" response for the given cert, but which is stale.
* "Revoked" response.
* "Unknown" response.
* More?

The test for the first case should ensure that we do the OCSP request (as long as we support making such requests).

The test for the second case should ensure we don't do the OCSP request even though the cert has an AIA ocsp responder URI.

The tests for the rest of the cases, except tryLater, should ensure that the connection fails.

Open question: What to do about tryLater? Either we should fail the connection or we should make the OCSP request via HTTP. I suggest that we fail the connection, because when we add "must-staple" support, tryLater doesn't make any sense if the goal is to make "must-staple" part of the justification for not doing OCSP fetches any more.

It will take some effort to get the infrastructure for testing this working. I have some ideas about how to do it relatively painlessly using GTest. We should sit down and discuss it.

Kai already added functions that can be used to generate OCSP responses for testing:

CERT_CreateOCSPSingleResponseGood;
CERT_CreateOCSPSingleResponseUnknown;
CERT_CreateOCSPSingleResponseRevoked;
CERT_CreateEncodedOCSPSuccessResponse;
CERT_CreateEncodedOCSPErrorResponse;

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +1130,5 @@
> +  const SECItemArray *csa = SSL_PeerStapledOCSPResponses(fd);
> +  if (csa && csa->len) {
> +      CERTCertDBHandle *handle = CERT_GetDefaultCertDB();
> +      PRTime now = PR_Now();
> +      for (unsigned i = 0; i < csa->len; ++i) {

This code is written with anticipated support of the OCSP multi-stapling draft. But, the call to CERT_CacheOCSPResponseFromSideChannel doesn't correctly handle the multi-stapling case, AFAICT. In particular, serverCert should only be passed for the EE OCSP response, not for every OCSP response in the list.

I suggest we forget about multi-stapling for now, and assert that there is only one entry in the list, and process just that one item, which will make the call below correct. Then we can file a follow-up bug to implement multi-stapling in Gecko. (libssl doesn't actually support multi-stapling yet anyway.)

This code doesn't support private browsing correctly. However, the current OCSP cache for non-stapled responses doesn't support private browsing either. After we've had upgraded from the classic certificate verification library, we can fix the private browsing issue. For now, we should just add a "XXX" comment about this and file a follow-up bug.

Ideally the value of "now" should match the value of "now" used for verifying the certificate. If this is difficult to do for some reason, then add a "XXX" comment about it.

The CERT_CacheOCSPResponseFromSideChannel call doesn't do path building as well as it should. Again, we will have to resolve that issue when we upgrade from the classic cert verification library. We should file a follow-up bug to do that.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +1789,5 @@
>        SSL_OptionSetDefault(SSL_ENABLE_FALSE_START, enabled);
>  #endif
>  
> +      mPrefBranch->GetBoolPref("security.ssl.enable_ocsp_stapling", &enabled);
> +      SSL_OptionSetDefault(SSL_ENABLE_OCSP_STAPLING, enabled);

This would affect DTLS too, which doesn't seem like a good idea to me. Instead, we should set the option on the socket in nsSSLIOLayerSetOptions. That means we will need to save a copy of the pref value in SharedSSLState, so we can access it from nsSSLIOLayerSetOptions, since nsSSLIOLayerSetOptions is done off the main thread, where nsIPrefService is not available.
(Reporter)

Updated

5 years ago
Depends on: 858231
Created attachment 739840 [details] [diff] [review]
PSM changes

Here's the PSM changes necessary for OCSP stapling as far as I've understood the previous patch and Brian's comments. This is still in progress, so I'm just asking for feedback for now.
Attachment #739840 - Flags: feedback?(bsmith)
Created attachment 739843 [details] [diff] [review]
OCSP stapling testing

This patch adds support for testing OCSP stapling. I'd appreciate feedback on if this is a good direction to go. It works on linux (apply the patches, ./mach build, ./mach xpcshell-test security/manager/ssl/tests/unit/test_ocsp_stapling.js), but it doesn't handle running the SSL server cross-platform, yet (probably just requires adding a binary-suffix). Also, I can't get it to reject some certificate/OCSP response combinations that I think should fail - maybe I'm not creating my certificates correctly?
Attachment #739843 - Flags: feedback?(bsmith)
Created attachment 741028 [details] [diff] [review]
OCSP stapling testing v2

Most up-to-date patch so we can talk about it tomorrow.
Attachment #739843 - Attachment is obsolete: true
Attachment #739843 - Flags: feedback?(bsmith)
Comment on attachment 739840 [details] [diff] [review]
PSM changes

Clearing feedback? for now.
Attachment #739840 - Flags: feedback?(bsmith)
Created attachment 742620 [details] [diff] [review]
PSM changes v2

It turns out, if ocsp stapling is requested and received, if the stapled response is "not satisfactory", the client must end the connection. So, there's no ambiguity as I originally thought.
Attachment #739840 - Attachment is obsolete: true
Attachment #742620 - Flags: review?(bsmith)
Created attachment 742622 [details] [diff] [review]
OCSP stapling testing v2.1

This is the most up-to-date patch for the OCSP tests. Unfortunately, it doesn't work on the try servers yet. I'm having trouble with some warnings-as-errors on some platforms. Also, it looks like while the test runs fine on my machine locally, it again doesn't work on the try machines. I'm still looking into that.
Attachment #741028 - Attachment is obsolete: true
Attachment #742622 - Flags: review?(bsmith)
Created attachment 744918 [details] [diff] [review]
OCSP stapling testing v3

Still having problem with the test infrastructure. I'll keep looking into it. In the meantime, this is the most current implementation of the testing. In particular, I've changed how the server works to be a bit more robust and extensible.
Attachment #742622 - Attachment is obsolete: true
Attachment #744918 - Flags: review?(bsmith)
Attachment #742622 - Flags: review?(bsmith)
Created attachment 745364 [details] [diff] [review]
OCSP stapling testing v4

Ok - now it works on the test infrastructure. Sorry for the bugspam - this is the last time I'll touch these patches until you have the opportunity to give feedback.
Attachment #744918 - Attachment is obsolete: true
Attachment #744918 - Flags: review?(bsmith)
Attachment #745364 - Flags: review?(bsmith)
Comment on attachment 742620 [details] [diff] [review]
PSM changes v2

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

Looks good. Just some minor issues.

::: netwerk/base/public/security-prefs.js
@@ +12,5 @@
>  pref("security.ssl.treat_unsafe_negotiation_as_broken", false);
>  pref("security.ssl.require_safe_negotiation",  false);
>  pref("security.ssl.warn_missing_rfc5746",  1);
>  pref("security.ssl.enable_false_start", false);
> +pref("security.ssl.enable_ocsp_stapling", false);

Why not just enable it now? If we find problems we can always turn it off later. The more people testing it right away, the better, IMO.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +1203,5 @@
> +  const SECItemArray *csa = SSL_PeerStapledOCSPResponses(fd);
> +  // we currently only support single stapled responses
> +  if (csa && csa->len == 1) {
> +      CERTCertDBHandle *handle = CERT_GetDefaultCertDB();
> +      PRTime now = PR_Now();

Ideally, this value of |now| would be the same value that is used to verify the certificate with CertVerifier.

@@ +1210,5 @@
> +      // If the OCSP response is invalid, CERT_CacheOCSPResponseFromSideChannel
> +      // returns SECFailure. We must abort the handshake with
> +      // bad_certificate_status_response(113).
> +      if (cacheResult != SECSuccess) {
> +          PR_SetError(SSL_ERROR_BAD_CERT_STATUS_RESPONSE_ALERT, 0);

This error code is used to mean that the peer sent us a bad_certificate_status_response alert. It isn't used to send a bad_certificate_status_response alert to the peer.

What does CERT_CacheOCSPResponseFromSideChannel set PR_GetError() to? Can we just pass through that value?

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +1986,5 @@
>  
>    // Do that before NSS init, to make sure we won't get unloaded.
>    RegisterObservers();
>  
> +  SharedSSLState::GlobalInit();

Shouldn't this be done in InitializeNSS, after NSS_Init* has been called? In general, it is better to initialize NSS as soon as possible so that other components (SharedSSLState in this case) do not have to worry about it being initialized.

Also, what happens if we call GlobalInit() and then InitializeNSS fails? It seems like we should make sure we're always calling GlobalCleanup if we ever call GlobalInit.

If it is needed to call GlobalInit and/or GlobalCleanup when NSS has not been initialized, then that should be noted in a comment of each of those methods.
Attachment #742620 - Flags: review?(bsmith) → review-
Created attachment 747679 [details] [diff] [review]
OCSP stapling testing v5

I sort-of got into a "Oh, I'll just change this... and this... and this..."
Attachment #745364 - Attachment is obsolete: true
Attachment #745364 - Flags: review?(bsmith)
Attachment #747679 - Flags: review?(bsmith)
Created attachment 747683 [details] [diff] [review]
PSM changes v3

Thanks for the review - here's the updated patch for when you're ready for another look.

(In reply to Brian Smith (:bsmith) from comment #18)
> Comment on attachment 742620 [details] [diff] [review]
> PSM changes v2
> ::: netwerk/base/public/security-prefs.js
> @@ +12,5 @@
> > +pref("security.ssl.enable_ocsp_stapling", false);
> 
> Why not just enable it now?

Sounds good.

> ::: security/manager/ssl/src/SSLServerCertVerification.cpp
> @@ +1203,5 @@
> > +      PRTime now = PR_Now();
> 
> Ideally, this value of |now| would be the same value that is used to verify
> the certificate with CertVerifier.

Ok - I think I changed what you were talking about. You should probably check that I did what you were thinking of, though.

> @@ +1210,5 @@
> > +          PR_SetError(SSL_ERROR_BAD_CERT_STATUS_RESPONSE_ALERT, 0);

Yeah, this was all wrong. I fixed it.

> What does CERT_CacheOCSPResponseFromSideChannel set PR_GetError() to? Can we
> just pass through that value?

Basically, yes.

> ::: security/manager/ssl/src/nsNSSComponent.cpp
> @@ +1986,5 @@
> > +  SharedSSLState::GlobalInit();
> 
> Shouldn't this be done in InitializeNSS, after NSS_Init* has been called? In
> general, it is better to initialize NSS as soon as possible so that other
> components (SharedSSLState in this case) do not have to worry about it being
> initialized.

In this specific case, it just matters that SharedSSLState::GlobalInit() is called before nsNSSComponent::setValidationOptions(). Anyway, I moved it into InitializeNSS().
Attachment #742620 - Attachment is obsolete: true
Attachment #747683 - Flags: review?(bsmith)
Summary: PSM changes to support OCSP stapling (off by default) → PSM changes and tests to support OCSP stapling
Attachment #747679 - Flags: feedback?(cviecco)
Attachment #747683 - Flags: feedback?(cviecco)
Comment on attachment 747683 [details] [diff] [review]
PSM changes v3

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

Looking good.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +1164,5 @@
> +
> +  // This value of "now" is used both here for OCSP stapling and later
> +  // when calling CreateCertErrorRunnable.
> +  PRTime now = PR_Now();
> +  // no ownership

If the SSL_PeerStapledOCSPResponses is guaranteed to return null when SSL_OptionSet(SSL_ENABLE_OCSP_STAPLING_ ..) is disabled then please add a comment, else add 'if ocspstable is enabled' to this whole block following this comment.

@@ +1172,5 @@
> +      CERTCertDBHandle *handle = CERT_GetDefaultCertDB();
> +      SECStatus cacheResult = CERT_CacheOCSPResponseFromSideChannel(
> +          handle, serverCert, now, &csa->items[0], arg);
> +      if (cacheResult != SECSuccess) {
> +          return SECFailure;

If the signature was invalid the call will return secfailure. And in that case (if ocsp is not required) then probably we want to cotinue processing. I would argue that only fail here if the result is a SEC_ERROR_REVOKED_CERTIFICATE. (and in that case do we do PR_seterror?)
Attachment #747683 - Flags: feedback?(cviecco) → feedback+
Comment on attachment 747679 [details] [diff] [review]
OCSP stapling testing v5

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

::: security/manager/ssl/tests/unit/test_ocsp_stapling.js
@@ +1,1 @@
> +"use strict";

It's nice to have a toplevel 1-2 sentence description of this test. E.g., "Connect to a bunch of domains with and without OCSP stapling enabled."

@@ +70,5 @@
> +  let statusNSS = base + offset;
> +  return nssErrorsService.getXPCOMFromNSSError(statusNSS);
> +}
> +
> +/*

uncommented code named whatIsThisCode should probably not be in the patch :)

@@ +148,5 @@
> +  do_check_true(ocspCertDir.exists());
> +  gServerProcess.run(false, [ocspCertDir.path], 1);
> +
> +  // It can take a bit for the server process to start up. Repeatedly
> +  // try to connect to it until we succeed.

Now that you know about promises, that seems like a much better idea :) Just wait on a promise that resolves when it connects to the server.

@@ +192,5 @@
> +  let goodStatus = makeRequest("https://ocsp-stapling-good.example.com");
> +  do_check_eq(goodStatus, Cr.NS_OK);
> +
> +  let revokedStatus = makeRequest("https://ocsp-stapling-revoked.example.com");
> +  // SEC_ERROR_REVOKED_CERTIFICATE = SEC_ERROR_BASE + 12

Link back to https://mxr.mozilla.org/mozilla-central/source/security/nss/lib/util/SECerrs.h (or whatever the canonical thing is)

::: security/manager/ssl/tests/unit/test_ocsp_stapling/OCSPStaplingServer.cpp
@@ +1,1 @@
> +#include <stdio.h>

Same here about top-level descriptions, e.g. Set up OCSP stapling server for use in test_ocsp_stapling.js and maybe a link to the canonical OCSP documentation.

@@ +15,5 @@
> +#define LISTEN_PORT 4443
> +#define MAX_CONNECTIONS 5
> +#define STACK_SIZE 1048576
> +
> +enum ocsp_staple_response_type {

All these need one line comments, or a link to where they're documented. To a novice, cert_expired and cert_expired_fresh_CA are indistinguishable.

@@ +264,5 @@
> +  }
> +  responses = PORT_ArenaNewArray(&arena, CERTOCSPSingleResponse *, 2);
> +  responses[0] = sr;
> +  responses[1] = NULL;
> +  // XXX does ca need freeing?

https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/ScopedNSSTypes.h#81

Use ScopedCERTCertificate and you will not have to worry (maybe)
Comment on attachment 747679 [details] [diff] [review]
OCSP stapling testing v5

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

::: security/manager/ssl/tests/unit/test_ocsp_stapling.js
@@ +34,5 @@
> +  let ios = Cc["@mozilla.org/network/io-service;1"]
> +              .getService(Ci.nsIIOService);
> +  let channel = ios.newChannel(aURL, null, null);
> +  let listener = new StreamListener();
> +  channel.asyncOpen(listener, null);

As we discussed on IRC, our HTTP stack is doing tricky things, and will be doing more tricky things, regarding connection handling. So, it isn't a good idea to make any assumption of what will happen with connections and how they are reused or whether connections will have OCSP stapling enabled or not in a test like this. Instead, please use the socket tranport service API to create a new raw socket for each version of makeRequest.

@@ +40,5 @@
> +  let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +  timer.initWithCallback(function timeout() {
> +    listener.status = Cr.NS_ERROR_NET_TIMEOUT;
> +    listener.done = true;
> +  }, REQUEST_TIMEOUT, Ci.nsITimer.TYPE_ONE_SHOT);

It seems like you only need to do this timeout logic for the first, dummy, connection, while you wait for the server to get started up. IMO, it would be better if we only did the timeout logic for that first dummy connection, and not for subsequent connections. That way, we can notice problems with the system that might be wallpapered by this timeout logic.

@@ +52,5 @@
> +
> +  return listener.status;
> +}
> +
> +function readFile(filename) {

Please move this to a head_psm.js and have xpcshell.ini's "head" reference head_psm.js. That way, the code can be shared by all tests. (See test_signed_apps.js and the certificate test that Camilo wrote, where this is also copy-pasted.)

@@ +61,5 @@
> +  fstream.close();
> +  return data;
> +}
> +
> +function getXPCOMStatusFromNSS(offset, baseIsSEC) {

Similarly, this should also be moved to head_psm.js.

@@ +70,5 @@
> +  let statusNSS = base + offset;
> +  return nssErrorsService.getXPCOMFromNSSError(statusNSS);
> +}
> +
> +/*

I agree. Or, put it in head_psm.js.

@@ +93,5 @@
> +function cleanup() {
> +  Services.prefs.clearUserPref("network.proxy.type");
> +  Services.prefs.clearUserPref("network.proxy.ssl");
> +  Services.prefs.clearUserPref("network.proxy.ssl_port");
> +  Services.prefs.clearUserPref("security.ssl.enable_ocsp_stapling");

You do not need to clean up during xpcshell tests, because each xpcshell test gets its own profile.

@@ +99,5 @@
> +  try {
> +    // this throws for some reason, even though it successfully kills the proc.
> +    // (hence the do_check_false(gServerProcess.isRunning) after)
> +    gServerProcess.kill();
> +  } catch (e) {}

Might be worth asking on #ateam or #developers about why this throws.

@@ +126,5 @@
> +  envSvc.set("LD_LIBRARY_PATH", greDir.path);
> +  envSvc.set("OCSP_SERVER_DEBUG_LEVEL", "3");
> +  envSvc.set("OCSP_SERVER_BURN_RUBBER", "1");
> +  let serverBin = directoryService.get("CurProcD", Ci.nsILocalFile);
> +  let isWindows = ("@mozilla.org/windows-registry-key;1" in Cc);

nit: isWindows would be another candidate for head_psm.js

@@ +164,5 @@
> +  nonStaplingStatus = makeRequest("https://ocsp-stapling-revoked.example.com");
> +  do_check_eq(nonStaplingStatus, Cr.NS_OK);
> +  nonStaplingStatus = makeRequest("https://ocsp-stapling-unknown.example.com");
> +  do_check_eq(nonStaplingStatus, Cr.NS_OK);
> +  nonStaplingStatus = makeRequest("https://ocsp-stapling-good-other.example.com");

What does "other" mean? Perhaps "wrong-end-entity-cert"?

@@ +166,5 @@
> +  nonStaplingStatus = makeRequest("https://ocsp-stapling-unknown.example.com");
> +  do_check_eq(nonStaplingStatus, Cr.NS_OK);
> +  nonStaplingStatus = makeRequest("https://ocsp-stapling-good-other.example.com");
> +  do_check_eq(nonStaplingStatus, Cr.NS_OK);
> +  nonStaplingStatus = makeRequest("https://ocsp-stapling-good-other-ca.example.com");

Nit: s/other/wrong-ca/

@@ +170,5 @@
> +  nonStaplingStatus = makeRequest("https://ocsp-stapling-good-other-ca.example.com");
> +  do_check_eq(nonStaplingStatus, Cr.NS_OK);
> +  nonStaplingStatus = makeRequest("https://ocsp-stapling-expired.example.com");
> +  do_check_eq(nonStaplingStatus, Cr.NS_OK);
> +  nonStaplingStatus = makeRequest("https://ocsp-stapling-expired-fresh-ca.example.com");

Most of the other tests are clear from the name, but this one is not. What is this testing?

@@ +193,5 @@
> +  do_check_eq(goodStatus, Cr.NS_OK);
> +
> +  let revokedStatus = makeRequest("https://ocsp-stapling-revoked.example.com");
> +  // SEC_ERROR_REVOKED_CERTIFICATE = SEC_ERROR_BASE + 12
> +  do_check_eq(revokedStatus, getXPCOMStatusFromNSS(12, true));

Up to you if you want to do this now, or somebody can do it in a follow-up bug: It would be great if we could put a bunch of constants in head_psm.js like NS_ERROR_SEC_ERROR_OCSP_UNKNOWN_CERT = getXPCOMStatusFromNSS(12, true) so that we could write these as do_check_eq(status, NS_ERROR_SEC_ERROR_OCSP_UNKNOWN_CERT). This would be useful in many tests.

Nit: I don't think it is good to name the variables "revokedStatus, "nonStaplingStatus," etc., so that we don't have to worry about the wrong variable being checked in the do_check_eq. Seems like a single "status" variable would be clear enough. or, you could even have makeRequest take the expected result:

   makeRequest("https://ocsp-stapling-revoked.example.com",
               NS_ERROR_ SEC_ERROR_REVOKED_CERTIFICATE):

and then have makeRequest do the do_check_eq call.

::: security/manager/ssl/tests/unit/test_ocsp_stapling/OCSPStaplingServer.cpp
@@ +15,5 @@
> +#define LISTEN_PORT 4443
> +#define MAX_CONNECTIONS 5
> +#define STACK_SIZE 1048576
> +
> +enum ocsp_staple_response_type {

I think that most of these are pretty obvious except for the "other" cases. I would ensure that the names of the subdomains match the names of the enum names used here though.

@@ +32,5 @@
> +  osrt_needssig,
> +  osrt_unauthorized
> +};
> +
> +struct ocsp_host{

whitespace.

@@ +52,5 @@
> +  { "ocsp-stapling-srverr.example.com", "srverr", osrt_srverr },
> +  { "ocsp-stapling-trylater.example.com", "trylater", osrt_trylater },
> +  { "ocsp-stapling-needssig.example.com", "needssig", osrt_needssig },
> +  { "ocsp-stapling-unauthorized.example.com", "unauthorized", osrt_unauthorized },
> +  { NULL, NULL, osrt_null }

nullptr.

@@ +118,5 @@
> +#define DEFAULT_PEEK_TIMEOUT 1000
> +uint32_t gPeekTimeout = DEFAULT_PEEK_TIMEOUT;
> +
> +void print_pr_error(const char *prefix) {
> +  int32_t errorlen = PR_GetErrorTextLength();

Try PR_ErrorToName, which is more useful for developers than what PR_GetErrorText gives us.

@@ +135,5 @@
> +    }
> +  }
> +}
> +
> +void send_all(connection *conn, const char *data, size_t data_len) {

If you make the change to use the socket transport service directly, then you can greatly simplify the protocol that the client and server speak. In fact, I don't think you actually need to send/receive any data in either direction for these tests if you are just using a bare SSL connection, except perhaps a single byte from the client to the server to tell it to close the connection. I think that would simplify this code quite a bit.

@@ +168,5 @@
> +  }
> +
> +  if (host->host_name == NULL) {
> +    if (gDebugLevel >= DEBUG_WARNINGS) {
> +      fprintf(stderr, "host '%s' not in pre-defined list - add it?\n", server_name);

Perhaps we should just MOZ_CRASH here.

@@ +176,5 @@
> +
> +  return host;
> +}
> +
> +void setup_tls(connection *conn) {

Brace on separate line, return value on separate line (and elsewhere)./

@@ +181,5 @@
> +  if (gDebugLevel >= DEBUG_VERBOSE) {
> +    fprintf(stderr, "found pre-defined host '%s'\n", conn->host->host_name);
> +  }
> +
> +  ScopedCERTCertificate cert(PK11_FindCertFromNickname(conn->host->cert_name, NULL));

I think it would be great if you could factor out a "CreateOCSPResponseForCert" function that encapsulates lines 185-314, to make this easier to read--in particular, easier to compare to other ssl server configuration code. (You might compare your SSL server setup code to the code in the test server in bug 702322.)

@@ +266,5 @@
> +  responses[0] = sr;
> +  responses[1] = NULL;
> +  // XXX does ca need freeing?
> +  CERTCertificate *ca = NULL;
> +  CERTCertificateList *certChain = NULL;

I agree with mmc. It is better to use Scoped* when possible, and add new Scoped* types (perhaps local to this source file) as necessary. This way, you can use "return SECFailure" whenever you encounter an error, to make the control flow (especially when an error occurs) much easier to understand.

@@ +275,5 @@
> +      conn->state = state_done;
> +      return;
> +    }
> +  } else {
> +    certChain = CERT_CertChainFromCert(cert, certUsageAnyCA, PR_TRUE);

I think you just need to use CERT_FindCertIssuer(cert), right?

If so, put an "XXX" comment here, because CERT_FindCertIssuer (and CERT_CertChainFromCert) are deprecated functions, because they use the old path building logic and not the PKIX-compliant path building logic.

@@ +311,5 @@
> +    response = CERT_CreateEncodedOCSPSuccessResponse(
> +      &arena, ca, ocspResponderID_byName, signTime, responses, NULL);
> +  }
> +
> +  SECItemArray *arr = SECITEM_AllocArray(&arena, NULL, 1);

ScopedSECItemArray.

@@ +533,5 @@
> +    print_pr_error("PR_Bind failed");
> +    return;
> +  }
> +
> +  if (PR_Listen(server_socket, MAX_CONNECTIONS) != PR_SUCCESS) {

Do we need this server to support more than one connection at a time? It seems like we can make this much simpler and avoid threads and the state machine you created: listen on the socket, wait for a single byte to be received, and the close the socket, rinse & repeat. Perhaps we could even inspect that single byte for a "shutdown" command to avoid the "kill" in the client code.
Attachment #747679 - Flags: review?(bsmith) → review-
(In reply to Camilo Viecco (:cviecco) from comment #21)
> ::: security/manager/ssl/src/SSLServerCertVerification.cpp
> @@ +1164,5 @@
> If the SSL_PeerStapledOCSPResponses is guaranteed to return null when
> SSL_OptionSet(SSL_ENABLE_OCSP_STAPLING_ ..) is disabled then please add a
> comment, else add 'if ocspstable is enabled' to this whole block following
> this comment.

Good call.

> @@ +1172,5 @@
> If the signature was invalid the call will return secfailure. And in that
> case (if ocsp is not required) then probably we want to cotinue processing.
> I would argue that only fail here if the result is a
> SEC_ERROR_REVOKED_CERTIFICATE. (and in that case do we do PR_seterror?)

The spec says clients must "abort the handshake if the response is not satisfactory". I think if there is any problem with the response, then it's not satisfactory, so we should kill the connection. PR_SetError gets called for us.

Thanks for the feedback!
(In reply to Brian Smith (:bsmith) from comment #23)
> ::: security/manager/ssl/tests/unit/test_ocsp_stapling.js
> @@ +40,5 @@
> It seems like you only need to do this timeout logic for the first, dummy,
> connection, while you wait for the server to get started up. IMO, it would
> be better if we only did the timeout logic for that first dummy connection,
> and not for subsequent connections. That way, we can notice problems with
> the system that might be wallpapered by this timeout logic.

The test now listens for the server to call it back when it's ready to go, so there's no need for this timeout logic anymore.

> @@ +52,5 @@
> Please move this to a head_psm.js and have xpcshell.ini's "head" reference
> head_psm.js. That way, the code can be shared by all tests. (See
> test_signed_apps.js and the certificate test that Camilo wrote, where this
> is also copy-pasted.)

Camilo started work on getting rid of the copy-pasting in bug 877441.

> @@ +99,5 @@
> > +  try {
> > +    // this throws for some reason, even though it successfully kills the proc.
> > +    // (hence the do_check_false(gServerProcess.isRunning) after)
> > +    gServerProcess.kill();
> > +  } catch (e) {}
> 
> Might be worth asking on #ateam or #developers about why this throws.

As it turns out, this no longer throws. I have no idea what's going on here, but I took out the try-catch, so we'll see if it happens again.

> @@ +170,5 @@
> > +  nonStaplingStatus = makeRequest("https://ocsp-stapling-expired-fresh-ca.example.com");
> 
> Most of the other tests are clear from the name, but this one is not. What
> is this testing?

The first expired test has an old signature on the response. This one has a fresh signature, but the time for which it is valid is more than a week ago.

> ::: security/manager/ssl/tests/unit/test_ocsp_stapling/OCSPStaplingServer.cpp
> @@ +135,5 @@
> If you make the change to use the socket transport service directly, then
> you can greatly simplify the protocol that the client and server speak. In
> fact, I don't think you actually need to send/receive any data in either
> direction for these tests if you are just using a bare SSL connection,
> except perhaps a single byte from the client to the server to tell it to
> close the connection. I think that would simplify this code quite a bit.

We still need to know which "host" the test is attempting to connect to, however, hence the CONNECT. Once there's the mechanism to handle that properly, the rest is easy. It might be worth not doing the whole request/response after that, but I'll think about it.

Thanks for the review. If I didn't reply to one of your comments, it means I agree and/or no other discussion is necessary.
Comment on attachment 747679 [details] [diff] [review]
OCSP stapling testing v5

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

Looks promising, but still lots of things to solve

Pretty much everything was covered between bsmith and mmc. (only giving feedback now to remove my needinfo).

::: security/manager/ssl/tests/unit/test_ocsp_stapling/OCSPStaplingServer.cpp
@@ +291,5 @@
> +    signTime = oldNow;
> +  }
> +
> +  SECItem *response = NULL;
> +  if (conn->host->osrt == osrt_malformed) {

I would put these into a switch for readability reasons.
Attachment #747679 - Flags: feedback?(cviecco) → feedback-
Created attachment 758891 [details] [diff] [review]
PSM changes v3.1

This is ready for final review from bsmith.
Attachment #747683 - Attachment is obsolete: true
Attachment #747683 - Flags: review?(bsmith)
Attachment #758891 - Flags: review?(bsmith)
Created attachment 758892 [details] [diff] [review]
OCSP stapling testing v6

This is ready for another, possibly final review from bsmith.
Attachment #747679 - Attachment is obsolete: true
Attachment #758892 - Flags: review?(bsmith)
Attachment #675574 - Attachment is obsolete: true
Comment on attachment 758891 [details] [diff] [review]
PSM changes v3.1

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

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +1164,5 @@
> +
> +  // This value of "now" is used both here for OCSP stapling and later
> +  // when calling CreateCertErrorRunnable.
> +  PRTime now = PR_Now();
> +  if (socketInfo->SharedState().GetOCSPStaplingEnabled()) {

s/Get/Is/ for boolean properties.

Do you think we should use socketInfo->SharedState().GetOCSPStaplingEnabled(), or should we use SSL_OptionGet()? If we use SSL_ENABLE_OCSP_STAPLING then we know we're being consistent with the SSL_OptionSet we used for this socket, even in the (unlikely) case where that property was modified between the time we created the socket and the time we're doing this test.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +1102,5 @@
>      firstNetworkRevo = FIRST_REVO_METHOD_DEFAULT;
> +
> +  bool ocspStaplingEnabled;
> +  rv = pref->GetBoolPref("security.ssl.enable_ocsp_stapling", &ocspStaplingEnabled);
> +  if (NS_FAILED(rv))

Please use braces around the body of if statements.

@@ +1104,5 @@
> +  bool ocspStaplingEnabled;
> +  rv = pref->GetBoolPref("security.ssl.enable_ocsp_stapling", &ocspStaplingEnabled);
> +  if (NS_FAILED(rv))
> +    ocspStaplingEnabled = OCSP_STAPLING_ENABLED_DEFAULT;
> +  if (!ocspEnabled)

ditto.

@@ +1771,5 @@
>  
>      if (problem_no_security_at_all != which_nss_problem) {
>  
>        mNSSInitialized = true;
> +      SharedSSLState::GlobalInit();

Nit: I suggest putting the NSS initialization (NSS_SetDomesticPolicy and Pk11_SetPasswordFunc) ahead of the PSM initialization (GlobalInit).
Attachment #758891 - Flags: review?(bsmith) → review+
Comment on attachment 758892 [details] [diff] [review]
OCSP stapling testing v6

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

::: security/manager/ssl/tests/unit/test_ocsp_stapling.js
@@ +49,5 @@
> +      available = aStream.available();
> +    } catch (e) {
> +      this.defer.resolve(e.result);
> +    }
> +    if (available > 0) {

I think you can rewrite this as:

try {
   let str = NetUtil.readInputStreamToString(aStream, available);
   do_check_eq(str, "0");
   this.inputStream.close();
   this.inputStream = null;
   this.outputStream.close();
   this.outputStream = null;
   this.transport = null;
   this.defer.resolve(Cr.NS_OK);
} catch (e) {
   this.defer.resolve(e.result);
}

@@ +144,5 @@
> +  aResponse.finish();
> +  run_test_body();
> +}
> +
> +function run_test_body() {

consider:

function add_ct(aEnabled, aServer, aExpectedResult) {
  add_test(function() {
    Services.prefs.setBoolPref("security.ssl.enable_ocsp_stapling", aEnabled);
    connectTo(aServer).then(function(aresult) {
      do_check_eq(aResult, aExpectedResult);
      run_next_test();
    });
  }
}

Then you could write:

do_register_cleanup(function() { gHttpServer.stop(do_test_finished); } )

add_ct(true, "ocsp-stapling-good-other-ca.example.com",
       getXPCOMStatusFromNSS(18)); // SEC_ERROR_OCSP_UNAUTHORIZED_RESPONSE
...

This would be clearer, with less repeated code, and it would match the style of more existing xpcshell tests throughout the project.

Up to you if you want to make this change or not.

@@ +183,5 @@
> +    .then(function() { return connectTo("ocsp-stapling-revoked.example.com"); })
> +    .then(function(aResult) { do_check_eq(aResult, getXPCOMStatusFromNSS(12)); })
> +    // This stapled response is from a CA that is untrusted and did not issue
> +    // the server's certificate.
> +    // SEC_ERROR_BAD_DATABASE = SEC_ERROR_BASE + 18

I am surprised that we get SEC_ERROR_BAD_DATABASE here. This seems like a bug in NSS that is worth filing.

::: security/manager/ssl/tests/unit/test_ocsp_stapling/OCSPStaplingServer.cpp
@@ +9,5 @@
> +// same byte back.
> +// This server also has the ability to "call back" another process waiting on
> +// it. That is, when the server is all set up and ready to receive connections,
> +// it will connect to a specified port and issue a simple HTTP request.
> +// Currently, only test_ocsp_stapling.js uses this server.

Might as well remove this last sentence before it gets out of date.

@@ +44,5 @@
> +  osrt_needssig,         // the response needs a signature
> +  osrt_unauthorized      // the responder is not authorized for this certificate
> +};
> +
> +struct ocsp_host {

Please use Mozilla naming convections and brace style:

struct OCSPHost
{

@@ +67,5 @@
> +  { "ocsp-stapling-unauthorized.example.com", "unauthorized", osrt_unauthorized },
> +  { nullptr, nullptr, osrt_null }
> +};
> +
> +struct connection {

struct Connection
{

@@ +77,5 @@
> +};
> +
> +connection::connection(PRFileDesc *socket) : host(nullptr), socket(socket), byte(0) {}
> +
> +using namespace mozilla;

Place this right after the #includes.

@@ +90,5 @@
> +uint32_t gPeekTimeout = DEFAULT_PEEK_TIMEOUT;
> +
> +uint32_t gCallbackPort = 0;
> +
> +void print_pr_error(const char *prefix) {

static void
PrintPRError(const char *prefix)
{

Similarly elsewhere.

@@ +125,5 @@
> +}
> +
> +nsresult reply_to_request(connection *conn) {
> +  char buf[2] = { conn->byte, 0 };
> +  return send_all(conn->socket, buf, 1);

Why do you need buf? Why not:

return send_all(conn->socket, &conn->byte, 1);

In particular, why of buf[] two instead of one? It seems like it doesn't need to be null-terminated but it is.

@@ +130,5 @@
> +}
> +
> +const ocsp_host *get_ocsp_host(const char *server_name) {
> +  const ocsp_host *host = ocsp_hosts;
> +  while (host->host_name != nullptr &&

you can use (x) instead of (x != nullptr) and you can use (!x) instead of (x == nullptr).

@@ +156,5 @@
> +  SSL_OptionSet(ssl_socket, SSL_SECURITY, true);
> +  SSL_OptionSet(ssl_socket, SSL_HANDSHAKE_AS_CLIENT, false);
> +  SSL_OptionSet(ssl_socket, SSL_HANDSHAKE_AS_SERVER, true);
> +
> +  if (SSL_ForceHandshake(ssl_socket) != SECSuccess) {

AFAICT, you should not need to call SSL_ForceHandshake here, but SSL_ResetHandshake(fd, /* asServer */ 1) instead. What is the SSL_ForceHandshake for?

@@ +172,5 @@
> +    print_pr_error("PR_Recv failed");
> +    return NS_ERROR_FAILURE;
> +  } else {
> +    if (gDebugLevel >= DEBUG_VERBOSE) {
> +      fprintf(stderr, "read '%c'\n", conn->byte);

Perhaps (char) conn->byte?

Will %c escape bytes? It would be clearer as fprintf(stderr, "read %d\n", (int) conn->byte);

@@ +186,5 @@
> +    rv = read_request(&conn);
> +  }
> +  if (NS_SUCCEEDED(rv)) {
> +    rv = reply_to_request(&conn);
> +  }

if (NS_FAILED(rv)) { exit(1) }; ?

@@ +201,5 @@
> +  PRNetAddr addr;
> +  PR_InitializeNetAddr(PR_IpAddrLoopback, gCallbackPort, &addr);
> +  if (PR_Connect(socket, &addr, PR_INTERVAL_NO_TIMEOUT) != PR_SUCCESS) {
> +    print_pr_error("PR_Connect failed");
> +    return;

exit(1)?

@@ +208,5 @@
> +  const char *request = "GET / HTTP/1.0\r\n\r\n";
> +  send_all(socket, request, strlen(request));
> +  char buf[4096];
> +  memset(buf, 0, sizeof(buf));
> +  PR_Recv(socket, buf, sizeof(buf) - 1, 0, PR_INTERVAL_NO_TIMEOUT);

Check return value and exit if error?

@@ +211,5 @@
> +  memset(buf, 0, sizeof(buf));
> +  PR_Recv(socket, buf, sizeof(buf) - 1, 0, PR_INTERVAL_NO_TIMEOUT);
> +  fprintf(stderr, "%s\n", buf);
> +  memset(buf, 0, sizeof(buf));
> +  PR_Recv(socket, buf, sizeof(buf) - 1, 0, PR_INTERVAL_NO_TIMEOUT);

Ditto.

@@ +259,5 @@
> +      if (gDebugLevel >= DEBUG_ERRORS) {
> +        fprintf(stderr, "bad ocsp response type: %d\n", osrt);
> +      }
> +      break;
> +  }

seems like we should check sr to make sure we succeeded in the cases where we called a CERT_* function.

@@ +261,5 @@
> +      }
> +      break;
> +  }
> +  CERTOCSPSingleResponse **responses;
> +  responses = PORT_ArenaNewArray(arena, CERTOCSPSingleResponse *, 2);

It would be clearer if you set up |responses| in the single case that requires it below. Also, does |responses| leak?

@@ +384,5 @@
> +  if (!hostInfo) {
> +    print_pr_error("SSL_GetNegotiatedHostInfo returned NULL (not fatal)");
> +    SSL_ResetHandshake(fd, /* asServer */ 1);
> +    SSL_ForceHandshake(fd);
> +  }

This seems very unusual to me. Are we trying to reuse the same TCP connection over and over again, instead of having a separate TCP connection for each request from the client?

@@ +424,5 @@
> +    return;
> +  }
> +
> +  if (SECSuccess != SSL_OptionSet(model_socket, SSL_NO_LOCKS, PR_TRUE)) {
> +    print_pr_error("SSL_OptionSet(SSL_NO_LOCKS) failed");

Why? I think you are doing this because you are mis-using the handshake callback. If we fix that part then this shouldn't be needed, right?

::: security/manager/ssl/tests/unit/test_ocsp_stapling/gen_ocsp_certs.sh
@@ +78,5 @@
> +  check_retval
> +  echo "running \"$RUN_MOZILLA $CERTUTIL -d $OUTPUT_DIR -L -n $NICKNAME -r > $OUTPUT_DIR/$DERFILE\""
> +  $RUN_MOZILLA $CERTUTIL -d $OUTPUT_DIR -L -n $NICKNAME -r > $OUTPUT_DIR/$DERFILE
> +  check_retval
> +  sleep 1

why sleep?

@@ +90,5 @@
> +
> +  echo "running 'echo -e \"$CERT_RESPONSES\" | $RUN_MOZILLA $CERTUTIL -d $OUTPUT_DIR -S -s \"$SUBJECT\" -n $NICKNAME -c $CA -t Pu,u,u $COMMON_ARGS'"
> +  echo -e "$CERT_RESPONSES" | $RUN_MOZILLA $CERTUTIL -d $OUTPUT_DIR -S -s "$SUBJECT" -n $NICKNAME -c $CA -t Pu,u,u $COMMON_ARGS
> +  check_retval
> +  sleep 1

why sleep?
Attachment #758892 - Flags: review?(bsmith)
Created attachment 763900 [details] [diff] [review]
PSM changes v3.2

Addressed comments - carrying over r+.
Attachment #758891 - Attachment is obsolete: true
Attachment #763900 - Flags: review+
Created attachment 763902 [details] [diff] [review]
OCSP stapling testing v7

Thank you for the review - it was definitely helpful in fixing some issues I had. A lot changed (albeit mostly stylistic) in OCSPStaplingServer.cpp, so maybe one more review would be a good idea.
As a reminder to me, we talked about testing using a delegated response signer, so I'll file a follow-up for that.
Attachment #758892 - Attachment is obsolete: true
Attachment #763902 - Flags: review?(bsmith)
Comment on attachment 763902 [details] [diff] [review]
OCSP stapling testing v7

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

r+.

::: security/manager/ssl/tests/unit/test_ocsp_stapling/OCSPStaplingServer.cpp
@@ +92,5 @@
> +, mSocket(aSocket)
> +, mByte(0)
> +{}
> +
> +void PrintPRError(const char *aPrefix)

return type on own line (here and elsewhere).

@@ +194,5 @@
> +    rv = ReadRequest(&conn);
> +  }
> +  if (NS_SUCCEEDED(rv)) {
> +    rv = ReplyToRequest(&conn);
> +  }

if SetupTLS failed then we shouldn't PR_Close(conn.mSocket);

@@ +461,5 @@
> +  if (!hostInfo) {
> +    PrintPRError("SSL_GetNegotiatedHostInfo returned NULL (not fatal)");
> +    // If the client hasn't negotiated SNI, redo the handshake in the
> +    // hopes that the client will try SNI (which is, in fact, what happens).
> +    SSL_ResetHandshake(aFd, /* asServer */ 1);

No, we can't reset the handshake here, as discussed in person. exit(1) is good enough for this test code.

@@ +467,5 @@
> +}
> +
> +void StartServer()
> +{
> +  PRFileDesc *serverSocket = PR_NewTCPSocket();

ScopedPRFileDesc

@@ +490,5 @@
> +    PrintPRError("PR_Listen failed");
> +    return;
> +  }
> +
> +  PRFileDesc *modelSocket = PR_NewTCPSocket();

ScopedPRFileDesc.
Attachment #763902 - Flags: review?(bsmith) → review+
Created attachment 765487 [details] [diff] [review]
OCSP stapling testing v7.1

Carrying over r+.

https://tbpl.mozilla.org/?tree=Try&rev=cbc8a743d45e
https://tbpl.mozilla.org/?tree=Try&rev=b3c93b90d1ac

https://hg.mozilla.org/integration/mozilla-inbound/rev/8cea4b4646ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/448ba56d9ba4
Attachment #763902 - Attachment is obsolete: true
Attachment #765487 - Flags: review+
Backed out for intermittent timeouts.
https://hg.mozilla.org/mozilla-central/rev/9274b04dcc5d

https://tbpl.mozilla.org/php/getParsedLog.php?id=24403192&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=24401818&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=24403699&tree=Mozilla-Inbound
Target Milestone: mozilla19 → ---
Created attachment 766115 [details] [diff] [review]
fix test timeouts

Brian reviewed this in person. This should fix the (or at least, one) problem with the test.
https://tbpl.mozilla.org/?tree=Try&rev=df4cadfc44cd
Attachment #766115 - Flags: review+
Created attachment 766142 [details] [diff] [review]
OCSP stapling testing v7.1.1

Looks like I forgot to add r=bsmith in this patch - here it is for completeness.
Attachment #765487 - Attachment is obsolete: true
Attachment #766142 - Flags: review+
The failures in that try run were all unrelated, and it looks like the ocsp test ran succesfully every time, so I went ahead and pushed to inbound again:

https://hg.mozilla.org/integration/mozilla-inbound/rev/98cfdd66fe87
https://hg.mozilla.org/integration/mozilla-inbound/rev/08d0a6af3557
https://hg.mozilla.org/integration/mozilla-inbound/rev/19d9d359ed5e
https://hg.mozilla.org/mozilla-central/rev/98cfdd66fe87
https://hg.mozilla.org/mozilla-central/rev/08d0a6af3557
https://hg.mozilla.org/mozilla-central/rev/19d9d359ed5e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Blocks: 896078
(Reporter)

Comment 40

4 years ago
I wish this enhancement were part of the ESR 24 branch. But I guess it's too late to propose it.

(The patches apply cleanly, and ESR 24 (currently beta) already contains the NSS version required for this feature to work.)

Updated

4 years ago
Depends on: 914034

Updated

4 years ago
Depends on: 928832

Updated

4 years ago
Depends on: 929068

Updated

4 years ago
Depends on: 929524
Depends on: 929617
You need to log in before you can comment on or make changes to this bug.