Skip checking certificate properties for L2TP/IPsec VPN using pre-shared key.

Skip checking certificate properties for L2TP/IPsec VPN using pre-shared key.

NetworkConnectionHandler::VerifyConfiguredAndConnect() did not
differentiate between the pre-shared key and certificate flow when
verifying the certificate properties of a L2TP/IPsec VPN connection
request. It always threw a 'configuration required' error and caused the
VPN configuration dialog to pop up even when all the credentials
information was available. This CL fixes this issue.

BUG=307665
TEST=Verified the following scenarios:
1. Add a 'L2TP/IPsec + pre-shared key' VPN with 'Save identity and
   password' unchecked. Connect to the VPN and then disconnect. Reconnect
   to the VPN and verify that it prompts for credentials.
2. Repeat 1 but with 'Save identity and password' checked and verify
   that it reconnects without prompting for credentials.

R=pneubeck@chromium.org, stevenjb@chromium.org

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

[stevenjb, 2014-02-13 00:04:30]

+pneubeck@, but this lgtm w/nit. Thanks for investigating and fixing this!

https://codereview.chromium.org/161083005/diff/1/chromeos/network/network_con...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/161083005/diff/1/chromeos/network/network_con...
chromeos/network/network_connection_handler.cc:408: client_cert_type =
client_cert::CONFIG_TYPE_IPSEC;
nit: {} around multi-line (including comments) if clause

[Ben Chan, 2014-02-13 00:13:02]

On 2014/02/13 00:04:30, stevenjb wrote:
> +pneubeck@, but this lgtm w/nit. Thanks for investigating and fixing this!
> 
>
https://codereview.chromium.org/161083005/diff/1/chromeos/network/network_con...
> File chromeos/network/network_connection_handler.cc (right):
> 
>
https://codereview.chromium.org/161083005/diff/1/chromeos/network/network_con...
> chromeos/network/network_connection_handler.cc:408: client_cert_type =
> client_cert::CONFIG_TYPE_IPSEC;
> nit: {} around multi-line (including comments) if clause

done

[Ben Chan, 2014-02-13 00:16:08]

The CQ bit was checked by benchan@chromium.org

[Ben Chan, 2014-02-13 00:16:11]

The CQ bit was unchecked by benchan@chromium.org

[pneubeck (no reviews), 2014-02-13 09:53:06]

https://codereview.chromium.org/161083005/diff/250001/chromeos/network/networ...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/161083005/diff/250001/chromeos/network/networ...
chromeos/network/network_connection_handler.cc:400: else if
(!vpn_client_cert_id.empty()) {
please ignore if my reasoning is incorrect:

this doesn't work when the cert is not set _yet_.
E.g. in case of a unresolved certificate pattern (see line 425), we would
mistakenly assume that the VPN is a PSK one.

Why not instead check for the PSK?

https://codereview.chromium.org/161083005/diff/250001/chromeos/network/networ...
chromeos/network/network_connection_handler.cc:488: // to connect.
this should be removed once the connectable property is fixed.

[pneubeck (no reviews), 2014-02-13 09:53:06]

https://codereview.chromium.org/161083005/diff/250001/chromeos/network/networ...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/161083005/diff/250001/chromeos/network/networ...
chromeos/network/network_connection_handler.cc:400: else if
(!vpn_client_cert_id.empty()) {
please ignore if my reasoning is incorrect:

this doesn't work when the cert is not set _yet_.
E.g. in case of a unresolved certificate pattern (see line 425), we would
mistakenly assume that the VPN is a PSK one.

Why not instead check for the PSK?

https://codereview.chromium.org/161083005/diff/250001/chromeos/network/networ...
chromeos/network/network_connection_handler.cc:488: // to connect.
this should be removed once the connectable property is fixed.

[Ben Chan, 2014-02-13 16:36:12]

https://codereview.chromium.org/161083005/diff/250001/chromeos/network/networ...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/161083005/diff/250001/chromeos/network/networ...
chromeos/network/network_connection_handler.cc:400: else if
(!vpn_client_cert_id.empty()) {
On 2014/02/13 09:53:07, pneubeck wrote:
> please ignore if my reasoning is incorrect:
> 
> this doesn't work when the cert is not set _yet_.
> E.g. in case of a unresolved certificate pattern (see line 425), we would
> mistakenly assume that the VPN is a PSK one.
> 
> Why not instead check for the PSK?

In the UI flow, VpnConfigView::CanLogin() already uses ClientCertId to
distinguish PSK from Cert. I guess the ONC flow is kinda different.  We could
potentially rely on the onc::ipsec::kAuthenticationType in such case.  How can I
construct a partial ONC with client cert ID not configured?

[pneubeck (no reviews), 2014-02-13 20:14:42]

lgtm.
Please consider adding a unit test if feasible.

[stevenjb, 2014-02-13 21:15:33]

lgtm w/ nit

https://codereview.chromium.org/161083005/diff/360001/chromeos/network/networ...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/161083005/diff/360001/chromeos/network/networ...
chromeos/network/network_connection_handler.cc:399: ui_data &&
(ui_data->certificate_type() != CLIENT_CERT_TYPE_NONE);
It looks like this bool is only used for kTypeVPN. We should probably move it
there, and even better do something like this:

if (vpn_provider_type == shill::kProviderOpenVpn) {
  client_cert_type = client_cert::CONFIG_TYPE_OPENVPN;
} else {
  // L2TP/IPSec only requires a certificate if one is specified in ONC
  // or one was configured by the UI. Otherwise it is L2TP/IPSec with
  // PSK and doesn't require a certificate.
  // TODO(benchan)...
  if (!vpn_client_cert_id.empty() ||
      (ui_data && ui_data->certificate_type() != CLIENT_CERT_TYPE_NONE))
    client_cert_type = client_cert::CONFIG_TYPE_IPSEC;
}

[Ben Chan, 2014-02-13 21:30:58]

https://codereview.chromium.org/161083005/diff/360001/chromeos/network/networ...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/161083005/diff/360001/chromeos/network/networ...
chromeos/network/network_connection_handler.cc:399: ui_data &&
(ui_data->certificate_type() != CLIENT_CERT_TYPE_NONE);
On 2014/02/13 21:15:33, stevenjb wrote:
> It looks like this bool is only used for kTypeVPN. We should probably move it
> there, and even better do something like this:
> 
> if (vpn_provider_type == shill::kProviderOpenVpn) {
>   client_cert_type = client_cert::CONFIG_TYPE_OPENVPN;
> } else {
>   // L2TP/IPSec only requires a certificate if one is specified in ONC
>   // or one was configured by the UI. Otherwise it is L2TP/IPSec with
>   // PSK and doesn't require a certificate.
>   // TODO(benchan)...
>   if (!vpn_client_cert_id.empty() ||
>       (ui_data && ui_data->certificate_type() != CLIENT_CERT_TYPE_NONE))
>     client_cert_type = client_cert::CONFIG_TYPE_IPSEC;
> }
> 

Done.

[Ben Chan, 2014-02-13 23:55:57]

Committed patchset #3 manually as r251193 (presubmit successful).