Add a check for server and CA certificates in device policies to the ONC validator.

chromeos/network/onc/onc_certificate_importer.cc

 // Copyright (c) 2012 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/onc/onc_certificate_importer.h"
 
 #include <cert.h>
 #include <keyhi.h>
 #include <pk11pub.h>
 
@@ skipping 16 matching lines @@
 // The PEM block header used for DER certificates
 const char kCertificateHeader[] = "CERTIFICATE";
 // This is an older PEM marker for DER certificates.
 const char kX509CertificateHeader[] = "X509 CERTIFICATE";
 
 }  // namespace
 
 namespace chromeos {
 namespace onc {
 
-CertificateImporter::CertificateImporter(
-    ONCSource onc_source,
-    bool allow_web_trust_from_policy)
-    : onc_source_(onc_source),
-      allow_web_trust_from_policy_(allow_web_trust_from_policy) {
+CertificateImporter::CertificateImporter(bool allow_web_trust)
+    : allow_web_trust_(allow_web_trust) {
 }
 
 CertificateImporter::ParseResult CertificateImporter::ParseAndStoreCertificates(
     const base::ListValue& certificates) {
   size_t successful_imports = 0;
   for (size_t i = 0; i < certificates.GetSize(); ++i) {
     const base::DictionaryValue* certificate = NULL;
-    if (!certificates.GetDictionary(i, &certificate)) {
-      ONC_LOG_ERROR("Certificate data malformed");
-      continue;
-    }
+    certificates.GetDictionary(i, &certificate);
+    DCHECK(certificate != NULL);
 
     VLOG(2) << "Parsing certificate at index " << i << ": " << *certificate;
 
     if (!ParseAndStoreCertificate(*certificate)) {
       ONC_LOG_ERROR(
           base::StringPrintf("Cannot parse certificate at index %zu", i));
     } else {
       VLOG(2) << "Successfully imported certificate at index " << i;
       ++successful_imports;
     }
   }
 
   if (successful_imports == certificates.GetSize())
     return IMPORT_OK;
   else if (successful_imports == 0)
     return IMPORT_FAILED;
   else
     return IMPORT_INCOMPLETE;
 }
 
 bool CertificateImporter::ParseAndStoreCertificate(
     const base::DictionaryValue& certificate) {
   // Get out the attributes of the given certificate.
   std::string guid;
-  if (!certificate.GetString(kGUID, &guid) || guid.empty()) {
-    ONC_LOG_ERROR("Certificate missing GUID identifier");
-    return false;
-  }
+  certificate.GetString(kGUID, &guid);
+  DCHECK(!guid.empty());
 
   bool remove = false;
   if (certificate.GetBoolean(kRemove, &remove) && remove) {
     if (!DeleteCertAndKeyByNickname(guid)) {
       ONC_LOG_ERROR("Unable to delete certificate");
       return false;
     } else {
       return true;
     }
   }
 
   // Not removing, so let's get the data we need to add this certificate.
   std::string cert_type;
   certificate.GetString(certificate::kType, &cert_type);
   if (cert_type == certificate::kServer || cert_type == certificate::kAuthority)
     return ParseServerOrCaCertificate(cert_type, guid, certificate);
-
-  if (cert_type == certificate::kClient)
+  else if (cert_type == certificate::kClient)

[Greg Spencer (Chromium), 2013/01/18 17:39:36]

Entire if clause should have braces..

if (...) {
} else {
}

[pneubeck (no reviews), 2013/01/24 19:36:11]

Done.

     return ParseClientCertificate(guid, certificate);
 
-  ONC_LOG_ERROR("Certificate of unknown type: " + cert_type);
+  NOTREACHED();
   return false;
 }
 
 // static
 void CertificateImporter::ListCertsWithNickname(const std::string& label,
                                                 net::CertificateList* result) {
   net::CertificateList all_certs;
   net::NSSCertDatabase::GetInstance()->ListCerts(&all_certs);
   result->clear();
   for (net::CertificateList::iterator iter = all_certs.begin();
@@ skipping 45 matching lines @@
     if (!net::NSSCertDatabase::GetInstance()->DeleteCertAndKey(iter->get()))
       result = false;
   }
   return result;
 }
 
 bool CertificateImporter::ParseServerOrCaCertificate(
     const std::string& cert_type,
     const std::string& guid,
     const base::DictionaryValue& certificate) {
-  // Device policy can't import certificates.
-  if (onc_source_ == ONC_SOURCE_DEVICE_POLICY) {
-    // This isn't a parsing error.
-    ONC_LOG_WARNING("Refusing to import certificate from device policy.");
-    return true;
-  }
-
   bool web_trust = false;
   const base::ListValue* trust_list = NULL;
   if (certificate.GetList(certificate::kTrust, &trust_list)) {
     for (size_t i = 0; i < trust_list->GetSize(); ++i) {
       std::string trust_type;
-      if (!trust_list->GetString(i, &trust_type)) {
-        ONC_LOG_ERROR("Certificate trust is invalid");
-        return false;
-      }
+      if (!trust_list->GetString(i, &trust_type))
+        NOTREACHED();
+
       if (trust_type == certificate::kWeb) {
         // "Web" implies that the certificate is to be trusted for SSL
         // identification.
         web_trust = true;
       } else {
         ONC_LOG_ERROR("Certificate contains unknown trust type " + trust_type);
         return false;
       }
     }
   }
 
-  // Web trust is only granted to certificates imported for a managed user
-  // on a managed device.
-  if (onc_source_ == ONC_SOURCE_USER_POLICY &&
-      web_trust && !allow_web_trust_from_policy_) {
+  if (web_trust && !allow_web_trust_) {
     LOG(WARNING) << "Web trust not granted for certificate: " << guid;
     web_trust = false;
   }
 
   std::string x509_data;
   if (!certificate.GetString(certificate::kX509, &x509_data) ||
       x509_data.empty()) {
     ONC_LOG_ERROR(
         "Certificate missing appropriate certificate data for type: " +
         cert_type);
@@ skipping 157 matching lines @@
     PK11_SetPrivateKeyNickname(private_key, const_cast<char*>(guid.c_str()));
     SECKEY_DestroyPrivateKey(private_key);
   } else {
     ONC_LOG_WARNING("Unable to find private key for certificate.");
   }
   return true;
 }
 
 }  // namespace onc
 }  // namespace chromeos