New issue
Advanced search Search tips

Issue 795231 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Task



Sign in to add a comment

Move untrusted intermediates handling into ClientCertStore

Project Member Reported by pmarko@chromium.org, Dec 15 2017

Issue description

Refactor code to move untrusted intermediates handling into ClientCertStore(NSS).
Change the tests introduced in CL:806456 to reflect this if necessary.

Discussion form https://chromium-review.googlesource.com/c/chromium/src/+/806456/5 - follow-up from crbug.com/723849:

rsleevi@:
Wondering if a future design (code cleanup rather than feature blocking) would be to supply the ONC certs to the ClientCertStore, now that we have that, and then have the tests check that the ClientCertStore had the ONC policies made available to them.

pmarko@:
What ClientCertStore also just hold them the way UntrustedAuthorityCache does now (after all, the cert matching logic is not in ClientCertStore)?
If yes, would we simply not test the actual client cert matching across intermediate cert then, and only test what arrived in ClientCertStore?

rsleevi@:
And regarding ClientCertStore testing - yes, exactly. That is, let's imagine we moved ClientCertStore matching to another implementation, or on another platform which didn't have the NSS side-effect (speaking hypothetically, but it's relevant for testing).

The expectation from an implementation side is that the ClientCertStore would use the list of certs it was provided as part of path building, trying to complete the chains in the SSLCertRequestInfo to match to client identities, using the server-supplied certificates as well as the ONC-provided certificates.

Right now, it's these tests that are implicitly relying on NSS behaviour. I'm willing to live with that, because we don't have any plans at the moment to change the client cert implementation on ChromeOS to use anything *other* than NSS, but it does mean that you've got an extra hidden dependency, and thus the testing has the risk of flakes (because we can't reset the state of that hidden dependency). The idea of having the ONC -> ClientCertStore path (rather than ONC -> Signin Frame) was that then we can move the 'hidden' dependency from the Signin logic into the ClientCertStoreNSS, which already has an _explicit_ dependency to begin with on NSS :)

It then lets us test the two bits somewhat separately

Test ONC -> ClientCertStore provides the right certs (and updates if/when device policy updates)
Test ClientCertStoreNSS's "cache" of the intermediates affects NSS path building


Again, I don't see this has a hard dependency right now - there's no plans to migrate that path building, and just like 'assume BoringSSL' is a reasonable assumption throughout the codebase for our TLS stack, 'assume NSS' is a reasonable assumption throughout ChromeOS right now for our key storage. But it does make testing somewhat simpler and give you a more localized assumption about behaviours, and I only realized it after seeing the tests :)
 

Comment 1 by pmarko@chromium.org, Dec 20 2017

Status: Assigned (was: Untriaged)

Sign in to add a comment