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

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

Checking for empty GUIDs and added a note to the ONC spec.
Cleaned up the CertificateImporter on the way.

TBR=stevenjb@chromium.org (small NetworkLibrary change reviewed by Greg, added a test file)
BUG=170357
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178820

[pneubeck (no reviews), 2013-01-16 14:46:56]

Please review according to
@David:
  onc_spec.html

@Greg:
  network_library*

@Joao:
  chromeos/network/onc/*

Thanks all!

[pastarmovj, 2013-01-16 14:49:38]

some drive by comments by me.

https://codereview.chromium.org/11970012/diff/2001/chromeos/docs/onc_spec.html
File chromeos/docs/onc_spec.html (right):

https://codereview.chromium.org/11970012/diff/2001/chromeos/docs/onc_spec.htm...
chromeos/docs/onc_spec.html:249: possible to update previously imported
configurations. Must have a length
Isn't "Must not be empty." Less robot sounding? :)

https://codereview.chromium.org/11970012/diff/2001/chromeos/network/onc/onc_v...
File chromeos/network/onc/onc_validator_unittest.cc (right):

https://codereview.chromium.org/11970012/diff/2001/chromeos/network/onc/onc_v...
chromeos/network/onc/onc_validator_unittest.cc:80: ONCSource onc_source =
ONC_SOURCE_NONE)
Default values are not allowed by the style guide.

[pneubeck (no reviews), 2013-01-16 14:53:18]

https://codereview.chromium.org/11970012/diff/2001/chromeos/docs/onc_spec.html
File chromeos/docs/onc_spec.html (right):

https://codereview.chromium.org/11970012/diff/2001/chromeos/docs/onc_spec.htm...
chromeos/docs/onc_spec.html:249: possible to update previously imported
configurations. Must have a length
On 2013/01/16 14:49:38, pastarmovj wrote:
> Isn't "Must not be empty." Less robot sounding? :)

I wanted to prevent that somebody may mix it up with the field not be assigned a
value at all.

I'm OK with both.

[Joao da Silva, 2013-01-16 15:03:56]

https://codereview.chromium.org/11970012/diff/2001/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/network_library_impl_base.cc (right):

https://codereview.chromium.org/11970012/diff/2001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1147:
allow_web_trust_from_policy);
Web trust is also allowed for ONC_SOURCE_USER_IMPORT.

bool allow_web_trust =
  (source == onc::ONC_SOURCE_USER_IMPORT) ||
  (source == onc::ONC_SOURCE_USER_POLICY && allow_web_trust_from_policy);

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

https://codereview.chromium.org/11970012/diff/2001/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer.cc:47: DCHECK(certificate !=
NULL);
|certificates| is user input, right? (It's from the ONC policiy, right?) So it's
very possible that it is malformed, and that should be handled.

https://codereview.chromium.org/11970012/diff/2001/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer.cc:73: DCHECK(!guid.empty());
Same

https://codereview.chromium.org/11970012/diff/2001/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer.cc:93: NOTREACHED();
Same

https://codereview.chromium.org/11970012/diff/2001/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer.cc:165: NOTREACHED();
User input?

https://codereview.chromium.org/11970012/diff/2001/chromeos/network/onc/onc_c...
File chromeos/network/onc/onc_certificate_importer.h (right):

https://codereview.chromium.org/11970012/diff/2001/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer.h:79: ONCSource onc_source_;
This field isn't used anymore.

https://codereview.chromium.org/11970012/diff/2001/chromeos/network/onc/onc_v...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/11970012/diff/2001/chromeos/network/onc/onc_v...
chromeos/network/onc/onc_validator.cc:358: const std::string &field_name) {
& next to std::string

https://codereview.chromium.org/11970012/diff/2001/chromeos/network/onc/onc_v...
File chromeos/network/onc/onc_validator.h (right):

https://codereview.chromium.org/11970012/diff/2001/chromeos/network/onc/onc_v...
chromeos/network/onc/onc_validator.h:214: const std::string &field_name);
& next to std::string

[pneubeck (no reviews), 2013-01-16 15:18:04]

https://codereview.chromium.org/11970012/diff/2001/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/network_library_impl_base.cc (right):

https://codereview.chromium.org/11970012/diff/2001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1147:
allow_web_trust_from_policy);
On 2013/01/16 15:03:57, Joao da Silva wrote:
> Web trust is also allowed for ONC_SOURCE_USER_IMPORT.
> 
> bool allow_web_trust =
>   (source == onc::ONC_SOURCE_USER_IMPORT) ||
>   (source == onc::ONC_SOURCE_USER_POLICY && allow_web_trust_from_policy);

Done.

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

https://codereview.chromium.org/11970012/diff/2001/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer.cc:47: DCHECK(certificate !=
NULL);
On 2013/01/16 15:03:57, Joao da Silva wrote:
> |certificates| is user input, right? (It's from the ONC policiy, right?) So
it's
> very possible that it is malformed, and that should be handled.

Policies are at first validated. The assumption here is that the argument is a
Certificate object according to the ONC spec.
This DCHECK should never occur.

The same below.

https://codereview.chromium.org/11970012/diff/2001/chromeos/network/onc/onc_c...
File chromeos/network/onc/onc_certificate_importer.h (right):

https://codereview.chromium.org/11970012/diff/2001/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer.h:79: ONCSource onc_source_;
On 2013/01/16 15:03:57, Joao da Silva wrote:
> This field isn't used anymore.

Done.

https://codereview.chromium.org/11970012/diff/2001/chromeos/network/onc/onc_v...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/11970012/diff/2001/chromeos/network/onc/onc_v...
chromeos/network/onc/onc_validator.cc:358: const std::string &field_name) {
On 2013/01/16 15:03:57, Joao da Silva wrote:
> & next to std::string

Done.

https://codereview.chromium.org/11970012/diff/2001/chromeos/network/onc/onc_v...
File chromeos/network/onc/onc_validator.h (right):

https://codereview.chromium.org/11970012/diff/2001/chromeos/network/onc/onc_v...
chromeos/network/onc/onc_validator.h:214: const std::string &field_name);
On 2013/01/16 15:03:57, Joao da Silva wrote:
> & next to std::string

Done.

[Joao da Silva, 2013-01-16 15:20:30]

lgtm

[pneubeck (no reviews), 2013-01-18 13:29:43]

@Greg:
Joao already reviewed the code changes. A quick look at the
network_library_impl_base* changes should be sufficient.

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

LGTM, modulo the one comment below.

https://codereview.chromium.org/11970012/diff/15001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_certificate_importer.cc (right):

https://codereview.chromium.org/11970012/diff/15001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_certificate_importer.cc:90: else if (cert_type ==
certificate::kClient)
Entire if clause should have braces..

if (...) {
} else {
}

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

https://codereview.chromium.org/11970012/diff/15001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_certificate_importer.cc (right):

https://codereview.chromium.org/11970012/diff/15001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_certificate_importer.cc:90: else if (cert_type ==
certificate::kClient)
On 2013/01/18 17:39:36, Greg Spencer (Chromium) wrote:
> Entire if clause should have braces..
> 
> if (...) {
> } else {
> }

Done.

[commit-bot: I haz the power, 2013-01-25 08:51:20]

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

[commit-bot: I haz the power, 2013-01-25 08:51:26]

Presubmit check for 11970012-21010 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chromeos
    chrome/browser/chromeos/cros

Presubmit checks took 1.4s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2013-01-25 08:55:57]

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

[commit-bot: I haz the power, 2013-01-25 15:39:10]

Change committed as 178820