ChromeOS: Add more host verification options for OpenVpn.

ChromeOS: Add more host verification options for OpenVpn.

Support the --verify-hash and --verify-x509-name options of OpenVPN via ONC.

Note that the ONC validator restricts the possible types passable to --verify-x509-name to "name", "name-prefix" and "subject". See also https://community.openvpn.net/openvpn/wiki/Openvpn23ManPage .

BUG=276291
R=armansito@chromium.org, bartfab@chromium.org, pstew@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234495

[pneubeck (no reviews), 2013-11-06 13:20:28]

@Bartosz, please review.
@David, FYI and please check the onc_spec.html .
@Paul, please take a look at the properties that are sent to Shill
(shill_openvpn.json) and at the documentation in onc_spec.html .

[Paul Stewart, 2013-11-06 14:22:20]

onc_translation_tables.cc LGTM (mapping to Shill constants).. The
shill_openvpn.json looks plausible, I suppose, but that's more or less for you
to define.

https://codereview.chromium.org/61903002/diff/30001/components/onc/docs/onc_s...
File components/onc/docs/onc_spec.html (right):

https://codereview.chromium.org/61903002/diff/30001/components/onc/docs/onc_s...
components/onc/docs/onc_spec.html:1234: (required)
The type value is not required in either shill or OpenVPN.  It's up to you
whether you wish to impose a requirement here.  I don't know what this means for
the behavior of potential ".ovpn" to ONC translators.

[pneubeck (no reviews), 2013-11-06 14:50:41]

https://codereview.chromium.org/61903002/diff/30001/components/onc/docs/onc_s...
File components/onc/docs/onc_spec.html (right):

https://codereview.chromium.org/61903002/diff/30001/components/onc/docs/onc_s...
components/onc/docs/onc_spec.html:1234: (required)
On 2013/11/06 14:22:20, Paul Stewart wrote:
> The type value is not required in either shill or OpenVPN.  It's up to you
> whether you wish to impose a requirement here.  I don't know what this means
for
> the behavior of potential ".ovpn" to ONC translators.

Ah. I see. I initially thought it unusual if there would be a command line
argument with a variable number of parameters. But you seem to be right.

I think, I'll change the requirement to "optional" and default to 'subject' as
currently described in the OpenVPN manpage.

Did you read the section "Which RDN is verified as name, depends on the
--x509-username-field option."
Does it even make sense to allow the selection of type=name or type=name-prefix
if "--x509-username-field" cannot be set? (the default is matching CN)

[Paul Stewart, 2013-11-06 15:55:38]

https://codereview.chromium.org/61903002/diff/30001/components/onc/docs/onc_s...
File components/onc/docs/onc_spec.html (right):

https://codereview.chromium.org/61903002/diff/30001/components/onc/docs/onc_s...
components/onc/docs/onc_spec.html:1234: (required)
On 2013/11/06 14:50:42, pneubeck wrote:
> On 2013/11/06 14:22:20, Paul Stewart wrote:
> > The type value is not required in either shill or OpenVPN.  It's up to you
> > whether you wish to impose a requirement here.  I don't know what this means
> for
> > the behavior of potential ".ovpn" to ONC translators.
> 
> Ah. I see. I initially thought it unusual if there would be a command line
> argument with a variable number of parameters. But you seem to be right.
> 
> I think, I'll change the requirement to "optional" and default to 'subject' as
> currently described in the OpenVPN manpage.
> 
> Did you read the section "Which RDN is verified as name, depends on the
> --x509-username-field option."
> Does it even make sense to allow the selection of type=name or
type=name-prefix
> if "--x509-username-field" cannot be set? (the default is matching CN)

I've read that section.  The CN is the most reasonable RDN to match against. 
"type=name" and "type=name-prefix" have a completely different behavior than
"subject", so I think all three options are expressive.  I'm agnostic to whether
you supply the "subject" value for the name option or leave it to OpenVPN to
perform its default behavior.

[bartfab (slow), 2013-11-07 10:48:55]

Given my limited understanding of ONC and its implementation in Chrome, LGTM.

It would be good if someone who knows this code better took another look before
you land it. Alternatively, maybe we should meet and go over this code to bring
me up to speed on its inner workings.

https://codereview.chromium.org/61903002/diff/30001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_signature.cc (right):

https://codereview.chromium.org/61903002/diff/30001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_signature.cc:135: { ::onc::verify_x509::kType,
&kStringSignature}, {NULL}};
Nit: Add line break before {NULL}

https://codereview.chromium.org/61903002/diff/30001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/61903002/diff/30001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_translator_shill_to_onc.cc:150:
TranslateAndAddNestedObject(::onc::openvpn::kVerifyX509);
Nit: Indent 2 spaces, not 4.

https://codereview.chromium.org/61903002/diff/30001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/61903002/diff/30001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:688: RequireField(*result, kName) &
RequireField(*result, kType);
Nit: &&

https://codereview.chromium.org/61903002/diff/30001/chromeos/test/data/networ...
File chromeos/test/data/network/valid_openvpn_with_cert_pems.onc (right):

https://codereview.chromium.org/61903002/diff/30001/chromeos/test/data/networ...
chromeos/test/data/network/valid_openvpn_with_cert_pems.onc:33: "Type": "some
x509 type"
My understanding of ONC is very limited but I wonder how this is valid. "Name"
is free-form but "Type" can only be one of a handful supported values. How is
"some x509 type" valid?

https://codereview.chromium.org/61903002/diff/30001/components/onc/docs/onc_s...
File components/onc/docs/onc_spec.html (right):

https://codereview.chromium.org/61903002/diff/30001/components/onc/docs/onc_s...
components/onc/docs/onc_spec.html:1205: which specifies the SHA1 fingerprint for
level-1 certificate.
Nit: s/for/for the/

[pneubeck (no reviews), 2013-11-11 11:53:43]

https://codereview.chromium.org/61903002/diff/30001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_signature.cc (right):

https://codereview.chromium.org/61903002/diff/30001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_signature.cc:135: { ::onc::verify_x509::kType,
&kStringSignature}, {NULL}};
On 2013/11/07 10:48:56, bartfab wrote:
> Nit: Add line break before {NULL}

Done.

https://codereview.chromium.org/61903002/diff/30001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/61903002/diff/30001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_translator_shill_to_onc.cc:150:
TranslateAndAddNestedObject(::onc::openvpn::kVerifyX509);
On 2013/11/07 10:48:56, bartfab wrote:
> Nit: Indent 2 spaces, not 4.

Done.

https://codereview.chromium.org/61903002/diff/30001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/61903002/diff/30001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:688: RequireField(*result, kName) &
RequireField(*result, kType);
On 2013/11/07 10:48:56, bartfab wrote:
> Nit: &&

Done.

https://codereview.chromium.org/61903002/diff/30001/chromeos/test/data/networ...
File chromeos/test/data/network/valid_openvpn_with_cert_pems.onc (right):

https://codereview.chromium.org/61903002/diff/30001/chromeos/test/data/networ...
chromeos/test/data/network/valid_openvpn_with_cert_pems.onc:33: "Type": "some
x509 type"
On 2013/11/07 10:48:56, bartfab wrote:
> My understanding of ONC is very limited but I wonder how this is valid. "Name"
> is free-form but "Type" can only be one of a handful supported values. How is
> "some x509 type" valid?

you're right. this file was used by the translator only and not validated. I
added this to the validation unittest.
Done.

https://codereview.chromium.org/61903002/diff/30001/components/onc/docs/onc_s...
File components/onc/docs/onc_spec.html (right):

https://codereview.chromium.org/61903002/diff/30001/components/onc/docs/onc_s...
components/onc/docs/onc_spec.html:1205: which specifies the SHA1 fingerprint for
level-1 certificate.
On 2013/11/07 10:48:56, bartfab wrote:
> Nit: s/for/for the/

Done.

[pneubeck (no reviews), 2013-11-11 11:58:05]

@Arman, since you looked into the ONC code before, please take a look. Thanks!

[armansito, 2013-11-11 20:01:40]

lgtm with one nit.

https://codereview.chromium.org/61903002/diff/150001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/61903002/diff/150001/chromeos/network/onc/onc...
chromeos/network/onc/onc_validator.cc:687: bool allRequiredExist =
RequireField(*result, kName);
nit: s/allRequiredExist/all_required_exist. You could technically also just
short-circuit the call to RequireField, since you only refer to
|allRequiredExist| in the return line. I.e.:

return !error_on_missing_field_ || RequireField(*result, kName);

Unless of course you're depending on the call to RequireField to log a message.

[commit-bot: I haz the power, 2013-11-11 20:42:07]

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

[pneubeck (no reviews), 2013-11-12 13:11:04]

https://codereview.chromium.org/61903002/diff/150001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/61903002/diff/150001/chromeos/network/onc/onc...
chromeos/network/onc/onc_validator.cc:687: bool allRequiredExist =
RequireField(*result, kName);
On 2013/11/11 20:01:41, armansito wrote:
> nit: s/allRequiredExist/all_required_exist. You could technically also just
> short-circuit the call to RequireField, since you only refer to
> |allRequiredExist| in the return line. I.e.:
> 
> return !error_on_missing_field_ || RequireField(*result, kName);
> 
> Unless of course you're depending on the call to RequireField to log a
message.

Good point. Actually, the intended behavior is:
if |error_on_missing_field_| is false, then missing required fields raise a
warning.
I.e. error_or_warning_found is true and the return value is true.

[pneubeck (no reviews), 2013-11-12 13:19:20]

Committed patchset #3 manually as r234495 (presubmit successful).