Refactor the client certificate code in chromeos/network/.

Refactor the client certificate code in chromeos/network/.

This prepares for the next commit, which adds automatic resolution of client certificates which doesn't require a manual trigger by the user anymore.

The only functional is that flimflam::kOpenVPNClientCertSlotProperty is not set anymore, because it was never supported by Shill and leads to errors with Service::SetProperties.

BUG=234983, 126870
R=stevenjb@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216997

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

PTAL

[stevenjb, 2013-08-07 19:24:47]

This cleanup seems fine, although it's going to be an ugly merge with my Connect
CL.

https://codereview.chromium.org/22588002/diff/12001/chromeos/network/client_c...
File chromeos/network/client_cert_util.cc (right):

https://codereview.chromium.org/22588002/diff/12001/chromeos/network/client_c...
chromeos/network/client_cert_util.cc:226: }
NOTREACHED()

https://codereview.chromium.org/22588002/diff/12001/chromeos/network/client_c...
File chromeos/network/client_cert_util.h (right):

https://codereview.chromium.org/22588002/diff/12001/chromeos/network/client_c...
chromeos/network/client_cert_util.h:42: // Fetches the matching certificate that
has the latest valid start date.
indent

https://codereview.chromium.org/22588002/diff/12001/chromeos/network/network_...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/22588002/diff/12001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:436: // value.
nit: Only 2 spaces before cpmment, and maybe use "Init" to fit trailing comment
on one line

[pneubeck (no reviews), 2013-08-08 11:28:29]

maybe I can wait until you committed your CL.

https://codereview.chromium.org/22588002/diff/12001/chromeos/network/client_c...
File chromeos/network/client_cert_util.cc (right):

https://codereview.chromium.org/22588002/diff/12001/chromeos/network/client_c...
chromeos/network/client_cert_util.cc:226: }
On 2013/08/07 19:24:47, stevenjb (chromium) wrote:
> NOTREACHED()

Done.

https://codereview.chromium.org/22588002/diff/12001/chromeos/network/client_c...
File chromeos/network/client_cert_util.h (right):

https://codereview.chromium.org/22588002/diff/12001/chromeos/network/client_c...
chromeos/network/client_cert_util.h:42: // Fetches the matching certificate that
has the latest valid start date.
On 2013/08/07 19:24:47, stevenjb (chromium) wrote:
> indent

Done.

https://codereview.chromium.org/22588002/diff/12001/chromeos/network/network_...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/22588002/diff/12001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:436: // value.
On 2013/08/07 19:24:47, stevenjb (chromium) wrote:
> nit: Only 2 spaces before cpmment, and maybe use "Init" to fit trailing
comment
> on one line

Done.

[stevenjb, 2013-08-08 16:36:30]

On 2013/08/08 11:28:29, pneubeck wrote:
> maybe I can wait until you committed your CL.
> 
That would help me out. LGTM though.

[pneubeck (no reviews), 2013-08-09 09:11:30]

I had to adapt to the new connection code.
PTAL at
  client_cert_util.*
  network_connection_handler.cc

[stevenjb, 2013-08-09 17:03:29]

lgtm w/ optional nits

https://codereview.chromium.org/22588002/diff/43001/chromeos/network/client_c...
File chromeos/network/client_cert_util.cc (right):

https://codereview.chromium.org/22588002/diff/43001/chromeos/network/client_c...
chromeos/network/client_cert_util.cc:238:
properties->SetStringWithoutPathExpansion(tpm_pin_property, tpm_pin);
nit: I'm not sure using tpm_pin_property simplifies this any. If you call
SetString... directly in each case you can return instead of break and add a
NOTREACHED at the end.

https://codereview.chromium.org/22588002/diff/43001/chromeos/network/network_...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/22588002/diff/43001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:412: }
nit: It might be nice to make this a helper in client_cert_util.cc, e.g.
client_cert::GetConfigType(type, vpn_provider_type)

https://codereview.chromium.org/22588002/diff/43001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:416: // If the client certificate
must be configured, this will be reset to a
nit: even though the call is 'reset', 'set' would be more clear here.

https://codereview.chromium.org/22588002/diff/43001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:446:
!CertificateIsConfigured(ui_data.get(), pkcs11_id.get())) {
nit: This is kind of a weird way to set a scoped_ptr. Maybe refactor this to:

if (cert_loader_->IsHardwareBacked())
  pkcs11_id.swap(GetMatchingCertificate(ui_data.get());
if (!pkcs11_id)
  Error... return;

[pneubeck (no reviews), 2013-08-09 22:40:20]

https://codereview.chromium.org/22588002/diff/43001/chromeos/network/client_c...
File chromeos/network/client_cert_util.cc (right):

https://codereview.chromium.org/22588002/diff/43001/chromeos/network/client_c...
chromeos/network/client_cert_util.cc:238:
properties->SetStringWithoutPathExpansion(tpm_pin_property, tpm_pin);
On 2013/08/09 17:03:29, stevenjb (chromium) wrote:
> nit: I'm not sure using tpm_pin_property simplifies this any. If you call
> SetString... directly in each case you can return instead of break and add a
> NOTREACHED at the end.

I know, i had that before. But I didn't like the additional "if
(!tpm_pin.empty())" either.
The difference is negligible i think.

https://codereview.chromium.org/22588002/diff/43001/chromeos/network/network_...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/22588002/diff/43001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:412: }
On 2013/08/09 17:03:29, stevenjb (chromium) wrote:
> nit: It might be nice to make this a helper in client_cert_util.cc, e.g.
> client_cert::GetConfigType(type, vpn_provider_type)
> 

It's not used anywhere else currently. I'll leave it until upcoming changes
after the branch point.

https://codereview.chromium.org/22588002/diff/43001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:416: // If the client certificate
must be configured, this will be reset to a
On 2013/08/09 17:03:29, stevenjb (chromium) wrote:
> nit: even though the call is 'reset', 'set' would be more clear here.

Done.

https://codereview.chromium.org/22588002/diff/43001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:446:
!CertificateIsConfigured(ui_data.get(), pkcs11_id.get())) {
On 2013/08/09 17:03:29, stevenjb (chromium) wrote:
> nit: This is kind of a weird way to set a scoped_ptr. Maybe refactor this to:
> 
> if (cert_loader_->IsHardwareBacked())
>   pkcs11_id.swap(GetMatchingCertificate(ui_data.get());
> if (!pkcs11_id)
>   Error... return;

Done.

[commit-bot: I haz the power, 2013-08-09 22:41:52]

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

[commit-bot: I haz the power, 2013-08-09 22:56:53]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2013-08-09 23:02:05]

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

[commit-bot: I haz the power, 2013-08-10 06:22:15]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-10 18:19:30]

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

[commit-bot: I haz the power, 2013-08-11 00:24:09]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-12 07:01:12]

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

[pneubeck (no reviews), 2013-08-12 15:22:57]

Committed patchset #5 manually as r216997 (presubmit successful).