Distinguish STS observation times from PKP observation times.

Distinguish STS observation times from PKP observation times.

Formerly, there was one time, called "created". "Observed" is more accurate.
This part of the overall effort to distinguish HSTS policies from key
pinning policies.

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

[palmer, 2013-07-02 17:09:33]

With luck, this patch will stay simple. PTAL, and thank you!

[Ryan Sleevi, 2013-07-02 23:41:09]

https://codereview.chromium.org/18554002/diff/2001/net/http/transport_securit...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/18554002/diff/2001/net/http/transport_securit...
net/http/transport_security_state.cc:205: if (i->second.sts_observed >= time &&
i->second.pkp_observed >= time) {
BUG: I'm not sure this is correct.

With DeleteAllDynamicDataSince(), it seems that if sts_observed >= time, you
should erase the HSTS data, and if pkp_observed >= time, you should erase all
the PKP data.

Only if both are expired do you erase the entry.

https://codereview.chromium.org/18554002/diff/2001/net/http/transport_securit...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/18554002/diff/2001/net/http/transport_securit...
net/http/transport_security_state.h:107: base::Time pkp_observed;
FWIW, should you also add these fields to net-internals? O:)

[cbentzel, 2013-07-05 15:42:04]

Am I needed for a particular portion of this review? I'll also still be on
vacation next week.

[palmer, 2013-07-08 22:48:16]

https://codereview.chromium.org/18554002/diff/2001/net/http/transport_securit...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/18554002/diff/2001/net/http/transport_securit...
net/http/transport_security_state.cc:205: if (i->second.sts_observed >= time &&
i->second.pkp_observed >= time) {
On 2013/07/02 23:41:10, Ryan Sleevi wrote:
> BUG: I'm not sure this is correct.
> 
> With DeleteAllDynamicDataSince(), it seems that if sts_observed >= time, you
> should erase the HSTS data, and if pkp_observed >= time, you should erase all
> the PKP data.
> 
> Only if both are expired do you erase the entry.

Done.

https://codereview.chromium.org/18554002/diff/2001/net/http/transport_securit...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/18554002/diff/2001/net/http/transport_securit...
net/http/transport_security_state.h:107: base::Time pkp_observed;
> FWIW, should you also add these fields to net-internals? O:)

I was debating it. Yeah I will if you think I should.

[Ryan Sleevi, 2013-07-17 23:32:43]

LGTM, but I wasn't sure if you were still planning on this approach or waiting
for Trevor's cleanups.

Also a question about the history clearing that can be investigated - I'm not
entirely sure though.

https://codereview.chromium.org/18554002/diff/15001/net/http/transport_securi...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/18554002/diff/15001/net/http/transport_securi...
net/http/transport_security_state.cc:216: }
FWIW, this seems weird.

Imagine the user visits site.example at T0, which sets up HSTS.

- They then revisit the site at T5, updating the observation time.
- They then revisit the site at T10, updating the observation time.
- They choose to clear all history from T6 to T10.
- This will reset site.example to pre-HSTS (which it existed at T0-T5).

That said, this is probably unavoidable. Do you know how we handle cookies? Do
we distinguish create from last_modified? I thought we did, and we only nuked
those created after T6, not those modified after T6.

[wtc, 2013-07-23 21:31:06]

Patch set 3 LGTM.

https://codereview.chromium.org/18554002/diff/15001/net/http/transport_securi...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/18554002/diff/15001/net/http/transport_securi...
net/http/transport_security_state.cc:213: i->second.dynamic_spki_hashes.clear();

Should we increment |i| in these two cases?

https://codereview.chromium.org/18554002/diff/15001/net/http/transport_securi...
net/http/transport_security_state.cc:844: pkp_observed(base::Time::Now()),

These two base::Time::Now() calls may potentially return different values.
It would be nice to ensure that sts_observed and pkp_observed be initialized
to the same value.

Alternatively, perhaps sts_observed and pkp_observed should just be initialized
to the default base::Time value.

[palmer, 2013-12-12 01:32:12]

https://codereview.chromium.org/18554002/diff/15001/net/http/transport_securi...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/18554002/diff/15001/net/http/transport_securi...
net/http/transport_security_state.cc:213: i->second.dynamic_spki_hashes.clear();
On 2013/07/23 21:31:07, wtc wrote:
> 
> Should we increment |i| in these two cases?

Done.

https://codereview.chromium.org/18554002/diff/15001/net/http/transport_securi...
net/http/transport_security_state.cc:216: }
> FWIW, this seems weird.
> 
> Imagine the user visits site.example at T0, which sets up HSTS.
> 
> - They then revisit the site at T5, updating the observation time.
> - They then revisit the site at T10, updating the observation time.
> - They choose to clear all history from T6 to T10.
> - This will reset site.example to pre-HSTS (which it existed at T0-T5).
> 
> That said, this is probably unavoidable. Do you know how we handle cookies? Do
> we distinguish create from last_modified? I thought we did, and we only nuked
> those created after T6, not those modified after T6.

I don't see a way to avoid this, short of keeping a record of every observation
ever (thus bloating the stored TSS).

I don't know how we handle cookies — nor am I sure how we should. :) Nor am I
sure if TSS should be handled the same way as cookies.

I suppose we could rename this method |DeleteAllDynamicDataModifiedAfter|, to
clarify? :\

https://codereview.chromium.org/18554002/diff/15001/net/http/transport_securi...
net/http/transport_security_state.cc:844: pkp_observed(base::Time::Now()),
On 2013/07/23 21:31:07, wtc wrote:
> 
> These two base::Time::Now() calls may potentially return different values.
> It would be nice to ensure that sts_observed and pkp_observed be initialized
> to the same value.
> 
> Alternatively, perhaps sts_observed and pkp_observed should just be
initialized
> to the default base::Time value.

Done.

[wtc, 2013-12-12 23:20:43]

Patch set 4 LGTM.

https://codereview.chromium.org/18554002/diff/21001/net/http/transport_securi...
File net/http/transport_security_persister.cc (right):

https://codereview.chromium.org/18554002/diff/21001/net/http/transport_securi...
net/http/transport_security_persister.cc:285: // We're migrating an old entry
with no creation date. Make sure we

Nit: should we change "creation date" in this comment to something that
describes sts_observed?

https://codereview.chromium.org/18554002/diff/21001/net/http/transport_securi...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/18554002/diff/21001/net/http/transport_securi...
net/http/transport_security_state.cc:218: }

Since we do |i++| in all cases, this can be written as a for loop.

[palmer, 2013-12-13 01:42:42]

https://codereview.chromium.org/18554002/diff/21001/net/http/transport_securi...
File net/http/transport_security_persister.cc (right):

https://codereview.chromium.org/18554002/diff/21001/net/http/transport_securi...
net/http/transport_security_persister.cc:285: // We're migrating an old entry
with no creation date. Make sure we
On 2013/12/12 23:20:44, wtc wrote:
> 
> Nit: should we change "creation date" in this comment to something that
> describes sts_observed?

Done.

https://codereview.chromium.org/18554002/diff/21001/net/http/transport_securi...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/18554002/diff/21001/net/http/transport_securi...
net/http/transport_security_state.cc:218: }
> Since we do |i++| in all cases, this can be written as a for loop.

Actually, that exposed a bug: when we do enabled_hosts_.erase, we were
invalidating |i|. New version avoids that.

[wtc, 2013-12-13 02:35:03]

https://codereview.chromium.org/18554002/diff/21001/net/http/transport_securi...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/18554002/diff/21001/net/http/transport_securi...
net/http/transport_security_state.cc:218: }

On 2013/12/13 01:42:42, Chromium Palmer wrote:
>
> Actually, that exposed a bug: when we do enabled_hosts_.erase, we were
> invalidating |i|. New version avoids that.

Ah, I remember this issue, and your original code is a trick to avoid this
problem. When we do
    enabled_hosts_.erase(i++);

three things happen in this order:
    1. |i| is incremented.
    2. A copy of the old value of |i| is created and
       passed to enabled_hosts_.erase.
    3. enabled_hosts_.erase invalidates the old value
       of |i|.

I'd prefer that we go back to the old code. We can simplify it as follows:

  while (i != enabled_hosts_.end()) {
    if (i->second.sts_observed >= time && i->second.pkp_observed >= time) {
      dirtied = true;
      enabled_hosts_.erase(i++);
      continue;
    }

    if (i->second.sts_observed >= time) {
      dirtied = true;
      i->second.upgrade_mode = DomainState::MODE_DEFAULT;
    } else if (i->second.pkp_observed >= time) {
      dirtied = true;
      i->second.dynamic_spki_hashes.clear();
    }
    ++i;  // Note the pre-increment operator for efficiency.
  }

[commit-bot: I haz the power, 2013-12-17 19:07:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/palmer@chromium.org/18554002/81001

[commit-bot: I haz the power, 2013-12-17 19:31:08]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[palmer, 2013-12-17 23:14:33]

+eroman,mmenke for

chrome/browser/resources/net_internals/hsts_view.js
chrome/browser/ui/webui/net_internals/net_internals_ui.cc

Could one of you please take a look? Thank you!

[mmenke, 2013-12-17 23:26:14]

On 2013/12/17 23:14:33, Chromium Palmer wrote:
> +eroman,mmenke for
> 
> chrome/browser/resources/net_internals/hsts_view.js
> chrome/browser/ui/webui/net_internals/net_internals_ui.cc
> 
> Could one of you please take a look? Thank you!

I'll do it tomorrow morning.

[eroman, 2013-12-18 01:21:57]

net_internals lgtm

https://codereview.chromium.org/18554002/diff/81001/chrome/browser/resources/...
File chrome/browser/resources/net_internals/hsts_view.js (right):

https://codereview.chromium.org/18554002/diff/81001/chrome/browser/resources/...
chrome/browser/resources/net_internals/hsts_view.js:136: t =
addNode(this.queryOutputDiv_, 'tt');
you can combine this with:

addNodeWithText(this.queryOutputDiv_, 'tt', results.sts_observed);

[mmenke, 2013-12-18 15:45:17]

I defer to eroman, no need to wait for my review.

On 2013/12/18 01:21:57, eroman wrote:
> net_internals lgtm
> 
>
https://codereview.chromium.org/18554002/diff/81001/chrome/browser/resources/...
> File chrome/browser/resources/net_internals/hsts_view.js (right):
> 
>
https://codereview.chromium.org/18554002/diff/81001/chrome/browser/resources/...
> chrome/browser/resources/net_internals/hsts_view.js:136: t =
> addNode(this.queryOutputDiv_, 'tt');
> you can combine this with:
> 
> addNodeWithText(this.queryOutputDiv_, 'tt', results.sts_observed);

[palmer, 2013-12-18 18:15:43]

https://codereview.chromium.org/18554002/diff/81001/chrome/browser/resources/...
File chrome/browser/resources/net_internals/hsts_view.js (right):

https://codereview.chromium.org/18554002/diff/81001/chrome/browser/resources/...
chrome/browser/resources/net_internals/hsts_view.js:136: t =
addNode(this.queryOutputDiv_, 'tt');
> you can combine this with:
> 
> addNodeWithText(this.queryOutputDiv_, 'tt', results.sts_observed);

I'll add a TODO(palmer) to do it later in a clean-up CL. Thanks for the tip!

[commit-bot: I haz the power, 2013-12-18 18:17:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/palmer@chromium.org/18554002/101001

[commit-bot: I haz the power, 2013-12-18 21:36:29]

Change committed as 241656