ForceTLS: hash hostnames, handle subdomains, canonicalise.

ForceTLS: hash hostnames, handle subdomains, canonicalise.

It turns out that JSON[Reader|Writer] cannot handle periods in key
names(!). Because of this, an also to avoid leaking a sort of ForceTLS
browser history in the state file, we hash the domain names.

Also, this patch tries to implement the RFCs with respect to
canonicalising the names. Since IDN processing has already occured by
the time the name reaches us, there's only so much that we can do
however.

[agl, 2009-09-04 23:39:21]

Not sure that this is quite ready yet, but I though that I would let you see it
in its current state in case you're going to be working in this code over the
weekend.

[abarth-chromium, 2009-09-05 14:57:26]

I checked in the name change, so you'll probably end up with a bunch of
conflicts with this patch...  Some comments below.

http://codereview.chromium.org/201033/diff/1/2
File net/base/force_tls_state.cc (right):

http://codereview.chromium.org/201033/diff/1/2#newcode194
Line 194: static std::wstring HashedDomainToExternalString(const std::string&
hashed) {
Maybe we should just base64 encode the hash.  Presumably we have a function in
base somewhere that does base64 encoding / decoding.

http://codereview.chromium.org/201033/diff/1/3
File net/base/force_tls_state.h (right):

http://codereview.chromium.org/201033/diff/1/3#newcode86
Line 86: static int DNSDomainFromDot(const std::string& dotted, std::string*
out);
These functions seem like they should be in dns_utils.h or something not
StrictTransportSecurity specific.

http://codereview.chromium.org/201033/diff/1/4
File net/base/force_tls_state_unittest.cc (right):

http://codereview.chromium.org/201033/diff/1/4#newcode185
Line 185: TEST_F(ForceTLSStateTest, DNSDomainFromDot) {
You should test things here with trailing dots like google.com. and google.com..

Also, leading dots: .google.com

GURL should canonicalize most of these, but it will leave one trailing dot!

[agl, 2009-09-08 18:08:37]

http://codereview.chromium.org/201033/diff/1/2
File net/base/force_tls_state.cc (right):

http://codereview.chromium.org/201033/diff/1/2#newcode194
Line 194: static std::wstring HashedDomainToExternalString(const std::string&
hashed) {
On 2009/09/05 14:57:26, abarth wrote:
> Maybe we should just base64 encode the hash.  Presumably we have a function in
> base somewhere that does base64 encoding / decoding.

Done.

http://codereview.chromium.org/201033/diff/1/3
File net/base/force_tls_state.h (right):

http://codereview.chromium.org/201033/diff/1/3#newcode86
Line 86: static int DNSDomainFromDot(const std::string& dotted, std::string*
out);
On 2009/09/05 14:57:26, abarth wrote:
> These functions seem like they should be in dns_utils.h or something not
> StrictTransportSecurity specific.

Done.

http://codereview.chromium.org/201033/diff/1/4
File net/base/force_tls_state_unittest.cc (right):

http://codereview.chromium.org/201033/diff/1/4#newcode185
Line 185: TEST_F(ForceTLSStateTest, DNSDomainFromDot) {
On 2009/09/05 14:57:26, abarth wrote:
> You should test things here with trailing dots like http://google.com. and
http://google.com..
> 
> Also, leading dots: .google.com
> 
> GURL should canonicalize most of these, but it will leave one trailing dot!

Done.

[abarth-chromium, 2009-09-08 18:24:40]

This looks good to me.  The only real issue is the style / licensing of the code
in dns_utils.cc.  I'm not quite sure what to do about that.  I'll leave that up
to your judgement.

http://codereview.chromium.org/201033/diff/4001/4002
File net/base/dns_util.cc (right):

http://codereview.chromium.org/201033/diff/4001/4002#newcode8
Line 8: 
Do we need a license block or anything citing DJB?  Also, this function is not
in the right style.

http://codereview.chromium.org/201033/diff/4001/4002#newcode33
Line 33: if (labellen >= sizeof label) return 0;
It looks like this function should return a bool, not an int.

http://codereview.chromium.org/201033/diff/4001/4005
File net/base/strict_transport_security_state.cc (right):

http://codereview.chromium.org/201033/diff/4001/4005#newcode292
Line 292: 
Do we really need the remainder of this function?  Won't GURL lower case the
host and do the rest of this for us?

http://codereview.chromium.org/201033/diff/4001/4005#newcode319
Line 319: bool StrictTransportSecurityState::IsSTD3ASCIIValidCharacter(char c) {
This seems like it should go in dns_utils.h

http://codereview.chromium.org/201033/diff/4001/4007
File net/base/strict_transport_security_state_unittest.cc (right):

http://codereview.chromium.org/201033/diff/4001/4007#newcode130
Line 130: }
Maybe test for mixed case issues here?

[agl, 2009-09-09 00:36:35]

http://codereview.chromium.org/201033/diff/4001/4002
File net/base/dns_util.cc (right):

http://codereview.chromium.org/201033/diff/4001/4002#newcode8
Line 8: 
On 2009/09/08 18:24:43, abarth wrote:
> Do we need a license block or anything citing DJB?  Also, this function is not
> in the right style.

The header file credits him, I've added a credit in the .cc file too. Dan has
put all his code in the public domain[1]. Also, I know him personally, so I'm
happy that everything above is true.

[1] http://cr.yp.to/publicdomain.html

http://codereview.chromium.org/201033/diff/4001/4002#newcode33
Line 33: if (labellen >= sizeof label) return 0;
On 2009/09/08 18:24:43, abarth wrote:
> It looks like this function should return a bool, not an int.

Done.

http://codereview.chromium.org/201033/diff/4001/4005
File net/base/strict_transport_security_state.cc (right):

http://codereview.chromium.org/201033/diff/4001/4005#newcode292
Line 292: 
On 2009/09/08 18:24:43, abarth wrote:
> Do we really need the remainder of this function?  Won't GURL lower case the
> host and do the rest of this for us?

Well, GURL might, but we get passed strings not GURLs. Also, nothing is
specified and we don't have a type system to check anything anyway. Certainly,
GURL allows cases which the STS spec disallows (like '-' at the beginning or end
of a label).

http://codereview.chromium.org/201033/diff/4001/4005#newcode319
Line 319: bool StrictTransportSecurityState::IsSTD3ASCIIValidCharacter(char c) {
On 2009/09/08 18:24:43, abarth wrote:
> This seems like it should go in dns_utils.h

Done.

http://codereview.chromium.org/201033/diff/4001/4007
File net/base/strict_transport_security_state_unittest.cc (right):

http://codereview.chromium.org/201033/diff/4001/4007#newcode130
Line 130: }
On 2009/09/08 18:24:43, abarth wrote:
> Maybe test for mixed case issues here?

Done.

[abarth-chromium, 2009-09-09 00:54:22]

LGTM!  Two nits below.

http://codereview.chromium.org/201033/diff/3017/5007
File net/base/strict_transport_security_state_unittest.cc (right):

http://codereview.chromium.org/201033/diff/3017/5007#newcode8
Line 8: namespace net {
I think we don't need to put this in the net namespace anymore now that we're
not using a friend test.

http://codereview.chromium.org/201033/diff/3017/5008
File net/net.gyp (right):

http://codereview.chromium.org/201033/diff/3017/5008#newcode55
Line 55: 'base/dns_util.cc',
What about dns_util.h?