Migrate Slot ID of client certs in network configuration.

Migrate slot ID of client certs in network configuration.

If a network was configured with a client certificate before R38, the slot ID will either be missing or not be valid in R38 and later because of the added device wide token.

This migration code will look up a configured client certificate by its PKCS#11 ID and update the slot id.
If the certificate can't be found, the client configuration will be removed in order to trigger a clean 'missing configuration' error in the UI.

This change is primarily targeted for manually configured networks and not policy-configured networks. The latter are automatically updated by the chromeos::ClientCertResolver.

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

[pneubeck (no reviews), 2014-08-14 19:39:05]

Steven, if you have a chance please take a rough view at it.
Tomorrow someone in my team will review it thoroughly.

+Arman, maybe you can verify it from Shill's point of view.

[armansito, 2014-08-14 19:49:26]

quiche@, can you help verify this?

[Paul Stewart, 2014-08-14 20:09:18]

Pardon my fly-by.

https://codereview.chromium.org/471183002/diff/1/chromeos/network/client_cert...
File chromeos/network/client_cert_util.cc (right):

https://codereview.chromium.org/471183002/diff/1/chromeos/network/client_cert...
chromeos/network/client_cert_util.cc:167: } else if (security ==
shill::kSecurity8021x) {
I think you're missing the 802.1x WEP case?  You also may want to be careful
that you're not in an 802.1x but not TLS case here.

https://codereview.chromium.org/471183002/diff/1/chromeos/network/network_cer...
File chromeos/network/network_cert_migrator.cc (right):

https://codereview.chromium.org/471183002/diff/1/chromeos/network/network_cer...
chromeos/network/network_cert_migrator.cc:70: (*it)->type() != shill::kTypeVPN)
{
This is another place where you are opting out of the WEP/802.1x case
("Enterprise WEP").  Also, what above EthernetEAP services?

[pneubeck (no reviews), 2014-08-14 23:06:00]

Thanks Paul

https://codereview.chromium.org/471183002/diff/1/chromeos/network/client_cert...
File chromeos/network/client_cert_util.cc (right):

https://codereview.chromium.org/471183002/diff/1/chromeos/network/client_cert...
chromeos/network/client_cert_util.cc:167: } else if (security ==
shill::kSecurity8021x) {
On 2014/08/14 20:09:17, Paul Stewart wrote:
> I think you're missing the 802.1x WEP case?
802.1x WEP is still not supported by Chrome as its priority was never considered
high enough: crbug.com/127070.

>  You also may want to be careful
> that you're not in an 802.1x but not TLS case here.  
I could add a
  if (pkcs11_id.empty())
condition to each assignment to |cert_config_type|.
That would take care of the cases where actually no client cert was configured.

To handle EthernetEAP this would also need a
  || type == shill::kTypeEthernetEap


Alteneratively, I could drop the checks for Type and Security and instead probe
each of the Cert/Key/Slot properties and the first existent will reveal what
kind of network it is.
I.e. something like:

if (provider_properties) {
  // Network must be a VPN

  if
(provider_properties->GetStringWithoutPathExpansion(..OpenVPNClientCertId...)) {
    // Must be a OpenVPN
    *cert_config_type = CONFIG_TYPE_OPENVPN;
  } else if (...Get...(..Ipsec..)) {
    // Must be a Ipsec VPN
    ...
  }
} else if (properties.Get...(kEapCertIdProperty, &cert_id)) {
  // Must be a network with EAP config and client certificate (e.g. WPA 802.1x
or Ethernet 802.1x).
  ...
}

https://codereview.chromium.org/471183002/diff/1/chromeos/network/network_cer...
File chromeos/network/network_cert_migrator.cc (right):

https://codereview.chromium.org/471183002/diff/1/chromeos/network/network_cer...
chromeos/network/network_cert_migrator.cc:70: (*it)->type() != shill::kTypeVPN)
{
On 2014/08/14 20:09:17, Paul Stewart wrote:
> This is another place where you are opting out of the WEP/802.1x case
> ("Enterprise WEP"). 

> Also, what above EthernetEAP services?
Right, that one is ignored so far.
I'll add the condition
  && (*it)->type() != shill::kTypeEthernetEap

[stevenjb, 2014-08-14 23:58:39]

Quick flyby since I'm OOO this week. an OK by Arman + Paul is fine by me.

https://codereview.chromium.org/471183002/diff/1/chromeos/network/client_cert...
File chromeos/network/client_cert_util.h (right):

https://codereview.chromium.org/471183002/diff/1/chromeos/network/client_cert...
chromeos/network/client_cert_util.h:67: std::string* pkcs11_id);
At minimum this needs a comment. It also seems oddly named since |properties| is
an input. Maybe GetCertConfigFromShillProperties?

[pneubeck (no reviews), 2014-08-15 13:15:21]

https://codereview.chromium.org/471183002/diff/1/chromeos/network/client_cert...
File chromeos/network/client_cert_util.cc (right):

https://codereview.chromium.org/471183002/diff/1/chromeos/network/client_cert...
chromeos/network/client_cert_util.cc:167: } else if (security ==
shill::kSecurity8021x) {
On 2014/08/14 23:06:00, pneubeck wrote:
> On 2014/08/14 20:09:17, Paul Stewart wrote:
> > I think you're missing the 802.1x WEP case?
> 802.1x WEP is still not supported by Chrome as its priority was never
considered
> high enough: crbug.com/127070.
> 
> >  You also may want to be careful
> > that you're not in an 802.1x but not TLS case here.  
> I could add a
>   if (pkcs11_id.empty())
> condition to each assignment to |cert_config_type|.
> That would take care of the cases where actually no client cert was
configured.
> 
> To handle EthernetEAP this would also need a
>   || type == shill::kTypeEthernetEap
> 
> 
> Alteneratively, I could drop the checks for Type and Security and instead
probe
> each of the Cert/Key/Slot properties and the first existent will reveal what
> kind of network it is.
> I.e. something like:
> 
> if (provider_properties) {
>   // Network must be a VPN
> 
>   if
> (provider_properties->GetStringWithoutPathExpansion(..OpenVPNClientCertId...))
{
>     // Must be a OpenVPN
>     *cert_config_type = CONFIG_TYPE_OPENVPN;
>   } else if (...Get...(..Ipsec..)) {
>     // Must be a Ipsec VPN
>     ...
>   }
> } else if (properties.Get...(kEapCertIdProperty, &cert_id)) {
>   // Must be a network with EAP config and client certificate (e.g. WPA 802.1x
> or Ethernet 802.1x).
>   ...
> }

Done.

https://codereview.chromium.org/471183002/diff/1/chromeos/network/client_cert...
File chromeos/network/client_cert_util.h (right):

https://codereview.chromium.org/471183002/diff/1/chromeos/network/client_cert...
chromeos/network/client_cert_util.h:67: std::string* pkcs11_id);
On 2014/08/14 23:58:39, stevenjb wrote:
> At minimum this needs a comment. It also seems oddly named since |properties|
is
> an input. Maybe GetCertConfigFromShillProperties?

Done.

https://codereview.chromium.org/471183002/diff/1/chromeos/network/network_cer...
File chromeos/network/network_cert_migrator.cc (right):

https://codereview.chromium.org/471183002/diff/1/chromeos/network/network_cer...
chromeos/network/network_cert_migrator.cc:70: (*it)->type() != shill::kTypeVPN)
{
On 2014/08/14 23:06:00, pneubeck wrote:
> On 2014/08/14 20:09:17, Paul Stewart wrote:
> > This is another place where you are opting out of the WEP/802.1x case
> > ("Enterprise WEP"). 
> 
> > Also, what above EthernetEAP services?
> Right, that one is ignored so far.
> I'll add the condition
>   && (*it)->type() != shill::kTypeEthernetEap

Done.

[pneubeck (no reviews), 2014-08-15 15:45:31]

@Paul, please take another look.

[pneubeck (no reviews), 2014-08-15 17:16:01]

Ethernet EAP is now also working.
Also added a unit test for that, which required more adaptions.

Tested both SNAX wifi and ethernet.
What can't be helped for now is that autoconnect breaks in the process.
- for ethernet, a re-login is required because the reconfiguration will be too
late and Shill already seems to decide to connect without EAP / or doesn't
retry.
- for wifi, a manually initiated connection is required because the
reconfiguration taints the network configuration (the property
'HasEverConnected' will be reset to false by Shill).

[Paul Stewart, 2014-08-15 21:51:28]

LGTM, from my limited perspective.

[pneubeck (no reviews), 2014-08-15 22:18:25]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-08-15 22:20:40]

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

[commit-bot: I haz the power, 2014-08-15 22:20:42]

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

[commit-bot: I haz the power, 2014-08-15 22:20:43]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[stevenjb, 2014-08-15 22:24:23]

lgtm

[stevenjb, 2014-08-15 22:24:27]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-08-15 22:25:59]

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

[commit-bot: I haz the power, 2014-08-15 23:21:56]

Committed patchset #4 (60001) as 290044