Certificate Transparency: Correctly handle SCT origin

Certificate Transparency: Correctly handle SCT origin

Because the Signed Certificate Timestamp's origin was not included in
the equality checks, the same SCT from different origins was treated
as the same object. This led to the SCT Viewer showing identical SCTs
as coming from the same origin.
Including the origin field in the LessThan comparison solves the problem.

BUG=464684
Committed: https://crrev.com/028c4d67399ceca87cdb727b1f5bd4352df59e22
Cr-Commit-Position: refs/heads/master@{#320681}

[Eran Messeri, 2015-03-10 11:59:53]

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

[Ryan Sleevi, 2015-03-10 17:08:35]

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

[Ryan Sleevi, 2015-03-10 17:08:35]

Sorry to force a round-trip, but can you explain why the SCT viewer would depend
on this LessThan behaviour for showing the origin?

This would have caused distinct origins to appear as equal SCTs, but how did
this result in the UI change?

And similarly, how do we know that the original behaviour (in which the origin
didn't participate) won't cause issues. I suspect for this question, I'll need
to dig in further can carefully look at all the places LessThan is used, but
hopefully you can answer the first question.

[Eran Messeri, 2015-03-10 21:10:37]

On 2015/03/10 17:08:35, Ryan Sleevi wrote:
> Sorry to force a round-trip, but can you explain why the SCT viewer would
depend
> on this LessThan behaviour for showing the origin?
> 
> This would have caused distinct origins to appear as equal SCTs, but how did
> this result in the UI change?
> 
> And similarly, how do we know that the original behaviour (in which the origin
> didn't participate) won't cause issues. I suspect for this question, I'll need
> to dig in further can carefully look at all the places LessThan is used, but
> hopefully you can answer the first question.

A valid question!

As I stated on the bug the correct origin shows up in the NetLog. So obviously
the SCTs are correctly represented in the net layer.
The culprit was the way they were serialized to the UI layer: The SCTs are saved
in the SignedCertificateTimestapStore
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/si...)
which maps an SCT to ID. That in turn uses the RenderDataMemoizingStore
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...)
which uses a map to map IDs to items and vice versa. AFAIK the LessThan operator
is defined exactly for SCTs to be used in this class.
SCTs are retrieved by the UI based on their IDs from here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
Inside the net layer the SCTs are always kept in a list so the LessThan operator
is not needed:
https://code.google.com/p/chromium/codesearch#chromium/src/net/ssl/signed_cer...

I hope this reasoning is clear enough as to why (1) the LessThan operator is
needed (2) it should take the origin field into consideration.

[davidben, 2015-03-14 21:20:20]

lgtm. Looked in codesearch for anything else that might reference LessThan and
didn't find anything else. (RendererDataMemoizingStore is the only thing which
references a templated T::LessThan and there are no direct references.)

[Eran Messeri, 2015-03-15 09:28:38]

The CQ bit was checked by eranm@chromium.org

[commit-bot: I haz the power, 2015-03-15 09:28:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/997533002/1

[commit-bot: I haz the power, 2015-03-15 10:26:07]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-03-15 10:26:32]

Patchset 1 (id:??) landed as
https://crrev.com/028c4d67399ceca87cdb727b1f5bd4352df59e22
Cr-Commit-Position: refs/heads/master@{#320681}