Align the CT Implementation with Policy

Align the CT implementation with the actual policy.

This resolves several bugs that were revealed inherent in the implementation and implements the new policy designed to avoid ambiguities with the old policy.

- Unique SCTs from the same log no longer count towards quorum, as it fails to ensure the ecosystem diversity that is intended by the quorum requirement
- Officially supports "method pooling" when using non-embedded SCTs (so long as at least one valid, qualified non-embedded SCT is present, only require 1 Google + 1 non-Google log, from any source)
- Fixes the bug where method pooling worked for embedded certificates
- Simplifies the implementation to make it easier to ensure that all SCTs delivered via TLS extension/OCSP stapling are qualified at time of check

BUG=605510, 607023
Committed: https://crrev.com/a66cf3e7e727b4b613828e71931f8e91be070509
Cr-Commit-Position: refs/heads/master@{#391674}

[Ryan Sleevi, 2016-05-02 23:34:20]

Description was changed from

==========
WIP: SCT Policy fix

BUG=
==========

to

==========
Align the CT implementation with the actual policy.

This resolves several bugs that were revealed inherent in the implementation and
implements the new policy designed to avoid ambiguities with the old policy.

- Unique SCTs from the same log no longer count towards quorum, as it fails to
ensure the ecosystem diversity that is intended by the quorum requirement
- Officially supports "method pooling" when using non-embedded SCTs (so long as
at least one valid, qualified non-embedded SCT is present, only require 1 Google
+ 1 non-Google log, from any source)
- Fixes the bug where method pooling worked for embedded certificates
- Simplifies the implementation to make it easier to ensure that all SCTs
delivered via TLS extension/OCSP stapling are qualified at time of check

BUG=605510, 607023
==========

[Ryan Sleevi, 2016-05-02 23:36:13]

rsleevi@chromium.org changed reviewers:
+ eroman@chromium.org, estark@chromium.org

[Ryan Sleevi, 2016-05-02 23:36:13]

Emily: Could you provide a fresh set of eyes on this for the actual
implementation of the policy (right now, stated at
https://groups.google.com/a/chromium.org/d/msg/ct-policy/Mp1dqTWAiSY/iURDhBgj...
, but I'm updating the PDF with the results.) The only change from the initial
message is the "the number of embedded SCTs shown in Table 1" to the language
suggested in
https://groups.google.com/a/chromium.org/d/msg/ct-policy/Mp1dqTWAiSY/QRwi2aN0...

Eric: Actual OWNERS review.

[Ryan Sleevi, 2016-05-02 23:41:24]

Annotating some of the additional changes to the tests

https://codereview.chromium.org/1941973002/diff/60001/net/cert/ct_policy_enfo...
File net/cert/ct_policy_enforcer.cc (right):

https://codereview.chromium.org/1941973002/diff/60001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer.cc:183: return false;
This was just a style cleanup for better short-circuits.

https://codereview.chromium.org/1941973002/diff/60001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer.cc:207: }
This is a functional change unspecified in policy, but considering such a cert
will break everything anyways, there's no harm in the early short-circuit.

https://codereview.chromium.org/1941973002/diff/60001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer.cc:221: issuance_date = std::min(sct->timestamp,
issuance_date);
This is intentionally pre-computed, because it will be used to check the
qualified/non-qualified status in a follow-up CL.

https://codereview.chromium.org/1941973002/diff/60001/net/cert/ct_policy_enfo...
File net/cert/ct_policy_enforcer_unittest.cc (right):

https://codereview.chromium.org/1941973002/diff/60001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer_unittest.cc:112: ct::SCTList scts;
Changed because the original code was duplicating the log_id, which shouldn't
have been allowed.

https://codereview.chromium.org/1941973002/diff/60001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer_unittest.cc:156: desired_log_ids.size(),
desired_log_ids, true, &scts);
Trying to reduce magic boiler-plate because it took me too damn log to keep
cross-referencing the "helpers".

https://codereview.chromium.org/1941973002/diff/60001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer_unittest.cc:188: std::vector<std::string>(), false,
&scts);
The old test relied on the TLS extension method, which implies sending precerts,
which itself was a broken test that relied on our bugged behaviour.

https://codereview.chromium.org/1941973002/diff/60001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer_unittest.cc:281:
FillListWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, 2,
The change to 2 here (and later on) is to ensure that the *diversity*
requirement is always met.

The new policy specifies diversity before amount, since that's even more
important.

[Ryan Sleevi, 2016-05-02 23:43:30]

The CQ bit was checked by rsleevi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-02 23:46:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1941973002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1941973002/60001

[commit-bot: I haz the power, 2016-05-03 01:00:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-03 01:00:57]

Dry run: This issue passed the CQ dry run.

[Eran Messeri, 2016-05-03 12:52:12]

eranm@chromium.org changed reviewers:
+ eranm@chromium.org

[Eran Messeri, 2016-05-03 12:52:13]

LGTM - as far as I can tell implements the outlined policy.

https://codereview.chromium.org/1941973002/diff/60001/net/cert/ct_policy_enfo...
File net/cert/ct_policy_enforcer.cc (right):

https://codereview.chromium.org/1941973002/diff/60001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer.cc:207: }
On 2016/05/02 23:41:23, Ryan Sleevi wrote:
> This is a functional change unspecified in policy, but considering such a cert
> will break everything anyways, there's no harm in the early short-circuit.

Nit: in that case issuance_date can be defined and initialized later on.

https://codereview.chromium.org/1941973002/diff/60001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer.cc:327: embedded_log_ids.begin(),
Question: Is the begin() iterator always valid after the std::unique invocation?

Seems to me it would be if std::unique is always guaranteed not to move the
first item it a sequence of identical items.

[estark, 2016-05-03 18:15:03]

Policy logic lgtm, just a comment quibble

https://codereview.chromium.org/1941973002/diff/60001/net/cert/ct_policy_enfo...
File net/cert/ct_policy_enforcer.cc (right):

https://codereview.chromium.org/1941973002/diff/60001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer.cc:296: // Note: This also covers the case for
embedded SCTs, as it's only
This comment doesn't make sense to me. Is "embedded" supposed to be
"non-embedded" by any chance?

[Ryan Sleevi, 2016-05-03 18:31:04]

https://codereview.chromium.org/1941973002/diff/60001/net/cert/ct_policy_enfo...
File net/cert/ct_policy_enforcer.cc (right):

https://codereview.chromium.org/1941973002/diff/60001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer.cc:296: // Note: This also covers the case for
embedded SCTs, as it's only
On 2016/05/03 18:15:03, estark wrote:
> This comment doesn't make sense to me. Is "embedded" supposed to be
> "non-embedded" by any chance?

Yup, thanks.

https://codereview.chromium.org/1941973002/diff/60001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer.cc:327: embedded_log_ids.begin(),
On 2016/05/03 12:52:13, Eran Messeri wrote:
> Question: Is the begin() iterator always valid after the std::unique
invocation?
> 
> Seems to me it would be if std::unique is always guaranteed not to move the
> first item it a sequence of identical items.

Nope, that's a bug :)

[Ryan Sleevi, 2016-05-03 18:32:29]

https://codereview.chromium.org/1941973002/diff/60001/net/cert/ct_policy_enfo...
File net/cert/ct_policy_enforcer.cc (right):

https://codereview.chromium.org/1941973002/diff/60001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer.cc:327: embedded_log_ids.begin(),
On 2016/05/03 18:31:03, Ryan Sleevi wrote:
> On 2016/05/03 12:52:13, Eran Messeri wrote:
> > Question: Is the begin() iterator always valid after the std::unique
> invocation?
> > 
> > Seems to me it would be if std::unique is always guaranteed not to move the
> > first item it a sequence of identical items.
> 
> Nope, that's a bug :)

I guess I should clarify - for std::vector it's defined because this just swaps
positions, but for safety/clarity of code, I opted to rely on the
non-invalidation of iterator behaviour.

[eroman, 2016-05-04 19:56:23]

Looks good, but haven't read the corresponding policy document yet.

Will do another pass to verify the constraints once I do (although looks like
others already have too).

https://codereview.chromium.org/1941973002/diff/80001/net/cert/ct_policy_enfo...
File net/cert/ct_policy_enforcer.cc (right):

https://codereview.chromium.org/1941973002/diff/80001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer.cc:182: if (!ev_whitelist ||
!ev_whitelist->IsValid())
Side-remark: When reading this it wasn't obvious that ev_whitelist being null
was a supported mode. Tracing the source back to DoesConformToCTEVPolicy(), its
API contract does not indicate that ev_whiltelist can be null. Suggest adding a
comment there.

https://codereview.chromium.org/1941973002/diff/80001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer.cc:249: //   response is from a log qualified at
time of check;
Not sure about this formatting... But I do like the level of detail in the
comments!

EDIT: after chatting I understand it is quotes from a policy document. Can you
make that clearer and/or link tot he document somewhere in the code?

https://codereview.chromium.org/1941973002/diff/80001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer.cc:292:
base::Time::FromInternalValue(13080182400000000);
I wouldn't have expected the compiler to be happy with specifying a 64-bit
number without any suffix.

Also in general I don't like the use of FromInternalValue() which makes
assumptions about the epoch and granularity of time.

Have you considered:

  (a) Using UTC exploded time -- most readable option
  (b) Using base::Time::UnixEpoch() + base::TimeDelta::FromSeconds(...)

[Ryan Sleevi, 2016-05-04 20:59:19]

https://codereview.chromium.org/1941973002/diff/80001/net/cert/ct_policy_enfo...
File net/cert/ct_policy_enforcer.cc (right):

https://codereview.chromium.org/1941973002/diff/80001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer.cc:182: if (!ev_whitelist ||
!ev_whitelist->IsValid())
On 2016/05/04 19:56:23, eroman wrote:
> Side-remark: When reading this it wasn't obvious that ev_whitelist being null
> was a supported mode. Tracing the source back to DoesConformToCTEVPolicy(),
its
> API contract does not indicate that ev_whiltelist can be null. Suggest adding
a
> comment there.

I'm not sure what you mean that it's not clear. It's being handled here, isn't
that clear?

https://codereview.chromium.org/1941973002/diff/80001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer.cc:292:
base::Time::FromInternalValue(13080182400000000);
On 2016/05/04 19:56:23, eroman wrote:
> I wouldn't have expected the compiler to be happy with specifying a 64-bit
> number without any suffix.

You're right, that should have been INT64_C(...). MSVC was the only one that had
issues, and that was pre-MSVC 2015.

> Also in general I don't like the use of FromInternalValue() which makes
> assumptions about the epoch and granularity of time.

I think that ship has long-since sailed, based on the usage of it in Chrome.

> Have you considered:
> 
>   (a) Using UTC exploded time -- most readable option

Yes. That involves a system-call overhead and suffers from the Y2038 overhead.
It cannot be optimized by the compiler.

>   (b) Using base::Time::UnixEpoch() + base::TimeDelta::FromSeconds(...)

Yes, from previous dives on the generated code, the compiler was not able to
optimize this. UnixEpoch can't be const-expr'd because it generates a new
object, and thus in experience, the compilers end up doing math on fixed
constants with no side-effects, because they can't optimize it enough.

[eroman, 2016-05-04 21:26:35]

lgtm

https://codereview.chromium.org/1941973002/diff/80001/net/cert/ct_policy_enfo...
File net/cert/ct_policy_enforcer.cc (right):

https://codereview.chromium.org/1941973002/diff/80001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer.cc:182: if (!ev_whitelist ||
!ev_whitelist->IsValid())
On 2016/05/04 20:59:19, Ryan Sleevi wrote:
> On 2016/05/04 19:56:23, eroman wrote:
> > Side-remark: When reading this it wasn't obvious that ev_whitelist being
null
> > was a supported mode. Tracing the source back to DoesConformToCTEVPolicy(),
> its
> > API contract does not indicate that ev_whiltelist can be null. Suggest
adding
> a
> > comment there.
> 
> I'm not sure what you mean that it's not clear. It's being handled here, isn't
> that clear?

My feedback was to update the public API, DoesConformToCTEVPolicy() to clarify
that -- which you have done.

https://codereview.chromium.org/1941973002/diff/80001/net/cert/ct_policy_enfo...
net/cert/ct_policy_enforcer.cc:292:
base::Time::FromInternalValue(13080182400000000);
On 2016/05/04 20:59:19, Ryan Sleevi wrote:
> On 2016/05/04 19:56:23, eroman wrote:
> > I wouldn't have expected the compiler to be happy with specifying a 64-bit
> > number without any suffix.
> 
> You're right, that should have been INT64_C(...). MSVC was the only one that
had
> issues, and that was pre-MSVC 2015.
> 
> > Also in general I don't like the use of FromInternalValue() which makes
> > assumptions about the epoch and granularity of time.
> 
> I think that ship has long-since sailed, based on the usage of it in Chrome.
> 
> > Have you considered:
> > 
> >   (a) Using UTC exploded time -- most readable option
> 
> Yes. That involves a system-call overhead and suffers from the Y2038 overhead.
> It cannot be optimized by the compiler.
> 
> >   (b) Using base::Time::UnixEpoch() + base::TimeDelta::FromSeconds(...)
> 
> Yes, from previous dives on the generated code, the compiler was not able to
> optimize this. UnixEpoch can't be const-expr'd because it generates a new
> object, and thus in experience, the compilers end up doing math on fixed
> constants with no side-effects, because they can't optimize it enough.

I still have issues with using FromInternalValue() but I suppose that is best
raised separately.

At minimum please add a comment explaining the number is microseconds since
Windows epoch, which is correct for all platforms. 

(I would be happier if we had a FromUnixTime()/FromWindowsTime() functions, or
FromInternalValue()'s documentation promised windows timestamps).

[Ryan Sleevi, 2016-05-04 21:30:23]

On 2016/05/04 21:26:35, eroman wrote:
> I still have issues with using FromInternalValue() but I suppose that is best
> raised separately.

Indeed

> At minimum please add a comment explaining the number is microseconds since
> Windows epoch, which is correct for all platforms. 

Based on the argument of consistency, I'm not comfortable making this change in
this CL. Given that the format MUST be stable based on it's effective usage,
that's something that belongs in base::Time, not here, where it should be
obvious and the actual value documented.

[Ryan Sleevi, 2016-05-04 21:30:27]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2016-05-04 21:30:27]

The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org,
estark@chromium.org
Link to the patchset: https://codereview.chromium.org/1941973002/#ps100001
(title: "Feedback")

[commit-bot: I haz the power, 2016-05-04 21:30:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1941973002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1941973002/100001

[commit-bot: I haz the power, 2016-05-04 22:56:00]

Description was changed from

==========
Align the CT implementation with the actual policy.

This resolves several bugs that were revealed inherent in the implementation and
implements the new policy designed to avoid ambiguities with the old policy.

- Unique SCTs from the same log no longer count towards quorum, as it fails to
ensure the ecosystem diversity that is intended by the quorum requirement
- Officially supports "method pooling" when using non-embedded SCTs (so long as
at least one valid, qualified non-embedded SCT is present, only require 1 Google
+ 1 non-Google log, from any source)
- Fixes the bug where method pooling worked for embedded certificates
- Simplifies the implementation to make it easier to ensure that all SCTs
delivered via TLS extension/OCSP stapling are qualified at time of check

BUG=605510, 607023
==========

to

==========
Align the CT implementation with the actual policy.

This resolves several bugs that were revealed inherent in the implementation and
implements the new policy designed to avoid ambiguities with the old policy.

- Unique SCTs from the same log no longer count towards quorum, as it fails to
ensure the ecosystem diversity that is intended by the quorum requirement
- Officially supports "method pooling" when using non-embedded SCTs (so long as
at least one valid, qualified non-embedded SCT is present, only require 1 Google
+ 1 non-Google log, from any source)
- Fixes the bug where method pooling worked for embedded certificates
- Simplifies the implementation to make it easier to ensure that all SCTs
delivered via TLS extension/OCSP stapling are qualified at time of check

BUG=605510, 607023
==========

[commit-bot: I haz the power, 2016-05-04 22:56:01]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-04 22:57:23]

Description was changed from

==========
Align the CT implementation with the actual policy.

This resolves several bugs that were revealed inherent in the implementation and
implements the new policy designed to avoid ambiguities with the old policy.

- Unique SCTs from the same log no longer count towards quorum, as it fails to
ensure the ecosystem diversity that is intended by the quorum requirement
- Officially supports "method pooling" when using non-embedded SCTs (so long as
at least one valid, qualified non-embedded SCT is present, only require 1 Google
+ 1 non-Google log, from any source)
- Fixes the bug where method pooling worked for embedded certificates
- Simplifies the implementation to make it easier to ensure that all SCTs
delivered via TLS extension/OCSP stapling are qualified at time of check

BUG=605510, 607023
==========

to

==========
Align the CT implementation with the actual policy.

This resolves several bugs that were revealed inherent in the implementation and
implements the new policy designed to avoid ambiguities with the old policy.

- Unique SCTs from the same log no longer count towards quorum, as it fails to
ensure the ecosystem diversity that is intended by the quorum requirement
- Officially supports "method pooling" when using non-embedded SCTs (so long as
at least one valid, qualified non-embedded SCT is present, only require 1 Google
+ 1 non-Google log, from any source)
- Fixes the bug where method pooling worked for embedded certificates
- Simplifies the implementation to make it easier to ensure that all SCTs
delivered via TLS extension/OCSP stapling are qualified at time of check

BUG=605510, 607023

Committed: https://crrev.com/a66cf3e7e727b4b613828e71931f8e91be070509
Cr-Commit-Position: refs/heads/master@{#391674}
==========

[commit-bot: I haz the power, 2016-05-04 22:57:25]

Patchset 6 (id:??) landed as
https://crrev.com/a66cf3e7e727b4b613828e71931f8e91be070509
Cr-Commit-Position: refs/heads/master@{#391674}