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)
@@ -6,6 +6,7 @@
 
 #include "base/base64.h"
 #include "base/json/json_value_serializer.h"
+#include "base/json/json_writer.h"  // for debug output only.
 #include "base/stringprintf.h"
 #include "base/values.h"
 #include "chrome/browser/chromeos/cros/native_network_constants.h"
@@ -22,34 +23,126 @@
 // 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 },
+const base::Value::Type TYPE_BOOLEAN = base::Value::TYPE_BOOLEAN;
+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;
+
+// Only used currently to keep NetworkParser superclass happy.
+EnumMapper<PropertyIndex>::Pair network_configuration_table[] = {
+  { "GUID", PROPERTY_INDEX_GUID }
 };
 
+OncValueSignature network_configuration_signature[] = {
+  // TODO(crosbug.com/23673): Support Ethernet settings.
+  { "GUID", PROPERTY_INDEX_GUID, TYPE_STRING },
+  { "Name", PROPERTY_INDEX_NAME, TYPE_STRING },
+  // TODO(crosbug.com/23674): Support ProxySettings.
+  // TODO(crosbug.com/23604): Handle removing networks.
+  { "Remove", PROPERTY_INDEX_ONC_REMOVE, TYPE_BOOLEAN },
+  { "Type", PROPERTY_INDEX_TYPE, TYPE_STRING },
+  { "WiFi", PROPERTY_INDEX_ONC_WIFI, TYPE_DICTIONARY },
+  { "VPN", PROPERTY_INDEX_ONC_VPN, TYPE_DICTIONARY }
+};
+
+OncValueSignature wifi_signature[] = {
+  { "AutoConnect", PROPERTY_INDEX_AUTO_CONNECT, TYPE_BOOLEAN },
+  { "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 },
+  { "ClientCertPattern", PROPERTY_INDEX_ONC_CLIENT_CERT_PATTERN,
+    TYPE_DICTIONARY },
+  { "ClientCertRef", PROPERTY_INDEX_ONC_CLIENT_CERT_REF, TYPE_STRING },
+  { "ClientCertType", PROPERTY_INDEX_ONC_CLIENT_CERT_TYPE, 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 },
+  { "ClientCertPattern", PROPERTY_INDEX_ONC_CLIENT_CERT_PATTERN,
+    TYPE_DICTIONARY },
+  { "ClientCertRef", PROPERTY_INDEX_ONC_CLIENT_CERT_REF, TYPE_STRING },
+  { "ClientCertType", PROPERTY_INDEX_ONC_CLIENT_CERT_TYPE, TYPE_STRING },
+  // 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 },
+  { "ClientCertPattern", PROPERTY_INDEX_ONC_CLIENT_CERT_PATTERN,
+    TYPE_DICTIONARY },
+  { "ClientCertRef", PROPERTY_INDEX_ONC_CLIENT_CERT_REF, TYPE_STRING },
+  { "ClientCertType", PROPERTY_INDEX_ONC_CLIENT_CERT_TYPE, TYPE_STRING },
+  { "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 },
+  { "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;
 }
@@ -72,6 +165,7 @@
     : NetworkParser(get_onc_mapper()),
       network_configs_(NULL),
       certificates_(NULL) {
+  VLOG(2) << __func__ << ": OncNetworkParser called on " << onc_blob;
   JSONStringValueSerializer deserializer(onc_blob);
   deserializer.set_allow_trailing_comma(true);
   scoped_ptr<base::Value> root(deserializer.Deserialize(NULL, &parse_error_));
@@ -81,8 +175,13 @@
   } else {
     root_dict_.reset(static_cast<DictionaryValue*>(root.release()));
     // At least one of NetworkConfigurations or Certificates is required.
-    if (!root_dict_->GetList("NetworkConfigurations", &network_configs_) &&
-        !root_dict_->GetList("Certificates", &certificates_)) {
+    bool has_network_configurations =
+      root_dict_->GetList("NetworkConfigurations", &network_configs_);
+    bool has_certificates =
+      root_dict_->GetList("Certificates", &certificates_);
+    VLOG(2) << "ONC file has " << GetNetworkConfigsSize() << " networks and "
+            << GetCertificatesSize() << " certificates";
+    if (!has_network_configurations || !has_certificates) {
       LOG(WARNING) << "ONC file has no NetworkConfigurations or Certificates.";
     }
   }
@@ -118,6 +217,14 @@
   certificates_->GetDictionary(cert_index, &certificate);
   CHECK(certificate);
 
+  if (VLOG_IS_ON(2)) {
+    std::string certificate_json;
+    base::JSONWriter::Write(static_cast<base::Value*>(certificate),
+                            true, &certificate_json);
+    VLOG(2) << "Parsing certificate at index " << cert_index
+            << ": " << certificate_json;
+  }
+
   // Get out the attributes of the given cert.
   std::string guid;
   bool remove = false;
@@ -155,7 +262,14 @@
   DictionaryValue* info = NULL;
   if (!network_configs_->GetDictionary(n, &info))
     return NULL;
-  // Parse Open Network Configuration blob into a temporary Network object.
+  if (VLOG_IS_ON(2)) {
+    std::string network_json;
+    base::JSONWriter::Write(static_cast<base::Value*>(info),
+                            true, &network_json);
+    VLOG(2) << "Parsing network at index " << n
+            << ": " << network_json;
+  }
+
   return CreateNetworkFromInfo(std::string(), *info);
 }
 
@@ -166,24 +280,19 @@
   if (type == TYPE_UNKNOWN)  // Return NULL if cannot parse network type.
     return NULL;
   scoped_ptr<Network> network(CreateNewNetwork(type, service_path));
-  // Update property with native value for type.
-  std::string str = NativeNetworkParser::network_type_mapper()->GetKey(type);
-  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))
+  if (!ParseNestedObject(network.get(),
+                         "NetworkConfiguration",
+                         static_cast<const base::Value&>(info),
+                         network_configuration_signature,
+                         ParseNetworkConfigurationValue)) {
+    LOG(WARNING) << "Network " << network->name() << " had problems.";

[stevenjb, 2011/12/05 18:38:18]

Maybe "failed to parse nested object" instead of "had problems"?

[kmixter1, 2011/12/05 20:34:43]

I get caught up on the "parse" verb used here and throughout this code because I
think of parsing as turning text into an internal representation, but we're
really partially translating it at the same time we're doing that.  So I'd think
of type-mismatch or unknown field as a parse error, but the ones that happen in
ParseNestedObject as translation errors.  It's a nit, I admit, so I'll just
change it to be consistent with the rest and say parse.

     return NULL;
-
-  // Add GUID from the parent dictionary.
-  dict->SetString("GUID", GetGuidFromDictionary(info));
-
-  UpdateNetworkFromInfo(*dict, network.get());
-  VLOG(2) << "Created Network '" << network->name()
-          << "' from info. Path:" << service_path
-          << " Type:" << ConnectionTypeToString(type);
+  }
+  if (VLOG_IS_ON(2)) {
+    VLOG(2) << "Created Network '" << network->name()
+            << "' from info. Path:" << service_path
+            << " Type:" << ConnectionTypeToString(type);
+  }
   return network.release();
 }
 
@@ -294,6 +403,8 @@
                  << " certificate at index " << cert_index;
     return false;
   }
+  VLOG(2) << "Successfully imported server/ca certificate at index "
+          << cert_index;
   return true;
 }
 
@@ -326,52 +437,178 @@
                  << " (error " << net::ErrorToString(result) << ").";
     return false;
   }
+  VLOG(2) << "Successfully imported client certificate at index "
+          << cert_index;
   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.IsType(base::Value::TYPE_DICTIONARY)) {
+    VLOG(1) << network->name() << ": expected object of type " << onc_type;
+    return false;
+  }
+  VLOG(2) << "Parsing nested object of type " << onc_type;
+  const DictionaryValue* dict = NULL;
+  value.GetAsDictionary(&dict);
+  for (DictionaryValue::key_iterator iter = dict->begin_keys();
+       iter != dict->end_keys(); ++iter) {
+    const std::string& key = *iter;
+    base::Value* inner_value = NULL;
+    dict->GetWithoutPathExpansion(key, &inner_value);
+    int field_index;
+    for (field_index = 0; signature[field_index].field != NULL; ++field_index) {
+      if (key == signature[field_index].field)
+        break;
+    }
+    if (signature[field_index].field == NULL) {
+      VLOG(1) << network->name() << ": unexpected field: "
+              << key << ", in type: " << onc_type;
+      any_errors = true;
+      continue;
+    }
+    if (!inner_value->IsType(signature[field_index].type)) {
+      VLOG(1) << network->name() << ": field with wrong type: " << key
+              << ", actual type: " << inner_value->GetType()
+              << ", expected type: " << signature[field_index].type;
+      any_errors = true;
+      continue;
+    }
+    PropertyIndex index = signature[field_index].index;
+    // We need to UpdatePropertyMap now since parser might want to
+    // change the mapped value.
+    network->UpdatePropertyMap(index, *inner_value);
+    if (!parser(this, index, *inner_value, network)) {
+      VLOG(1) << network->name() << ": problem(s) with field: " << key;
+      any_errors = true;
+      continue;
+    }
+    if (VLOG_IS_ON(2)) {
+      std::string value_json;
+      base::JSONWriter::Write(inner_value, true, &value_json);
+      VLOG(2) << network->name() << ": Successfully parsed [" << key
+              << "(" << index << ")] = " << value_json;
+    }
+  }
+  return !any_errors;
+}
+
+bool OncNetworkParser::CheckNetworkType(Network* network,
+                                        ConnectionType expected,
+                                        const std::string& onc_type) {
+  if (expected != network->type()) {

[stevenjb, 2011/12/05 18:38:18]

Since this is unexpected, we should really log a warning here, not just a vlog.

[kmixter1, 2011/12/05 20:34:43]

Done.

+    VLOG(2) << network->name()
+            << ": " << onc_type << " field unexpected for this type network";
+    return false;
+  }
+  return true;
+}
+

[Mattias Nissler (ping if slow), 2011/12/05 09:33:32]

we usually put "// static" comments in the implementation in the line before the
implementation of a static member function (also below).

[kmixter1, 2011/12/05 20:34:43]

Done.

+std::string OncNetworkParser::GetStringValue(const base::Value& value) {
+  std::string string_value;
+  value.GetAsString(&string_value);
+  return string_value;
+}
+
+const bool OncNetworkParser::GetBooleanValue(const base::Value& value) {
+  bool bool_value = false;
+  value.GetAsBoolean(&bool_value);
+  return bool_value;
+}
+
+std::string OncNetworkParser::ConvertValueToString(
+    const base::Value& value) {
+  std::string value_json;
+  base::JSONWriter::Write(&value, false, &value_json);
+  return value_json;
+}
+
+bool OncNetworkParser::ParseNetworkConfigurationValue(
+    OncNetworkParser* parser,
+    PropertyIndex index,
+    const base::Value& value,
+    Network* network) {
+  switch (index) {
+    case PROPERTY_INDEX_ONC_WIFI: {
+      return parser->ParseNestedObject(network,
+                                       "WiFi",
+                                       value,
+                                       wifi_signature,
+                                       OncWifiNetworkParser::ParseWifiValue);
+    }
+    case PROPERTY_INDEX_ONC_VPN: {
+      if (!CheckNetworkType(network, TYPE_VPN, "VPN"))
+        return false;
+      VirtualNetwork* virtual_network = static_cast<VirtualNetwork*>(network);
+      // Got the "VPN" field.  Immediately store the VPN.Type field
+      // value so that we can properly validate fields in the VPN
+      // object based on the type.
+      const DictionaryValue* dict = NULL;
+      CHECK(value.GetAsDictionary(&dict));
+      std::string provider_type_string;
+      if (!dict->GetString("Type", &provider_type_string)) {
+        VLOG(1) << network->name() << ": VPN.Type is missing";
+        return false;
+      }
+      ProviderType provider_type =
+        OncVirtualNetworkParser::ParseProviderType(provider_type_string);
+      virtual_network->set_provider_type(provider_type);
+      return parser->ParseNestedObject(network,
+                                       "VPN",
+                                       value,
+                                       vpn_signature,
+                                       OncVirtualNetworkParser::ParseVPNValue);
+      return true;
+    }
+    case PROPERTY_INDEX_ONC_REMOVE:
+      VLOG(1) << network->name() << ": Remove field not yet implemented";
+      return false;
+    case PROPERTY_INDEX_TYPE: {
+      // Update property with native value for type.
+      std::string str =
+        NativeNetworkParser::network_type_mapper()->GetKey(network->type());
+      scoped_ptr<StringValue> val(Value::CreateStringValue(str));
+      network->UpdatePropertyMap(PROPERTY_INDEX_TYPE, *val.get());
+      return true;
+    }
+    case PROPERTY_INDEX_GUID:
+    case PROPERTY_INDEX_NAME:
+      // Fall back to generic parser for these.
+      return parser->ParseValue(index, value, network);
+    default:
+      break;
+  }
+  return false;
+}
+
 // -------------------- OncWirelessNetworkParser --------------------
 
 OncWirelessNetworkParser::OncWirelessNetworkParser() {}
 OncWirelessNetworkParser::~OncWirelessNetworkParser() {}
 
-bool OncWirelessNetworkParser::ParseValue(PropertyIndex index,
-                                          const base::Value& value,
-                                          Network* network) {
-  DCHECK_NE(TYPE_ETHERNET, network->type());
-  DCHECK_NE(TYPE_VPN, network->type());
-  return OncNetworkParser::ParseValue(index, value, network);
-}
-
 // -------------------- OncWifiNetworkParser --------------------
 
 OncWifiNetworkParser::OncWifiNetworkParser() {}
+
 OncWifiNetworkParser::~OncWifiNetworkParser() {}
 
-bool OncWifiNetworkParser::ParseValue(PropertyIndex index,
-                                      const base::Value& value,
-                                      Network* network) {
-  DCHECK_EQ(TYPE_WIFI, network->type());
+bool OncWifiNetworkParser::ParseWifiValue(OncNetworkParser* parser,
+                                          PropertyIndex index,
+                                          const base::Value& value,
+                                          Network* network) {
+  if (!CheckNetworkType(network, TYPE_WIFI, "WiFi"))
+    return false;
   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_SSID:
+      wifi_network->SetName(GetStringValue(value));
       return true;
-    }
-    case PROPERTY_INDEX_GUID: {
-      std::string unique_id;
-      if (!value.GetAsString(&unique_id))
-        break;
-      wifi_network->set_unique_id(unique_id);
-      return true;
-    }
     case PROPERTY_INDEX_SECURITY: {
-      std::string security_string;
-      if (!value.GetAsString(&security_string))
-        break;
-      ConnectionSecurity security = ParseSecurity(security_string);
+      ConnectionSecurity security = ParseSecurity(GetStringValue(value));
       wifi_network->set_encryption(security);
       // Also update property with native value for security.
       std::string str =
@@ -380,62 +617,44 @@
       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(GetStringValue(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(GetStringValue(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:
+      network->set_auto_connect(GetBooleanValue(value));
+      return true;
+    case PROPERTY_INDEX_HIDDEN_SSID:
+      // Pass this 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) {
+  if (!CheckNetworkType(network, TYPE_WIFI, "EAP"))
+    return false;
+  WifiNetwork* wifi_network = static_cast<WifiNetwork*>(network);
   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(GetStringValue(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(GetStringValue(value));
       wifi_network->set_eap_method(eap_method);
       // Also update property with native value for EAP method.
       std::string str =
@@ -445,10 +664,8 @@
       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(
+          GetStringValue(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 +674,27 @@
       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(GetStringValue(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(GetStringValue(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(GetStringValue(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(GetBooleanValue(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(GetStringValue(value));
       return true;
-    }
+    case PROPERTY_INDEX_ONC_CLIENT_CERT_PATTERN:
+    case PROPERTY_INDEX_ONC_CLIENT_CERT_REF:
+    case PROPERTY_INDEX_ONC_CLIENT_CERT_TYPE:
+      // TODO(crosbug.com/19409): Support certificate patterns.
+      // Ignore for now.
+      return true;
     default:
       break;
   }
@@ -503,9 +705,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));
@@ -554,111 +756,247 @@
           << "': Server: " << virtual_network->server_hostname()
           << " Type: "
           << ProviderTypeToString(virtual_network->provider_type());
-  if (virtual_network->provider_type() == PROVIDER_TYPE_L2TP_IPSEC_PSK) {
-    if (!virtual_network->client_cert_id().empty())
-      virtual_network->set_provider_type(PROVIDER_TYPE_L2TP_IPSEC_USER_CERT);
-  }
   return true;
 }
 
-bool OncVirtualNetworkParser::ParseValue(PropertyIndex index,
-                                         const base::Value& value,
-                                         Network* network) {
-  DCHECK_EQ(TYPE_VPN, network->type());
+bool OncVirtualNetworkParser::ParseVPNValue(OncNetworkParser* parser,
+                                            PropertyIndex index,
+                                            const base::Value& value,
+                                            Network* network) {
+  if (!CheckNetworkType(network, TYPE_VPN, "VPN"))
+    return false;
   VirtualNetwork* virtual_network = static_cast<VirtualNetwork*>(network);
   switch (index) {
-    case PROPERTY_INDEX_PROVIDER: {
-      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* 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();
-        }
+    case PROPERTY_INDEX_HOST: {
+      virtual_network->set_server_hostname(GetStringValue(value));
+      // Flimflam requires a domain property which is unused.
+      network->UpdatePropertyMap(PROPERTY_INDEX_VPN_DOMAIN,
+                                 base::StringValue(""));
+      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;
       }
+      return parser->ParseNestedObject(network,
+                                       "IPsec",
+                                       value,
+                                       ipsec_signature,
+                                       ParseIPsecValue);
+    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;
+      }
+      return parser->ParseNestedObject(network,
+                                       "L2TP",
+                                       value,
+                                       l2tp_signature,
+                                       ParseL2TPValue);
+    case PROPERTY_INDEX_ONC_OPENVPN:
+      if (virtual_network->provider_type() != PROVIDER_TYPE_OPEN_VPN) {
+        VLOG(1) << "OpenVPN field not allowed with this VPN type";
+        return false;
+      }
+      return parser->ParseNestedObject(network,
+                                       "OpenVPN",
+                                       value,
+                                       openvpn_signature,
+                                       ParseOpenVPNValue);
+    case PROPERTY_INDEX_PROVIDER_TYPE: {
+      // Update property with native value for provider type.
+      ProviderType provider_type = GetCanonicalProviderType(
+          virtual_network->provider_type());
+      std::string str =
+        NativeVirtualNetworkParser::provider_type_mapper()->GetKey(
+            provider_type);
+      scoped_ptr<StringValue> val(Value::CreateStringValue(str));
+      network->UpdatePropertyMap(PROPERTY_INDEX_PROVIDER_TYPE, *val.get());
       return true;
     }
     default:
-      return OncNetworkParser::ParseValue(index, value, network);
       break;
   }
   return false;
 }
 
-bool OncVirtualNetworkParser::ParseProviderValue(PropertyIndex index,
-                                                 const base::Value& value,
-                                                 VirtualNetwork* network) {
+bool OncVirtualNetworkParser::ParseIPsecValue(OncNetworkParser* parser,
+                                              PropertyIndex index,
+                                              const base::Value& value,
+                                              Network* network) {
+  if (!CheckNetworkType(network, TYPE_VPN, "IPsec"))
+    return false;
+  VirtualNetwork* virtual_network = static_cast<VirtualNetwork*>(network);
   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_IPSEC_AUTHENTICATIONTYPE:
+      virtual_network->set_provider_type(
+          UpdateProviderTypeWithAuthType(virtual_network->provider_type(),
+                                         GetStringValue(value)));
       return true;
-    }
-    case PROPERTY_INDEX_NAME: {
-      std::string name;
-      if (!value.GetAsString(&name))
-        break;
-      network->set_name(name);
+    case PROPERTY_INDEX_L2TPIPSEC_CA_CERT_NSS:
+      virtual_network->set_ca_cert_nss(GetStringValue(value));
       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_L2TPIPSEC_PSK:
+      virtual_network->set_psk_passphrase(GetStringValue(value));
       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_L2TPIPSEC_GROUP_NAME:
+      virtual_network->set_group_name(GetStringValue(value));
       return true;
-    }
-    case PROPERTY_INDEX_L2TPIPSEC_PSK: {
-      std::string psk_passphrase;
-      if (!value.GetAsString(&psk_passphrase))
-        break;
-      network->set_psk_passphrase(psk_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(GetBooleanValue(value));
       return true;
-    }
-    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);
+    case PROPERTY_INDEX_ONC_CLIENT_CERT_PATTERN:
+    case PROPERTY_INDEX_ONC_CLIENT_CERT_REF:
+    case PROPERTY_INDEX_ONC_CLIENT_CERT_TYPE:
+      // TODO(crosbug.com/19409): Support certificate patterns.
+      // Ignore for now.
       return true;
-    }
-    case PROPERTY_INDEX_L2TPIPSEC_USER: {
-      std::string username;
-      if (!value.GetAsString(&username))
+    case PROPERTY_INDEX_IPSEC_IKEVERSION:
+      if (!value.IsType(TYPE_STRING)) {
+        // Flimflam wants all provider properties to be strings.
+        virtual_network->UpdatePropertyMap(
+            index,
+            StringValue(ConvertValueToString(value)));
+      }
+      return true;
+    default:
+      break;
+  }
+  return false;
+}
+
+ProviderType OncVirtualNetworkParser::UpdateProviderTypeWithAuthType(
+    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_username(username);
-      return true;
+      }
+      return PROVIDER_TYPE_L2TP_IPSEC_PSK;
     }
-    case PROPERTY_INDEX_L2TPIPSEC_PASSWORD: {
-      std::string user_passphrase;
-      if (!value.GetAsString(&user_passphrase))
-        break;
-      network->set_user_passphrase(user_passphrase);
+  default:
+    VLOG(1) << "Unexpected provider type with authentication type "
+            << auth_type;
+    break;
+  }
+  return provider;
+}
+
+ProviderType OncVirtualNetworkParser::GetCanonicalProviderType(
+    ProviderType provider_type) {
+  if (provider_type == PROVIDER_TYPE_L2TP_IPSEC_USER_CERT)
+    return PROVIDER_TYPE_L2TP_IPSEC_PSK;
+  return provider_type;
+}
+
+bool OncVirtualNetworkParser::ParseL2TPValue(OncNetworkParser*,
+                                             PropertyIndex index,
+                                             const base::Value& value,
+                                             Network* network) {
+  if (!CheckNetworkType(network, TYPE_VPN, "L2TP"))
+    return false;
+  VirtualNetwork* virtual_network = static_cast<VirtualNetwork*>(network);
+  switch (index) {
+    case PROPERTY_INDEX_L2TPIPSEC_PASSWORD:
+      virtual_network->set_user_passphrase(GetStringValue(value));
       return true;
+    case PROPERTY_INDEX_L2TPIPSEC_USER:
+      virtual_network->set_username(GetStringValue(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.
+      virtual_network->set_save_credentials(GetBooleanValue(value));
+      return true;
+    default:
+      break;
+  }
+  return false;
+}
+
+bool OncVirtualNetworkParser::ParseOpenVPNValue(OncNetworkParser*,
+                                                PropertyIndex index,
+                                                const base::Value& value,
+                                                Network* network) {
+  if (!CheckNetworkType(network, TYPE_VPN, "OpenVPN"))
+    return false;
+  VirtualNetwork* virtual_network = static_cast<VirtualNetwork*>(network);
+  switch (index) {
+    case PROPERTY_INDEX_OPEN_VPN_PASSWORD:
+      virtual_network->set_user_passphrase(GetStringValue(value));
+      return true;
+    case PROPERTY_INDEX_OPEN_VPN_USER:
+      virtual_network->set_username(GetStringValue(value));
+      return true;
+    case PROPERTY_INDEX_SAVE_CREDENTIALS:
+      virtual_network->set_save_credentials(GetBooleanValue(value));
+      return true;
+    case PROPERTY_INDEX_OPEN_VPN_CACERT:
+      virtual_network->set_ca_cert_nss(GetStringValue(value));
+      return true;
+    case PROPERTY_INDEX_OPEN_VPN_REMOTECERTKU: {
+      // ONC supports a list of these, but we flimflam supports only one
+      // today.  So extract the first.
+      const base::ListValue* value_list = NULL;
+      value.GetAsList(&value_list);
+      base::Value* first_item = NULL;
+      if (!value_list->Get(0, &first_item) ||
+          !first_item->IsType(base::Value::TYPE_STRING)) {
+        VLOG(1) << "RemoteCertKU must be non-empty list of strings";
+        return false;
+      }
+      virtual_network->UpdatePropertyMap(index, *first_item);
+      return true;
     }
-    case PROPERTY_INDEX_L2TPIPSEC_GROUP_NAME: {
-      std::string group_name;
-      if (!value.GetAsString(&group_name))
-        break;
-      network->set_group_name(group_name);
+    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_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:
+      if (!value.IsType(TYPE_STRING)) {
+        // Flimflam wants all provider properties to be strings.
+        virtual_network->UpdatePropertyMap(
+            index,
+            StringValue(ConvertValueToString(value)));
+      }
       return true;
-    }
+    case PROPERTY_INDEX_ONC_CLIENT_CERT_PATTERN:
+    case PROPERTY_INDEX_ONC_CLIENT_CERT_REF:
+    case PROPERTY_INDEX_ONC_CLIENT_CERT_TYPE:
+      // TODO(crosbug.com/19409): Support certificate patterns.
+      // Ignore for now.
+      return true;
+
     default:
       break;
   }
@@ -668,8 +1006,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));