Improve TransportSecurityState data storage.

Improve TransportSecurityState data storage.

Introduce "DynamicEntry" objects stores in std::maps
to store HSTS, HPKP, and similar state separately.

BUG=156152

[unsafe, 2013-03-21 01:25:12]

Alright...  This is the hump CL in the TSS refactor: not the last but the
biggest, and after it things should be easier.

Missing/broke the unit tests - if feedback is positive I'll add them.


Some things might have gone sideways or backwards, see if you're OK with these:

1) "--hsts-hosts" command-line arg is removed.  I doubt it has any real use, and
it's annoying to have more datastructures just for this.

2) net-internals/#hsts display format is changed.  Previously it didn't
distinguish data *stored* at the domain vs applicable to the domain, and the
field names were unclear.  Now it reflects the HSTS/HPKP data applicable to the
domain (i.e. the DomainState), and fields are named more clearly.  A future CL
should probably *also* display stored data in a separate box or something.

OLD:
"""
Found: mode: OPPORTUNISTIC include_subdomains:true domain:google.com
pubkey_hashes:sha1/4n972HfV354KP560yw4uqe/baXc=,sha1/IvGeLsbqzPxdI0b0wuj2xVTdXgc=,sha1/QMVAHW+MuvCLAO3vse6H0AWzuc0=,sha1/AbkhxY0L343gKf+cki7NVWp+ozk=,sha1/SOZo+SvSspXXR9gjIBBPM5iQn9Q=,sha1/wHqYaI2J+6sFZAwRfap9ZbjKzE4=
"""

NEW:
"""
Found: HSTS:false
Public_Key_Pins_Good:sha1/4n972HfV354KP560yw4uqe/baXc=,sha1/IvGeLsbqzPxdI0b0wuj2xVTdXgc=,sha1/QMVAHW+MuvCLAO3vse6H0AWzuc0=,sha1/AbkhxY0L343gKf+cki7NVWp+ozk=,sha1/fVujyo43ZR18ccPjt3TN6XsbWUM=,sha1/vq7OyjSnqOco9nyMCDGdy77eijM=,sha1/SOZo+SvSspXXR9gjIBBPM5iQn9Q=,sha1/wHqYaI2J+6sFZAwRfap9ZbjKzE4=
Public_Key_Pins_Bad:sha1/klKqFN6/gK4wqtlOYDhwJKVDLxo=,sha1/DsYq91myCBCQJW/D3f2KZjEwK8U=,sha1/gzuEEAB/bkqdQS3EIjk2by7lW+k=,sha1/DX/hXFUUNmiZ/EDWIgjvIuvRFRw=
"""

3) The browser no longer attempts to serialize dynamic HPKP.  Previously it
would write dynamic JSON which might have "dynamic_spki_hashes_expiry",
"dynamic_spki_hashes", and "static_spki_hashes".  But since "include_subdomains"
and "created" aren't duplicated for HSTS and HPKP, this format wasn't good.

So, the serialization is changed to omit HPKP, i.e.:

   "J/NjOPsgIyxyvJft0TWCRO9zQPbVu6dV2+mbc7YC6QA=": {
      "created": 1363815838.01568,
      "expiry": 1450215838.01568,
      "include_subdomains": false,
      "mode": "force-https"
   },

Serializing dynamic HPKP will be done in a future CL.

4) The code no longer deletes expired dynamic entries that it discovers during
lookups.  While it's probably good to prune expired dynamic entries periodically
or something, I think this an incomplete approach to pruning that isn't worth
the weirdness of non-const Get methods.  

A future CL should probably set limits on number of dynamic entries and do
periodic sweeps for expired entries, or something.

5) The way static/dynamic entries are prioritized/merged into a DomainState has
changed.  Previously the code would prefer whichever had the more specific entry
(i.e. www.example.com would take priority over example.com, regardless of static
vs dynamic).  Now it does:
 - HSTS if either static *or* dynamic HSTS is specified
 - HPKP if either static *or* dynamic HPKP is specified, but with static pins
taking priority.
 
A future CL can apply a better strategy to take the most recent data.

6) Preloaded entries are no longer compiled into the build if non-OFFICIAL or
OS_ANDROID.  This changes behavior in the case where a preloaded domain, on a
non-OFFICIAL or ANDROID build, sets a dynamic pin.  This would previously be
treated the same as a preload failure, i.e. recorded as a histogram failure, and
a report would be sent to Google if this was a google property.  I'm not sure it
was intentional for dynamic pins on non-OFFICIAL builds to be triggering
preload-associated behavior, so it no longer does.

TODO FOR THIS CL
=================
 - Unit tests
 - More testing in general

TODO IN FUTURE CLs
===================
 - improve net_internals UI
 - serialize dynamic HPKP
 - purging of expired dynamic entries
 - improve merging/prioritizing of static/dynamic data
 - phase out hashed hostnames for plaintext hostnames 
 - call GetDomainState() once per connection

[Ryan Sleevi, 2013-03-23 04:24:12]

Sorry for the delay, Trevor.

I've taken a very quick pass at it - and yeah, true to form, I'm mostly pointing
out a lot of nits.

Overall, the CL seems to be heading in the right direction. The one thing that
makes me a bit nervous is the template wonkiness. I've made a suggestion below -
I'm not sure if it's a *good* suggestion, but overall, I'd like to try to keep
the implementation details from leaking out too much into the header.

I'm open to alternatives here; given that it's an std::map, we're not exactly
avoiding heap allocations or benefiting from high cache locality, so it may be
worth just using forward-declares and storing pointer types in the maps, thus
avoiding some of the template overhead.

Anyways, just a thought.

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
File net/base/transport_security_state.cc (right):

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.cc:71: return std::string("");
return std::string()

The "" technically incurs overhead (in scanning the string). Pedantry, really.

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.cc:153: found = GetDynamicDomainState(now,
host, &dynamic_state) || found;
Why not simply

found |= GetDynamicDomainState

Because you're not using logical or, you wouldn't have to worry about
short-circuiting (which I interpret your || found to be worried about)

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.cc:161: }
style: omit braces for one-liner

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.cc:165:
dynamic_state.public_key_pins_good_hashes_;
style: four spaces indent for continuation

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.cc:206: #ifdef PRELOADS_PRESENT
pedantry nit: #if defined() for non-header-guard ifdefs

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.cc:232: 
nit: unnecessary newline

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.cc:233: // should_upgrade
the comment here and on 237 seem unnecessary.

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.cc:303: public_key_pin_hashes);
style nit: because this line wraps, you should have braces for the if beginning
on line 299 and the else beginning 301

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.cc:312: include_subdomains);
style nit: indent
style: just return (no need for the retval temporary)

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.cc:331: bool retval =
hsts_entries_.AddEntry(hashed_host, entry);
naming nit: s/retval/result/

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.cc:388: : include_subdomains_(false),
created_(), expiry_() {
style nit: each member initializer goes on a single line if the entire ctor
cannot fit on one line

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...

Please check all the ctors in this file.

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.cc:397:
include_subdomains_(include_subdomains), created_(created), expiry_(expiry) {
style nit: 

TransportSecurityState::DynamicEntry::DynamicEntry(
    bool include_subdomains,
    const base::Time& created,
    const base::Time& expiry)
    : include_subdomains_(include_subdomains),
      created_(created),
      expiry_(expiry) {
}

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.cc:445:
HashesIntersect(public_key_pins_good_hashes_, hashes))
style nit: braces

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
File net/base/transport_security_state.h (right):

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.h:246: class DynamicEntryMap : public
std::map<std::string, T> {
It's not "legal" C++ to derive from containers within the std:: namespace (or
most types, for that matter - eg: std::string)

One technique would be to define the member functions as template functions
within the implementation file, rather than as a class. Then you pass the
std::map<std::string, DynamicEntry> or std::map<std::string, HPKPEntry> to the
member functions, with a possible out value of bool.

I haven't fully thought through this - this is just initial remark on the
design. I'll see if I can't come up with something cleaner, but did want to
provide the feedback that this seems like a bit of a smell (in leaking out
internal details).

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.h:280: typename std::map<std::string,
T>::iterator
Per
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Prepro...
, don't #define macros in a .h file

https://codereview.chromium.org/12974003/diff/27003/net/url_request/url_reque...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/12974003/diff/27003/net/url_request/url_reque...
net/url_request/url_request_http_job.cc:763: &domain_state) &&
domain_state.IsGooglePinnedProperty();
style nit: indentation levels -> 2 spaces to 4

const bool report_to_google =
    context->transport_security_state() &&
    context->transport_security_state()->GetDomainState(
        request_info_.url.host(),
        SSLConfigService::IsSNIAvailable(
            context->ssl_config_service()),
        &domain_state) &&
    domain_state.IsGooglePinnedProperty();

[unsafe, 2013-03-23 08:19:39]

I moved the template definitions into the .c.

I thought about pointers in the map, but they'd need to be smart pointers, and
there'd need to be downcasting on retrieving HPKPEntries, so that seemed uglier
overall.

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
File net/base/transport_security_state.cc (right):

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.cc:71: return std::string("");
On 2013/03/23 04:24:13, Ryan Sleevi wrote:
> return std::string()
> 
> The "" technically incurs overhead (in scanning the string). Pedantry, really.

Done.

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.cc:153: found = GetDynamicDomainState(now,
host, &dynamic_state) || found;
On 2013/03/23 04:24:13, Ryan Sleevi wrote:
> Why not simply
> 
> found |= GetDynamicDomainState
> 
> Because you're not using logical or, you wouldn't have to worry about
> short-circuiting (which I interpret your || found to be worried about)

its sort of a logical operation, bit arithmetic looks wrong to me here, but it
could be done..  yeah, i was worried about short-circuiting

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.cc:161: }
On 2013/03/23 04:24:13, Ryan Sleevi wrote:
> style: omit braces for one-liner

Done.

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.cc:165:
dynamic_state.public_key_pins_good_hashes_;
On 2013/03/23 04:24:13, Ryan Sleevi wrote:
> style: four spaces indent for continuation

Done.

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.cc:206: #ifdef PRELOADS_PRESENT
On 2013/03/23 04:24:13, Ryan Sleevi wrote:
> pedantry nit: #if defined() for non-header-guard ifdefs

Done.

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.cc:232: 
On 2013/03/23 04:24:13, Ryan Sleevi wrote:
> nit: unnecessary newline

Done.

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.cc:233: // should_upgrade
On 2013/03/23 04:24:13, Ryan Sleevi wrote:
> the comment here and on 237 seem unnecessary.

Done.

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.cc:303: public_key_pin_hashes);
On 2013/03/23 04:24:13, Ryan Sleevi wrote:
> style nit: because this line wraps, you should have braces for the if
beginning
> on line 299 and the else beginning 301

Done.

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.cc:312: include_subdomains);
On 2013/03/23 04:24:13, Ryan Sleevi wrote:
> style nit: indent
> style: just return (no need for the retval temporary)

Done.

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.cc:331: bool retval =
hsts_entries_.AddEntry(hashed_host, entry);
On 2013/03/23 04:24:13, Ryan Sleevi wrote:
> naming nit: s/retval/result/

Done.

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
File net/base/transport_security_state.h (right):

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.h:246: class DynamicEntryMap : public
std::map<std::string, T> {
On 2013/03/23 04:24:13, Ryan Sleevi wrote:
> It's not "legal" C++ to derive from containers within the std:: namespace (or
> most types, for that matter - eg: std::string)

Hmmm (going to stackoverflow, coming back)... so, they don't have virtual
destructors, but its not really illegal, is it?... just sometimes ill-advised?

> One technique would be to define the member functions as template functions
> within the implementation file, rather than as a class. Then you pass the
> std::map<std::string, DynamicEntry> or std::map<std::string, HPKPEntry> to the
> member functions, with a possible out value of bool.

Sounds good to me, I'll do that if you want.

https://codereview.chromium.org/12974003/diff/27003/net/base/transport_securi...
net/base/transport_security_state.h:280: typename std::map<std::string,
T>::iterator
On 2013/03/23 04:24:13, Ryan Sleevi wrote:
> Per
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Prepro...
> , don't #define macros in a .h file

Can't typedef this, due to templates.. perhaps a "Be very cautious" exemption?
:)

https://codereview.chromium.org/12974003/diff/27003/net/url_request/url_reque...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/12974003/diff/27003/net/url_request/url_reque...
net/url_request/url_request_http_job.cc:763: &domain_state) &&
domain_state.IsGooglePinnedProperty();
On 2013/03/23 04:24:13, Ryan Sleevi wrote:
> style nit: indentation levels -> 2 spaces to 4
> 
> const bool report_to_google =
>     context->transport_security_state() &&
>     context->transport_security_state()->GetDomainState(
>         request_info_.url.host(),
>         SSLConfigService::IsSNIAvailable(
>             context->ssl_config_service()),
>         &domain_state) &&
>     domain_state.IsGooglePinnedProperty();

Done (more or less, didn't need so many linebreaks):

      const bool report_to_google = context->transport_security_state() &&
          context->transport_security_state()->GetDomainState(
              request_info_.url.host(),
              SSLConfigService::IsSNIAvailable(context->ssl_config_service()),
              &domain_state) && domain_state.IsGooglePinnedProperty();

[unsafe, 2013-03-25 22:59:04]

> > One technique would be to define the member functions as template functions
> > within the implementation file, rather than as a class. Then you pass the
> > std::map<std::string, DynamicEntry> or std::map<std::string, HPKPEntry> to
the
> > member functions

I did this (template functions in .cc only), to clean up the header.

[palmer, 2013-03-25 23:54:34]

More later; just wanted to get these comments published.

https://codereview.chromium.org/12974003/diff/50001/chrome/browser/net/chrome...
File chrome/browser/net/chrome_fraudulent_certificate_reporter.cc (right):

https://codereview.chromium.org/12974003/diff/50001/chrome/browser/net/chrome...
chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:70: // domains
(when we start supporting that), we will ask for user permission.
So what code now enforces this comment?

I think we need to keep the IsGooglePinnedProperty check up here in
chrome/browser code, because at some point we're going to fire off UI to ask the
user if they want to report more kinds of pinning failure, and we can't do that
in e.g. the net/ layer.

https://codereview.chromium.org/12974003/diff/50001/chrome/browser/net/transp...
File chrome/browser/net/transport_security_persister.cc (right):

https://codereview.chromium.org/12974003/diff/50001/chrome/browser/net/transp...
chrome/browser/net/transport_security_persister.cc:163: iter =
transport_security_state_->GetHSTSEntries().begin();
This variable should be scoped to be inside the for loop. IF the trouble is that
you are having a hard time fitting the long expression into the for loop,
consider a typedef.

https://codereview.chromium.org/12974003/diff/50001/chrome/browser/net/transp...
chrome/browser/net/transport_security_persister.cc:164: for (; iter !=
transport_security_state_->GetHSTSEntries().end(); iter++) {
Chromium style is ++iter.

https://codereview.chromium.org/12974003/diff/50001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.cc (left):

https://codereview.chromium.org/12974003/diff/50001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:677:
transport_security_persister_.get()->DeserializeFromCommandLine(serialized);
I tried to get rid of this once before, and cevans got cranky. He has a blog
post documenting its use, FWIW.

https://codereview.chromium.org/12974003/diff/50001/net/base/transport_securi...
File net/base/transport_security_state.cc (right):

https://codereview.chromium.org/12974003/diff/50001/net/base/transport_securi...
net/base/transport_security_state.cc:87: return name_[index_] == 0;
I am not sure that std::string guarantees that this will work. Check index
against name_length() instead.

https://codereview.chromium.org/12974003/diff/50001/net/base/transport_securi...
net/base/transport_security_state.cc:94: for (index_++; name_[index_] != '.' &&
name_[index_] != 0; index_++);
Chromium style is pre-increment.

https://codereview.chromium.org/12974003/diff/50001/net/base/transport_securi...
net/base/transport_security_state.cc:113: #define DynamicEntryMapConstIter \
use typedef

https://codereview.chromium.org/12974003/diff/50001/net/base/transport_securi...
File net/base/transport_security_state.h (right):

https://codereview.chromium.org/12974003/diff/50001/net/base/transport_securi...
net/base/transport_security_state.h:31: // HTTP strict transport security (HSTS)
is defined in RFC 6797, and
Oh good, thank you.

https://codereview.chromium.org/12974003/diff/50001/net/base/transport_securi...
net/base/transport_security_state.h:48: // ShouldUpgradeToSSL returns true iff
HTTP requests should be internally
Just say, "Returns true iff...". No need to restate the function name in the
comment.

https://codereview.chromium.org/12974003/diff/50001/net/base/transport_securi...
net/base/transport_security_state.h:53: // ShouldSSLErrorsBeFatal returns true
iff HTTPS errors should cause
Same here; also end the sentence with a ".".

https://codereview.chromium.org/12974003/diff/50001/net/base/transport_securi...
net/base/transport_security_state.h:88: // Provide direct read-only access for
net-internals
Punctuation. Also maybe be more obvious: say chrome://net-internals.

https://codereview.chromium.org/12974003/diff/50001/net/base/transport_securi...
net/base/transport_security_state.h:103: size_t second_level_domain_name_;  //
if report_uma_on_pin_failure_
The meaning of these comments (lines 100 – 103) is not immediately clear to me.

https://codereview.chromium.org/12974003/diff/50001/net/base/transport_securi...
net/base/transport_security_state.h:169: // in dynamic state if necessary. 
ssl_info is used to check that
Denote identifiers with pipes: |ssl_info|

[unsafe, 2013-03-26 01:42:22]

https://codereview.chromium.org/12974003/diff/50001/chrome/browser/net/chrome...
File chrome/browser/net/chrome_fraudulent_certificate_reporter.cc (right):

https://codereview.chromium.org/12974003/diff/50001/chrome/browser/net/chrome...
chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:70: // domains
(when we start supporting that), we will ask for user permission.
On 2013/03/25 23:54:34, Chris P. wrote:
> So what code now enforces this comment?

URLRequestHttpJob::OnStartCompleted().  

> I think we need to keep the IsGooglePinnedProperty check up here in
> chrome/browser code, because at some point we're going to fire off UI to ask
the
> user if they want to report more kinds of pinning failure, and we can't do
that
> in e.g. the net/ layer.

The reason for here is that IsGooglePinnedProperty() was moved into DomainState,
so that all relevant preloaded state gets loaded via
TSS::GetPreloadDomainState(), instead of needing a different function for
searching preload entries.

The URLRequestHttpJob function already deals with a TSS instance for
ShouldSSLErrorsBeFatal(), so it seemed easier to handle the check at that level,
instead of pushing sni_available down to here, then figuring some way to get at
the TSS object from here...

But I could move the check back to here, if there's some way to get at a TSS
object?

https://codereview.chromium.org/12974003/diff/50001/chrome/browser/net/transp...
File chrome/browser/net/transport_security_persister.cc (right):

https://codereview.chromium.org/12974003/diff/50001/chrome/browser/net/transp...
chrome/browser/net/transport_security_persister.cc:163: iter =
transport_security_state_->GetHSTSEntries().begin();
On 2013/03/25 23:54:34, Chris P. wrote:
> This variable should be scoped to be inside the for loop. IF the trouble is
that
> you are having a hard time fitting the long expression into the for loop,
> consider a typedef.

yeah, line's too long.  I could add a typedef just for this one use, I guess. 
Worth it?

https://codereview.chromium.org/12974003/diff/50001/chrome/browser/net/transp...
chrome/browser/net/transport_security_persister.cc:164: for (; iter !=
transport_security_state_->GetHSTSEntries().end(); iter++) {
On 2013/03/25 23:54:34, Chris P. wrote:
> Chromium style is ++iter.

Done.

https://codereview.chromium.org/12974003/diff/50001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.cc (left):

https://codereview.chromium.org/12974003/diff/50001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:677:
transport_security_persister_.get()->DeserializeFromCommandLine(serialized);
On 2013/03/25 23:54:34, Chris P. wrote:
> I tried to get rid of this once before, and cevans got cranky. He has a blog
> post documenting its use, FWIW.

http://scarybeastsecurity.blogspot.com/2011/04/fiddling-with-chromiums-new-ce...

The example cmdline is broken - ("public_key_hashes" is the wrong name).  But
the return value is ignored (above), so anyone who tries it will have no idea.

I think exposing the internal format is a bad idea, exposing it without feedback
on whether it's working is even worse, and no one uses it anyways.

So I think it's a waste of effort to keep porting and maintaining, as TSS
evolves.

https://codereview.chromium.org/12974003/diff/50001/net/base/transport_securi...
File net/base/transport_security_state.cc (right):

https://codereview.chromium.org/12974003/diff/50001/net/base/transport_securi...
net/base/transport_security_state.cc:87: return name_[index_] == 0;
On 2013/03/25 23:54:34, Chris P. wrote:
> I am not sure that std::string guarantees that this will work. Check index
> against name_length() instead.

gah, good catch!

https://codereview.chromium.org/12974003/diff/50001/net/base/transport_securi...
net/base/transport_security_state.cc:94: for (index_++; name_[index_] != '.' &&
name_[index_] != 0; index_++);
On 2013/03/25 23:54:34, Chris P. wrote:
> Chromium style is pre-increment.

Done.

https://codereview.chromium.org/12974003/diff/50001/net/base/transport_securi...
net/base/transport_security_state.cc:113: #define DynamicEntryMapConstIter \
On 2013/03/25 23:54:34, Chris P. wrote:
> use typedef

Not possible!  templates!

It's amazing how poorly the features of C++ interact...

https://codereview.chromium.org/12974003/diff/50001/net/base/transport_securi...
File net/base/transport_security_state.h (right):

https://codereview.chromium.org/12974003/diff/50001/net/base/transport_securi...
net/base/transport_security_state.h:48: // ShouldUpgradeToSSL returns true iff
HTTP requests should be internally
On 2013/03/25 23:54:34, Chris P. wrote:
> Just say, "Returns true iff...". No need to restate the function name in the
> comment.

Done.

https://codereview.chromium.org/12974003/diff/50001/net/base/transport_securi...
net/base/transport_security_state.h:53: // ShouldSSLErrorsBeFatal returns true
iff HTTPS errors should cause
On 2013/03/25 23:54:34, Chris P. wrote:
> Same here; also end the sentence with a ".".

Done.

https://codereview.chromium.org/12974003/diff/50001/net/base/transport_securi...
net/base/transport_security_state.h:88: // Provide direct read-only access for
net-internals
On 2013/03/25 23:54:34, Chris P. wrote:
> Punctuation. Also maybe be more obvious: say chrome://net-internals.

Done.

https://codereview.chromium.org/12974003/diff/50001/net/base/transport_securi...
net/base/transport_security_state.h:103: size_t second_level_domain_name_;  //
if report_uma_on_pin_failure_
On 2013/03/25 23:54:34, Chris P. wrote:
> The meaning of these comments (lines 100 – 103) is not immediately clear
to me.

Added comment:
    // The following values are only relevant if one of the above
    // booleans is true:

https://codereview.chromium.org/12974003/diff/50001/net/base/transport_securi...
net/base/transport_security_state.h:169: // in dynamic state if necessary. 
ssl_info is used to check that
On 2013/03/25 23:54:34, Chris P. wrote:
> Denote identifiers with pipes: |ssl_info|

Done.

[palmer, 2013-04-02 22:56:04]

Some more comments. More to come.

https://codereview.chromium.org/12974003/diff/63001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/12974003/diff/63001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:3479: domain_state.ReportUMAOnPinFailure();
Re-reading the code around this line, which is my fault not yours, I see it is
unnecessarily confusing. I'm ginning up a CL that will clarify ("NO REALLY
PLEASE BELIEVE ME") this area, but it will probably cause you a merge conflict
when you rebase. You'll thank me later. ;)

https://codereview.chromium.org/12974003/diff/63001/net/url_request/url_reque...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/12974003/diff/63001/net/url_request/url_reque...
net/url_request/url_request_http_job.cc:761: request_info_.url.host(),
Use |host| for this (declared and assigned on line 755)?

[unsafe, 2013-04-03 05:21:42]

https://codereview.chromium.org/12974003/diff/63001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/12974003/diff/63001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:3479: domain_state.ReportUMAOnPinFailure();
On 2013/04/02 22:56:05, Chris P. wrote:
> Re-reading the code around this line, which is my fault not yours, I see it is
> unnecessarily confusing. I'm ginning up a CL that will clarify ("NO REALLY
> PLEASE BELIEVE ME") this area, but it will probably cause you a merge conflict
> when you rebase. You'll thank me later. ;)

Yeah, I think the build-age and reporting should maybe go inside
CheckPublicKeyPins(), to make this a simpler call site.  You could potentially
go as far as removing HasPublicKeyPins(), IsBuildTimely(), and
ReportUMAOnPinFailure() from the public interface.

https://codereview.chromium.org/12974003/diff/63001/net/url_request/url_reque...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/12974003/diff/63001/net/url_request/url_reque...
net/url_request/url_request_http_job.cc:761: request_info_.url.host(),
On 2013/04/02 22:56:05, Chris P. wrote:
> Use |host| for this (declared and assigned on line 755)?

Done.  Of course, we should really have a single GetDomainState()
per-connection, and then have the DomainState stored somewhere.  That would be
much more efficient and would clean up all these awful call sites... Is that
doable?

[Ryan Sleevi, 2013-04-29 22:06:14]

This is another huge change, so naturally it takes a lot of time to review
everything.

I think I would have almost preferred to just see the internal storage on TSS
refactored, but with all of the 'broken' public API in one CL, then a second CL
to update the storage/persistence, then another for net-internals. That said,
this CL is likely too intertwined at this point to sensibly break up.

This is yet another pass, but I haven't even begun to look closely at all of the
details - because there are so many.

I'm especially nervous about the lack of HPKP storage/persistence, but I'm also
as nervous because of the added code overhead that would bring into this CL.

I don't think pair programming is going to help review this CL that much -
there's simply too much going on to successfully page in, let alone with another
person trying to explain.

Most of this is style and comments as I've attempted to re-understand everything
from scratch, but there are a few design comments - particularly around
DynamicEntry.

https://codereview.chromium.org/12974003/diff/86001/chrome/browser/net/transp...
File chrome/browser/net/transport_security_persister.cc (right):

https://codereview.chromium.org/12974003/diff/86001/chrome/browser/net/transp...
chrome/browser/net/transport_security_persister.cc:163: iter =
transport_security_state_->GetHSTSEntries().begin();
So I understand why you felt the format should change, but I don't think we want
to just drop serializing HPKP.

https://codereview.chromium.org/12974003/diff/86001/chrome/browser/net/transp...
chrome/browser/net/transport_security_persister.cc:213: double created_double =
0, expiry_double = 0;
I think it was better without the default initialization, because it would cause
a compiler error if we attempted to use a value that had not been initialized
yet (eg: via lines 217-224)

https://codereview.chromium.org/12974003/diff/86001/chrome/browser/net/transp...
chrome/browser/net/transport_security_persister.cc:242: dirtied = true;
style: You should have braces around this if/else clause

comment: You dropped some very important comments that helped explain this code,
and it took me a while to understand how this code mapped onto the old. 

In some ways, the old flow was easier to read, because it reduced the nesting of
conditions.

BUG? You're explicitly ignoring HPKP, but you're not setting dirtied. Curious
the rationale for this.

Is my understanding of this correct:

if (mode_string != kForceHTTPS && mode_string != kStrict) {
  // Serialized HPKP information. Skip (and don't mark dirty).
  continue;

  // BUG?: Note the old code logged for unhandled TSS mode
  // strings, but here you no longer do.
}

if (expiry <= current_time) {
  // When dropping an entry due to expiration, it's still necessary
  // to mark the state as dirtied.
  dirtied = true;
  continue;
}

state->AddHSTSHashedHost(...)

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
File net/base/transport_security_state.cc (right):

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.cc:77: }
Seems like moving this function back to its original location will make it
easier to see the diff between these two.

Additionally, the previous code could "never" fail - have you ensured that all
new callers handle empty strings?

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.cc:115: #define DynamicEntryMapIter \
I still really dislike these #defines, even in the .cc.

template <typename T, typename MapType = std::map<std::string, T> >
bool GetDynamicEntry(const MapType& entries, ...) {
  MapType::const_iter find_iter = entries.find(...);
}

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.cc:121: bool is_full_hostname, T*
result_entry) {
style nit: one param per line for multi-line functions here, per
http://dev.chromium.org/developers/coding-style#Code_Formatting

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.cc:124: if (find_iter != entries.end()) {
Prefer early returns rather than nested conditionals
if (find_iter == entries.end())
  return false;

if ((is_full_hostname || ... ) && ...) {
  *result_entry = ...
  return true;
}
return false;

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.cc:138: TransportSecurityState* state) {
design: Rather than passing |state|, which is quite broad, you should be passing
"bool* dirty" instead (eg: the most-specific dependency)

This avoids issues with single-responsibility-principle scope creep, and helps
keeps the interfaces decoupled.

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.cc:139: bool dirty = false;
seems like |dirty| is entirely unnecessary - you always set it to be true (line
146, 149)

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.cc:147: } else {
style nit: Prefer error handling up front, followed by success handling.

if (find_iter == entries.end()) {
  ...
} else {
  ...
}

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.cc:209:
DeleteDynamicEntriesSince(hpkp_entries_, time, this);
For example, why the "bool* dirty" is nice, is this will currently end up
dirtying the state twice - forcing double serialization.

Ideally we only provide one notice.

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.cc:304: memcmp(entry.dns_name, name.data(),
entry.length) == 0 &&
seems like this memcmp should be moved to the last part of the conditional, to
allow earlier return

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.cc:364: bool include_subdomains = false;  //
TODO(trevp) PARSE FROM HEADER
Seems like entirely the wrong place to put this TODO.

I would see it omitted entirely - it's an impl. detail of ParseHPKPHeader

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
File net/base/transport_security_state.h (left):

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.h:145: class NET_EXPORT Iterator {
The intent of this class was/is to provide a de-coupling between the internal
state details of how TSS stores its members and how they're
serialized/deserialized.

It's a real shame to lose that, and I'm not entirely sure why this is necessary.

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
File net/base/transport_security_state.h (right):

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.h:87: // Provides direct read-only access for
chrome://net-internals.
comment nit: Shouldn't be mentioning net-internals
comment nit: Shouldn't be describing how it is/will be used
comment nit: Saying "read only" is a bit superfluous here with the const-ness.

// Returns associated known-good pins for the host. Only
// applicable if HasPublicKeyPins() is true.
// Note: This may be empty if a host only contains blacklisted
// pins.

// Returns associated known-bad pins for a host. Only applicable ... (etc)

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.h:104: size_t second_level_domain_name_;  //
if report_uma_on_pin_failure_
Why aren't you using the enum here, and instead casting to size_t? Not
guaranteed to be safe (enums scale to the largest necessary integer type, and no
remarks on signedness)

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.h:120: struct HPKPEntry : public DynamicEntry
{
This inheritance model still creates opportunity for type shearing.

Namely, it's possible to take an HPKPEntry (where all of its fields relate to
HPKP) and treat it as a DynamicEntry (where all of its fields relate to HSTS) -
eg: confusing include_subdomains, created, and expiry.

If you are going to go the struct/inheritance model, then it seems like you need
at least three types

DynamicEntry - the shared state between the two
HSTSEntry
HPKPEntry

Of course, the inheritance just seems to be class short-hand, and it may make
the most sense to just have
HSTSEntry
HPKPEntry

And not conflate the two with inheritance (which is part of how we got to this
problem in the first place)

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.h:178: // expiry < now or created > now.
comment nit: Seems like you should just be saying "true iff an entry was added".

comment nit: drop the "(used for net-internals and unit tests)"

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.h:188: // entries in hashed form).
comment nit: Leaking abstractions about serialization here (eg: talking about
JSON)

https://codereview.chromium.org/12974003/diff/86001/net/url_request/url_reque...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/12974003/diff/86001/net/url_request/url_reque...
net/url_request/url_request_http_job.cc:762: &domain_state) &&
domain_state.IsGooglePinnedProperty();
style nit: The wrapping here is... a bit unreadable :)

const bool report_to_google =
    context->transport_security_state() &&
    context->transport_security_state()->GetDomainState(
        host,
        SSLConfigService::IsSNIAvailable(...),
        &domain_state) &&
    domain_state.IsGooglePinnedProperty();

Note how when wrapping multiple parameters to 4 line indent, you ALSO wrap the
first parameter ( "GetDomainState(host" ),  and similarly with the conditional
for "transport_security_state() &&"

[unsafe, 2013-04-30 10:42:57]

https://codereview.chromium.org/12974003/diff/86001/chrome/browser/net/transp...
File chrome/browser/net/transport_security_persister.cc (right):

https://codereview.chromium.org/12974003/diff/86001/chrome/browser/net/transp...
chrome/browser/net/transport_security_persister.cc:163: iter =
transport_security_state_->GetHSTSEntries().begin();
On 2013/04/29 22:06:14, Ryan Sleevi wrote:
> So I understand why you felt the format should change, but I don't think we
want
> to just drop serializing HPKP.

I can follow-up this CL with HPKP serialization.

But the existing JSON format doesn't work (which "include_subdomains" and
"created" should I write for a domain where both HSTS and HPKP are set?)

Since the design decisions and code around the HPKP json can be separated out
from this (already large) CL, I'd rather do that.

https://codereview.chromium.org/12974003/diff/86001/chrome/browser/net/transp...
chrome/browser/net/transport_security_persister.cc:213: double created_double =
0, expiry_double = 0;
On 2013/04/29 22:06:14, Ryan Sleevi wrote:
> I think it was better without the default initialization, because it would
cause
> a compiler error if we attempted to use a value that had not been initialized
> yet (eg: via lines 217-224)

Done.

https://codereview.chromium.org/12974003/diff/86001/chrome/browser/net/transp...
chrome/browser/net/transport_security_persister.cc:242: dirtied = true;
On 2013/04/29 22:06:14, Ryan Sleevi wrote:
> style: You should have braces around this if/else clause

Done.

> comment: You dropped some very important comments that helped explain this
code,

Did I?  I think I only dropped 1 relevant line: " Make sure we dirty the state
if we drop an entry".  Which seemed redundant with code.


> and it took me a while to understand how this code mapped onto the old. 


I've added more comments to describe the dirtied flag.


> In some ways, the old flow was easier to read, because it reduced the nesting
of
> conditions.
> 
> BUG? You're explicitly ignoring HPKP, but you're not setting dirtied. Curious
> the rationale for this.

Is the "dirtied" flag important for anything besides "created"? 

Dirtying the state and rewriting the JSON due to expired entries, or HPKP
entries, or unparseable entries, doesn't seem super useful to me.  The old code
was dirtying for expired but not unparseable entries, was there some reason for
that?

Anyways, I changed it to be consistent and dirty the state on anything that
causes the loaded state to differ from JSON (expired entries, HPKP,
unparseable).


> Is my understanding of this correct:
> 
> if (mode_string != kForceHTTPS && mode_string != kStrict) {
>   // Serialized HPKP information. Skip (and don't mark dirty).
>   continue;

Changed to mark dirty, in this case.

> 
>   // BUG?: Note the old code logged for unhandled TSS mode
>   // strings, but here you no longer do.

Could add back if it matters, figured it was just a debugging aid.


> if (expiry <= current_time) {
>   // When dropping an entry due to expiration, it's still necessary
>   // to mark the state as dirtied.
>   dirtied = true;
>   continue;
> }
> 
> state->AddHSTSHashedHost(...)

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
File net/base/transport_security_state.cc (right):

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.cc:77: }
On 2013/04/29 22:06:14, Ryan Sleevi wrote:
> Seems like moving this function back to its original location will make it
> easier to see the diff between these two.

Done.

> Additionally, the previous code could "never" fail - have you ensured that all
> new callers handle empty strings?

Agreed it's safer and consistent with old code to return a hash of empty string
in case of DNS name failure, so I changed back to that.

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.cc:115: #define DynamicEntryMapIter \
On 2013/04/29 22:06:14, Ryan Sleevi wrote:
> I still really dislike these #defines, even in the .cc.
> 
> template <typename T, typename MapType = std::map<std::string, T> >
> bool GetDynamicEntry(const MapType& entries, ...) {
>   MapType::const_iter find_iter = entries.find(...);
> }

Done.

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.cc:121: bool is_full_hostname, T*
result_entry) {
On 2013/04/29 22:06:14, Ryan Sleevi wrote:
> style nit: one param per line for multi-line functions here, per
> http://dev.chromium.org/developers/coding-style#Code_Formatting

Done.

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.cc:124: if (find_iter != entries.end()) {
On 2013/04/29 22:06:14, Ryan Sleevi wrote:
> Prefer early returns rather than nested conditionals
> if (find_iter == entries.end())
>   return false;
> 
> if ((is_full_hostname || ... ) && ...) {
>   *result_entry = ...
>   return true;
> }
> return false;

Done.

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.cc:138: TransportSecurityState* state) {
On 2013/04/29 22:06:14, Ryan Sleevi wrote:
> design: Rather than passing |state|, which is quite broad, you should be
passing
> "bool* dirty" instead (eg: the most-specific dependency)
> 
> This avoids issues with single-responsibility-principle scope creep, and helps
> keeps the interfaces decoupled.

I tried that.  It could be done, but it makes the call sites to these template
functions uglier, as they have to collect the dirty results and call
StateIsDirty themselves, so I preferred it this way.  But could change it.

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.cc:139: bool dirty = false;
On 2013/04/29 22:06:14, Ryan Sleevi wrote:
> seems like |dirty| is entirely unnecessary - you always set it to be true
(line
> 146, 149)

Done.

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.cc:147: } else {
On 2013/04/29 22:06:14, Ryan Sleevi wrote:
> style nit: Prefer error handling up front, followed by success handling.
> 
> if (find_iter == entries.end()) {
>   ...
> } else {
>   ...
> }

It's not really an error, just 2 cases?  Does it matter which comes first?

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.cc:209:
DeleteDynamicEntriesSince(hpkp_entries_, time, this);
On 2013/04/29 22:06:14, Ryan Sleevi wrote:
> For example, why the "bool* dirty" is nice, is this will currently end up
> dirtying the state twice - forcing double serialization.
> 
> Ideally we only provide one notice.

I think the persister doesn't kick in right away, so it'll probably only do a
single serialization, and this isn't called frequently anyways.

Don't feel much urgency to change this, as future CLs are going to need to do
more rework of the persister and serialization, but happy to do so if you'd
rather just pass a bool*.

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.cc:304: memcmp(entry.dns_name, name.data(),
entry.length) == 0 &&
On 2013/04/29 22:06:14, Ryan Sleevi wrote:
> seems like this memcmp should be moved to the last part of the conditional, to
> allow earlier return

Done.

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.cc:364: bool include_subdomains = false;  //
TODO(trevp) PARSE FROM HEADER
On 2013/04/29 22:06:14, Ryan Sleevi wrote:
> Seems like entirely the wrong place to put this TODO.
> 
> I would see it omitted entirely - it's an impl. detail of ParseHPKPHeader

It's not an implementation detail - ParseHPKP will need to return an
include_subdomains, since HPKP is now using an "include_subdomains" directive,
but for now it's still hardcoded false.

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
File net/base/transport_security_state.h (right):

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.h:87: // Provides direct read-only access for
chrome://net-internals.
On 2013/04/29 22:06:14, Ryan Sleevi wrote:
> comment nit: Shouldn't be mentioning net-internals
> comment nit: Shouldn't be describing how it is/will be used
> comment nit: Saying "read only" is a bit superfluous here with the const-ness.

DomainState already has a high-level CheckPublicKeyPins(), it's ugly to also
expose these internal members.  So I'm explaining why they're here:
net-internals needs to display the underlying data.

That's helpful commentary I'd like to know when reading this.

I'll remove it if you want:  is this a general rule, function comments shouldn't
mention who uses them?

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.h:104: size_t second_level_domain_name_;  //
if report_uma_on_pin_failure_
On 2013/04/29 22:06:14, Ryan Sleevi wrote:
> Why aren't you using the enum here, and instead casting to size_t? Not
> guaranteed to be safe (enums scale to the largest necessary integer type, and
no
> remarks on signedness)

I'd have to pull the enum definition out into a header file (it's currently in a
.h which is included in a .c but is not a proper header file)..

But I guess that's the right thing to do, I'll get to it.

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.h:120: struct HPKPEntry : public DynamicEntry
{
On 2013/04/29 22:06:14, Ryan Sleevi wrote:
> This inheritance model still creates opportunity for type shearing.
> 
> Namely, it's possible to take an HPKPEntry (where all of its fields relate to
> HPKP) and treat it as a DynamicEntry (where all of its fields relate to HSTS)
-
> eg: confusing include_subdomains, created, and expiry.

I don't see a problem, but I could derive an HSTSEntry from DynamicEntry if you
think that's clearer.

> Of course, the inheritance just seems to be class short-hand, and it may make
> the most sense to just have
> HSTSEntry
> HPKPEntry

It is just shorthand, could do that too.

https://codereview.chromium.org/12974003/diff/86001/net/url_request/url_reque...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/12974003/diff/86001/net/url_request/url_reque...
net/url_request/url_request_http_job.cc:762: &domain_state) &&
domain_state.IsGooglePinnedProperty();
On 2013/04/29 22:06:14, Ryan Sleevi wrote:
> style nit: The wrapping here is... a bit unreadable :)
> 
> const bool report_to_google =
>     context->transport_security_state() &&
>     context->transport_security_state()->GetDomainState(
>         host,
>         SSLConfigService::IsSNIAvailable(...),
>         &domain_state) &&
>     domain_state.IsGooglePinnedProperty();
> 
> Note how when wrapping multiple parameters to 4 line indent, you ALSO wrap the
> first parameter ( "GetDomainState(host" ),  and similarly with the conditional
> for "transport_security_state() &&"

Done.

[Ryan Sleevi, 2013-04-30 19:55:08]

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
File net/base/transport_security_state.cc (right):

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.cc:209:
DeleteDynamicEntriesSince(hpkp_entries_, time, this);
On 2013/04/30 10:42:57, unsafe wrote:
> On 2013/04/29 22:06:14, Ryan Sleevi wrote:
> > For example, why the "bool* dirty" is nice, is this will currently end up
> > dirtying the state twice - forcing double serialization.
> > 
> > Ideally we only provide one notice.
> 
> I think the persister doesn't kick in right away, so it'll probably only do a
> single serialization, and this isn't called frequently anyways.

That's a wrong coupling of layers though. This code should make no assumptions
(save for the worst assumptions) about the TSS.

> 
> Don't feel much urgency to change this, as future CLs are going to need to do
> more rework of the persister and serialization, but happy to do so if you'd
> rather just pass a bool*.
> 
> 
> 

Preferably.

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
File net/base/transport_security_state.h (right):

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.h:87: // Provides direct read-only access for
chrome://net-internals.
On 2013/04/30 10:42:57, unsafe wrote:
> On 2013/04/29 22:06:14, Ryan Sleevi wrote:
> > comment nit: Shouldn't be mentioning net-internals
> > comment nit: Shouldn't be describing how it is/will be used
> > comment nit: Saying "read only" is a bit superfluous here with the
const-ness.
> 
> DomainState already has a high-level CheckPublicKeyPins(), it's ugly to also
> expose these internal members.  So I'm explaining why they're here:
> net-internals needs to display the underlying data.
> 
> That's helpful commentary I'd like to know when reading this.
> 
> I'll remove it if you want:  is this a general rule, function comments
shouldn't
> mention who uses them?

Yes.

A function comment should be about how to use it - not how it is used. This is
important for keeping code understandable and reducing churn. That is, there is
absolutely no reason a caller should expect to update a comment if they change
how it's used.

Additionally, comments should reflect the layering of the code. That is, if a
high-level component (like net-internals) depends on a low-level component (like
this), the re shouldn't be any comments going "up" the hierarchy - the same
reason why forward declaring 'higher level' classes is not desired.

The comments should follow the class design - that is, 'knowledge' only flows in
one way - from a higher level component to a lower level component.

https://codereview.chromium.org/12974003/diff/86001/net/base/transport_securi...
net/base/transport_security_state.h:120: struct HPKPEntry : public DynamicEntry
{
On 2013/04/30 10:42:57, unsafe wrote:
> On 2013/04/29 22:06:14, Ryan Sleevi wrote:
> > This inheritance model still creates opportunity for type shearing.
> > 
> > Namely, it's possible to take an HPKPEntry (where all of its fields relate
to
> > HPKP) and treat it as a DynamicEntry (where all of its fields relate to
HSTS)
> -
> > eg: confusing include_subdomains, created, and expiry.
> 
> I don't see a problem, but I could derive an HSTSEntry from DynamicEntry if
you
> think that's clearer.

I suppose the same problem would exist with the inheritance model - HSTSEntry
would be implicitly converted to a DynamicEntry which would then be used to
initialize an HPKPEntry.

The issue here is that DynamicEntry (implicitly) has the following PUBLIC
operators:

DynamicEntry(const DynamicEntry& other);
DynamicEntry& operator=(const DynamicEntry& other);

As such, that means HPKPEntry has the following public operators:
HPKPEntry(const DynamicEntry& other);
DynamicEntry& operator=(const DynamicEntry& other);

That means you can do something like:

DynamicEntry hsts_data;
hsts_data.include_subdomains = false;
HPKPEntry entry(hsts_data);

will work just fine.

The issue with this is, from the caller's perspective, it's easy to get things
wrong

HPKPEntry i_want_the_hpkp_entry = TSS.ButICallAFunctionReturningAnHSTSEntry();

And there will be no compile errors with that.

That's the general messiness of having these two types share an inheritance
tree. I would suggest breaking it entirely (eg: NO shared base).

There's no real overhead to the code (except in the debug symbols, and COMDAT
folding will handle that), and it will ensure you get meaningful compiler errors
when you conflate HPKP with HSTS.

> 
> > Of course, the inheritance just seems to be class short-hand, and it may
make
> > the most sense to just have
> > HSTSEntry
> > HPKPEntry
> 
> It is just shorthand, could do that too.

[unsafe, 2013-05-01 21:22:09]

OK, I think I handled all these issues:

On 2013/04/30 19:55:08, Ryan Sleevi wrote:
> > > For example, why the "bool* dirty" is nice, is this will currently end up
> > > dirtying the state twice - forcing double serialization.
> > > 
> > > Ideally we only provide one notice.
> > 
> > I think the persister doesn't kick in right away, so it'll probably only do
a
> > single serialization, and this isn't called frequently anyways.
> 
> That's a wrong coupling of layers though. This code should make no assumptions
> (save for the worst assumptions) about the TSS.
> 
> > 
> > Don't feel much urgency to change this, as future CLs are going to need to
do
> > more rework of the persister and serialization, but happy to do so if you'd
> > rather just pass a bool*.
> > 
> > 
> > 
> 
> Preferably.

I removed passing a state* but actually didn't need the bool* either, as
dirtiness can be inferred from the functions' return values.


> > I'll remove it if you want:  is this a general rule, function comments
> shouldn't
> > mention who uses them?
> 
> Yes.

Not totally sold on this.  But I changed the comments.


> > I don't see a problem, but I could derive an HSTSEntry from DynamicEntry if
> you
> > think that's clearer.
> 
> I suppose the same problem would exist with the inheritance model - HSTSEntry
> would be implicitly converted to a DynamicEntry which would then be used to
> initialize an HPKPEntry.
> 
> The issue here is that DynamicEntry (implicitly) has the following PUBLIC
> operators:
> 
> DynamicEntry(const DynamicEntry& other);
> DynamicEntry& operator=(const DynamicEntry& other);
> 
> As such, that means HPKPEntry has the following public operators:
> HPKPEntry(const DynamicEntry& other);
> DynamicEntry& operator=(const DynamicEntry& other);
> 
> That means you can do something like:
> 
> DynamicEntry hsts_data;
> hsts_data.include_subdomains = false;
> HPKPEntry entry(hsts_data);
> 
> will work just fine.
> 
> The issue with this is, from the caller's perspective, it's easy to get things
> wrong
> 
> HPKPEntry i_want_the_hpkp_entry = TSS.ButICallAFunctionReturningAnHSTSEntry();
> 
> And there will be no compile errors with that.
> 
> That's the general messiness of having these two types share an inheritance
> tree. I would suggest breaking it entirely (eg: NO shared base).

Done.  

I could imagine that at some point we might want non-templated code which
polymorphically can handle HSTSEntry or HPKPEntry, at which point the base class
might become useful.

But we don't need this now, so there's no loss to having separate types.


ALSO:  I changed the implicit enum conversion for "second_level_domain_name" to
an "int" instead of "size_t".  This is how UMA_HISTOGRAM_ENUMERATION is handling
the same value, so I assume it's safe.