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

chromeos/network/network_connection_handler.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chromeos/network/network_connection_handler.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/json/json_reader.h"
 #include "base/location.h"
@@ skipping 356 matching lines @@
   // been recently configured), we need to check Connectable again.
   if (connectable && type != shill::kTypeVPN) {
     // TODO(stevenjb): Shill needs to properly set Connectable for VPN.
     CallShillConnect(service_path);
     return;
   }
 
   // Get VPN provider type and host (required for configuration) and ensure
   // that required VPN non-cert properties are set.
   const base::DictionaryValue* provider_properties = NULL;
-  std::string vpn_provider_type, vpn_provider_host;
+  std::string vpn_provider_type, vpn_provider_host, vpn_client_cert_id;
   if (type == shill::kTypeVPN) {
     // VPN Provider values are read from the "Provider" dictionary, not the
     // "Provider.Type", etc keys (which are used only to set the values).
     if (service_properties.GetDictionaryWithoutPathExpansion(
             shill::kProviderProperty, &provider_properties)) {
       provider_properties->GetStringWithoutPathExpansion(
           shill::kTypeProperty, &vpn_provider_type);
       provider_properties->GetStringWithoutPathExpansion(
           shill::kHostProperty, &vpn_provider_host);
+      provider_properties->GetStringWithoutPathExpansion(
+          shill::kL2tpIpsecClientCertIdProperty, &vpn_client_cert_id);
     }
     if (vpn_provider_type.empty() || vpn_provider_host.empty()) {
       ErrorCallbackForPendingRequest(service_path, kErrorConfigurationRequired);
       return;
     }
   }
 
   client_cert::ConfigType client_cert_type = client_cert::CONFIG_TYPE_NONE;
   if (type == shill::kTypeVPN) {
     if (vpn_provider_type == shill::kProviderOpenVpn)
       client_cert_type = client_cert::CONFIG_TYPE_OPENVPN;
-    else
+    else if (!vpn_client_cert_id.empty()) {

[pneubeck (no reviews), 2014/02/13 09:53:07]

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?

[Ben Chan, 2014/02/13 16:36:13]

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?

+      // If |vpn_client_cert_id| is empty, it's L2TP/IPsec PSK and doesn't
+      // require a certificate.
+
+      // TODO(benchan): Modify shill to include the authentication type,
+      // pre-shared key or certificate, in the VPN provider properties, so that
+      // Chrome doesn't need to deduce the authentication type based on the
+      // kL2tpIpsecClientCertIdProperty here (and also in VPNConfigView).
       client_cert_type = client_cert::CONFIG_TYPE_IPSEC;
+    }
   } else if (type == shill::kTypeWifi && security == shill::kSecurity8021x) {
     client_cert_type = client_cert::CONFIG_TYPE_EAP;
   }
 
   base::DictionaryValue config_properties;
   if (client_cert_type != client_cert::CONFIG_TYPE_NONE) {
     // If the client certificate must be configured, this will be set to a
     // non-empty string.
     std::string pkcs11_id;
 
@@ skipping 56 matching lines @@
   if (type == shill::kTypeVPN) {
     // VPN may require a username, and/or passphrase to be set. (Check after
     // ensuring that any required certificates are configured).
     DCHECK(provider_properties);
     if (VPNRequiresCredentials(
             service_path, vpn_provider_type, *provider_properties)) {
       NET_LOG_USER("VPN Requires Credentials", service_path);
       ErrorCallbackForPendingRequest(service_path, kErrorConfigurationRequired);
       return;
     }
+
+    // If it's L2TP/IPsec PSK, there is no properties to configure, so proceed
+    // to connect.

[pneubeck (no reviews), 2014/02/13 09:53:07]

this should be removed once the connectable property is fixed.

+    if (client_cert_type == client_cert::CONFIG_TYPE_NONE) {
+      CallShillConnect(service_path);
+      return;
+    }
   }
 
   if (!config_properties.empty()) {
     NET_LOG_EVENT("Configuring Network", service_path);
     network_configuration_handler_->SetProperties(
         service_path,
         config_properties,
         base::Bind(&NetworkConnectionHandler::CallShillConnect,
                    AsWeakPtr(),
                    service_path),
@@ skipping 190 matching lines @@
 
 void NetworkConnectionHandler::HandleShillDisconnectSuccess(
     const std::string& service_path,
     const base::Closure& success_callback) {
   NET_LOG_EVENT("Disconnect Request Sent", service_path);
   if (!success_callback.is_null())
     success_callback.Run();
 }
 
 }  // namespace chromeos