Rename ONC field Trust to TrustBits.

Rename ONC field Trust to TrustBits.

BUG=159058, 216495
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193111

[pneubeck (no reviews), 2013-04-02 14:55:29]

@David, we have to rename the ONC field Trust to TrustBits. Could you please
verify that the field "Trust" isn't set yet by CPanel?


@Joao, please review.

[Joao da Silva, 2013-04-02 15:06:02]

Looks good to me but there are more files to change:

- chromeos/network/onc/onc_certificate_importer.h mentions the Trust attribute

- chromeos/test/data/network/invalid_settings_with_repairs.json

- ONC files in chromeos/test/data/network/

https://codereview.chromium.org/13473003/diff/1/chromeos/network/onc/onc_cert...
File chromeos/network/onc/onc_certificate_importer.cc (right):

https://codereview.chromium.org/13473003/diff/1/chromeos/network/onc/onc_cert...
chromeos/network/onc/onc_certificate_importer.cc:177:
ONC_LOG_WARNING("Certificate contains unknown trust type " +
Please confirm this change: new flags that are not recognized will not
invalidate the ONC blob, is that right?

[David Roche, 2013-04-02 15:08:17]

We do set Trust=Web in the CPanel, but the feature isn't exposed in production
yet, so we can change it.

Why do you need to change this?

[pneubeck (no reviews), 2013-04-02 15:11:47]

The existing implementation (upto R27) is unsafe (it doesn't tell the user that
the Admin installed a trusted CA cert and can effectively be MitM).
If we make this feature productive, we don't want it to be enabled on old
clients but only on new clients. Thus, we have to rename it. Old clients will
ignore the old field (but show a warning/error during parsing).

On 2013/04/02 15:08:17, David Roche wrote:
> We do set Trust=Web in the CPanel, but the feature isn't exposed in production
> yet, so we can change it.
> 
> Why do you need to change this?

[pneubeck (no reviews), 2013-04-03 08:13:06]

@Joao, PTAL.

https://codereview.chromium.org/13473003/diff/1/chromeos/network/onc/onc_cert...
File chromeos/network/onc/onc_certificate_importer.cc (right):

https://codereview.chromium.org/13473003/diff/1/chromeos/network/onc/onc_cert...
chromeos/network/onc/onc_certificate_importer.cc:177:
ONC_LOG_WARNING("Certificate contains unknown trust type " +
On 2013/04/02 15:06:02, Joao da Silva wrote:
> Please confirm this change: new flags that are not recognized will not
> invalidate the ONC blob, is that right?

Clarified here and in the ONC spec.

[Joao da Silva, 2013-04-03 12:49:53]

lgtm

https://codereview.chromium.org/13473003/diff/9001/chromeos/network/onc/onc_c...
File chromeos/network/onc/onc_certificate_importer.cc (right):

https://codereview.chromium.org/13473003/diff/9001/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer.cc:180: trust_type);
It'd be nice to add a test for this: have an ONC file with a "Foo" flag, and
check that the certificate is still imported.

https://codereview.chromium.org/13473003/diff/9001/chromeos/test/data/network...
File chromeos/test/data/network/managed_toplevel2.onc (right):

https://codereview.chromium.org/13473003/diff/9001/chromeos/test/data/network...
chromeos/test/data/network/managed_toplevel2.onc:78: "TrustBits": [],
Remove?

[pneubeck (no reviews), 2013-04-04 11:24:49]

https://codereview.chromium.org/13473003/diff/9001/chromeos/network/onc/onc_c...
File chromeos/network/onc/onc_certificate_importer.cc (right):

https://codereview.chromium.org/13473003/diff/9001/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer.cc:180: trust_type);
On 2013/04/03 12:49:53, Joao da Silva wrote:
> It'd be nice to add a test for this: have an ONC file with a "Foo" flag, and
> check that the certificate is still imported.

Added the flag to an existing test.

https://codereview.chromium.org/13473003/diff/9001/chromeos/test/data/network...
File chromeos/test/data/network/managed_toplevel2.onc (right):

https://codereview.chromium.org/13473003/diff/9001/chromeos/test/data/network...
chromeos/test/data/network/managed_toplevel2.onc:78: "TrustBits": [],
On 2013/04/03 12:49:53, Joao da Silva wrote:
> Remove?
No. I wanted to have the empty list in some test.

[Joao da Silva, 2013-04-04 11:36:31]

Still lgtm

https://codereview.chromium.org/13473003/diff/9001/chromeos/test/data/network...
File chromeos/test/data/network/managed_toplevel2.onc (right):

https://codereview.chromium.org/13473003/diff/9001/chromeos/test/data/network...
chromeos/test/data/network/managed_toplevel2.onc:78: "TrustBits": [],
On 2013/04/04 11:24:49, pneubeck wrote:
> On 2013/04/03 12:49:53, Joao da Silva wrote:
> > Remove?
> No. I wanted to have the empty list in some test.

Fair.

https://codereview.chromium.org/13473003/diff/21001/chromeos/test/data/networ...
File chromeos/test/data/network/certificate-server-update.onc (right):

https://codereview.chromium.org/13473003/diff/21001/chromeos/test/data/networ...
chromeos/test/data/network/certificate-server-update.onc:6: "Web", "Foo" // Test
that certs with unknown flags are still imported.
FYI: JSON doesn't support comments, but our parser does.

[commit-bot: I haz the power, 2013-04-04 11:37:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/13473003/21001

[David Roche, 2013-04-04 14:29:10]

The CPanel has been updated to use TrustBits instead.  I'm assuming that it's
okay if current devices see this value.

[pneubeck (no reviews), 2013-04-04 15:39:20]

On 2013/04/04 14:29:10, David Roche wrote:
> The CPanel has been updated to use TrustBits instead.  I'm assuming that it's
> okay if current devices see this value.

Yes, it is. Old clients will show an error and not import that Certificate.
But as you said, "the feature isn't exposed in production".

Do you send an empty TrustBits array to the clients in production?

[David Roche, 2013-04-04 15:56:22]

No, the CPanel doesn't send an empty TrustBits.  If there are no values, then
the field is left off entirely.

[commit-bot: I haz the power, 2013-04-04 16:15:14]

Retried try job too often on win_rel for step(s) browser_tests,
chrome_frame_net_tests, chrome_frame_tests, chrome_frame_unittests,
content_browsertests, installer_util_unittests, mini_installer_test,
nacl_integration, sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-04-04 16:20:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/13473003/21001

[commit-bot: I haz the power, 2013-04-04 16:20:53]

Failed to apply patch for chromeos/network/onc/onc_certificate_importer.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chromeos/network/onc/onc_certificate_importer.cc
  Hunk #1 succeeded at 130 with fuzz 1 (offset 60 lines).
  Hunk #2 succeeded at 145 with fuzz 1 (offset 60 lines).
  Hunk #3 succeeded at 165 with fuzz 2 (offset 4 lines).
  Hunk #4 FAILED at 174.
  Hunk #5 succeeded at 259 (offset 7 lines).
  Hunk #6 succeeded at 313 (offset 10 lines).
  1 out of 6 hunks FAILED -- saving rejects to file
chromeos/network/onc/onc_certificate_importer.cc.rej

Patch:       chromeos/network/onc/onc_certificate_importer.cc
Index: chromeos/network/onc/onc_certificate_importer.cc
diff --git a/chromeos/network/onc/onc_certificate_importer.cc
b/chromeos/network/onc/onc_certificate_importer.cc
index
34079715a2a5f0cfdd521dd340976d9ec6484b40..3a5f88fa083a648927500bd6120c6289e37870ee
100644
--- a/chromeos/network/onc/onc_certificate_importer.cc
+++ b/chromeos/network/onc/onc_certificate_importer.cc
@@ -70,11 +70,11 @@ bool CertificateImporter::ParseAndStoreCertificate(
     const base::DictionaryValue& certificate) {
   // Get out the attributes of the given certificate.
   std::string guid;
-  certificate.GetString(certificate::kGUID, &guid);
+  certificate.GetStringWithoutPathExpansion(certificate::kGUID, &guid);
   DCHECK(!guid.empty());
 
   bool remove = false;
-  if (certificate.GetBoolean(kRemove, &remove) && remove) {
+  if (certificate.GetBooleanWithoutPathExpansion(kRemove, &remove) && remove) {
     if (!DeleteCertAndKeyByNickname(guid)) {
       ONC_LOG_ERROR("Unable to delete certificate");
       return false;
@@ -85,7 +85,7 @@ bool CertificateImporter::ParseAndStoreCertificate(
 
   // Not removing, so let's get the data we need to add this certificate.
   std::string cert_type;
-  certificate.GetString(certificate::kType, &cert_type);
+  certificate.GetStringWithoutPathExpansion(certificate::kType, &cert_type);
   if (cert_type == certificate::kServer ||
       cert_type == certificate::kAuthority) {
     return ParseServerOrCaCertificate(cert_type, guid, certificate);
@@ -161,10 +161,12 @@ bool CertificateImporter::ParseServerOrCaCertificate(
     const base::DictionaryValue& certificate) {
   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) {
+  if (certificate.GetListWithoutPathExpansion(certificate::kTrustBits,
+                                              &trust_list)) {
+    for (base::ListValue::const_iterator it = trust_list->begin();
+         it != trust_list->end(); ++it) {
       std::string trust_type;
-      if (!trust_list->GetString(i, &trust_type))
+      if (!(*it)->GetAsString(&trust_type))
         NOTREACHED();
 
       if (trust_type == certificate::kWeb) {
@@ -172,19 +174,22 @@ bool CertificateImporter::ParseServerOrCaCertificate(
         // identification.
         web_trust = true;
       } else {
-        ONC_LOG_ERROR("Certificate contains unknown trust type " + trust_type);
-        return false;
+        // Trust bits should only increase trust and never restrict. Thus,
+        // ignoring unknown bits should be safe.
+        ONC_LOG_WARNING("Certificate contains unknown trust type " +
+                        trust_type);
       }
     }
   }
 
   if (web_trust && !allow_web_trust_) {
-    LOG(WARNING) << "Web trust not granted for certificate: " << guid;
+    ONC_LOG_WARNING("Web trust not granted for certificate: " + guid);
     web_trust = false;
   }
 
   std::string x509_data;
-  if (!certificate.GetString(certificate::kX509, &x509_data) ||
+  if (!certificate.GetStringWithoutPathExpansion(certificate::kX509,
+                                                 &x509_data) ||
       x509_data.empty()) {
     ONC_LOG_ERROR(
         "Certificate missing appropriate certificate data for type: " +
@@ -250,11 +255,10 @@ bool CertificateImporter::ParseServerOrCaCertificate(
     }
 
     // Reload the cert here to get an actual temporary cert instance.
-    x509_cert =
-        net::X509Certificate::CreateFromBytesWithNickname(
-            decoded_x509.data(),
-            decoded_x509.size(),
-            guid.c_str());
+    x509_cert = net::X509Certificate::CreateFromBytesWithNickname(
+        decoded_x509.data(),
+        decoded_x509.size(),
+        guid.c_str());
     if (!x509_cert.get()) {
       ONC_LOG_ERROR("Unable to create X509 certificate from bytes.");
       return false;
@@ -302,7 +306,8 @@ bool CertificateImporter::ParseClientCertificate(
     const std::string& guid,
     const base::DictionaryValue& certificate) {
   std::string pkcs12_data;
-  if (!certificate.GetString(certificate::kPKCS12, &pkcs12_data) ||
+  if (!certificate.GetStringWithoutPathExpansion(certificate::kPKCS12,
+                                                 &pkcs12_data) ||
       pkcs12_data.empty()) {
     ONC_LOG_ERROR("PKCS12 data is missing for client certificate.");
     return false;

[commit-bot: I haz the power, 2013-04-09 11:45:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/13473003/47001

[commit-bot: I haz the power, 2013-04-09 11:45:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/13473003/47001

[commit-bot: I haz the power, 2013-04-09 12:43:01]

Retried try job too often on linux_chromeos for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-04-09 13:15:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/13473003/47001

[commit-bot: I haz the power, 2013-04-09 15:43:55]

Change committed as 193111