Automatically resolve ClientCertificatePatterns.

Automatically resolve ClientCertificatePatterns.

This adds a new ClientCertResolver to chromeos/network, which automatically resolves ClientCertificatePatterns and writes the cert id of the matching certificate to Shill.

This should fix several issues like updating client certs and auto-connect immediately after installing EAP networks from policy.
It's required for Ethernet EAP policies where the current pattern matching on each manual connect isn't sufficient.

BUG=234983, 126870
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217257

[pneubeck (no reviews), 2013-08-07 16:59:55]

Hi Steven,

I tested this CL already quickly with Google-A, and yes! Google-A now
automatically connects after installing the ClientCert.

However, it's still missing some method comments and a unit test, which I will
add tomorrow.

Maybe you could take a quick look already. Thanks!

[stevenjb, 2013-08-07 19:04:51]

Cool. Some early feedback. Greg should probably review this too when it's ready,
now that he is back.

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
File chromeos/network/client_cert_resolver.cc (right):

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.cc:61: };
This is kind of confusing up here, maybe move to just after the anon namespace?

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.cc:68: : cert(certificate),
pem_encoded_issuer(issuer) {}
one member per line

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.cc:121:
std::vector<ClientCertResolver::NetworkAndMatchingCert>* matches) {
nit: typedef these vectors for better readability here and below

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.cc:249: weak_ptr_factory_(this) {}
nit: } on next line

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.cc:301: }
I'm a little concerned with this. Because Shill forgets about networks after
~30s, and creates new Services on a scan, we can end up seeing thousands of
services over the course of a session. While not a huge bloat, it could become
significant.

I think we need to only do this for Favorites. With
https://codereview.chromium.org/21961003/, GetFavoriteList() should be robust.

Alternately we can ignore networks without a guid, see note below.

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.cc:342: const NetworkState& network =
**it;
Fwiw, most of the rest of the code uses a const* instead of a const& when
iterating over NetworkStateList. I don't think it really matters much, but I
have a slight preference for consistency.

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.cc:347: // If this network is not managed,
thus cannot have a ClientCertPattern.
s/thus/it

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.cc:349: continue;
We can also use this to filter out networks to consider and track. Actually, we
could track networks by guid which should easily avoid the bloat problem.

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.cc:351:
DCHECK(!network.profile_path().empty());
Is this a valid assertion? Shill will never set a GUID for a Service? What if we
forget a network, with the Service guid get cleared? It might be safer just to
skip networks without a profile_path.

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.cc:422:
->SetProperties(dbus::ObjectPath(it->service_path),
nit: -> on prev line

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
File chromeos/network/client_cert_resolver.h (right):

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.h:35: struct NetworkAndMatchingCert;
Should be private?

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.h:38: virtual ~ClientCertResolver();
WS

[pneubeck (no reviews), 2013-08-08 12:41:03]

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
File chromeos/network/client_cert_resolver.cc (right):

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.cc:61: };
On 2013/08/07 19:04:51, stevenjb (chromium) wrote:
> This is kind of confusing up here, maybe move to just after the anon
namespace?

It's used below in FindCertificateMatches, thus C++ requires the full
declaration there.

I moved it to the very top, maybe that's better than in between.

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.cc:68: : cert(certificate),
pem_encoded_issuer(issuer) {}
On 2013/08/07 19:04:51, stevenjb (chromium) wrote:
> one member per line

Done.

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.cc:121:
std::vector<ClientCertResolver::NetworkAndMatchingCert>* matches) {
On 2013/08/07 19:04:51, stevenjb (chromium) wrote:
> nit: typedef these vectors for better readability here and below

Done.

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.cc:249: weak_ptr_factory_(this) {}
On 2013/08/07 19:04:51, stevenjb (chromium) wrote:
> nit: } on next line

Done.

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.cc:301: }
On 2013/08/07 19:04:51, stevenjb (chromium) wrote:
> I'm a little concerned with this. Because Shill forgets about networks after
> ~30s, and creates new Services on a scan, we can end up seeing thousands of
> services over the course of a session.

That's not right. The new list (resolved_networks_) will not contain old, no
more existing networks (old_resolved_networks). See the swap above (line 286).
The loop iterates over the new network list.

> While not a huge bloat, it could become
> significant.
> 

> I think we need to only do this for Favorites. With
> https://codereview.chromium.org/21961003/, GetFavoriteList() should be robust.

I totally agree. Again, it's easy to fix that afterwards as well.

> 
> Alternately we can ignore networks without a guid, see note below.

I think that's a good easy solution for now.

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.cc:342: const NetworkState& network =
**it;
On 2013/08/07 19:04:51, stevenjb (chromium) wrote:
> Fwiw, most of the rest of the code uses a const* instead of a const& when
> iterating over NetworkStateList. I don't think it really matters much, but I
> have a slight preference for consistency.

I strongly prefer the enforcement of not-null by types.
Done.

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.cc:347: // If this network is not managed,
thus cannot have a ClientCertPattern.
On 2013/08/07 19:04:51, stevenjb (chromium) wrote:
> s/thus/it

Done.

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.cc:349: continue;
On 2013/08/07 19:04:51, stevenjb (chromium) wrote:
> We can also use this to filter out networks to consider and track. Actually,
we
> could track networks by guid which should easily avoid the bloat problem.

Done.

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.cc:351:
DCHECK(!network.profile_path().empty());
On 2013/08/07 19:04:51, stevenjb (chromium) wrote:
> Is this a valid assertion? Shill will never set a GUID for a Service? What if
we
> forget a network, with the Service guid get cleared? It might be safer just to
> skip networks without a profile_path.

Done.

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.cc:422:
->SetProperties(dbus::ObjectPath(it->service_path),
On 2013/08/07 19:04:51, stevenjb (chromium) wrote:
> nit: -> on prev line

as all other formatting issues, this is what clang does.
(Btw. this particular case is consistent with what we usually do in case of '.')

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
File chromeos/network/client_cert_resolver.h (right):

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.h:35: struct NetworkAndMatchingCert;
On 2013/08/07 19:04:51, stevenjb (chromium) wrote:
> Should be private?

it's public so that FindCertificateMatches (in the unnamed namespace of the .cc)
can create matches.

not ideal, I know, but the c'tor/d'tor are still not public. So no harm done.

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.h:38: virtual ~ClientCertResolver();
On 2013/08/07 19:04:51, stevenjb (chromium) wrote:
> WS

Done.

[pneubeck (no reviews), 2013-08-08 13:21:30]

@Greg, please also take a look.

Unit tests are still missing, because I must at first hurry to implement the ONC
to Shill translation for Ethernet EAP.

[stevenjb, 2013-08-08 16:35:11]

waiting for unit tests, but otherwise looks good.

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
File chromeos/network/client_cert_resolver.cc (right):

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.cc:61: };
On 2013/08/08 12:41:03, pneubeck wrote:
> On 2013/08/07 19:04:51, stevenjb (chromium) wrote:
> > This is kind of confusing up here, maybe move to just after the anon
> namespace?
> 
> It's used below in FindCertificateMatches, thus C++ requires the full
> declaration there.
> 
> I moved it to the very top, maybe that's better than in between.

That helps I think, thanks.

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.cc:301: }
On 2013/08/08 12:41:03, pneubeck wrote:
> On 2013/08/07 19:04:51, stevenjb (chromium) wrote:
> > I'm a little concerned with this. Because Shill forgets about networks after
> > ~30s, and creates new Services on a scan, we can end up seeing thousands of
> > services over the course of a session.
> 
> That's not right. The new list (resolved_networks_) will not contain old, no
> more existing networks (old_resolved_networks). See the swap above (line 286).
> The loop iterates over the new network list.
> 
> > While not a huge bloat, it could become
> > significant.
> > 
> 
> > I think we need to only do this for Favorites. With
> > https://codereview.chromium.org/21961003/, GetFavoriteList() should be
robust.
> 
> I totally agree. Again, it's easy to fix that afterwards as well.
> 
> > 
> > Alternately we can ignore networks without a guid, see note below.
> 
> I think that's a good easy solution for now.

Ahh... right... it's a swap not a merge. OK, that's fine then, especially with
skipping empty guids.

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.cc:422:
->SetProperties(dbus::ObjectPath(it->service_path),
On 2013/08/08 12:41:03, pneubeck wrote:
> On 2013/08/07 19:04:51, stevenjb (chromium) wrote:
> > nit: -> on prev line
> 
> as all other formatting issues, this is what clang does.
> (Btw. this particular case is consistent with what we usually do in case of
'.')

Yeah, the clang formatting came up somewhere else. Guess we'll have to get used
to -> on the second line, but I'm not the only one who prefers -> on the
previous line.

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
File chromeos/network/client_cert_resolver.h (right):

https://codereview.chromium.org/22327005/diff/4001/chromeos/network/client_ce...
chromeos/network/client_cert_resolver.h:35: struct NetworkAndMatchingCert;
On 2013/08/08 12:41:03, pneubeck wrote:
> On 2013/08/07 19:04:51, stevenjb (chromium) wrote:
> > Should be private?
> 
> it's public so that FindCertificateMatches (in the unnamed namespace of the
.cc)
> can create matches.
> 
> not ideal, I know, but the c'tor/d'tor are still not public. So no harm done.
Yeah, it's a bit confusing. Maybe add a comment. Otherwise maybe
FindCertificateMatches would be better off as a member of NetworkAndMatchingCert
which could then be private? Just a thought.

https://codereview.chromium.org/22327005/diff/30001/chromeos/network/client_c...
File chromeos/network/client_cert_resolver.cc (right):

https://codereview.chromium.org/22327005/diff/30001/chromeos/network/client_c...
chromeos/network/client_cert_resolver.cc:130: // Search for matches between
|networks| and |certs| and write matches to
nit: Searches, writes

https://codereview.chromium.org/22327005/diff/30001/chromeos/network/client_c...
chromeos/network/client_cert_resolver.cc:182: // Determine the type of the
CertificatePattern configuration, i.e. is it a
nit: Determines

https://codereview.chromium.org/22327005/diff/30001/chromeos/network/client_c...
File chromeos/network/client_cert_resolver.h (right):

https://codereview.chromium.org/22327005/diff/30001/chromeos/network/client_c...
chromeos/network/client_cert_resolver.h:32: // Eachtime a match it finds a
match, it configures the network accordingly.
nit: s/Eachtime a match/Each time/

[pneubeck (no reviews), 2013-08-09 22:07:44]

Patch set 4 added a unit test (I know, that's not comprehensive yet).
That required a small change to CertLoader, so that it does TPM initialization
during the test.

https://codereview.chromium.org/22327005/diff/30001/chromeos/network/client_c...
File chromeos/network/client_cert_resolver.cc (right):

https://codereview.chromium.org/22327005/diff/30001/chromeos/network/client_c...
chromeos/network/client_cert_resolver.cc:130: // Search for matches between
|networks| and |certs| and write matches to
On 2013/08/08 16:35:11, stevenjb (chromium) wrote:
> nit: Searches, writes

Done.

https://codereview.chromium.org/22327005/diff/30001/chromeos/network/client_c...
chromeos/network/client_cert_resolver.cc:182: // Determine the type of the
CertificatePattern configuration, i.e. is it a
On 2013/08/08 16:35:11, stevenjb (chromium) wrote:
> nit: Determines

Done.

https://codereview.chromium.org/22327005/diff/30001/chromeos/network/client_c...
File chromeos/network/client_cert_resolver.h (right):

https://codereview.chromium.org/22327005/diff/30001/chromeos/network/client_c...
chromeos/network/client_cert_resolver.h:32: // Eachtime a match it finds a
match, it configures the network accordingly.
On 2013/08/08 16:35:11, stevenjb (chromium) wrote:
> nit: s/Eachtime a match/Each time/

Done.

[stevenjb, 2013-08-09 22:22:43]

I only looked at the unittest briefly, I'll try to look more closely over the
weekend, but lgtm in the meanwhile.

https://codereview.chromium.org/22327005/diff/40001/chromeos/cert_loader.h
File chromeos/cert_loader.h (right):

https://codereview.chromium.org/22327005/diff/40001/chromeos/cert_loader.h#ne...
chromeos/cert_loader.h:74: // before SetCryptoTaskRunner().
Second sentence is incomplete / unclear.

https://codereview.chromium.org/22327005/diff/40001/chromeos/network/client_c...
File chromeos/network/client_cert_resolver.cc (right):

https://codereview.chromium.org/22327005/diff/40001/chromeos/network/client_c...
chromeos/network/client_cert_resolver.cc:182: // Determine the type of the
CertificatePattern configuration, i.e. is it a
Determines

[pneubeck (no reviews), 2013-08-09 22:46:43]

https://codereview.chromium.org/22327005/diff/40001/chromeos/cert_loader.h
File chromeos/cert_loader.h (right):

https://codereview.chromium.org/22327005/diff/40001/chromeos/cert_loader.h#ne...
chromeos/cert_loader.h:74: // before SetCryptoTaskRunner().
On 2013/08/09 22:22:44, stevenjb (chromium) wrote:
> Second sentence is incomplete / unclear.

Done.

https://codereview.chromium.org/22327005/diff/40001/chromeos/network/client_c...
File chromeos/network/client_cert_resolver.cc (right):

https://codereview.chromium.org/22327005/diff/40001/chromeos/network/client_c...
chromeos/network/client_cert_resolver.cc:182: // Determine the type of the
CertificatePattern configuration, i.e. is it a
On 2013/08/09 22:22:44, stevenjb (chromium) wrote:
> Determines

Done.

[pneubeck (no reviews), 2013-08-09 22:50:57]

This CL depends on
https://codereview.chromium.org/22588002/ (Refactor the client certificate code
in chromeos/network/)

and

https://codereview.chromium.org/22588002/ (Make CryptohomeClientImplStub and
ScopedTestNSSDB use the same TokenName)

So I might be able to commit it tomorrow.

[stevenjb, 2013-08-10 00:11:57]

unittest nits

https://codereview.chromium.org/22327005/diff/40001/chromeos/network/client_c...
File chromeos/network/client_cert_resolver_unittest.cc (right):

https://codereview.chromium.org/22327005/diff/40001/chromeos/network/client_c...
chromeos/network/client_cert_resolver_unittest.cc:38: //const char* kFakePEM =
"pem";
remove until used

https://codereview.chromium.org/22327005/diff/40001/chromeos/network/client_c...
chromeos/network/client_cert_resolver_unittest.cc:90: "websocket_cacert.pem",
FILE_PATH_LITERAL() ?
Also, make this a const with a descriptive name (e.g. kTestCertsWebsocketCA").
Would be ideal if this were defined somewhere in net.

https://codereview.chromium.org/22327005/diff/40001/chromeos/network/client_c...
chromeos/network/client_cert_resolver_unittest.cc:105:
net::GetTestCertsDirectory().Append("websocket_client_cert.p12"),
named const

https://codereview.chromium.org/22327005/diff/40001/chromeos/network/client_c...
chromeos/network/client_cert_resolver_unittest.cc:182:
ASSERT_TRUE(policy_value->GetAsList(&policy) && policy);
nit: && policy redundant; GetAsList only returns true if policy != NULL

[pneubeck (no reviews), 2013-08-11 18:37:01]

https://codereview.chromium.org/22327005/diff/40001/chromeos/network/client_c...
File chromeos/network/client_cert_resolver_unittest.cc (right):

https://codereview.chromium.org/22327005/diff/40001/chromeos/network/client_c...
chromeos/network/client_cert_resolver_unittest.cc:38: //const char* kFakePEM =
"pem";
On 2013/08/10 00:11:58, stevenjb (chromium) wrote:
> remove until used

Done.

https://codereview.chromium.org/22327005/diff/40001/chromeos/network/client_c...
chromeos/network/client_cert_resolver_unittest.cc:90: "websocket_cacert.pem",
On 2013/08/10 00:11:58, stevenjb (chromium) wrote:
> FILE_PATH_LITERAL() ?
> Also, make this a const with a descriptive name (e.g. kTestCertsWebsocketCA").
> Would be ideal if this were defined somewhere in net.

AFAIU, FILE_PATH_LITERAL helps for cross-platform support. Since we're in
chromeos it seems unnecessary.
net::CreateCertificateListFromFile also has a std::string argument and not a
file path.

The filename is already a name of some object (the certificate in the file).
What exactly does the globally named constant improve? Here the string itself is
important. Any additional indirection seems more complicated.

Added some clarifying comments instead.

https://codereview.chromium.org/22327005/diff/40001/chromeos/network/client_c...
chromeos/network/client_cert_resolver_unittest.cc:105:
net::GetTestCertsDirectory().Append("websocket_client_cert.p12"),
On 2013/08/10 00:11:58, stevenjb (chromium) wrote:
> named const

ditto.

https://codereview.chromium.org/22327005/diff/40001/chromeos/network/client_c...
chromeos/network/client_cert_resolver_unittest.cc:182:
ASSERT_TRUE(policy_value->GetAsList(&policy) && policy);
On 2013/08/10 00:11:58, stevenjb (chromium) wrote:
> nit: && policy redundant; GetAsList only returns true if policy != NULL

Done.

[commit-bot: I haz the power, 2013-08-12 15:45:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/22327005/98001

[commit-bot: I haz the power, 2013-08-12 17:32:15]

Retried try job too often on linux_chromeos for step(s) chromeos_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-08-12 20:50:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/22327005/115001

[commit-bot: I haz the power, 2013-08-13 15:04:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/22327005/115001

[commit-bot: I haz the power, 2013-08-13 15:30:08]

Change committed as 217257