Build a chain in ClientCertStoreNSS to send intermediates to the server.

Build a chain in ClientCertStoreNSS to send intermediates to the server.

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.

For comparison, see:
https://code.google.com/p/chromium/codesearch#chromium/src/net/third_party/nss/ssl/ssl3con.c&l=7412

BUG=548631

[davidben, 2015-12-14 22:10:34]

Description was changed from

==========
Build a chain in ClientCertStoreNSS to send intermediates to the server.

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.

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

to

==========
Build a chain in ClientCertStoreNSS to send intermediates to the server.

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.

For comparison, see:
https://code.google.com/p/chromium/codesearch#chromium/src/net/third_party/ns...

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

[davidben, 2015-12-14 22:26:32]

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

[davidben, 2015-12-14 22:26:33]

https://codereview.chromium.org/1526783002/diff/1/net/ssl/client_cert_store_n...
File net/ssl/client_cert_store_nss.cc (right):

https://codereview.chromium.org/1526783002/diff/1/net/ssl/client_cert_store_n...
net/ssl/client_cert_store_nss.cc:111: if (!query_nssdb) {
This is only false in tests. Which... you could argue this means we're barely
testing anything and I would agree. Didn't see a clear way to avoid that, so I
left things as they were here.

[Ryan Sleevi, 2015-12-16 01:46:54]

Where my tests at? ;)

https://codereview.chromium.org/1526783002/diff/1/net/ssl/client_cert_store_n...
File net/ssl/client_cert_store_nss.cc (right):

https://codereview.chromium.org/1526783002/diff/1/net/ssl/client_cert_store_n...
net/ssl/client_cert_store_nss.cc:111: if (!query_nssdb) {
On 2015/12/14 22:26:32, davidben wrote:
> This is only false in tests. Which... you could argue this means we're barely
> testing anything and I would agree. Didn't see a clear way to avoid that, so I
> left things as they were here.

Wait, it's false for all tests? There shouldn't be any reason we can't enable
this for tests at this point.

https://codereview.chromium.org/1526783002/diff/1/net/ssl/client_cert_store_n...
net/ssl/client_cert_store_nss.cc:118: // out of the local store.
https://crbug.com/548631
// Attempt to construct a complete certificate chain from |handle|,
// as some servers expect the client to send intermediates.
//
// Consider a chain of EE -> A -> B, where -> indicates "issued by".
// The server may indicate that "B" is an acceptable certificate
// authority, and expect the client to send both EE and A, so that it
// can construct the path back to B. See https://crbug.com/548631 for
// an example.
//
// Note that there is an edge case, in the event of cross-certificates;
// namely, consider the following chains:
// EE -> A -> B
// EE -> A' -> C
// Where A and A' have the same subject and public key, but different
// issuers. Unfortunately, NSS does not offer an API to indicate which
// trust anchor(s) are preferred in chain building, thus it's possible
// for the client to send EE and A, when the server needs EE and A'.
//
// Ideally, servers would be properly configured such that they can
// always handle clients sending only EE, and that should be the first
// response if any other edge cases emerge. However, this best-effort
// case is enough to resolve some incompatabilities.

https://codereview.chromium.org/1526783002/diff/1/net/ssl/client_cert_store_n...
net/ssl/client_cert_store_nss.cc:124: continue;
Wow, NSS is dumb; it shouldn't choke if it can't build a chain, it should just
send the leaf.

[davidben, 2015-12-16 04:15:42]

On 2015/12/16 01:46:54, Ryan Sleevi wrote:
> Where my tests at? ;)

Not sure how to build one since it's sensitive to what's in the trust store. (I
imagine this is why all of the tests were completely neutered to the point of
not actually testing the production code.) I'm not super-familiar with NSS. How
would I go about convincing it that intermediates, etc., exist? Do I just
(ab)use the in-memory certificate store and just have the objects hanging around
in memory somewhere?

[Ryan Sleevi, 2015-12-16 19:08:20]

On 2015/12/16 04:15:42, davidben wrote:
> On 2015/12/16 01:46:54, Ryan Sleevi wrote:
> > Where my tests at? ;)
> 
> Not sure how to build one since it's sensitive to what's in the trust store.
(I
> imagine this is why all of the tests were completely neutered to the point of
> not actually testing the production code.) I'm not super-familiar with NSS.
How
> would I go about convincing it that intermediates, etc., exist? Do I just
> (ab)use the in-memory certificate store and just have the objects hanging
around
> in memory somewhere?

Using the in-memory store is fine; it's not an abuse, it's an explicit layer in
the nssTrustDomain concept.

If you wanted to do an actual on-disk change, something like
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
should show all the right components getting exercised

- Creating a ScopedTestNSSDB
- Importing some client certs
- Testing the selector

I would expect that a test here would
1) Load a fake root (ScopedTestRoot or w/e)
2) Load a client cert & intermediate into the test DB (see above)
3) Release the in-memory handles to those certs (to bypass the memory pool)
4) Faking a CertificateRequest that requests a client cert from Root
5) Verifying that it picks up the client cert & intermediate

I'm fairly sure that for this logic to work, you at least have to use a
ScopedTestRoot, and it MIGHT have to be tweaked what flags it sets on the
CERTTrust object to also mark it trusted for issuing client certs (I don't think
it does, but it may)

[davidben, 2015-12-16 19:11:26]

On 2015/12/16 19:08:20, Ryan Sleevi wrote:
> On 2015/12/16 04:15:42, davidben wrote:
> > On 2015/12/16 01:46:54, Ryan Sleevi wrote:
> > > Where my tests at? ;)
> > 
> > Not sure how to build one since it's sensitive to what's in the trust store.
> (I
> > imagine this is why all of the tests were completely neutered to the point
of
> > not actually testing the production code.) I'm not super-familiar with NSS.
> How
> > would I go about convincing it that intermediates, etc., exist? Do I just
> > (ab)use the in-memory certificate store and just have the objects hanging
> around
> > in memory somewhere?
> 
> Using the in-memory store is fine; it's not an abuse, it's an explicit layer
in
> the nssTrustDomain concept.
> 
> If you wanted to do an actual on-disk change, something like
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> should show all the right components getting exercised
> 
> - Creating a ScopedTestNSSDB
> - Importing some client certs
> - Testing the selector
> 
> I would expect that a test here would
> 1) Load a fake root (ScopedTestRoot or w/e)
> 2) Load a client cert & intermediate into the test DB (see above)
> 3) Release the in-memory handles to those certs (to bypass the memory pool)
> 4) Faking a CertificateRequest that requests a client cert from Root
> 5) Verifying that it picks up the client cert & intermediate
> 
> I'm fairly sure that for this logic to work, you at least have to use a
> ScopedTestRoot, and it MIGHT have to be tweaked what flags it sets on the
> CERTTrust object to also mark it trusted for issuing client certs (I don't
think
> it does, but it may)

From testing locally, I don't believe this code cares about trust bits. At
least, I didn't have to make the root trusted for anything to get things
working. (Actually, I didn't even need to have the root. Putting an intermediate
in the store was sufficient.)

[Ryan Sleevi, 2016-02-05 23:51:24]

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

https://codereview.chromium.org/1526783002/diff/20001/net/ssl/client_cert_sto...
net/ssl/client_cert_store_nss.cc:111: // out of the local store.
https://crbug.com/548631
Blah, I remember what the 'bug' was now.

So NSS_CmpCertChainWCANames builds the chain up to a depth of 20 using
CERT_FindCertByName() to validate that |ca_names| matches, but
CERT_CertChainFromCert may find a different chain.

Both are bugged in that they don't handle cross-certification properly, which is
why I thought it should DIAF, but to be fair, the OS X logic doesn't handle it
properly either (the Windows code, unsurprisingly, does)

At least now I remember more about my hatred ;)

https://codereview.chromium.org/1526783002/diff/20001/net/ssl/client_cert_sto...
net/ssl/client_cert_store_nss.cc:126:
X509Certificate::CreateFromDERCertChain(der_chain);
This actually forces NSS through a full reparse and token scan (to see if it
already has handles allocated)

It would be better to build from the OSCertHandles returned in |chain| and
CreateFromOSCertHandles

[Ryan Sleevi, 2016-03-11 17:54:32]

Old CL poke?

[Ryan Sleevi, 2016-05-16 20:55:50]

On 2016/03/11 17:54:32, Ryan Sleevi wrote:
> Old CL poke?

Poke?

[davidben, 2016-06-10 18:26:24]

On 2016/05/16 20:55:50, Ryan Sleevi wrote:
> On 2016/03/11 17:54:32, Ryan Sleevi wrote:
> > Old CL poke?
> 
> Poke?

Did we ever make a decision here? I thought you didn't want to do chain-building
at all. I think that, as long as we do it on other platforms, we should do it on
all of them. (And I don't think we have the time to fight removing it on other
platforms right now.) There was also some concern about infinite loops and
whether it happens before or after the prompt. I never understood that. If
something can infinite loop, that's a problem whenever we do it.

[davidben, 2016-06-10 18:28:24]

On 2016/06/10 18:26:24, davidben wrote:
> On 2016/05/16 20:55:50, Ryan Sleevi wrote:
> > On 2016/03/11 17:54:32, Ryan Sleevi wrote:
> > > Old CL poke?
> > 
> > Poke?
> 
> Did we ever make a decision here? I thought you didn't want to do
chain-building
> at all. I think that, as long as we do it on other platforms, we should do it
on
> all of them. (And I don't think we have the time to fight removing it on other
> platforms right now.) There was also some concern about infinite loops and
> whether it happens before or after the prompt. I never understood that. If
> something can infinite loop, that's a problem whenever we do it.

I would propose that, since we have to do something vaguely resembling
path-building for matching the naming *anyway* (currently
NSS_CmpCertChainWCANames), we find a way to arrange for them to be the same
operation. Which would suggest that the ClientCertStore should retain the
certificates *with the chains* rather than doing something weird where it
happens after selecting a cert. That was previously proposed, but it ends up
being a huge hassle to route.

[Ryan Sleevi, 2016-06-16 00:32:58]

On 2016/06/10 18:28:24, davidben wrote:
> I would propose that, since we have to do something vaguely resembling
> path-building for matching the naming *anyway* (currently
> NSS_CmpCertChainWCANames), we find a way to arrange for them to be the same
> operation. Which would suggest that the ClientCertStore should retain the
> certificates *with the chains* rather than doing something weird where it
> happens after selecting a cert. That was previously proposed, but it ends up
> being a huge hassle to route.

I'm not sure I follow. Then again, I also have trouble following this code
(which I admittedly approved during review).

AIUI you're proposing we memoize the intermediates during the
NSS_CmpCertChainWCANames-ish thing, and return those in |filtered_certs| as the
intermediate, and let them surface up to the UI and then down to the SSLSocket?
Yeah, that should work.

And yeah, I don't necessarily want to, but I realize I don't have the time to
fight w/ Enterprise on that this quarter. I'm fine with simply closing this out
as a "No time" - I am mostly just trying to get dead or abandoned CLs off my
dashboard :)

[davidben, 2016-06-16 16:58:31]

On 2016/06/16 00:32:58, Ryan Sleevi wrote:
> On 2016/06/10 18:28:24, davidben wrote:
> > I would propose that, since we have to do something vaguely resembling
> > path-building for matching the naming *anyway* (currently
> > NSS_CmpCertChainWCANames), we find a way to arrange for them to be the same
> > operation. Which would suggest that the ClientCertStore should retain the
> > certificates *with the chains* rather than doing something weird where it
> > happens after selecting a cert. That was previously proposed, but it ends up
> > being a huge hassle to route.
> 
> I'm not sure I follow. Then again, I also have trouble following this code
> (which I admittedly approved during review).
> 
> AIUI you're proposing we memoize the intermediates during the
> NSS_CmpCertChainWCANames-ish thing, and return those in |filtered_certs| as
the
> intermediate, and let them surface up to the UI and then down to the
SSLSocket?
> Yeah, that should work.

Yup. If I recall, one of your objections to this CL was that
CERT_CertChainFromCert did something crazy and we didn't want to do it on
certificates the user didn't select? I'm not sure what the difference is between
CERT_CertChainFromCert and NSS_CmpCertChainWCANames, but since we have to do the
latter anyway, I figure we may as well use the latter's algorithm which
apparently we're fine with. (NSS_CmpCertChainWCANames is already vendored, so we
can give it an extra return value.)

> And yeah, I don't necessarily want to, but I realize I don't have the time to
> fight w/ Enterprise on that this quarter. I'm fine with simply closing this
out
> as a "No time" - I am mostly just trying to get dead or abandoned CLs off my
> dashboard :)

:-)

Eh, it's a regression, even though it's a regression to a state we wished
everything else where at. I think it's probably worth doing. I'll try to polish
this up and implement the other version.