Add command-line control of the HSTS preload list.

Add command-line control of the HSTS preload list.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=82409

[Chris Evans, 2011-04-16 17:58:01]

What do you think of this approach to adding command-line control to HSTS? If
you think it's reasonable, I'll finish the CL by generally adding to and
enhancing the test.

[abarth-chromium, 2011-04-16 18:02:03]

You don't think the net-internals UI is better?

[cevans, 2011-04-17 05:17:55]

On Sat, Apr 16, 2011 at 11:02 AM, <abarth@chromium.org> wrote:

> You don't think the net-internals UI is better?


I'm wondering if command-line control might nicely complement the
net-internals UI? I like the idea of giving out command line "recipes" for
very secure usage of certain sites, for example.


Cheers
Chris


>
>
> http://codereview.chromium.org/6869043/
>

[abarth-chromium, 2011-04-17 05:34:41]

On Sat, Apr 16, 2011 at 10:17 PM, Chris Evans <cevans@google.com> wrote:
> On Sat, Apr 16, 2011 at 11:02 AM, <abarth@chromium.org> wrote:
>> You don't think the net-internals UI is better?
>
> I'm wondering if command-line control might nicely complement the
> net-internals UI? I like the idea of giving out command line "recipes" for
> very secure usage of certain sites, for example.

Ok.  I don't feel that strongly about it.  Your call.

Adam

[abarth-chromium, 2011-04-17 05:39:23]

No tests for the command line parsing?

http://codereview.chromium.org/6869043/diff/1/net/base/transport_security_sta...
File net/base/transport_security_state.cc (right):

http://codereview.chromium.org/6869043/diff/1/net/base/transport_security_sta...
net/base/transport_security_state.cc:341: base::Base64Decode(hash.substr(5,
hash.size() - 5), &hash_str) &&
It would be nicer if 5 wasn't a magic number here.

http://codereview.chromium.org/6869043/diff/1/net/base/transport_security_sta...
net/base/transport_security_state.cc:491: out->created = out->expiry =
base::Time::FromTimeT(0);
I'm not sure this is allowed by the style guide.

http://codereview.chromium.org/6869043/diff/1/net/base/transport_security_sta...
net/base/transport_security_state.cc:495:
CommandLine::ForCurrentProcess()->GetSwitchValueASCII("hsts-hosts");
Command line constants should be in a switches.cc file.

http://codereview.chromium.org/6869043/diff/1/net/base/transport_security_sta...
net/base/transport_security_state.cc:501: ++i) {
I would merge these last two lines.

http://codereview.chromium.org/6869043/diff/1/net/base/transport_security_sta...
net/base/transport_security_state.cc:514: if (parts[2] == "true")
True?  Why not "includeSubdomains" ?

http://codereview.chromium.org/6869043/diff/1/net/base/transport_security_sta...
net/base/transport_security_state.cc:519: base::SplitString(parts[3], '|',
&str_fingerprints);
This is getting complicated.

[cevans, 2011-04-17 05:40:14]

On Sat, Apr 16, 2011 at 10:34 PM, Adam Barth <abarth@chromium.org> wrote:

> On Sat, Apr 16, 2011 at 10:17 PM, Chris Evans <cevans@google.com> wrote:
> > On Sat, Apr 16, 2011 at 11:02 AM, <abarth@chromium.org> wrote:
> >> You don't think the net-internals UI is better?
> >
> > I'm wondering if command-line control might nicely complement the
> > net-internals UI? I like the idea of giving out command line "recipes"
> for
> > very secure usage of certain sites, for example.
>
> Ok.  I don't feel that strongly about it.  Your call.
>

I'll wait for Adam Langley's opinion before investing additional effort.


>
> Adam
>

[cevans, 2011-04-17 05:44:18]

On Sat, Apr 16, 2011 at 10:39 PM, <abarth@chromium.org> wrote:

> No tests for the command line parsing?
>

Yeah, as per original message, I intend to beef up the testing and fix a few
other things -- if we decide the overall idea is worthwhile.


Cheers
Chris


>
>
>
>
http://codereview.chromium.org/6869043/diff/1/net/base/transport_security_sta...
> File net/base/transport_security_state.cc (right):
>
>
>
http://codereview.chromium.org/6869043/diff/1/net/base/transport_security_sta...
> net/base/transport_security_state.cc:341:
> base::Base64Decode(hash.substr(5, hash.size() - 5), &hash_str) &&
> It would be nicer if 5 wasn't a magic number here.
>
>
>
http://codereview.chromium.org/6869043/diff/1/net/base/transport_security_sta...
> net/base/transport_security_state.cc:491: out->created = out->expiry =
> base::Time::FromTimeT(0);
> I'm not sure this is allowed by the style guide.
>
>
>
http://codereview.chromium.org/6869043/diff/1/net/base/transport_security_sta...
> net/base/transport_security_state.cc:495:
> CommandLine::ForCurrentProcess()->GetSwitchValueASCII("hsts-hosts");
> Command line constants should be in a switches.cc file.
>
>
>
http://codereview.chromium.org/6869043/diff/1/net/base/transport_security_sta...
> net/base/transport_security_state.cc:501: ++i) {
> I would merge these last two lines.
>
>
>
http://codereview.chromium.org/6869043/diff/1/net/base/transport_security_sta...
> net/base/transport_security_state.cc:514: if (parts[2] == "true")
> True?  Why not "includeSubdomains" ?
>
>
>
http://codereview.chromium.org/6869043/diff/1/net/base/transport_security_sta...
> net/base/transport_security_state.cc:519: base::SplitString(parts[3],
> '|', &str_fingerprints);
> This is getting complicated.
>
>
> http://codereview.chromium.org/6869043/
>

[agl, 2011-04-18 13:54:46]

I'm in committee all day so can't do a proper review until much later. I'm luke
warm on the utility of adding command line arguments for this, but I'm not
against it. abarth's comments look like a good start.

[Chris Evans, 2011-04-20 02:53:24]

Changes since last time:
- Fix "empty domain string" UI bug + add test (this bug already existed)
- Fix TransportSecurityState::DeleteSince (my fault ;-)
- Rewrite command-line parsing to leverage existing deserialization support.
- Some simplifications / refactoring.

And inline:

On 2011/04/17 05:39:23, abarth wrote:
> No tests for the command line parsing?

Fixed; see below.

> 
>
http://codereview.chromium.org/6869043/diff/1/net/base/transport_security_sta...
> File net/base/transport_security_state.cc (right):
> 
>
http://codereview.chromium.org/6869043/diff/1/net/base/transport_security_sta...
> net/base/transport_security_state.cc:341: base::Base64Decode(hash.substr(5,
> hash.size() - 5), &hash_str) &&
> It would be nicer if 5 wasn't a magic number here.
> 
>
http://codereview.chromium.org/6869043/diff/1/net/base/transport_security_sta...
> net/base/transport_security_state.cc:491: out->created = out->expiry =
> base::Time::FromTimeT(0);
> I'm not sure this is allowed by the style guide.

This code was moved from elsewhere in the file -- but fixed.

> 
>
http://codereview.chromium.org/6869043/diff/1/net/base/transport_security_sta...
> net/base/transport_security_state.cc:495:
> CommandLine::ForCurrentProcess()->GetSwitchValueASCII("hsts-hosts");
> Command line constants should be in a switches.cc file.

Fixed.

> 
>
http://codereview.chromium.org/6869043/diff/1/net/base/transport_security_sta...
> net/base/transport_security_state.cc:501: ++i) {
> I would merge these last two lines.
> 
>
http://codereview.chromium.org/6869043/diff/1/net/base/transport_security_sta...
> net/base/transport_security_state.cc:514: if (parts[2] == "true")
> True?  Why not "includeSubdomains" ?
> 
>
http://codereview.chromium.org/6869043/diff/1/net/base/transport_security_sta...
> net/base/transport_security_state.cc:519: base::SplitString(parts[3], '|',
> &str_fingerprints);
> This is getting complicated.

Yes, I agree. Maintaining another parser is pretty crazy when we already have a
JSON one. I don't think this CL is worth it unless the number of added lines in
minimal. So I redid it around the existing parser; the benefits aside from less
lines is that new capabilities won't need new code in a second parser.

The change in now pretty small, especially with some tidyups that collapsed some
existing lines into a small function.

[abarth-chromium, 2011-04-20 04:49:32]

http://codereview.chromium.org/6869043/diff/12001/net/base/net_switches.cc
File net/base/net_switches.cc (right):

http://codereview.chromium.org/6869043/diff/12001/net/base/net_switches.cc#ne...
net/base/net_switches.cc:12: // point will be hit.
What?

[Chris Evans, 2011-04-20 04:55:07]

On 2011/04/20 04:49:32, abarth wrote:
> http://codereview.chromium.org/6869043/diff/12001/net/base/net_switches.cc
> File net/base/net_switches.cc (right):
> 
>
http://codereview.chromium.org/6869043/diff/12001/net/base/net_switches.cc#ne...
> net/base/net_switches.cc:12: // point will be hit.
> What?

Fixed

[agl, 2011-04-20 13:24:48]

LGTM

[eroman, 2011-04-29 23:58:34]

Please move this command line out of net/ and into chrome/
(chrome/common/chrome_switches.cc).

We do all of our net command lines as part of Chrome. The goal is for the net
module to be a separable library, and it should not have CommandLine
dependencies.

In general Chrome code prefers to centralize all of the commandlines into
"chrome_switches.cc" (in stark contrast to google3 where they are littered
throughout all the files).

Thanks.

[cevans, 2011-04-30 00:45:45]

Thanks. http://code.google.com/p/chromium/issues/detail?id=81042 and I
ReleaseBlock'ed it so it doesn't fester.
I'll clarify exactly what we need in the bug and then fix it.

On Fri, Apr 29, 2011 at 4:58 PM, <eroman@chromium.org> wrote:

> Please move this command line out of net/ and into chrome/
> (chrome/common/chrome_switches.cc).
>
> We do all of our net command lines as part of Chrome. The goal is for the
> net
> module to be a separable library, and it should not have CommandLine
> dependencies.
>
> In general Chrome code prefers to centralize all of the commandlines into
> "chrome_switches.cc" (in stark contrast to google3 where they are littered
> throughout all the files).
>
> Thanks.
>
> http://codereview.chromium.org/6869043/
>

[eroman, 2011-04-30 01:03:18]

Thanks Chris!