Garrett Robinson [:grobinson] Assignee
	Description • 10 years ago

We'd like to be able to access the full peer certificate chain provided in the SSL handshake from a channel. This is useful for SSL error reporting, and maybe other things as well.

SSL_PeerCertificateChain [0] is the relevant underlying NSS call.

[0] http://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl.h#441

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 1 • 10 years ago

Somewhere (in another bug that I can't find right now) we discussed that connections that result in non-overridable errors do not have an SSLStatus object associated with them. They should still have a TransportSecurityInfo, though, so we can put this there.

	Garrett Robinson [:grobinson] Assignee
	Comment 2 • 10 years ago

First pass at storing the chain for failed certs. I decided to put it on TransportSecurityInfo instead of nsSSLStatus, since TSI's presence is not used as a signal elsewhere in the code.

This patch punts on the issue of serialization (see comment). We might need it for e10s though. I think it's probably fine to serialize since we only set it for failed connections, so it shouldn't cause cache bloat in the common case.

It's kind of bummer that we dup the chain twice (once for the thread, and again to store it on TSI). There's other approaches here, but this one seems ok to me (see comment).

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 3 • 10 years ago

Comment on attachment 8453267 [details] [diff] [review]
1029155_expose_peer_certificate_chain.patch

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

Looks good!

::: netwerk/socket/nsITransportSecurityInfo.idl
@@ +22,5 @@
> +     * TODO: will this be serialized? We don't want to store it for every
> +     * connection, since that would bloat the cache and we only need it for
> +     * failed verifications anyway. Perhaps it will be ok to serialize since
> +     * it is only stored if verification fails. On the other hand, do we ever
> +     * cache failed channels? If not, then it doesn't matter either way.

I don't think we do cache failed channels, but we do need to serialize it so we can send it from the parent to the child to the parent again (see bug 948574 comment 21).
You'll need to rev the uuid at https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/TransportSecurityInfo.cpp#292

::: security/manager/ssl/public/nsISSLStatus.idl
@@ +6,5 @@
>  
>  #include "nsISupports.idl"
>  
>  interface nsIX509Cert;
> +interface nsIX509CertList;

Doesn't look like this is necessary anymore.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +801,5 @@
>  
> +  if (rv != SECSuccess) {
> +    // Certificate validation failed; store the peer certificate chain on
> +    // infoObject so it can be used for error reporting
> +    infoObject->SetFailedCertChain(peerCertChain);

Can you pass this as peerCertChain.forget() and make it a ScopedCERTCertList& everywhere you need to pass it around in this file?

::: security/manager/ssl/src/TransportSecurityInfo.cpp
@@ +335,5 @@
> +  // We do not serialize mFailedCertChain. The problem is that doing so would
> +  // bloat every cache entry by the size of the chain. Since we only want the
> +  // chain for reporting SSL errors, and we never cache requests that failed
> +  // the TLS handshake, it's ok not to serialize it.
> +  // TODO this might not be true if we need to serialize the cert across the

Right - see earlier comment.

@@ +1099,5 @@
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +
> +  nsCOMPtr<nsIX509CertList> comCertList;
> +  // XXX it kind of sucks that we have to dup the cert list again... but we do.

See comment in SSLServerCertVerification.cpp.

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +1612,5 @@
>    return newList;
>  }
>  
> +CERTCertList*
> +nsNSSCertList::DupCertListAssumingLock(CERTCertList* aCertList)

What you can do to get around this is just declare an nsNSSShutDownPreventionLock on the stack and pass it to the other function. If you don't check isAlreadyShutDown() (which you can't because that's only available to objects that implement nsNSSShutDownObject), it technically doesn't provide shutdown protection, but since that code is in an NSS callback, NSS should already not be shutting down.

	Garrett Robinson [:grobinson] Assignee
	Comment 4 • 10 years ago

Addresses your feedback comments. I think this is now ready for a review. Does it need tests? What would they be? Maybe:

1. the peer certificate chain should be stored if the certificate fails to verify
2. an nsIX509CertList object A should be equivalent to an nsIX509CertList object B, where B is deserialize(serialize(A))

	Garrett Robinson [:grobinson] Assignee
	Comment 5 • 10 years ago

Removes TODO (because it's TODONE) from TransportSecurityInfo.idl

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 6 • 10 years ago

Comment on attachment 8454047 [details] [diff] [review]
1029155_expose_peer_certificate_chain.patch

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

This is looking pretty good. I should probably take another look, though, so r- for now.
I think the tests suggested in comment 4 are good. I would also check that a successful connection has a null failedCertChain.

::: netwerk/socket/nsITransportSecurityInfo.idl
@@ +15,5 @@
> +
> +    /**
> +     * If certificate verification failed, this will be the peer certificate
> +     * chain provided in the handshake, so it can be used for error reporting.
> +     * May be null if certificate verification succeeded.

How about "Will be null..."

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +805,5 @@
>  
> +  if (rv != SECSuccess) {
> +    // Certificate validation failed; store the peer certificate chain on
> +    // infoObject so it can be used for error reporting
> +    infoObject->SetFailedCertChain(peerCertChain);

We should note that infoObject indirectly takes ownership of peerCertChain.

@@ +831,5 @@
>    }
>  
>    RefPtr<SSLServerCertVerificationJob> job(
>      new SSLServerCertVerificationJob(certVerifier, fdForLogging, infoObject,
> +                                     serverCert, peerCertChain,

Instead of copying the peerCertChain in SSLServerCertVerificationJob's constructor, I think you should copy it here and then take ownership in the constructor (i.e. pass it in as chainCopy.forget()).

::: security/manager/ssl/src/TransportSecurityInfo.cpp
@@ +326,1 @@
>      return rv;

You'll need to rev TRANSPORTSECURITYINFOMAGIC. For better or worse, the first 4 bytes need to remain the same, I think :/

@@ +331,5 @@
>    if (NS_FAILED(rv)) {
>      return rv;
>    }
> +
> +  nsCOMPtr<nsISerializable> fscSer = do_QueryInterface(mFailedCertChain);

Can you not pass mFailedCertChain directly to NS_WriteOptionalCompoundObject? Anyway, "fscSer" isn't a very descriptive name (even though it's pretty clear it doesn't have much functional meaning). In any case, it should probably at least be "fccSer". Maybe call it "serializableFailedCertChain" (similar to in ::Read).

@@ +401,5 @@
> +  rv = NS_ReadOptionalObject(stream, true, getter_AddRefs(supportsFailedCertChain));
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +  // XXX reinterpret_cast like above?

Shouldn't need a reinterpret_cast - I think the above is because mSSLStatus is an nsSSLStatus, not an nsISSLStatus.

@@ +404,5 @@
> +  }
> +  // XXX reinterpret_cast like above?
> +  mFailedCertChain = do_QueryInterface(supportsFailedCertChain);
> +  // XXX assert that this worked? it's an optional object?
> +  NS_ASSERTION(mFailedCertChain, "need a failed cert chain to de-serialize");

Well, if it's an optional object, it could be null, right? In fact, is it safe to do_QueryInterface a nullptr? In any case we should make sure mFailedCertChain gets properly nulled if the chain isn't in the serialization.

@@ +1113,5 @@
> +  }
> +
> +  nsCOMPtr<nsIX509CertList> comCertList;
> +  // nsNSSCertList takes ownership of certList
> +  comCertList = new nsNSSCertList(certList, lock);

why not just mFailedCertChain = new NSSCertList(certList, lock); ?

::: security/manager/ssl/src/TransportSecurityInfo.h
@@ +6,5 @@
>  
>  #ifndef _MOZILLA_PSM_TRANSPORTSECURITYINFO_H
>  #define _MOZILLA_PSM_TRANSPORTSECURITYINFO_H
>  
> +#include "pkix/pkixtypes.h"

add this sorted in the list of #includes

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +1670,5 @@
>  
>  NS_IMETHODIMP
> +nsNSSCertList::Write(nsIObjectOutputStream* aStream)
> +{
> +  NS_ENSURE_STATE(mCertList);

check for NSS shutdown, too

@@ +1690,5 @@
> +  for (node = CERT_LIST_HEAD(mCertList);
> +       !CERT_LIST_END(node, mCertList);
> +       node = CERT_LIST_NEXT(node))
> +  {
> +    nsNSSCertificate* cert = nsNSSCertificate::Create(node->cert);

Pretty sure this leaks - use nsCOMPtr<nsIX509Cert>

@@ +1695,5 @@
> +    if (!cert) {
> +      rv = NS_ERROR_OUT_OF_MEMORY;
> +      break;
> +    }
> +    rv = cert->Write(aStream);

Shouldn't this be:

nsICOMPtr<nsISerializable> serializableCert(cert);
rv = aStream->WriteCompoundObject(serializableCert, NS_GET_IID(nsIX509Cert), true);

@@ +1709,5 @@
> +nsNSSCertList::Read(nsIObjectInputStream* aStream)
> +{
> +  // Is this correct? Won't mCertList already exist because of our
> +  // constructor?
> +  //NS_ENSURE_STATE(!mCertList);

I think NS_ENSURE_STATE(mCertList) would be correct to assert.
Also, check for NSS shutdown here.

@@ +1719,5 @@
> +    return rv;
> +  }
> +
> +  // Initialize the cert list - or is it already guaranteed to be initialized
> +  // by the constructor?

Yes, mCertList should already be initialized (to an empty list, I believe).

@@ +1722,5 @@
> +  // Initialize the cert list - or is it already guaranteed to be initialized
> +  // by the constructor?
> +  for(uint32_t i = 0; i < certListLen; ++i) {
> +    nsNSSCertificate* cert = new nsNSSCertificate();
> +    rv = cert->Read(aStream);

Shouldn't this be:

nsCOMPtr<nsISupports> certSupports;
aStream->ReadObject(true, getter_AddRefs(certSupports));

@@ +1726,5 @@
> +    rv = cert->Read(aStream);
> +    if (NS_FAILED(rv)) {
> +      break;
> +    }
> +    rv = this->AddCert(cert);

"this->" shouldn't be necessary

	Garrett Robinson [:grobinson] Assignee
	Comment 7 • 10 years ago

(In reply to David Keeler (:keeler) [use needinfo?] from comment #6)
> Comment on attachment 8454047 [details] [diff] [review]
> 1029155_expose_peer_certificate_chain.patch
> 
> Review of attachment 8454047 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is looking pretty good. I should probably take another look, though, so
> r- for now.
> I think the tests suggested in comment 4 are good. I would also check that a
> successful connection has a null failedCertChain.

Updated patch to address comments below. Tests coming next!

> @@ +404,5 @@
> > +  }
> > +  // XXX reinterpret_cast like above?
> > +  mFailedCertChain = do_QueryInterface(supportsFailedCertChain);
> > +  // XXX assert that this worked? it's an optional object?
> > +  NS_ASSERTION(mFailedCertChain, "need a failed cert chain to de-serialize");
> 
> Well, if it's an optional object, it could be null, right? In fact, is it
> safe to do_QueryInterface a nullptr? In any case we should make sure
> mFailedCertChain gets properly nulled if the chain isn't in the
> serialization.

It is safe - NS_ReadOptionalCompoundObject returns a nullptr if it wasn't there, and do_QueryInterface(nullptr) = nullptr so it works exactly as it should.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 8 • 10 years ago

Comment on attachment 8463705 [details] [diff] [review]
1029155_expose_peer_certificate_chain.patch

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

Awesome - this looks great. Just a few nits.

::: netwerk/socket/nsITransportSecurityInfo.idl
@@ +17,5 @@
> +     * If certificate verification failed, this will be the peer certificate
> +     * chain provided in the handshake, so it can be used for error reporting.
> +     * If verification succeeded, this will be null.
> +     */
> +    readonly attribute nsIX509CertList  failedCertChain;

nit: only one space before "failedCertChain"

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +829,5 @@
>  
> +  // Copy the certificate list so the runnable can take ownership of it in the
> +  // constructor.
> +  //
> +  // We need to give DupCertList a lock (even though it doesn't check it). We

nit: technically the check is if NSS has shut down, not any property on the lock. The lock just prevents NSS shutting down while we're doing something that requires it.

::: security/manager/ssl/src/TransportSecurityInfo.h
@@ +17,5 @@
>  #include "nsIAssociatedContentSecurity.h"
>  #include "nsNSSShutDown.h"
>  #include "nsDataHashtable.h"
> +#include "pkix/pkixtypes.h"
> +#include "ScopedNSSTypes.h"

technically, these should all be sorted, but it's not a big deal

	Garrett Robinson [:grobinson] Assignee
	Comment 9 • 10 years ago

Fixed nits. Carrying over r+ from keeler. Next up: tests.

	Garrett Robinson [:grobinson] Assignee
	Comment 10 • 10 years ago

Un-bitrotted.

	Garrett Robinson [:grobinson] Assignee
	Comment 11 • 10 years ago

Tests for nsIX509CertList serialization and storing the peer certificate chain for failed connections on TransportSecurityInfo.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 12 • 10 years ago

Comment on attachment 8468095 [details] [diff] [review]
1029155_tests.patch

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

Looks good, but it needs a slight bit of cleanup, and I think it is important to check that the failedCertChain is what we're expecting. You should just be able to export_cert the relevant ones in generate_certs.sh and then read them in the test. r- for now.

::: security/manager/ssl/public/nsIX509CertList.idl
@@ +29,5 @@
>  };
>  
>  %{C++
>  
> +#define NS_X509CERTLIST_CID { /* a539759b-e22d-462f-94ea-2915b11b33e8 */ \

I'm not actually sure this needs to change, but the interface uuid should certainly get an update.

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +1545,5 @@
>  }
>  
>  } // namespace mozilla
>  
> +NS_IMPL_CLASSINFO(nsNSSCertList,

What's this for?

@@ +1550,5 @@
> +                  nullptr,
> +                  // inferred from nsIX509Cert
> +                  nsIClassInfo::THREADSAFE,
> +                  NS_X509CERTLIST_CID)
> +                  

nit: trailing whitespace

@@ +1769,5 @@
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +nsNSSCertList::Equals(nsIX509CertList* other, bool* result)

initialize result to false so we don't have an uninitialized output

@@ +1776,5 @@
> +  if (isAlreadyShutDown()) {
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +
> +  nsresult rv = NS_OK;

don't initialize rv so we know if it gets used uninitialized

@@ +1812,5 @@
> +        equal = false;
> +        break;
> +      }
> +
> +      // Note: keeler says nsIX509Cert.equal might be bogus. In which case, he

Well, if nsIX509Cert.equal doesn't work, let's fix it and leave this as is

::: security/manager/ssl/tests/unit/test_error_reporting.js
@@ +1,1 @@
> +// -*- Mode: javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-

Since this doesn't test error reporting end-to-end, maybe call this "test_failedCertChain.js" or something? (Or maybe "test_cert_chains.js"?)

@@ +12,5 @@
> +    return constructCertFromFile("tlsserver/" + certName + ".der");
> +  });
> +  let certList = Cc["@mozilla.org/security/x509certlist;1"]
> +                   .createInstance(Ci.nsIX509CertList);
> +  certs.forEach(function(cert) {

why not just:

certNames.forEach(function(certName) {
  let cert = constructCertFromFile(...);
  certList.addCert(cert);
}

(it probably only saves 2 lines, but it seems more straightforward to me)

@@ +23,5 @@
> +  certList.QueryInterface(Ci.nsISerializable);
> +  let serialized = serHelper.serializeToString(certList);
> +
> +  // Deserialize from the string and compare to the original object
> +  let deserialized= serHelper.deserializeObject(serialized);

nit: "deserialized ="

@@ +25,5 @@
> +
> +  // Deserialize from the string and compare to the original object
> +  let deserialized= serHelper.deserializeObject(serialized);
> +  deserialized.QueryInterface(Ci.nsIX509CertList);
> +  certList.QueryInterface(Ci.nsIX509CertList);

this second QI isn't necessary

@@ +27,5 @@
> +  let deserialized= serHelper.deserializeObject(serialized);
> +  deserialized.QueryInterface(Ci.nsIX509CertList);
> +  certList.QueryInterface(Ci.nsIX509CertList);
> +  do_check_true(certList.equals(deserialized));
> +

nit: unnecessary blank line

@@ +45,5 @@
> +  add_connection_test(
> +    // re-use pinning certs (keeler)
> +    "good.include-subdomains.pinning.example.com", Cr.NS_OK, null,
> +    function withSecurityInfo(aTransportSecurityInfo) {
> +      dump("in connection_test callback");

I'm not a fan of leaving extra debug output in tests

@@ +60,5 @@
> +    function withSecurityInfo(securityInfo) {
> +      securityInfo.QueryInterface(Ci.nsITransportSecurityInfo);
> +      do_check_neq(securityInfo.failedCertChain, null);
> +      // TODO check the peer certificate chain is what we served
> +      // Is this necessary?

I think it would be a good idea, yes.

@@ +64,5 @@
> +      // Is this necessary?
> +    }
> +  );
> +
> +  // Test non-overrideable error (peerCertChain should be non-null)

"failedCertChain", right?

@@ +65,5 @@
> +    }
> +  );
> +
> +  // Test non-overrideable error (peerCertChain should be non-null)
> +  // List of overrideable errors: https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/NSSErrorsService.cpp#137

I'm not sure this comment is necessary. Also, the location of the list is liable to change.

@@ +74,5 @@
> +    function withSecurityInfo(securityInfo) {
> +      securityInfo.QueryInterface(Ci.nsITransportSecurityInfo);
> +      do_check_neq(securityInfo.failedCertChain, null);
> +      // TODO check the peer certificate chain is what we served
> +      // ... likewise?

Same here, yes.

::: security/manager/ssl/tests/unit/tlsserver/generate_certs.sh
@@ +83,5 @@
>                                                     -s "$SUBJECT" \
>                                                     -t "CT,," \
>                                                     -x $COMMON_ARGS
>    $RUN_MOZILLA $CERTUTIL -d $DB_ARGUMENT -L -n $NICKNAME -r > $OUTPUT_DIR/$DERFILE
> +  #export_cert $NICKNAME $DERFILE

unnecessary line

@@ +154,5 @@
>  # Make an EE cert issued by otherCA
>  make_EE otherIssuerEE 'CN=Wrong CA Pin Test End-Entity' otherCA "*.include-subdomains.pinning.example.com,*.exclude-subdomains.pinning.example.com,*.pinning.example.com"
>  
>  $RUN_MOZILLA $CERTUTIL -d $DB_ARGUMENT -L -n localhostAndExampleCom -r > $OUTPUT_DIR/default-ee.der
> +$RUN_MOZILLA $CERTUTIL -d $DB_ARGUMENT -L -n otherIssuerEE -r > $OUTPUT_DIR/other-ee.der

These can be replaced with calls to export_cert

@@ +195,5 @@
>  make_INT self-signed-EE-with-cA-true 'CN=Test Self-signed End-entity with CA true' unused "-x -8 self-signed-end-entity-with-cA-true.example.com"
>  
>  make_delegated badKeysizeDelegatedSigner 'CN=Bad Keysize Delegated Responder' testCA "--extKeyUsage ocspResponder -g 1008"
>  
> +# export selected keys

unnecessary lines?

::: security/manager/ssl/tests/unit/xpcshell.ini
@@ +93,5 @@
>  # Bug 1009158: this test times out on Android
>  skip-if = os == "android"
>  [test_add_preexisting_cert.js]
>  [test_keysize.js]
> +[test_error_reporting.js]

Since this test uses a tlsserver, these lines are necessary:

run-sequentially = hardcoded ports
# Bug 1009158: this test times out on Android
skip-if = os == "android"

	Garrett Robinson [:grobinson] Assignee
	Comment 13 • 10 years ago

(In reply to David Keeler (:keeler) [use needinfo?] from comment #12)
> Comment on attachment 8468095 [details] [diff] [review]
> 1029155_tests.patch
> 
> Review of attachment 8468095 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, but it needs a slight bit of cleanup, and I think it is
> important to check that the failedCertChain is what we're expecting. You
> should just be able to export_cert the relevant ones in generate_certs.sh
> and then read them in the test. r- for now.

Fixed nits, now I just need to check the failed cert chain in the test. Will have an updated patch soon. Some questions below.

> ::: security/manager/ssl/public/nsIX509CertList.idl
> @@ +29,5 @@
> >  };
> >  
> >  %{C++
> >  
> > +#define NS_X509CERTLIST_CID { /* a539759b-e22d-462f-94ea-2915b11b33e8 */ \
> 
> I'm not actually sure this needs to change, but the interface uuid should
> certainly get an update.

The interface uuid is updated in the first patch. I thought this was supposed to match that (since the CID is also a UUID), but I guess it does not. Will revert.

> ::: security/manager/ssl/src/nsNSSCertificate.cpp
> @@ +1545,5 @@
> >  }
> >  
> >  } // namespace mozilla
> >  
> > +NS_IMPL_CLASSINFO(nsNSSCertList,
> 
> What's this for?

Any class that implements nsISerializable must also implement nsIClassInfo (see [0]). This macro allows me to support the required functions with a minimum of boilerplate.

[0] http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsISerializable.idl#28

Perhaps these changes rightfully belong in the previous patch, which implements nsNSSCertList::Read and ::Write?

> @@ +1550,5 @@
> > +                  nullptr,
> > +                  // inferred from nsIX509Cert
> > +                  nsIClassInfo::THREADSAFE,
> > +                  NS_X509CERTLIST_CID)
> > +                  
> 
> nit: trailing whitespace

Fixed.

> @@ +1769,5 @@
> >    return NS_OK;
> >  }
> >  
> > +NS_IMETHODIMP
> > +nsNSSCertList::Equals(nsIX509CertList* other, bool* result)
> 
> initialize result to false so we don't have an uninitialized output

Fixed (although I initialize result to true instead of false because of how the rest of the code is written. I also got rid of the unnecessary "equals" variable).

> @@ +1776,5 @@
> > +  if (isAlreadyShutDown()) {
> > +    return NS_ERROR_NOT_AVAILABLE;
> > +  }
> > +
> > +  nsresult rv = NS_OK;
> 
> don't initialize rv so we know if it gets used uninitialized

Fixed.

> @@ +1812,5 @@
> > +        equal = false;
> > +        break;
> > +      }
> > +
> > +      // Note: keeler says nsIX509Cert.equal might be bogus. In which case, he
> 
> Well, if nsIX509Cert.equal doesn't work, let's fix it and leave this as is

It appears to work since the tests pass. I added another test case specifically for nsIX509Cert.equal.

> ::: security/manager/ssl/tests/unit/test_error_reporting.js
> @@ +1,1 @@
> > +// -*- Mode: javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> 
> Since this doesn't test error reporting end-to-end, maybe call this
> "test_failedCertChain.js" or something? (Or maybe "test_cert_chains.js"?)

Will do.

> @@ +12,5 @@
> > +    return constructCertFromFile("tlsserver/" + certName + ".der");
> > +  });
> > +  let certList = Cc["@mozilla.org/security/x509certlist;1"]
> > +                   .createInstance(Ci.nsIX509CertList);
> > +  certs.forEach(function(cert) {
> 
> why not just:
> 
> certNames.forEach(function(certName) {
>   let cert = constructCertFromFile(...);
>   certList.addCert(cert);
> }
> 
> (it probably only saves 2 lines, but it seems more straightforward to me)

Sure.

> @@ +23,5 @@
> > +  certList.QueryInterface(Ci.nsISerializable);
> > +  let serialized = serHelper.serializeToString(certList);
> > +
> > +  // Deserialize from the string and compare to the original object
> > +  let deserialized= serHelper.deserializeObject(serialized);
> 
> nit: "deserialized ="

Fixed.

> @@ +25,5 @@
> > +
> > +  // Deserialize from the string and compare to the original object
> > +  let deserialized= serHelper.deserializeObject(serialized);
> > +  deserialized.QueryInterface(Ci.nsIX509CertList);
> > +  certList.QueryInterface(Ci.nsIX509CertList);
> 
> this second QI isn't necessary

Fixed.

> @@ +27,5 @@
> > +  let deserialized= serHelper.deserializeObject(serialized);
> > +  deserialized.QueryInterface(Ci.nsIX509CertList);
> > +  certList.QueryInterface(Ci.nsIX509CertList);
> > +  do_check_true(certList.equals(deserialized));
> > +
> 
> nit: unnecessary blank line

Fixed.

> @@ +45,5 @@
> > +  add_connection_test(
> > +    // re-use pinning certs (keeler)
> > +    "good.include-subdomains.pinning.example.com", Cr.NS_OK, null,
> > +    function withSecurityInfo(aTransportSecurityInfo) {
> > +      dump("in connection_test callback");
> 
> I'm not a fan of leaving extra debug output in tests

:) Fixed.

> @@ +60,5 @@
> > +    function withSecurityInfo(securityInfo) {
> > +      securityInfo.QueryInterface(Ci.nsITransportSecurityInfo);
> > +      do_check_neq(securityInfo.failedCertChain, null);
> > +      // TODO check the peer certificate chain is what we served
> > +      // Is this necessary?
> 
> I think it would be a good idea, yes.
> 
> @@ +64,5 @@
> > +      // Is this necessary?
> > +    }
> > +  );
> > +
> > +  // Test non-overrideable error (peerCertChain should be non-null)
> 
> "failedCertChain", right?

Yes, fixed.

> @@ +65,5 @@
> > +    }
> > +  );
> > +
> > +  // Test non-overrideable error (peerCertChain should be non-null)
> > +  // List of overrideable errors: https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/NSSErrorsService.cpp#137
> 
> I'm not sure this comment is necessary. Also, the location of the list is
> liable to change.

On the other hand, otherwise it's not obvious how I determined the distinction (it's not well documented). I'm in favor of leaving in, at least it provides a clue to later readers. What do you think?

> @@ +74,5 @@
> > +    function withSecurityInfo(securityInfo) {
> > +      securityInfo.QueryInterface(Ci.nsITransportSecurityInfo);
> > +      do_check_neq(securityInfo.failedCertChain, null);
> > +      // TODO check the peer certificate chain is what we served
> > +      // ... likewise?
> 
> Same here, yes.
> 
> ::: security/manager/ssl/tests/unit/tlsserver/generate_certs.sh
> @@ +83,5 @@
> >                                                     -s "$SUBJECT" \
> >                                                     -t "CT,," \
> >                                                     -x $COMMON_ARGS
> >    $RUN_MOZILLA $CERTUTIL -d $DB_ARGUMENT -L -n $NICKNAME -r > $OUTPUT_DIR/$DERFILE
> > +  #export_cert $NICKNAME $DERFILE
> 
> unnecessary line

I added export_cert, but then was decided not to use it because I wanted to make minimal changes to the file. I will go ahead and use it, here and elsewhere in the file.

> @@ +154,5 @@
> >  # Make an EE cert issued by otherCA
> >  make_EE otherIssuerEE 'CN=Wrong CA Pin Test End-Entity' otherCA "*.include-subdomains.pinning.example.com,*.exclude-subdomains.pinning.example.com,*.pinning.example.com"
> >  
> >  $RUN_MOZILLA $CERTUTIL -d $DB_ARGUMENT -L -n localhostAndExampleCom -r > $OUTPUT_DIR/default-ee.der
> > +$RUN_MOZILLA $CERTUTIL -d $DB_ARGUMENT -L -n otherIssuerEE -r > $OUTPUT_DIR/other-ee.der
> 
> These can be replaced with calls to export_cert

Ok

> @@ +195,5 @@
> >  make_INT self-signed-EE-with-cA-true 'CN=Test Self-signed End-entity with CA true' unused "-x -8 self-signed-end-entity-with-cA-true.example.com"
> >  
> >  make_delegated badKeysizeDelegatedSigner 'CN=Bad Keysize Delegated Responder' testCA "--extKeyUsage ocspResponder -g 1008"
> >  
> > +# export selected keys
> 
> unnecessary lines?
> 
> ::: security/manager/ssl/tests/unit/xpcshell.ini
> @@ +93,5 @@
> >  # Bug 1009158: this test times out on Android
> >  skip-if = os == "android"
> >  [test_add_preexisting_cert.js]
> >  [test_keysize.js]
> > +[test_error_reporting.js]
> 
> Since this test uses a tlsserver, these lines are necessary:
> 
> run-sequentially = hardcoded ports
> # Bug 1009158: this test times out on Android
> skip-if = os == "android"

Fixed.

	Garrett Robinson [:grobinson] Assignee
	Comment 14 • 10 years ago

Updated with comments from review. Additionally tests that the failed cert chains stored on TransportSecurityInfo are the cert chains that were served in the handshake.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 15 • 10 years ago

Comment on attachment 8471979 [details] [diff] [review]
1029155_tests.patch

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

Great - r=me with comments addressed.

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +1810,5 @@
> +        *result = false;
> +        break;
> +      }
> +
> +      // Note: keeler says nsIX509Cert.equal might be bogus. In which case, he

I would just take this comment out :)

::: security/manager/ssl/tests/unit/test_cert_chains.js
@@ +22,5 @@
> +    let cert = constructCertFromFile("tlsserver/" + certName + ".der");
> +    certs.push(cert);
> +  });
> +
> +  do_check_true(certs[0].equals(certs[0]))

Hmmm - I'm not sure this tests what we want. I was thinking more like this:

let certA = constructCertFromFile("tlsserver/default-ee.der");
let certB = constructCertFromFile("tlsserver/default-ee.der");
do_check_false(certA == certB);
do_check_true(certA.equals(certB));

(if I understand correctly, this would decode and create two certificates that should be equal but are backed by two different xpcom objects)

@@ +27,5 @@
> +  do_check_false(certs[0].equals(certs[1]))
> +}
> +
> +function test_cert_list_serialization() {
> +  let certList = build_cert_chain(['default-ee', 'other-ee']);

If you use expired-ee.der or inadequatekeyusage-ee.der here, we won't have to export other-ee.der.

::: security/manager/ssl/tests/unit/tlsserver/generate_certs.sh
@@ +153,5 @@
>  # Make an EE cert issued by otherCA
>  make_EE otherIssuerEE 'CN=Wrong CA Pin Test End-Entity' otherCA "*.include-subdomains.pinning.example.com,*.exclude-subdomains.pinning.example.com,*.pinning.example.com"
>  
> +export_cert localhostAndExampleCom default-ee.der
> +export_cert otherIssuerEE other-ee.der

See comment in test_cert_chains.js regarding exporting other-ee.der.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 16 • 10 years ago

Hmmm - so it looks like nsITransportSecurityInfo has its uuid updated in the first patch, but nsIX509CertList doesn't - that should definitely be taken care of before this lands :)

	Garrett Robinson [:grobinson] Assignee
	Comment 17 • 10 years ago

Changes from last review and subsequent comment.

	Garrett Robinson [:grobinson] Assignee
	Comment 18 • 10 years ago

Try: https://tbpl.mozilla.org/?tree=Try&rev=2a5a6634b308 (trees were closed all day yesterday)

	Garrett Robinson [:grobinson] Assignee
	Comment 19 • 10 years ago

Adds an entry for SSL_PeerCertificateChain to config/external/nss/nss.def to fix build errors on some platforms (see last try run).

	Garrett Robinson [:grobinson] Assignee
	Comment 20 • 10 years ago

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

	Garrett Robinson [:grobinson] Assignee
	Comment 21 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a00db3bafbd
https://hg.mozilla.org/integration/mozilla-inbound/rev/229181c88a3c

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/8a00db3bafbd
https://hg.mozilla.org/mozilla-central/rev/229181c88a3c

	Kevin Grandon :kgrandon
	Comment 23 • 10 years ago

David - Since this is causing several bad regressions for features in our aurora branch, and as far as I can tell is not being used yet, can we back this out from central/aurora?

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 24 • 10 years ago

(In reply to Kevin Grandon :kgrandon from comment #23)
> David - Since this is causing several bad regressions for features in our
> aurora branch, and as far as I can tell is not being used yet, can we back
> this out from central/aurora?

That shouldn't be necessary. See bug 1055238 comment 14 and onwards.