This applies GUIDs to certificate and key nicknames when

chrome/browser/chromeos/cros/onc_network_parser.cc

 // Copyright (c) 2011 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 "chrome/browser/chromeos/cros/onc_network_parser.h"
 
 #include "base/base64.h"
 #include "base/json/json_value_serializer.h"
 #include "base/stringprintf.h"
 #include "base/values.h"
 #include "chrome/browser/chromeos/cros/native_network_constants.h"
 #include "chrome/browser/chromeos/cros/native_network_parser.h"
 #include "chrome/browser/chromeos/cros/network_library.h"
 #include "net/base/cert_database.h"
 #include "net/base/crypto_module.h"
 #include "net/base/net_errors.h"
 #include "net/base/x509_certificate.h"
+#include "net/base/x509_util_nss.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 namespace chromeos {
 
 // Local constants.
 namespace {
 
 EnumMapper<PropertyIndex>::Pair property_index_table[] = {
     { "GUID", PROPERTY_INDEX_GUID },
     { "Name", PROPERTY_INDEX_NAME },
@@ skipping 28 matching lines @@
 
 ConnectionType ParseNetworkType(const std::string& type) {
   static EnumMapper<ConnectionType>::Pair table[] = {
     { "WiFi", TYPE_WIFI },
     { "VPN", TYPE_VPN },
   };
   CR_DEFINE_STATIC_LOCAL(EnumMapper<ConnectionType>, parser,
       (table, arraysize(table), TYPE_UNKNOWN));
   return parser.Get(type);
 }
-

[wtc, 2011/12/08 00:07:43]

Nit: you should keep this blank line to match the blank line
on line 24 above.

[Greg Spencer (Chromium), 2011/12/09 18:51:38]

Done.

 }  // namespace
 
 // -------------------- OncNetworkParser --------------------
 
 OncNetworkParser::OncNetworkParser(const std::string& onc_blob)
     : NetworkParser(get_onc_mapper()),
       network_configs_(NULL),
       certificates_(NULL) {
   JSONStringValueSerializer deserializer(onc_blob);
   deserializer.set_allow_trailing_comma(true);
@@ skipping 26 matching lines @@
 }
 
 int OncNetworkParser::GetNetworkConfigsSize() const {
   return network_configs_ ? network_configs_->GetSize() : 0;
 }
 
 int OncNetworkParser::GetCertificatesSize() const {
   return certificates_ ? certificates_->GetSize() : 0;
 }
 
-bool OncNetworkParser::ParseCertificate(int cert_index) {
+scoped_refptr<net::X509Certificate> OncNetworkParser::ParseCertificate(
+    int cert_index) {
   CHECK(certificates_);
   CHECK(static_cast<size_t>(cert_index) < certificates_->GetSize());
   CHECK(cert_index >= 0);
   base::DictionaryValue* certificate = NULL;
   certificates_->GetDictionary(cert_index, &certificate);
   CHECK(certificate);
 
-  // Get out the attributes of the given cert.
+  // Get out the attributes of the given certificate.
   std::string guid;
   bool remove = false;
   if (!certificate->GetString("GUID", &guid) || guid.empty()) {
     LOG(WARNING) << "ONC File: certificate missing identifier at index"
                  << cert_index;
-    return false;
+    return NULL;
   }
 
   if (!certificate->GetBoolean("Remove", &remove))
     remove = false;
 
   net::CertDatabase cert_database;
-  if (remove)
-    return cert_database.DeleteCertAndKeyByLabel(guid);
+  if (remove) {
+    bool success = DeleteCertAndKeyByLabel(guid);
+    DCHECK(success);
+    // TODO(gspencer): return removed certificate?
+    return NULL;
+  }
 
-  // Not removing, so let's get the data we need to add this cert.
+  // Not removing, so let's get the data we need to add this certificate.
   std::string cert_type;
   certificate->GetString("Type", &cert_type);
   if (cert_type == "Server" || cert_type == "Authority") {
-    return ParseServerOrCaCertificate(cert_index, cert_type, certificate);
+    return ParseServerOrCaCertificate(cert_index, cert_type, guid, certificate);
   }
   if (cert_type == "Client") {
-    return ParseClientCertificate(cert_index, certificate);
+    return ParseClientCertificate(cert_index, guid, certificate);
   }
 
   LOG(WARNING) << "ONC File: certificate of unknown type: " << cert_type
                << " at index " << cert_index;
-  return false;
+  return NULL;
 }
 
 Network* OncNetworkParser::ParseNetwork(int n) {
   if (!network_configs_)
     return NULL;
   DictionaryValue* info = NULL;
   if (!network_configs_->GetDictionary(n, &info))
     return NULL;
   // Parse Open Network Configuration blob into a temporary Network object.
   return CreateNetworkFromInfo(std::string(), *info);
@@ skipping 55 matching lines @@
   return type_string;
 }
 
 std::string OncNetworkParser::GetGuidFromDictionary(
     const base::DictionaryValue& info) {
   std::string guid_string;
   info.GetString("GUID", &guid_string);
   return guid_string;
 }
 
-bool OncNetworkParser::ParseServerOrCaCertificate(
+scoped_refptr<net::X509Certificate>
+OncNetworkParser::ParseServerOrCaCertificate(
     int cert_index,
     const std::string& cert_type,
+    const std::string& guid,
     base::DictionaryValue* certificate) {
   net::CertDatabase cert_database;
   bool web_trust = false;
   base::ListValue* trust_list = NULL;
   if (certificate->GetList("Trust", &trust_list)) {
     for (size_t i = 0; i < trust_list->GetSize(); ++i) {
       std::string trust_type;
       if (!trust_list->GetString(i, &trust_type)) {
         LOG(WARNING) << "ONC File: certificate trust is invalid at index "
                      << cert_index;
-        return false;
+        return NULL;
       }
       if (trust_type == "Web") {
         web_trust = true;
       } else {
         LOG(WARNING) << "ONC File: certificate contains unknown "
                      << "trust type: " << trust_type
                      << " at index " << cert_index;
-        return false;
+        return NULL;
       }
     }
   }
 
   std::string x509_data;
   if (!certificate->GetString("X509", &x509_data) || x509_data.empty()) {
     LOG(WARNING) << "ONC File: certificate missing appropriate "
                  << "certificate data for type: " << cert_type
                  << " at index " << cert_index;
-    return false;
+    return NULL;
   }
 
   std::string decoded_x509;
   if (!base::Base64Decode(x509_data, &decoded_x509)) {
     LOG(WARNING) << "Unable to base64 decode X509 data: \""
                  << x509_data << "\".";
-    return false;
+    return NULL;
   }
 
-  scoped_refptr<net::X509Certificate> x509_cert(
-      net::X509Certificate::CreateFromBytes(decoded_x509.c_str(),
-                                            decoded_x509.size()));
+  scoped_refptr<net::X509Certificate> x509_cert =
+      net::X509Certificate::CreateFromBytesWithNickname(
+          decoded_x509.c_str(),
+          decoded_x509.size(),
+          guid.c_str());
   if (!x509_cert.get()) {
     LOG(WARNING) << "Unable to create X509 certificate from bytes.";
-    return false;
+    return NULL;
   }
+
+  if (!net::x509_util::SetLabel(x509_cert, guid))
+    return NULL;

[wtc, 2011/12/08 00:07:43]

IMPORTANT: why do you set the label on x509_cert when you
already pass the label to
net::X509Certificate::CreateFromBytesWithNickname() above?

[Greg Spencer (Chromium), 2011/12/09 18:51:38]

I've removed the Label setting code, and I'm now just relying on the nickname
passed in to the creation routines.

+
   net::CertificateList cert_list;
   cert_list.push_back(x509_cert);
   net::CertDatabase::ImportCertFailureList failures;
   bool success = false;
   if (cert_type == "Server") {
     success = cert_database.ImportServerCert(cert_list, &failures);
   } else { // Authority cert
     net::CertDatabase::TrustBits trust = web_trust ?
                                          net::CertDatabase::TRUSTED_SSL :
                                          net::CertDatabase::UNTRUSTED;
     success = cert_database.ImportCACerts(cert_list, trust, &failures);
   }
   if (!failures.empty()) {
     LOG(WARNING) << "ONC File: Error ("
                  << net::ErrorToString(failures[0].net_error)
                  << ") importing " << cert_type << " certificate at index "
                  << cert_index;
-    return false;
+    return NULL;
   }
   if (!success) {
     LOG(WARNING) << "ONC File: Unknown error importing " << cert_type
                  << " certificate at index " << cert_index;
-    return false;
+    return NULL;
   }
-  return true;
+
+  // Have to set the label again, because PKCS#11 seems to want to set
+  // it to token:nickname.  We have to set it the first time so that
+  // it gets properly imported into the low-level token inside of
+  // ImportCACerts or ImportServerCert.
+  if (!net::x509_util::SetLabel(x509_cert, guid))

[wtc, 2011/12/08 00:07:43]

IMPORTANT: this is a sign that we don't fully understand what's
going on.  I don't understand why we have to set the label
three times (once at certificate creation and twice afterwards).

I believe the "token:" prefix is added by NSS, and the
underlying CKA_LABEL attribute does NOT have "token:".

+    return NULL;
+
+  return x509_cert;
 }
 
-bool OncNetworkParser::ParseClientCertificate(
+scoped_refptr<net::X509Certificate> OncNetworkParser::ParseClientCertificate(
     int cert_index,
+    const std::string& guid,
     base::DictionaryValue* certificate) {
   net::CertDatabase cert_database;
   std::string pkcs12_data;
   if (!certificate->GetString("PKCS12", &pkcs12_data) ||
       pkcs12_data.empty()) {
     LOG(WARNING) << "ONC File: PKCS12 data is missing for Client "
                  << "certificate at index " << cert_index;
-    return false;
+    return NULL;
   }
 
   std::string decoded_pkcs12;
   if (!base::Base64Decode(pkcs12_data, &decoded_pkcs12)) {
     LOG(WARNING) << "Unable to base64 decode PKCS#12 data: \""
                  << pkcs12_data << "\".";
-    return false;
+    return NULL;
   }
 
   // Since this has a private key, always use the private module.
   scoped_refptr<net::CryptoModule> module(cert_database.GetPrivateModule());
+  net::CertificateList imported_certs;
   int result = cert_database.ImportFromPKCS12(
-      module.get(), decoded_pkcs12, string16(), false);
+      module.get(), decoded_pkcs12, string16(), false, &imported_certs);
   if (result != net::OK) {
     LOG(WARNING) << "ONC File: Unable to import Client certificate at index "
                  << cert_index
                  << " (error " << net::ErrorToString(result) << ").";
-    return false;
+    return NULL;
   }
-  return true;
+
+  if (imported_certs.size() == 0ul) {

[wtc, 2011/12/08 00:07:43]

Nit: you should be able to use 0 and 1 without the ul suffix
here.

[Greg Spencer (Chromium), 2011/12/09 18:51:38]

Yes, I know that, but for some reason I was getting an error here at one point
during my process (or I wouldn't have added the "ul" in the first place), but
now that seems to not be the case anymore.

[wtc, 2011/12/10 00:58:43]

I suspect you may have used a DCHECK_EQ here before.
DCHECKS requires the "ul" suffix for disambiguation.

+    LOG(WARNING) << "ONC File: PKCS12 data contains no importable certificates"
+        << " at index " << cert_index;
+    return NULL;
+  }
+
+  if (imported_certs.size() != 1ul) {
+    LOG(WARNING) << "ONC File: PKCS12 data at index " << cert_index
+        << " contains more than one certificate.  Only the first one will"
+        << " be imported.";
+  }
+
+  scoped_refptr<net::X509Certificate> cert_result = imported_certs[0];
+  if (!net::x509_util::SetLabel(cert_result.get(), guid))

[wtc, 2011/12/08 00:07:43]

IMPORTANT: you can specify the certificate "friendly name"
when you generate the PKCS #12 file.  I hope (but haven't verified)
NSS honors the certificate friendly name in a PKCS #12 file.

[Greg Spencer (Chromium), 2011/12/09 18:51:38]

That may be true, but I don't have control over the certificate creation itself.
 The user will import their own pre-generated certificates into the Spigots tool
(the network configuration tool that generates the ONC files), so I don't have
the chance to set it.

+    return NULL;
+
+  return cert_result;
 }
 
+// static
+bool OncNetworkParser::DeleteCertAndKeyByLabel(const std::string& label) {
+  net::CertificateList cert_list;
+  ListCertsWithLabel(label, &cert_list);
+  net::CertDatabase cert_db;
+  bool result = true;
+  for (net::CertificateList::iterator iter = cert_list.begin();
+       iter != cert_list.end(); ++iter) {
+    // If we fail, we try and delete the rest still.
+    // TODO(gspencer): this isn't very "transactional".  If we fail on some, but
+    // not all, then it's possible to leave things in a weird state.
+    // Luckily there should only be one cert with a particular
+    // label, and the cert not being found is one of the few reasons this could
+    // fail, but still...
+    if (!cert_db.DeleteCertAndKey(iter->get()))
+      result = false;
+  }
+  return result;
+}
+
+// static
+void OncNetworkParser::ListCertsWithLabel(const std::string& label,
+                                          net::CertificateList* result) {
+  net::CertificateList all_certs;
+  net::CertDatabase cert_db;
+  cert_db.ListCerts(&all_certs);
+  net::CertificateList new_list;
+  for (net::CertificateList::iterator iter = all_certs.begin();
+       iter != all_certs.end(); ++iter) {
+    if (net::x509_util::GetLabel(iter->get()).find(label) != std::string::npos)

[wtc, 2011/12/08 00:07:43]

IMPORTANT: document that DeleteCertAndKeyByLabel and
ListCertsWithLabel perform substring match of |label|.
This is not obvious.

[Greg Spencer (Chromium), 2011/12/09 18:51:38]

I switched this so that they are using exact matches, and documented that.

+      new_list.push_back(*iter);

[wtc, 2011/12/08 00:07:43]

Nit: why don't you just push *iter to |result| directly?

[Greg Spencer (Chromium), 2011/12/09 18:51:38]

Because if there were failures we could abort and not have already modified
|result|.  I tend to program these sorts of things so that output parameters
aren't modified until I know that the modification is going to completely
succeed. It also means I don't have to call "clear" on |result|.  And swap is a
very fast operation.

Regardless, there's no failure mode here, so I changed it as you suggested.

+  }
+  result->swap(new_list);
+}
+
+
+
 // -------------------- 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());
@@ skipping 328 matching lines @@
   static EnumMapper<ProviderType>::Pair table[] = {
     { flimflam::kProviderL2tpIpsec, PROVIDER_TYPE_L2TP_IPSEC_PSK },
     { flimflam::kProviderOpenVpn, PROVIDER_TYPE_OPEN_VPN },
   };
   CR_DEFINE_STATIC_LOCAL(EnumMapper<ProviderType>, parser,
       (table, arraysize(table), PROVIDER_TYPE_MAX));
   return parser.Get(type);
 }
 
 }  // namespace chromeos