Add ONC VPN support for OpenVPN and L2TP/IPsec VPNs

chrome/browser/chromeos/cros/onc_network_parser.cc

Index: chrome/browser/chromeos/cros/onc_network_parser.cc
===================================================================
--- chrome/browser/chromeos/cros/onc_network_parser.cc	(revision 112277)
+++ chrome/browser/chromeos/cros/onc_network_parser.cc	(working copy)
@@ -22,34 +22,114 @@
 // Local constants.
 namespace {
 
-EnumMapper<PropertyIndex>::Pair property_index_table[] = {
-    { "GUID", PROPERTY_INDEX_GUID },
-    { "Name", PROPERTY_INDEX_NAME },
-    { "Remove", PROPERTY_INDEX_REMOVE },
-    { "ProxyURL", PROPERTY_INDEX_PROXY_CONFIG },
-    { "Type", PROPERTY_INDEX_TYPE },
-    { "SSID", PROPERTY_INDEX_SSID },
-    { "Passphrase", PROPERTY_INDEX_PASSPHRASE },
-    { "AutoConnect", PROPERTY_INDEX_AUTO_CONNECT },
-    { "HiddenSSID", PROPERTY_INDEX_HIDDEN_SSID },
-    { "Security", PROPERTY_INDEX_SECURITY },
-    { "EAP", PROPERTY_INDEX_EAP },
-    { "Outer", PROPERTY_INDEX_EAP_METHOD },
-    { "Inner", PROPERTY_INDEX_EAP_PHASE_2_AUTH },
-    { "UseSystemCAs", PROPERTY_INDEX_EAP_USE_SYSTEM_CAS },
-    { "ServerCARef", PROPERTY_INDEX_EAP_CA_CERT },
-    { "ClientCARef", PROPERTY_INDEX_EAP_CLIENT_CERT },
-    { "ClientCertPattern", PROPERTY_INDEX_EAP_CLIENT_CERT_PATTERN },
-    { "Identity", PROPERTY_INDEX_EAP_IDENTITY },
-    { "Password", PROPERTY_INDEX_EAP_PASSWORD },
-    { "AnonymousIdentity", PROPERTY_INDEX_EAP_ANONYMOUS_IDENTITY },
+EnumMapper<PropertyIndex>::Pair network_configuration_table[] = {
+  { "GUID", PROPERTY_INDEX_GUID },
+  { "Name", PROPERTY_INDEX_NAME },
+  { "Remove", PROPERTY_INDEX_ONC_REMOVE },
+  { "Type", PROPERTY_INDEX_TYPE },
+  { "WiFi", PROPERTY_INDEX_ONC_WIFI },
+  { "VPN", PROPERTY_INDEX_ONC_VPN }
 };
 
+const base::Value::Type TYPE_BOOLEAN = base::Value::TYPE_BOOLEAN;

[Mattias Nissler (ping if slow), 2011/12/01 13:09:10]

why not just:

using base::Value::TYPE_BOOLEAN;

[kmixter1, 2011/12/01 19:48:41]

using doesn't work with things inside of classes.

+const base::Value::Type TYPE_DICTIONARY = base::Value::TYPE_DICTIONARY;
+const base::Value::Type TYPE_INTEGER = base::Value::TYPE_INTEGER;
+const base::Value::Type TYPE_LIST = base::Value::TYPE_LIST;
+const base::Value::Type TYPE_STRING = base::Value::TYPE_STRING;
+
+OncValueSignature wifi_signature[] = {
+  { "AutoConnect", PROPERTY_INDEX_AUTO_CONNECT, TYPE_STRING },
+  { "EAP", PROPERTY_INDEX_EAP, TYPE_DICTIONARY },
+  { "HiddenSSID", PROPERTY_INDEX_HIDDEN_SSID, TYPE_BOOLEAN },
+  { "Passphrase", PROPERTY_INDEX_PASSPHRASE, TYPE_STRING },
+  { "ProxyURL", PROPERTY_INDEX_PROXY_CONFIG, TYPE_STRING },
+  { "Security", PROPERTY_INDEX_SECURITY, TYPE_STRING },
+  { "SSID", PROPERTY_INDEX_SSID, TYPE_STRING },
+  { NULL }
+};
+
+OncValueSignature eap_signature[] = {
+  { "AnonymousIdentity", PROPERTY_INDEX_EAP_ANONYMOUS_IDENTITY, TYPE_STRING },
+  { "ClientCARef", PROPERTY_INDEX_EAP_CLIENT_CERT, TYPE_STRING },
+  { "ClientCertPattern", PROPERTY_INDEX_EAP_CLIENT_CERT_PATTERN, TYPE_STRING },
+  { "Identity", PROPERTY_INDEX_EAP_IDENTITY, TYPE_STRING },
+  { "Inner", PROPERTY_INDEX_EAP_PHASE_2_AUTH, TYPE_STRING },
+  { "Outer", PROPERTY_INDEX_EAP_METHOD, TYPE_STRING },
+  { "Password", PROPERTY_INDEX_EAP_PASSWORD, TYPE_STRING },
+  { "ServerCARef", PROPERTY_INDEX_EAP_CA_CERT, TYPE_STRING },
+  { "UseSystemCAs", PROPERTY_INDEX_EAP_USE_SYSTEM_CAS, TYPE_BOOLEAN },
+  { NULL }
+};
+
+OncValueSignature vpn_signature[] = {
+  { "Host", PROPERTY_INDEX_HOST, TYPE_STRING },
+  { "IPsec", PROPERTY_INDEX_ONC_IPSEC, TYPE_DICTIONARY },
+  { "L2TP", PROPERTY_INDEX_ONC_L2TP, TYPE_DICTIONARY },
+  { "OpenVPN", PROPERTY_INDEX_ONC_OPENVPN, TYPE_DICTIONARY },
+  { "Type", PROPERTY_INDEX_PROVIDER_TYPE, TYPE_STRING },
+  { NULL }
+};
+
+OncValueSignature ipsec_signature[] = {
+  { "AuthenticationType", PROPERTY_INDEX_IPSEC_AUTHENTICATIONTYPE,
+    TYPE_STRING },
+  { "Group", PROPERTY_INDEX_L2TPIPSEC_GROUP_NAME, TYPE_STRING },
+  { "IKEVersion", PROPERTY_INDEX_IPSEC_IKEVERSION, TYPE_INTEGER },
+  // Note: ClientCertRef, ClientCertPattern, ClientCertType are
+  // purposefully avoided.
+  // Note: EAP and XAUTH not yet supported.
+  { "PSK", PROPERTY_INDEX_L2TPIPSEC_PSK, TYPE_STRING },
+  { "SaveCredentials", PROPERTY_INDEX_SAVE_CREDENTIALS, TYPE_BOOLEAN },
+  { "ServerCARef", PROPERTY_INDEX_L2TPIPSEC_CA_CERT_NSS, TYPE_STRING },
+  { NULL }
+};
+
+OncValueSignature l2tp_signature[] = {
+  { "Password", PROPERTY_INDEX_L2TPIPSEC_PASSWORD, TYPE_STRING },
+  { "SaveCredentials", PROPERTY_INDEX_SAVE_CREDENTIALS, TYPE_BOOLEAN },
+  { "Username", PROPERTY_INDEX_L2TPIPSEC_USER, TYPE_STRING },
+  { NULL }
+};
+
+OncValueSignature openvpn_signature[] = {
+  { "Auth", PROPERTY_INDEX_OPEN_VPN_AUTH, TYPE_STRING },
+  { "AuthRetry", PROPERTY_INDEX_OPEN_VPN_AUTHRETRY, TYPE_STRING },
+  { "AuthNoCache", PROPERTY_INDEX_OPEN_VPN_AUTHNOCACHE, TYPE_BOOLEAN },
+  { "Cipher", PROPERTY_INDEX_OPEN_VPN_CIPHER, TYPE_STRING },
+  // Note: ClientCertRef, ClientCertPattern, ClientCertType are
+  // purposefully avoided.
+  { "CompLZO", PROPERTY_INDEX_OPEN_VPN_COMPLZO, TYPE_STRING },
+  { "CompNoAdapt", PROPERTY_INDEX_OPEN_VPN_COMPNOADAPT, TYPE_BOOLEAN },
+  { "KeyDirection", PROPERTY_INDEX_OPEN_VPN_KEYDIRECTION, TYPE_STRING },
+  { "NsCertType", PROPERTY_INDEX_OPEN_VPN_NSCERTTYPE, TYPE_STRING },
+  { "Password", PROPERTY_INDEX_OPEN_VPN_PASSWORD, TYPE_STRING },
+  { "Port", PROPERTY_INDEX_OPEN_VPN_PORT, TYPE_INTEGER },
+  { "Proto", PROPERTY_INDEX_OPEN_VPN_PROTO, TYPE_STRING },
+  { "PushPeerInfo", PROPERTY_INDEX_OPEN_VPN_PUSHPEERINFO, TYPE_BOOLEAN },
+  { "RemoteCertEKU", PROPERTY_INDEX_OPEN_VPN_REMOTECERTEKU, TYPE_STRING },
+  // TODO(kmixter): Extract just the first item from this list before
+  // sending it down.
+  { "RemoteCertKU", PROPERTY_INDEX_OPEN_VPN_REMOTECERTKU, TYPE_LIST },
+  { "RemoteCertTLS", PROPERTY_INDEX_OPEN_VPN_REMOTECERTTLS, TYPE_STRING },
+  { "RenegSec", PROPERTY_INDEX_OPEN_VPN_RENEGSEC, TYPE_INTEGER },
+  { "SaveCredentials", PROPERTY_INDEX_SAVE_CREDENTIALS, TYPE_BOOLEAN },
+  { "ServerCARef", PROPERTY_INDEX_OPEN_VPN_CACERT, TYPE_STRING },
+  { "ServerCertRef", PROPERTY_INDEX_OPEN_VPN_CERT, TYPE_STRING },
+  { "ServerPollTimeout", PROPERTY_INDEX_OPEN_VPN_SERVERPOLLTIMEOUT,
+    TYPE_INTEGER },
+  { "Shaper", PROPERTY_INDEX_OPEN_VPN_SHAPER, TYPE_INTEGER },
+  { "StaticChallenge", PROPERTY_INDEX_OPEN_VPN_STATICCHALLENGE, TYPE_STRING },
+  { "TLSAuthContents", PROPERTY_INDEX_OPEN_VPN_TLSAUTHCONTENTS, TYPE_STRING },
+  { "TLSRemote", PROPERTY_INDEX_OPEN_VPN_TLSREMOTE, TYPE_STRING },
+  { "Username", PROPERTY_INDEX_OPEN_VPN_USER, TYPE_STRING },
+  { NULL }
+};
+
 // Serve the singleton mapper instance.
 const EnumMapper<PropertyIndex>* get_onc_mapper() {
   CR_DEFINE_STATIC_LOCAL(const EnumMapper<PropertyIndex>, mapper,
-      (property_index_table,
-       arraysize(property_index_table),
+      (network_configuration_table,
+       arraysize(network_configuration_table),
        PROPERTY_INDEX_UNKNOWN));
   return &mapper;
 }
@@ -156,6 +236,7 @@
   if (!network_configs_->GetDictionary(n, &info))
     return NULL;
   // Parse Open Network Configuration blob into a temporary Network object.
+  // TODO(crosbug.com/23604): Handle removing networks.
   return CreateNetworkFromInfo(std::string(), *info);
 }
 
@@ -171,16 +252,7 @@
   scoped_ptr<StringValue> val(Value::CreateStringValue(str));
   network->UpdatePropertyMap(PROPERTY_INDEX_TYPE, *val.get());
 
-  // Get the child dictionary with properties for the network.
-  // And copy all the values from this network type dictionary to parent.
-  DictionaryValue* dict;
-  if (!info.GetDictionary(GetTypeFromDictionary(info), &dict))
-    return NULL;
-
-  // Add GUID from the parent dictionary.
-  dict->SetString("GUID", GetGuidFromDictionary(info));
-
-  UpdateNetworkFromInfo(*dict, network.get());
+  UpdateNetworkFromInfo(info, network.get());
   VLOG(2) << "Created Network '" << network->name()
           << "' from info. Path:" << service_path
           << " Type:" << ConnectionTypeToString(type);
@@ -329,6 +401,51 @@
   return true;
 }
 
+bool OncNetworkParser::ParseNestedObject(Network* network,
+                                         const std::string& onc_type,
+                                         const base::Value& value,
+                                         OncValueSignature* signature,
+                                         ParserPointer parser) {
+  bool any_errors = false;
+  if (value.GetType() != Value::TYPE_DICTIONARY) {

[Mattias Nissler (ping if slow), 2011/12/01 13:09:10]

Shortcut: value.IsType(base::Value::TYPE_DICTIONARY)

[Mattias Nissler (ping if slow), 2011/12/01 13:09:10]

also, Value is in namespace base nowadays, so you want to make sure new code has
the correct namespace prefix here and below.

[kmixter1, 2011/12/01 19:48:41]

Done.

[kmixter1, 2011/12/01 19:48:41]

Added the prefix to be exact but somewhere else it must have already been pulled
into scope using "using", because it built without it.

+    VLOG(1) << network->name() << ": expected object of type " << onc_type;
+    return false;
+  }
+  const DictionaryValue& dict = static_cast<const DictionaryValue&>(value);
+  for (DictionaryValue::key_iterator iter = dict.begin_keys();
+       iter != dict.end_keys(); ++iter) {
+    const std::string& key = *iter;
+    base::Value* inner_value;
+    bool res = dict.GetWithoutPathExpansion(key, &inner_value);
+    if (!res) {
+      DCHECK(res);

[Mattias Nissler (ping if slow), 2011/12/01 13:09:10]

I'd just remove the if and make this a CHECK(). If it ever triggers, that means
the DictionaryValue code is severely broken and we have larger problems anyway.

[Charlie Lee, 2011/12/01 18:53:18]

Probably don't even need a CHECK(). If it ever fails, there will just be a null
pointer exception.

[kmixter1, 2011/12/01 19:48:41]

Done.

[kmixter1, 2011/12/01 19:48:41]

Done.

+      continue;
+    }
+    PropertyIndex index = PROPERTY_INDEX_UNKNOWN;
+    int field;

[Charlie Lee, 2011/12/01 18:53:18]

should probably name this something other than field to avoid confusion.

[kmixter1, 2011/12/01 19:48:41]

Done.

+    for (field = 0; signature[field].field != NULL; ++field) {

[Charlie Lee, 2011/12/01 18:53:18]

consider using a map instead of an array so it's more efficient. similar to our
property maps, it will be signature maps.

[kmixter1, 2011/12/01 19:48:41]

Yeah, I suspect what we'll want to do is construct something similar to a
"Mapper" from the tables and have multiple ones of them in the class.  Then we
can do things in the WiFi and VirtualNetworkParser initializers like create the
appropriate ONCObjectParsers for the different ONC typed objects specific to
those kinds of networks, but also add more complex validation rules like what
string values are allowed for JSON-style enums (string must be either "WEP-PSK"
or ...) and bit-fields (list of strings which each must be one of N constants). 
We can also add "required" and "optional" checks at that stage, especially those
"required if" checks (like EAP is required if Security is WPA-EAP). At that
point adding hash maps is probably the way to go.  I wanted something simple
now, and even 400 strcmps isn't very expensive when parsing the most complex
objects.

At that point we can probably make the Parse{VPN,EAP}Value functions specific to
those individual ONC object parsers.

+      if (key == signature[field].field) break;

[Mattias Nissler (ping if slow), 2011/12/01 13:09:10]

I actually missed that break and were going to complain about unnecessary
curlies, so I'd like to see the break on the next line.

[kmixter1, 2011/12/01 19:48:41]

Done.

+    }
+    if (signature[field].field != NULL) {
+      if (inner_value->GetType() != signature[field].type) {
+        VLOG(1) << network->name() << ": " << onc_type
+                << " unhandled field: " << key << " Type: "
+                << inner_value->GetType();
+      } else {
+        if (parser(this, index, *inner_value, network)) {
+          continue;
+        }
+      }
+    }

[Mattias Nissler (ping if slow), 2011/12/01 13:09:10]

I think you can just collapse these nested conditionals to:

if (signature[field].field != NULL &&
    inner_value->GetType() != signature[field].type &&
    parser(this, index, inner_value, network) {
  // success
} else {
  // report error
}

[kmixter1, 2011/12/01 19:48:41]

I preferred to give more specific error messages for the different kinds of
errors.  So I've moved things around to that end.

+    network->UpdatePropertyMap(index, *inner_value);

[Mattias Nissler (ping if slow), 2011/12/01 13:09:10]

Do we really want to update the map for unrecognized fields? 

[kmixter1, 2011/12/01 19:48:41]

No, but we do need to update it before we run the parser function because the
parser functions unfortunately want to have the last say in translating those
values.  So the new code does this.

+    VLOG(1) << network->name() << ": " << onc_type
+            << " unhandled field: " << key << " Type: "
+            << inner_value->GetType();
+    any_errors = true;
+  }
+  return !any_errors;
+}
+
 // -------------------- OncWirelessNetworkParser --------------------
 
 OncWirelessNetworkParser::OncWirelessNetworkParser() {}
@@ -344,7 +461,9 @@
 
 // -------------------- OncWifiNetworkParser --------------------
 
-OncWifiNetworkParser::OncWifiNetworkParser() {}
+OncWifiNetworkParser::OncWifiNetworkParser() {

[Charlie Lee, 2011/12/01 18:53:18]

nit: probably unnecessary

[kmixter1, 2011/12/01 19:48:41]

Done.

+}
+
 OncWifiNetworkParser::~OncWifiNetworkParser() {}
 
 bool OncWifiNetworkParser::ParseValue(PropertyIndex index,
@@ -353,25 +472,33 @@
   DCHECK_EQ(TYPE_WIFI, network->type());
   WifiNetwork* wifi_network = static_cast<WifiNetwork*>(network);
   switch (index) {
-    case PROPERTY_INDEX_SSID: {
-      std::string ssid;
-      if (!value.GetAsString(&ssid))
-        break;
-      wifi_network->SetName(ssid);
+    case PROPERTY_INDEX_ONC_WIFI: {
+      // TODO(kmixter): Purposefully ignore return value?
+      ParseNestedObject(wifi_network, "WiFi", value,
+                        wifi_signature, ParseWifiValue);
       return true;
     }
+    default:
+      return OncNetworkParser::ParseValue(index, value, network);
+  }
+}
+
+bool OncWifiNetworkParser::ParseWifiValue(OncNetworkParser* parser,
+                                          PropertyIndex index,
+                                          const base::Value& value,
+                                          Network* network) {
+  WifiNetwork* wifi_network = static_cast<WifiNetwork*>(network);
+  std::string string_value;
+  value.GetAsString(&string_value);
+  switch (index) {
+    case PROPERTY_INDEX_SSID:
+      wifi_network->SetName(string_value);
+      return true;
     case PROPERTY_INDEX_GUID: {
-      std::string unique_id;
-      if (!value.GetAsString(&unique_id))
-        break;
-      wifi_network->set_unique_id(unique_id);
+      wifi_network->set_unique_id(string_value);
       return true;
-    }
     case PROPERTY_INDEX_SECURITY: {
-      std::string security_string;
-      if (!value.GetAsString(&security_string))
-        break;
-      ConnectionSecurity security = ParseSecurity(security_string);
+      ConnectionSecurity security = ParseSecurity(string_value);
       wifi_network->set_encryption(security);
       // Also update property with native value for security.
       std::string str =
@@ -380,62 +507,49 @@
       wifi_network->UpdatePropertyMap(index, *val.get());
       return true;
     }
-    case PROPERTY_INDEX_PASSPHRASE: {
-      std::string passphrase;
-      if (!value.GetAsString(&passphrase))
-        break;
-      wifi_network->set_passphrase(passphrase);
+    case PROPERTY_INDEX_PASSPHRASE:
+      wifi_network->set_passphrase(string_value);
       return true;
-    }
-    case PROPERTY_INDEX_IDENTITY: {
-      std::string identity;
-      if (!value.GetAsString(&identity))
-        break;
-      wifi_network->set_identity(identity);
+    case PROPERTY_INDEX_IDENTITY:
+      wifi_network->set_identity(string_value);
       return true;
-    }
-    case PROPERTY_INDEX_EAP: {
-      DCHECK_EQ(value.GetType(), Value::TYPE_DICTIONARY);
-      const DictionaryValue& dict = static_cast<const DictionaryValue&>(value);
-      for (DictionaryValue::key_iterator iter = dict.begin_keys();
-           iter != dict.end_keys(); ++iter) {
-        const std::string& key = *iter;
-        base::Value* eap_value;
-        bool res = dict.GetWithoutPathExpansion(key, &eap_value);
-        DCHECK(res);
-        if (res) {
-          PropertyIndex index = mapper().Get(key);
-          wifi_network->UpdatePropertyMap(index, *eap_value);
-          if (!ParseEAPValue(index, *eap_value, wifi_network))
-            VLOG(1) << network->name() << ": EAP unhandled key: " << key
-                    << " Type: " << eap_value->GetType();
-        }
-      }
+    case PROPERTY_INDEX_EAP:
+      parser->ParseNestedObject(wifi_network,
+                                "EAP",
+                                value,
+                                eap_signature,
+                                ParseEAPValue);
       return true;
-    }
+
+    case PROPERTY_INDEX_AUTO_CONNECT:
+    case PROPERTY_INDEX_HIDDEN_SSID:
+      // Pass these through to connection manager as is.
+      return true;
+
     default:
-      return OncWirelessNetworkParser::ParseValue(index, value, network);
+      break;
   }
   return false;
 }
 
 
-bool OncWifiNetworkParser::ParseEAPValue(PropertyIndex index,
+bool OncWifiNetworkParser::ParseEAPValue(OncNetworkParser*,
+                                         PropertyIndex index,
                                          const base::Value& value,
-                                         WifiNetwork* wifi_network) {
+                                         Network* network) {
+  WifiNetwork* wifi_network = static_cast<WifiNetwork*>(network);
+  std::string string_value;
+  bool bool_value = false;
+
+  value.GetAsString(&string_value);
+  value.GetAsBoolean(&bool_value);
+
   switch (index) {
-    case PROPERTY_INDEX_EAP_IDENTITY: {
-      std::string eap_identity;
-      if (!value.GetAsString(&eap_identity))
-        break;
-      wifi_network->set_eap_identity(eap_identity);
+    case PROPERTY_INDEX_EAP_IDENTITY:
+      wifi_network->set_eap_identity(string_value);
       return true;
-    }
     case PROPERTY_INDEX_EAP_METHOD: {
-      std::string eap_method_str;
-      if (!value.GetAsString(&eap_method_str))
-        break;
-      EAPMethod eap_method = ParseEAPMethod(eap_method_str);
+      EAPMethod eap_method = ParseEAPMethod(string_value);
       wifi_network->set_eap_method(eap_method);
       // Also update property with native value for EAP method.
       std::string str =
@@ -445,10 +559,7 @@
       return true;
     }
     case PROPERTY_INDEX_EAP_PHASE_2_AUTH: {
-      std::string eap_phase_2_auth_str;
-      if (!value.GetAsString(&eap_phase_2_auth_str))
-        break;
-      EAPPhase2Auth eap_phase_2_auth = ParseEAPPhase2Auth(eap_phase_2_auth_str);
+      EAPPhase2Auth eap_phase_2_auth = ParseEAPPhase2Auth(string_value);
       wifi_network->set_eap_phase_2_auth(eap_phase_2_auth);
       // Also update property with native value for EAP phase 2 auth.
       std::string str = NativeNetworkParser::network_eap_auth_mapper()->GetKey(
@@ -457,42 +568,21 @@
       wifi_network->UpdatePropertyMap(index, *val.get());
       return true;
     }
-    case PROPERTY_INDEX_EAP_ANONYMOUS_IDENTITY: {
-      std::string eap_anonymous_identity;
-      if (!value.GetAsString(&eap_anonymous_identity))
-        break;
-      wifi_network->set_eap_anonymous_identity(eap_anonymous_identity);
+    case PROPERTY_INDEX_EAP_ANONYMOUS_IDENTITY:
+      wifi_network->set_eap_anonymous_identity(string_value);
       return true;
-    }
-    case PROPERTY_INDEX_EAP_CERT_ID: {
-      std::string eap_client_cert_pkcs11_id;
-      if (!value.GetAsString(&eap_client_cert_pkcs11_id))
-        break;
-      wifi_network->set_eap_client_cert_pkcs11_id(eap_client_cert_pkcs11_id);
+    case PROPERTY_INDEX_EAP_CERT_ID:
+      wifi_network->set_eap_client_cert_pkcs11_id(string_value);
       return true;
-    }
-    case PROPERTY_INDEX_EAP_CA_CERT_NSS: {
-      std::string eap_server_ca_cert_nss_nickname;
-      if (!value.GetAsString(&eap_server_ca_cert_nss_nickname))
-        break;
-      wifi_network->set_eap_server_ca_cert_nss_nickname(
-          eap_server_ca_cert_nss_nickname);
+    case PROPERTY_INDEX_EAP_CA_CERT_NSS:
+      wifi_network->set_eap_server_ca_cert_nss_nickname(string_value);
       return true;
-    }
-    case PROPERTY_INDEX_EAP_USE_SYSTEM_CAS: {
-      bool eap_use_system_cas;
-      if (!value.GetAsBoolean(&eap_use_system_cas))
-        break;
-      wifi_network->set_eap_use_system_cas(eap_use_system_cas);
+    case PROPERTY_INDEX_EAP_USE_SYSTEM_CAS:
+      wifi_network->set_eap_use_system_cas(bool_value);
       return true;
-    }
-    case PROPERTY_INDEX_EAP_PASSWORD: {
-      std::string eap_passphrase;
-      if (!value.GetAsString(&eap_passphrase))
-        break;
-      wifi_network->set_eap_passphrase(eap_passphrase);
+    case PROPERTY_INDEX_EAP_PASSWORD:
+      wifi_network->set_eap_passphrase(string_value);
       return true;
-    }
     default:
       break;
   }
@@ -503,9 +593,9 @@
     const std::string& security) {
   static EnumMapper<ConnectionSecurity>::Pair table[] = {
     { "None", SECURITY_NONE },
-    { "WEP", SECURITY_WEP },
-    { "WPA", SECURITY_WPA },
-    { "WPA2", SECURITY_8021X },
+    { "WEP-PSK", SECURITY_WEP },
+    { "WPA-PSK", SECURITY_WPA },
+    { "WPA-EAP", SECURITY_8021X },
   };
   CR_DEFINE_STATIC_LOCAL(EnumMapper<ConnectionSecurity>, parser,
       (table, arraysize(table), SECURITY_UNKNOWN));
@@ -568,21 +658,25 @@
   VirtualNetwork* virtual_network = static_cast<VirtualNetwork*>(network);
   switch (index) {
     case PROPERTY_INDEX_PROVIDER: {
-      DCHECK_EQ(value.GetType(), Value::TYPE_DICTIONARY);
+      // Got the "VPN" field.  Immediately store the VPN.Type field value
+      // so that we can properly validate nested objects based on the type.
+      if (value.GetType() != Value::TYPE_DICTIONARY) {

[Mattias Nissler (ping if slow), 2011/12/01 13:09:10]

there's value.IsType() for this.

[kmixter1, 2011/12/01 19:48:41]

Done.

+        VLOG(1) << network->name() << ": expected object of type VPN";
+        return false;
+      }
       const DictionaryValue& dict = static_cast<const DictionaryValue&>(value);

[Mattias Nissler (ping if slow), 2011/12/01 13:09:10]

To avoid the cast, you could replace lines 663-667 with:

const DictionaryValue* dict = NULL;
if (!value.GetAsDictionary(&dict)) {
  // error
}

[kmixter1, 2011/12/01 19:48:41]

Done.

-      for (DictionaryValue::key_iterator iter = dict.begin_keys();
-           iter != dict.end_keys(); ++iter) {
-        const std::string& key = *iter;
-        base::Value* provider_value;
-        bool res = dict.GetWithoutPathExpansion(key, &provider_value);
-        DCHECK(res);
-        if (res) {
-          PropertyIndex index = mapper().Get(key);
-          if (!ParseProviderValue(index, *provider_value, virtual_network))
-            VLOG(1) << network->name() << ": Provider unhandled key: " << key
-                    << " Type: " << provider_value->GetType();
-        }
+      Value *provider_type_value;
+      dict.GetWithoutPathExpansion("Type", &provider_type_value);
+      if (provider_type_value == NULL) {
+        VLOG(1) << network->name() << ": VPN.Type is missing";
+        return false;
       }
+      std::string provider_type_string;
+      if (!provider_type_value->GetAsString(&provider_type_string))
+        break;

[Mattias Nissler (ping if slow), 2011/12/01 13:09:10]

Lines 668-676 shorter:

std::string provider_type_string;
if (!dict.GetString("Type", &provider_type_string))
  break;

[kmixter1, 2011/12/01 19:48:41]

Done.

+      virtual_network->set_provider_type(
+          ParseProviderType(provider_type_string));
+      ParseNestedObject(network, "VPN", value, vpn_signature, ParseVPNValue);
       return true;
     }
     default:
@@ -592,75 +686,196 @@
   return false;
 }
 
-bool OncVirtualNetworkParser::ParseProviderValue(PropertyIndex index,
-                                                 const base::Value& value,
-                                                 VirtualNetwork* network) {
+bool OncVirtualNetworkParser::ParseVPNValue(OncNetworkParser* parser,
+                                            PropertyIndex index,
+                                            const base::Value& value,
+                                            Network* network) {
+  VirtualNetwork* virtual_network = static_cast<VirtualNetwork*>(network);
+  std::string string_value;
+  value.GetAsString(&string_value);
   switch (index) {
-    case PROPERTY_INDEX_HOST: {
-      std::string server_hostname;
-      if (!value.GetAsString(&server_hostname))
-        break;
-      network->set_server_hostname(server_hostname);
+    case PROPERTY_INDEX_HOST:
+      virtual_network->set_server_hostname(string_value);
       return true;
+    case PROPERTY_INDEX_ONC_IPSEC: {
+      if (virtual_network->provider_type() != PROVIDER_TYPE_L2TP_IPSEC_PSK &&
+          virtual_network->provider_type() !=
+          PROVIDER_TYPE_L2TP_IPSEC_USER_CERT) {
+        VLOG(1) << "IPsec field not allowed with this VPN type";
+        return false;
+      }
+      parser->ParseNestedObject(network,
+                                "L2TP",
+                                value,
+                                l2tp_signature,
+                                ParseL2TPValue);
+      return true;
     }
-    case PROPERTY_INDEX_NAME: {
-      std::string name;
-      if (!value.GetAsString(&name))
-        break;
-      network->set_name(name);
+    case PROPERTY_INDEX_ONC_L2TP: {
+      if (virtual_network->provider_type() != PROVIDER_TYPE_L2TP_IPSEC_PSK) {
+        VLOG(1) << "L2TP field not allowed with this VPN type";
+        return false;
+      }
+      parser->ParseNestedObject(network,
+                                "IPsec",
+                                value,
+                                ipsec_signature,
+                                ParseIPsecValue);
       return true;
     }
-    case PROPERTY_INDEX_TYPE: {
-      std::string provider_type_string;
-      if (!value.GetAsString(&provider_type_string))
-        break;
-      network->set_provider_type(ParseProviderType(provider_type_string));
+    case PROPERTY_INDEX_ONC_OPENVPN: {
+      if (network->provider_type() != PROVIDER_TYPE_L2TP_IPSEC_PSK) {
+        VLOG(1) << "OpenVPN field not allowed with this VPN type";
+        return false;
+      }
+      parser->ParseNestedObject(network,
+                                "OpenVPN",
+                                value,
+                                openvpn_signature,
+                                ParseOpenVPNValue);
       return true;
     }
-    case PROPERTY_INDEX_L2TPIPSEC_CA_CERT_NSS: {
-      std::string ca_cert_nss;
-      if (!value.GetAsString(&ca_cert_nss))
-        break;
-      network->set_ca_cert_nss(ca_cert_nss);
+    case PROPERTY_INDEX_PROVIDER_TYPE: {
+      // This was handled when we found the VPN field.
       return true;
     }
-    case PROPERTY_INDEX_L2TPIPSEC_PSK: {
-      std::string psk_passphrase;
-      if (!value.GetAsString(&psk_passphrase))
+    default:
+      break;
+  }
+  return false;
+}
+
+bool OncVirtualNetworkParser::ParseIPsecValue(OncWifiNetworkParser* parser,
+                                              PropertyIndex index,
+                                              const base::Value& value,
+                                              Network* network) {
+  VirtualNetwork* virtual_network = static_cast<VirtualNetwork*>(network);
+  std::string server_hostname;
+  bool bool_value = false;
+  value.GetAsString(&string_value);
+  value.GetAsBoolean(&bool_value);
+
+  switch (index) {
+    case PROPERTY_INDEX_IPSEC_AUTHENTICATIONTYPE:
+      network->set_provider_type(UpdateProviderTypeWithAuth(network->provider(),
+                                                            string_value));
+      return true;
+    case PROPERTY_INDEX_L2TPIPSEC_CA_CERT_NSS:
+      network->set_ca_cert_nss(string_value);
+      return true;
+    case PROPERTY_INDEX_L2TPIPSEC_PSK:
+      network->set_psk_passphrase(string_value);
+      return true;
+    case PROPERTY_INDEX_L2TPIPSEC_GROUP_NAME:
+      network->set_group_name(string_value);
+      return true;
+    case PROPERTY_INDEX_SAVE_CREDENTIALS:
+      // Note that the specification allows different settings for
+      // IPsec credentials (PSK) and L2TP credentials (username and
+      // password) but we merge them in our implementation as is required
+      // with the current connection manager.
+      network->set_save_credentials(bool_value);
+      return true;
+  }
+  return false;
+}
+
+ProviderType OncVirtualNetworkParser::UpdateProviderTypeWithAuth(
+    ProviderType provider,
+    const std::string& auth_type) {
+  switch (provider) {
+  case PROVIDER_TYPE_L2TP_IPSEC_PSK:
+  case PROVIDER_TYPE_L2TP_IPSEC_USER_CERT:
+    if (auth_type == "Cert") {
+      return PROVIDER_TYPE_L2TP_IPSEC_USER_CERT;
+    } else {
+      if (auth_type != "PSK") {
+        VLOG(1) << "Unexpected authentication type " << auth_type;
         break;
-      network->set_psk_passphrase(psk_passphrase);
-      return true;
+      }
+      return PROVIDER_TYPE_L2TP_IPSEC_PSK;
     }
-    case PROPERTY_INDEX_L2TPIPSEC_CLIENT_CERT_ID: {
-      std::string client_cert_id;
-      if (!value.GetAsString(&client_cert_id))
-        break;
-      network->set_client_cert_id(client_cert_id);
+  default:
+    VLOG(1) << "Unexpected provider type with authentication type "
+            << auth_type;
+    break;
+  }
+  return provider;
+}
+
+bool OncVirtualNetworkParser::ParseL2TPValue(OncNetworkParser*,
+                                             PropertyIndex index,
+                                             const base::Value& value,
+                                             Network* network) {
+  VirtualNetwork* virtual_network = static_cast<VirtualNetwork*>(network);
+  std::string string_value;
+  bool bool_value = false;
+  value.GetAsString(&string_value);
+  value.GetAsBoolean(&bool_value);
+  switch (index) {
+    case PROPERTY_INDEX_L2TPIPSEC_PASSWORD:
+      virtual_network->set_user_passphrase(string_value);
       return true;
-    }
     case PROPERTY_INDEX_L2TPIPSEC_USER: {
-      std::string username;
-      if (!value.GetAsString(&username))
-        break;
-      network->set_username(username);
+      virtual_network->set_username(string_value);
       return true;
-    }
-    case PROPERTY_INDEX_L2TPIPSEC_PASSWORD: {
-      std::string user_passphrase;
-      if (!value.GetAsString(&user_passphrase))
-        break;
-      network->set_user_passphrase(user_passphrase);
+    case PROPERTY_INDEX_SAVE_CREDENTIALS:
+      // Note that the specification allows different settings for
+      // IPsec credentials (PSK) and L2TP credentials (username and
+      // password) but we merge them in our implementation as is required
+      // with the current connection manager.
+      virtual_network->set_save_credentials(bool_value);
       return true;
-    }
-    case PROPERTY_INDEX_L2TPIPSEC_GROUP_NAME: {
-      std::string group_name;
-      if (!value.GetAsString(&group_name))
-        break;
-      network->set_group_name(group_name);
+  }
+  return false;
+}
+
+bool OncVirtualNetworkParser::ParseOpenVPNValue(OncNetworkParser*,
+                                                PropertyIndex index,
+                                                const base::Value& value,
+                                                Network* network) {
+  VirtualNetwork* virtual_network = static_cast<VirtualNetwork*>(network);
+  std::string string_value;
+  bool bool_value = false;
+  value.GetAsString(&string_value);
+  value.GetAsBoolean(&bool_value);
+  switch (index) {
+    case PROPERTY_INDEX_OPEN_VPN_PASSWORD:
+      virtual_network->set_user_passphrase(string_value);
       return true;
-    }
-    default:
-      break;
+    case PROPERTY_INDEX_OPEN_VPN_USER:
+      virtual_network->set_username(string_value);
+      return true;
+    case PROPERTY_INDEX_SAVE_CREDENTIALS:
+      virtual_network->set_save_credentials(bool_value);
+      return true;
+    case PROPERTY_INDEX_OPEN_VPN_CACERT:
+      virtual_network->set_ca_cert_nss(string_value);
+      return true;
+
+    case PROPERTY_INDEX_OPEN_VPN_AUTH:
+    case PROPERTY_INDEX_OPEN_VPN_AUTHRETRY:
+    case PROPERTY_INDEX_OPEN_VPN_AUTHNOCACHE:
+    case PROPERTY_INDEX_OPEN_VPN_CERT:
+    case PROPERTY_INDEX_OPEN_VPN_CIPHER:
+    case PROPERTY_INDEX_OPEN_VPN_COMPLZO:
+    case PROPERTY_INDEX_OPEN_VPN_COMPNOADAPT:
+    case PROPERTY_INDEX_OPEN_VPN_KEYDIRECTION:
+    case PROPERTY_INDEX_OPEN_VPN_NSCERTTYPE:
+    case PROPERTY_INDEX_OPEN_VPN_PORT:
+    case PROPERTY_INDEX_OPEN_VPN_PROTO:
+    case PROPERTY_INDEX_OPEN_VPN_PUSHPEERINFO:
+    case PROPERTY_INDEX_OPEN_VPN_REMOTECERTEKU:
+    case PROPERTY_INDEX_OPEN_VPN_REMOTECERTKU:
+    case PROPERTY_INDEX_OPEN_VPN_REMOTECERTTLS:
+    case PROPERTY_INDEX_OPEN_VPN_RENEGSEC:
+    case PROPERTY_INDEX_OPEN_VPN_SERVERPOLLTIMEOUT:
+    case PROPERTY_INDEX_OPEN_VPN_SHAPER:
+    case PROPERTY_INDEX_OPEN_VPN_STATICCHALLENGE:
+    case PROPERTY_INDEX_OPEN_VPN_TLSAUTHCONTENTS:
+    case PROPERTY_INDEX_OPEN_VPN_TLSREMOTE:
+      // Pass these through to connection manager as is.
+      return true;
   }
   return false;
 }
@@ -668,8 +883,10 @@
 ProviderType OncVirtualNetworkParser::ParseProviderType(
     const std::string& type) {
   static EnumMapper<ProviderType>::Pair table[] = {
-    { flimflam::kProviderL2tpIpsec, PROVIDER_TYPE_L2TP_IPSEC_PSK },
-    { flimflam::kProviderOpenVpn, PROVIDER_TYPE_OPEN_VPN },
+    // We initially map to L2TP-IPsec PSK and then fix this up based
+    // on the value of AuthenticationType.
+    { "L2TP-IPsec", PROVIDER_TYPE_L2TP_IPSEC_PSK },
+    { "OpenVPN", PROVIDER_TYPE_OPEN_VPN },
   };
   CR_DEFINE_STATIC_LOCAL(EnumMapper<ProviderType>, parser,
       (table, arraysize(table), PROVIDER_TYPE_MAX));