Return the certificate chain in ClientCertStoreNSS.

Return the certificate chain in ClientCertStoreNSS.

NSS used to build a chain internally in the SSL stack which got lost
when switching to BoringSSL. Align with other platforms by building the
chain externally in ClientCertStoreNSS.

Although this is inherently somewhat flaky, some servers do not have
intermediates configured locally and expect the client to supply them.

This modifies (really completely rewrites) our bundled
NSS_CmpCertChainWCANames to return the chain it found. That is returned
out of ClientCertStoreNSS.

Note that this is not completely the same as the old behavior. Rather
than building as much of a path as we can manage from the leaf, we will
stop at the issuer list supplied by the server. It is assumed that the
server accepts the issuers it claims to accept. We also only do
name-based matching (which we were doing anyway) to avoid adding a more
expensive global operation in the candidate matching path.

In doing so, this syncs NSS with other platforms in removing the ancient
workaround for Netscape Enterprise Server 2.0, released in 1996.

Tested with unit tests and also manually against a custom Go server.

BUG=548631
Committed: https://crrev.com/8d569f5901989954c0b39a83f16ebd36375e98b8
Cr-Commit-Position: refs/heads/master@{#408647}

[davidben, 2016-07-28 21:29:13]

The CQ bit was checked by davidben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 21:29:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[davidben, 2016-07-28 21:30:55]

The CQ bit was checked by davidben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 21:31:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[davidben, 2016-07-28 21:31:54]

davidben@chromium.org changed reviewers:
+ rsleevi@chromium.org

[davidben, 2016-07-28 21:31:54]

I got tired of all the people complaining about intermediates. Here's a new
version that now modifies cmpcert.c as discussed. The test I just stole from the
last attempt, but now it actually works.

[commit-bot: I haz the power, 2016-07-28 21:36:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-28 21:36:36]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[davidben, 2016-07-28 22:00:41]

The CQ bit was checked by davidben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 22:01:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 23:03:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-28 23:03:23]

Dry run: This issue passed the CQ dry run.

[Ryan Sleevi, 2016-07-29 00:37:45]

LGTM % one set of nits that I'm sure will drive you batty

https://codereview.chromium.org/2185403003/diff/40001/net/ssl/client_cert_sto...
File net/ssl/client_cert_store_nss.cc (right):

https://codereview.chromium.org/2185403003/diff/40001/net/ssl/client_cert_sto...
net/ssl/client_cert_store_nss.cc:105: }
Why do you indirect through the DER, when we've got OSCertHandles? We're already
guaranteed below (c.f. line 140)

X509Certificate::OSCertHandles certs;
std::transform(chain.begin(), chain.end(), std::back_inserter(certs), [](const
ScopedCERTCertificate& cert) { return cert.get(); });
if (certs.empty())
  continue;
X509Certificate::OSCertHandle handle = certs[0];
certs.erase(certs.begin());

scoped_refptr<X509Certificate> cert = X509Certificate::CreateFromHandle(handle,
certs);


(or plenty of other ways to write it)

It just seems silly to go from handle->der->handle, especially since going from
der->handle invokes a full smartcard search to see if it has an existing
CERTCertificate.

https://codereview.chromium.org/2185403003/diff/40001/net/third_party/nss/ssl...
File net/third_party/nss/ssl/cmpcert.cc (right):

https://codereview.chromium.org/2185403003/diff/40001/net/third_party/nss/ssl...
net/third_party/nss/ssl/cmpcert.cc:44: // TODO(davidben): Can this be removed?
Yup. No other platforms have this.

[davidben, 2016-07-29 15:13:07]

The CQ bit was checked by davidben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 15:13:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[davidben, 2016-07-29 15:14:11]

https://codereview.chromium.org/2185403003/diff/40001/net/ssl/client_cert_sto...
File net/ssl/client_cert_store_nss.cc (right):

https://codereview.chromium.org/2185403003/diff/40001/net/ssl/client_cert_sto...
net/ssl/client_cert_store_nss.cc:105: }
On 2016/07/29 00:37:44, Ryan Sleevi (slow) wrote:
> Why do you indirect through the DER, when we've got OSCertHandles? We're
already
> guaranteed below (c.f. line 140)
> 
> X509Certificate::OSCertHandles certs;
> std::transform(chain.begin(), chain.end(), std::back_inserter(certs), [](const
> ScopedCERTCertificate& cert) { return cert.get(); });
> if (certs.empty())
>   continue;
> X509Certificate::OSCertHandle handle = certs[0];
> certs.erase(certs.begin());
> 
> scoped_refptr<X509Certificate> cert =
X509Certificate::CreateFromHandle(handle,
> certs);
> 
> 
> (or plenty of other ways to write it)
> 
> It just seems silly to go from handle->der->handle, especially since going
from
> der->handle invokes a full smartcard search to see if it has an existing
> CERTCertificate.

You know, the for loop is actually much much less typing than the std::transform
version. :-P

Done. In doing so, I made the cmpcert.cc helper spit out a list of intermediates
rather than the whole chain to match what X509Certificate wants.

https://codereview.chromium.org/2185403003/diff/40001/net/third_party/nss/ssl...
File net/third_party/nss/ssl/cmpcert.cc (right):

https://codereview.chromium.org/2185403003/diff/40001/net/third_party/nss/ssl...
net/third_party/nss/ssl/cmpcert.cc:44: // TODO(davidben): Can this be removed?
On 2016/07/29 00:37:44, Ryan Sleevi (slow) wrote:
> Yup. No other platforms have this.

Removed.

At this point this function can only passingly be considered to be derived from
the NSS function, but I suppose I did start from that file. :-)

[davidben, 2016-07-29 15:18:20]

Description was changed from

==========
Return the certificate chain in ClientCertStoreNSS.

NSS used to build a chain internally in the SSL stack which got lost
when switching to BoringSSL. Align with other platforms by building the
chain externally in ClientCertStoreNSS.

Although this is inherently somewhat flaky, some servers do not have
intermediates configured locally and expect the client to supply them.

This modifies (really completely rewrites) our bundled
NSS_CmpCertChainWCANames to return the chain it found. That is returned
out of ClientCertStoreNSS.

BUG=548631
==========

to

==========
Return the certificate chain in ClientCertStoreNSS.

NSS used to build a chain internally in the SSL stack which got lost
when switching to BoringSSL. Align with other platforms by building the
chain externally in ClientCertStoreNSS.

Although this is inherently somewhat flaky, some servers do not have
intermediates configured locally and expect the client to supply them.

This modifies (really completely rewrites) our bundled
NSS_CmpCertChainWCANames to return the chain it found. That is returned
out of ClientCertStoreNSS.

Note that this is not completely the same as the old behavior. Rather
than building as much of a path as we can manage from the leaf, we will
stop at the issuer list supplied by the server. It is assumed that the
server accepts the issuers it claims to accept. We also only do
name-based matching (which we were doing anyway) to avoid adding a more
expensive global operation in the candidate matching path.

In doing so, this syncs NSS with other platforms in removing the ancient
workaround for Netscape Enterprise Server 2.0, released in 1996.

BUG=548631
==========

[davidben, 2016-07-29 15:18:41]

Description was changed from

==========
Return the certificate chain in ClientCertStoreNSS.

NSS used to build a chain internally in the SSL stack which got lost
when switching to BoringSSL. Align with other platforms by building the
chain externally in ClientCertStoreNSS.

Although this is inherently somewhat flaky, some servers do not have
intermediates configured locally and expect the client to supply them.

This modifies (really completely rewrites) our bundled
NSS_CmpCertChainWCANames to return the chain it found. That is returned
out of ClientCertStoreNSS.

Note that this is not completely the same as the old behavior. Rather
than building as much of a path as we can manage from the leaf, we will
stop at the issuer list supplied by the server. It is assumed that the
server accepts the issuers it claims to accept. We also only do
name-based matching (which we were doing anyway) to avoid adding a more
expensive global operation in the candidate matching path.

In doing so, this syncs NSS with other platforms in removing the ancient
workaround for Netscape Enterprise Server 2.0, released in 1996.

BUG=548631
==========

to

==========
Return the certificate chain in ClientCertStoreNSS.

NSS used to build a chain internally in the SSL stack which got lost
when switching to BoringSSL. Align with other platforms by building the
chain externally in ClientCertStoreNSS.

Although this is inherently somewhat flaky, some servers do not have
intermediates configured locally and expect the client to supply them.

This modifies (really completely rewrites) our bundled
NSS_CmpCertChainWCANames to return the chain it found. That is returned
out of ClientCertStoreNSS.

Note that this is not completely the same as the old behavior. Rather
than building as much of a path as we can manage from the leaf, we will
stop at the issuer list supplied by the server. It is assumed that the
server accepts the issuers it claims to accept. We also only do
name-based matching (which we were doing anyway) to avoid adding a more
expensive global operation in the candidate matching path.

In doing so, this syncs NSS with other platforms in removing the ancient
workaround for Netscape Enterprise Server 2.0, released in 1996.

Tested with unit tests and also manually against a custom Go server.

BUG=548631
==========

[davidben, 2016-07-29 15:18:50]

The CQ bit was unchecked by davidben@chromium.org

[davidben, 2016-07-29 15:18:55]

The CQ bit was checked by davidben@chromium.org

[davidben, 2016-07-29 15:18:55]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/2185403003/#ps60001
(title: "rsleevi comments")

[commit-bot: I haz the power, 2016-07-29 15:19:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 16:03:37]

Description was changed from

==========
Return the certificate chain in ClientCertStoreNSS.

NSS used to build a chain internally in the SSL stack which got lost
when switching to BoringSSL. Align with other platforms by building the
chain externally in ClientCertStoreNSS.

Although this is inherently somewhat flaky, some servers do not have
intermediates configured locally and expect the client to supply them.

This modifies (really completely rewrites) our bundled
NSS_CmpCertChainWCANames to return the chain it found. That is returned
out of ClientCertStoreNSS.

Note that this is not completely the same as the old behavior. Rather
than building as much of a path as we can manage from the leaf, we will
stop at the issuer list supplied by the server. It is assumed that the
server accepts the issuers it claims to accept. We also only do
name-based matching (which we were doing anyway) to avoid adding a more
expensive global operation in the candidate matching path.

In doing so, this syncs NSS with other platforms in removing the ancient
workaround for Netscape Enterprise Server 2.0, released in 1996.

Tested with unit tests and also manually against a custom Go server.

BUG=548631
==========

to

==========
Return the certificate chain in ClientCertStoreNSS.

NSS used to build a chain internally in the SSL stack which got lost
when switching to BoringSSL. Align with other platforms by building the
chain externally in ClientCertStoreNSS.

Although this is inherently somewhat flaky, some servers do not have
intermediates configured locally and expect the client to supply them.

This modifies (really completely rewrites) our bundled
NSS_CmpCertChainWCANames to return the chain it found. That is returned
out of ClientCertStoreNSS.

Note that this is not completely the same as the old behavior. Rather
than building as much of a path as we can manage from the leaf, we will
stop at the issuer list supplied by the server. It is assumed that the
server accepts the issuers it claims to accept. We also only do
name-based matching (which we were doing anyway) to avoid adding a more
expensive global operation in the candidate matching path.

In doing so, this syncs NSS with other platforms in removing the ancient
workaround for Netscape Enterprise Server 2.0, released in 1996.

Tested with unit tests and also manually against a custom Go server.

BUG=548631
==========

[commit-bot: I haz the power, 2016-07-29 16:03:37]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-29 16:06:48]

Description was changed from

==========
Return the certificate chain in ClientCertStoreNSS.

NSS used to build a chain internally in the SSL stack which got lost
when switching to BoringSSL. Align with other platforms by building the
chain externally in ClientCertStoreNSS.

Although this is inherently somewhat flaky, some servers do not have
intermediates configured locally and expect the client to supply them.

This modifies (really completely rewrites) our bundled
NSS_CmpCertChainWCANames to return the chain it found. That is returned
out of ClientCertStoreNSS.

Note that this is not completely the same as the old behavior. Rather
than building as much of a path as we can manage from the leaf, we will
stop at the issuer list supplied by the server. It is assumed that the
server accepts the issuers it claims to accept. We also only do
name-based matching (which we were doing anyway) to avoid adding a more
expensive global operation in the candidate matching path.

In doing so, this syncs NSS with other platforms in removing the ancient
workaround for Netscape Enterprise Server 2.0, released in 1996.

Tested with unit tests and also manually against a custom Go server.

BUG=548631
==========

to

==========
Return the certificate chain in ClientCertStoreNSS.

NSS used to build a chain internally in the SSL stack which got lost
when switching to BoringSSL. Align with other platforms by building the
chain externally in ClientCertStoreNSS.

Although this is inherently somewhat flaky, some servers do not have
intermediates configured locally and expect the client to supply them.

This modifies (really completely rewrites) our bundled
NSS_CmpCertChainWCANames to return the chain it found. That is returned
out of ClientCertStoreNSS.

Note that this is not completely the same as the old behavior. Rather
than building as much of a path as we can manage from the leaf, we will
stop at the issuer list supplied by the server. It is assumed that the
server accepts the issuers it claims to accept. We also only do
name-based matching (which we were doing anyway) to avoid adding a more
expensive global operation in the candidate matching path.

In doing so, this syncs NSS with other platforms in removing the ancient
workaround for Netscape Enterprise Server 2.0, released in 1996.

Tested with unit tests and also manually against a custom Go server.

BUG=548631

Committed: https://crrev.com/8d569f5901989954c0b39a83f16ebd36375e98b8
Cr-Commit-Position: refs/heads/master@{#408647}
==========

[commit-bot: I haz the power, 2016-07-29 16:06:49]

Patchset 4 (id:??) landed as
https://crrev.com/8d569f5901989954c0b39a83f16ebd36375e98b8
Cr-Commit-Position: refs/heads/master@{#408647}

[davidben, 2016-07-29 16:38:45]

https://codereview.chromium.org/2185403003/diff/60001/net/third_party/nss/ssl...
File net/third_party/nss/ssl/cmpcert.cc (right):

https://codereview.chromium.org/2185403003/diff/60001/net/third_party/nss/ssl...
net/third_party/nss/ssl/cmpcert.cc:13: #include "base/logging.h"
Oops. Left this in here while debugging. I'll upload a quick follow-up to remove
it.