Surface SCT (Signed Certificate Timestamp) counts in the Security panel.

Surface SCT (Signed Certificate Timestamp) counts in the Security panel.

BUG=551705
TEST=Do the following:
1) Visit mozilla.org, open the Security panel in DevTools, refresh, click on "https://www.mozilla.org" under "Main Origin", check that the "Certificate" section has an "SCTs" value of "3 valid SCTs".
2) Do the same for sha1-2017.badssl.com and check that the value is "0 SCTs".
3) Do the same for selecadm.name and check that the value is "2 valid SCTs". (This sanity checks that dynamically served SCTs are treated the same as embedded SCTs.)

Committed: https://crrev.com/9a2730e610983086ba1e2512506f79d682a6e270
Cr-Commit-Position: refs/heads/master@{#373420}

[lgarron, 2016-01-15 16:12:01]

Patchset #1 (id:1) has been deleted

[lgarron, 2016-01-15 16:13:19]

Description was changed from

==========
Plumb CT info to DevTools.

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

to

==========
Surface Certificate Transparency summaries for certs in the DevTools Security
panel.

TEST=Do the following:
1) Visit mozilla.org, open the Security panel in DevTools, refresh, click on
"https://www.mozilla.org" under "Main Origin", check that the "Certificate"
section has an "SCTs" value of "Present, valid, and sufficient for Extended
Validation (3 valid SCTs)".
2) Do the same for sha1-2017.badssl.com and check that the value is "Not present
(0 SCTs)".
==========

[lgarron, 2016-01-15 16:21:53]

Description was changed from

==========
Surface Certificate Transparency summaries for certs in the DevTools Security
panel.

TEST=Do the following:
1) Visit mozilla.org, open the Security panel in DevTools, refresh, click on
"https://www.mozilla.org" under "Main Origin", check that the "Certificate"
section has an "SCTs" value of "Present, valid, and sufficient for Extended
Validation (3 valid SCTs)".
2) Do the same for sha1-2017.badssl.com and check that the value is "Not present
(0 SCTs)".
==========

to

==========
Surface Certificate Transparency summaries for certs in the DevTools Security
panel.

BUG=551705
TEST=Do the following:
1) Visit mozilla.org, open the Security panel in DevTools, refresh, click on
"https://www.mozilla.org" under "Main Origin", check that the "Certificate"
section has an "SCTs" value of "Present, valid, and sufficient for Extended
Validation (3 valid SCTs)".
2) Do the same for sha1-2017.badssl.com and check that the value is "Not present
(0 SCTs)".
==========

[lgarron, 2016-01-15 16:32:50]

lgarron@chromium.org changed reviewers:
+ pfeldman@chromium.org, sky@chromium.org

[lgarron, 2016-01-15 16:32:51]

sky@Could you review chrome/browser/ui/browser.{h, cc}?
pfeldman@: Could you review the entire CL?

The string depends on EV status (which already takes into account the
requirement for multiple SCTs) and the number of unknown/valid/invalid
certificates. This CL plumps all the information flows along similar paths used
by the Security panel already.
Since SCT information is a property of the resource's SSL info rather than the
certificate itself [1], we pass the information down to the DevTools and make a
query to the browser with the information to get the string.

The logic for GetCertificateTransparencySummary() in the browser is based on the
current logic in [2], and implements the plan I discussed with eranm@ earlier
this week.

[1] Not all SCTs are embedded, and there is no way to look up SCTs for a given
cert using the cert store in `NetworkHandler::GetCertificateDetails()`.
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[lgarron, 2016-01-15 16:37:42]

Description was changed from

==========
Surface Certificate Transparency summaries for certs in the DevTools Security
panel.

BUG=551705
TEST=Do the following:
1) Visit mozilla.org, open the Security panel in DevTools, refresh, click on
"https://www.mozilla.org" under "Main Origin", check that the "Certificate"
section has an "SCTs" value of "Present, valid, and sufficient for Extended
Validation (3 valid SCTs)".
2) Do the same for sha1-2017.badssl.com and check that the value is "Not present
(0 SCTs)".
==========

to

==========
Surface Certificate Transparency summaries for certs in the DevTools Security
panel.

BUG=551705
TEST=Do the following:
1) Visit mozilla.org, open the Security panel in DevTools, refresh, click on
"https://www.mozilla.org" under "Main Origin", check that the "Certificate"
section has an "SCTs" value of "Present, valid, and sufficient for Extended
Validation (3 valid SCTs)".
2) Do the same for sha1-2017.badssl.com and check that the value is "Not present
(0 SCTs)".
3) Do the same for selecadm.name and check that the value is "Present and valid
(2 valid SCTs)".
==========

[sky, 2016-01-19 16:36:13]

browser LGTM

[pfeldman, 2016-01-19 17:54:49]

Please look at the protocol.json comment first.

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:183:
originState.certificateTransparencySummaryPromise =
request.target().networkManager.certificateTransparencySummaryPromise(securityDetails.certificateValidationDetails);
You issue the request here, but don't claim the result. We typically don't do
that.

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:801: var
validationDetails = originState.securityDetails.certificateValidationDetails;
Extract it above the check.

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:802: if
(validationDetails.numValidScts > 0)
if (validationDetails.numValidScts)
   ...

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:809: var
sctTypeCounts = (sctTypeList.length == 0) ? WebInspector.UIString("0 SCTs") :
sctTypeList.join(", ");
sctTypeList.length ? ...

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:811: if
(originState.certificateTransparencySummaryPromise) {
Issue the request here instead.

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:832:
sctValue = new Promise(executor);
Why do you use promise here?

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:846:
table.addRow("SCTs", sctValue);
You should only pass values into the UI components.

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:951: *
@param {string|!Node|!Promise<string>} value
Resolve value prior to getting here, we try to not use | unless necessary.

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:963:
function resolve(value) {
By now you are formatting {} poorly.

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/protocol.json (right):

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/protocol.json:1532: { "name": "isValidEV",
"type": "boolean", "description": "Whether the certificate is valid as an
Extended Validation (EV) certificate." },
These seem to match the ones returned with the SecurityDetails. Why is the
summary not a part of the details?

[Ryan Sleevi, 2016-01-19 19:39:00]

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

[Ryan Sleevi, 2016-01-19 19:39:00]

https://codereview.chromium.org/1589703002/diff/40001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/1589703002/diff/40001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:255:
static_cast<size_t>(std::numeric_limits<int>::max()));
SECURITY BUG: This is a security check hidden in a DCHECK, which will do nothing
in real code and overflow.

Please consider using safe_numerics or make this check a proper conditional.

https://codereview.chromium.org/1589703002/diff/40001/content/public/browser/...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/1589703002/diff/40001/content/public/browser/...
content/public/browser/web_contents_delegate.h:525: std::string* summary);
DESIGN: Please consider using the appropriate C++ types for this (size_t), as
per https://www.chromium.org/developers/coding-style#TOC-Types

I'm aware that you'll need to convert to int as you cross the boundary to JS,
but the safer way to do that is to keep your API preferring the safe size
(size_t) throughout, and in your bindings glue, doing the safe_numerics casts
there.

[lgarron, 2016-01-22 22:51:28]

https://codereview.chromium.org/1589703002/diff/40001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/1589703002/diff/40001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:255:
static_cast<size_t>(std::numeric_limits<int>::max()));
On 2016/01/19 at 19:39:00, Ryan Sleevi wrote:
> SECURITY BUG: This is a security check hidden in a DCHECK, which will do
nothing in real code and overflow.
> 
> Please consider using safe_numerics or make this check a proper conditional.

I've opted to what you recommend below and use size_t until the last possible
moment where we need to convert to int for the DevTools protocol.

https://codereview.chromium.org/1589703002/diff/40001/content/public/browser/...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/1589703002/diff/40001/content/public/browser/...
content/public/browser/web_contents_delegate.h:525: std::string* summary);
On 2016/01/19 at 19:39:00, Ryan Sleevi wrote:
> DESIGN: Please consider using the appropriate C++ types for this (size_t), as
per https://www.chromium.org/developers/coding-style#TOC-Types
> 
> I'm aware that you'll need to convert to int as you cross the boundary to JS,
but the safer way to do that is to keep your API preferring the safe size
(size_t) throughout, and in your bindings glue, doing the safe_numerics casts
there.

This *particular* function is actually the wrong side of the API [1], but I've
gone ahead and pushed the cases down into `InspectorResourceAgent.cpp`.
safe_numerics isn't available there, but I asked Justin and he recommended
safeCast() as a Blink alternative.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:183:
originState.certificateTransparencySummaryPromise =
request.target().networkManager.certificateTransparencySummaryPromise(securityDetails.certificateValidationDetails);
On 2016/01/19 at 17:54:49, pfeldman wrote:
> You issue the request here, but don't claim the result. We typically don't do
that.

I want to issue the request to the proper target for the request. I thought
copying the behaviour of certificateDetailsPromise made the most sense.

Alternatives I see:
- Have the originState explicitly hold on to the target (for the request whose
CT summary we care about).
- Wrap the promise call into a closure so that we can issue it later in the code
where we're about to claim it.
- Make it a documented DevTools protocol API guarantee (for now) that
certificateTransparencySummaryPromise() is target-independent, and use
WebInspector.multitargetNetworkManager below. I don't like this as much, but I'm
okay with it (until a need arises to change it, which may or may not happen in
the near future as CT policy becomes more mature).

Do you have a preference?

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:801: var
validationDetails = originState.securityDetails.certificateValidationDetails;
On 2016/01/19 at 17:54:49, pfeldman wrote:
> Extract it above the check.

Done.

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:802: if
(validationDetails.numValidScts > 0)
On 2016/01/19 at 17:54:49, pfeldman wrote:
> if (validationDetails.numValidScts)
>    ...

*grumble* Okay, done.

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:809: var
sctTypeCounts = (sctTypeList.length == 0) ? WebInspector.UIString("0 SCTs") :
sctTypeList.join(", ");
On 2016/01/19 at 17:54:49, pfeldman wrote:
> sctTypeList.length ? ...

I am checking if the list (of strings) I'm concatenating is empty and showing
another string in that case. Would you prefer something else? e.g.

    var sctTypeCounts = (sctTypeList.length) ? sctTypeList.join(", ") :
WebInspector.UIString("0 SCTs");

or

    var sctTypeCounts = sctTypeList.join(", ");
    if (sctTypeCounts === "") {
      WebInspector.UIString("0 SCTs")
    }

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:811: if
(originState.certificateTransparencySummaryPromise) {
On 2016/01/19 at 17:54:49, pfeldman wrote:
> Issue the request here instead.

See above.

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:832:
sctValue = new Promise(executor);
On 2016/01/19 at 17:54:49, pfeldman wrote:
> Why do you use promise here?

Removed in a refactor.

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:846:
table.addRow("SCTs", sctValue);
On 2016/01/19 at 17:54:49, pfeldman wrote:
> You should only pass values into the UI components.

I wanted to satisfy two design constraints:

- Be able to insert this row before/after whatever rows we'd like,
deterministically.
- Construct most of the table without waiting for the promise for this
particular row to resolve.

I've handled this in the next patch by modifying the addRow() function.

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:951: *
@param {string|!Node|!Promise<string>} value
On 2016/01/19 at 17:54:49, pfeldman wrote:
> Resolve value prior to getting here, we try to not use | unless necessary.

Rather than allowing a promise to introduce asynchronicity inside this function,
I've opted to return `valueDiv` so that it can be modified by the caller later.
Let me know if you think this is not a good solution.

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:963:
function resolve(value) {
On 2016/01/19 at 17:54:49, pfeldman wrote:
> By now you are formatting {} poorly.

Thanks for catching; some habits die hard without a linter/formatter. In any
case, this was the only place I made the mistake, and it's gone in the next
patch.

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/protocol.json (right):

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/protocol.json:1532: { "name": "isValidEV",
"type": "boolean", "description": "Whether the certificate is valid as an
Extended Validation (EV) certificate." },
On 2016/01/19 at 17:54:49, pfeldman wrote:
> These seem to match the ones returned with the SecurityDetails. Why is the
summary not a part of the details?

The calculation is a Chrome-specific policy that should be done in /chrome
(right now, it's done in
/chrome/browser/ui/website_settings/website_settings.cc).
In order to attach it to the details, we'd either need to calculate and attach
the string to the security info for every resource [1] (regardless of whether
DevTools is active), or do it in /content/child/web_url_loader_impl.cc or below
(which is inappropriate).

This CL contains the best way I currently know how to do it, which is to send
these values to DevTools, and have it send a request up to the browser when it
needs the summary string. Similar to how we handle certificate details (a
conditional query to the browser process) and certificateViewer (which needs to
call up to /chrome/browse).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo...

https://codereview.chromium.org/1589703002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/1589703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:948:
addRow: function(key, value)
Although I added a return value, I decided to keep the name addRow() to avoid
unnecessary line changes above.

pfeldman@: If you're happy with this approach but would prefer a different name
like `addEntry()`, I'm happy to change it. (Alternatively, I can just return the
`div` and rely on the caller to add its contents.)

[pfeldman, 2016-01-23 00:57:25]

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/protocol.json (right):

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/protocol.json:1532: { "name": "isValidEV",
"type": "boolean", "description": "Whether the certificate is valid as an
Extended Validation (EV) certificate." },
Is there a primary key that you could use to reference it? certificateId?

[Ryan Sleevi, 2016-01-23 01:00:50]

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/protocol.json (right):

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/protocol.json:1530: "description": "Returns a
phrase summarizing the Certificate Transparency state of a certificate with the
given parameters..",
This is tied to an individual socket, right? Because the CT status is
per-connection.

[lgarron, 2016-01-23 01:04:03]

On 2016/01/23 at 00:57:25, pfeldman wrote:
>
https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/devtools/protocol.json (right):
> 
>
https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/protocol.json:1532: { "name": "isValidEV",
"type": "boolean", "description": "Whether the certificate is valid as an
Extended Validation (EV) certificate." },
> Is there a primary key that you could use to reference it? certificateId?

Unfortunately not. There is a SignedCertificateTimestampStore. However, it only
stores individual SCTs. In order to look them up, we'd need a list of their IDs,
which is what I was trying to avoid by having counts of their types. (In
addition to that, it requires knowing the right renderer.)

https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...

[pfeldman, 2016-01-23 01:04:56]

https://codereview.chromium.org/1589703002/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1589703002/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1417: void
Browser::GetCertificateTransparencySummary(bool is_valid_ev,
So we are introducing remote debugging method so that we could format the string
here? Provided that this is low level info and we don't localize devtools,
cloning this message format would make more sense.

[lgarron, 2016-01-23 01:09:03]

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/protocol.json (right):

https://codereview.chromium.org/1589703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/protocol.json:1530: "description": "Returns a
phrase summarizing the Certificate Transparency state of a certificate with the
given parameters..",
On 2016/01/23 at 01:00:50, Ryan Sleevi wrote:
> This is tied to an individual socket, right? Because the CT status is
per-connection.

Yes, you're right, is a slightly sloppy description. At the DevTools level,
we're aware that this is request-specific, but currently still show results from
the first request to the origin.

How about changing "certificate" to "request"?

[lgarron, 2016-01-23 01:20:15]

https://codereview.chromium.org/1589703002/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1589703002/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1417: void
Browser::GetCertificateTransparencySummary(bool is_valid_ev,
On 2016/01/23 at 01:04:55, pfeldman wrote:
> So we are introducing remote debugging method so that we could format the
string here?

Yes, exactly. :-/

> Provided that this is low level info and we don't localize devtools, cloning
this message format would make more sense.

My impression (Ryan, feel free to chip in) is that this strictly a
//chrome-layer policy, and that lower levels should not make any assumptions
about the specific policy.
We can talk to the networking team about what they're comfortable with if you'd
prefer. Perhaps there's a way to do it near `web_url_loader_impl`.

[Ryan Sleevi, 2016-01-23 01:24:34]

https://codereview.chromium.org/1589703002/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1589703002/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1417: void
Browser::GetCertificateTransparencySummary(bool is_valid_ev,
On 2016/01/23 01:20:15, lgarron wrote:
> My impression (Ryan, feel free to chip in) is that this strictly a
> //chrome-layer policy, and that lower levels should not make any assumptions
> about the specific policy.
> We can talk to the networking team about what they're comfortable with if
you'd
> prefer. Perhaps there's a way to do it near `web_url_loader_impl`.

I'm not sure I understand what you're asking.

[lgarron, 2016-01-26 02:33:27]

https://codereview.chromium.org/1589703002/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1589703002/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1417: void
Browser::GetCertificateTransparencySummary(bool is_valid_ev,
On 2016/01/23 at 01:24:34, Ryan Sleevi wrote:
> On 2016/01/23 01:20:15, lgarron wrote:
> > My impression (Ryan, feel free to chip in) is that this strictly a
> > //chrome-layer policy, and that lower levels should not make any assumptions
> > about the specific policy.
> > We can talk to the networking team about what they're comfortable with if
you'd
> > prefer. Perhaps there's a way to do it near `web_url_loader_impl`.
> 
> I'm not sure I understand what you're asking.

Fundamentally, this CL is about calculating a string using the logic right below
this comment.

Two questions:
1) Does this logic need to live in //chrome?
2) If not, does this logic still need to live in the browser process?


{1: no, 2: no}
We can do what Pavel wants and implement this logic in SecurityPanel.js.


{1: no, 2: yes}
We can place the logic in `web_url_loader_impl` (specifically,
`content/child/web_url_loader_impl.cc::SetSecurityStyleAndDetails ()`).
This is the place where we deserialize security info and stick it into the
request's ResourceResponse, conditional on DevTools asking for us to attach this
info. [1]

(InspectorResourceAgent takes this ResourceResponse and uses it to send
information to DevTools at [1].)


{1: yes, 2: yes}
- a) Attach the string to the serialized security info for every request
(inefficient).
- b) Call up to `//chrome` before sending security info for a request to
DevTools (e.g. in `web_url_loader_impl`).
- c) Call up to `//chrome` using a new DevTools protocol command (current
approach in this CL).


My impression is that the answers are {1: yes, 2: yes}, and that c) is the most
idiomatic way to do this.


Here's what I meant with "near `web_url_loader_impl`":
- If the answer is {1: no, 2: yes}, then this is straightforward.
- If we want option b) for {1: yes, 2: yes} we need to find a way to call up to
//chrome from  `web_url_loader_impl`
- If I didn't quite lay out the options correctly, it may be appropriate to do
something else at `web_url_loader_impl`.


Ryan, which of these options seem acceptable to you?


[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Ryan Sleevi, 2016-01-26 22:45:34]

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

[Ryan Sleevi, 2016-01-26 22:45:34]

I'm honestly still a little confused, but I defer to estark here, who's working
on expect-ct and would be able to best tell you what her thoughts are, given the
changes.

I don't think, generally speaking, we should be inferring the policy based on
the num_valid_scts - the valid SCTs just means the SCTs come from known logs,
not whether or not the certificate complied with the CT policy. That's expressed
in the CertStatus.

So if your goal is to determine whether or not a given certificate complied with
CT policy, use CertStatus. If your goal is to explain the status for each of the
SCTs, then you should use the CTVerifyResult to figure out which of which,
rather than the number of them.

Whether or not a certificate matched CT policy is part of //net and exported
from there, and shouldn't be re-interpreted above.

I don't know if that fully answers what you're asking, but Emily can likely add
additional insight and guidance.

[lgarron, 2016-01-27 03:36:47]

On 2016/01/26 at 22:45:34, rsleevi wrote:
> I'm honestly still a little confused, but I defer to estark here, who's
working on expect-ct and would be able to best tell you what her thoughts are,
given the changes.
> 
> I don't think, generally speaking, we should be inferring the policy based on
the num_valid_scts - the valid SCTs just means the SCTs come from known logs,
not whether or not the certificate complied with the CT policy. That's expressed
in the CertStatus.
> 
> So if your goal is to determine whether or not a given certificate complied
with CT policy, use CertStatus. If your goal is to explain the status for each
of the SCTs, then you should use the CTVerifyResult to figure out which of
which, rather than the number of them.
> 
> Whether or not a certificate matched CT policy is part of //net and exported
from there, and shouldn't be re-interpreted above.
> 
> I don't know if that fully answers what you're asking, but Emily can likely
add additional insight and guidance.

[lgarron, 2016-01-27 21:02:32]

On 2016/01/27 at 03:36:47, lgarron wrote:
> On 2016/01/26 at 22:45:34, rsleevi wrote:
> > I'm honestly still a little confused, but I defer to estark here, who's
working on expect-ct and would be able to best tell you what her thoughts are,
given the changes.
> > 
> > I don't think, generally speaking, we should be inferring the policy based
on the num_valid_scts - the valid SCTs just means the SCTs come from known logs,
not whether or not the certificate complied with the CT policy. That's expressed
in the CertStatus.
> > 
> > So if your goal is to determine whether or not a given certificate complied
with CT policy, use CertStatus. If your goal is to explain the status for each
of the SCTs, then you should use the CTVerifyResult to figure out which of
which, rather than the number of them.
> > 
> > Whether or not a certificate matched CT policy is part of //net and exported
from there, and shouldn't be re-interpreted above.
> > 
> > I don't know if that fully answers what you're asking, but Emily can likely
add additional insight and guidance.

So, CertStatus has bits for IS_EV and CT_COMPLIANCE_FAILED.
However, CT_COMPLIANCE_FAILED is only set for EV certs (since I guess only EV
certs can fail compliance), and IS_EV is unset if CT_COMPLIANCE_FAILED is set.

If we don't look at SCTs, this allows us to distinguish three states: DV,
EV_CT_VERIFIED, and EV_CT_INVALID.

However, I'd ideally like to maintain at least the folllowing distinctions for
feature parity with connection tab:
1) For DV, I'd still like to highlight "had at least one valid SCT", since we
currently show a string for that in the connection tab (and want to encourage CT
adoption).
2) For EV, I'd also like to distinguish between CT_COMPLIANCE_FAILED and NO_CT.
(For example, there are domains like https://www.telesec.de that still have EV
certs without SCTs.)

Ryan/Emily: It seems that using CTVerifyResult to count the kinds of SCTs
requires sct_list_from_tls_extension, which is not available from the SSLStatus
in web_url_loader_impl. Is it acceptable to iterate over the
SignedCertificateTimestampIDStatusList to see whether 1) there are any valid
ones, and 2) there are any at all?
If so, I propose doing the following in `SetSecurityStyleAndDetails()` in
`web_url_loader_impl.cc`:

    // Summary string names are based on [1].
    if (IS_EV)
      summary = EV_CT_VERIFIED;
    else if (CT_COMPLIANCE_FAILED) {
      if (sct_list.empty())
        summary = EV_NO_CT;
      else
        summary = EV_CT_INVALID_OR_UNVERIFIED;
    } else {
      bool at_least_one_valid_SCT = false;
      for (sct in sct_list)
        if (sct is valid)
          at_least_one_valid_SCT = true;

      if (at_least_one_valid_SCT)
        summary = CT_VERIFIED;
      else
        summary = NO_CT;
    }

Eran: To avoid making assumptions: Are you okay with just a single string in
DevTools distinguishing DV, EV_CT_VERIFIED, and EV_CT_INVALID. Do 1) and 2)
matter to the CT team?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

(Apologies for the dearth of reference links. codereview.chromium.org ate my
draft comment last night, and I feel too defeated to reconstruct everything.)

[estark, 2016-01-27 21:59:58]

+certificate-transparency-chrome

Sorry for the slow reply, I've been conference-ing. I just skimmed the code and
will be able to take a closer look later, but my initial impression is that
right now the Connection tab doesn't seem to actually describe any policy checks
or anything like that -- it's just a string describing the number of
valid/invalid SCTs that were sent. So to have parity with that, I think it would
be okay to send an enum to devtools (HAS_VALID_SCTS, HAS_INVALID_SCTS,
HAS_UNKNOWN_SCTS, or whatever the values are that we care about) and construct
the string in the devtools based on the enum value.

At some point, I suspect we might want to be able to say something more useful,
like "This certificate's EV status was stripped because the SCTs were not
diverse enough." I'm right now chewing on a way to expose that, so that we can
send such information to devtools as well. (See
https://codereview.chromium.org/1578993003/ -- the tricky thing is how to deal
with the absence of this information when the resource has been cached.) But for
now, it seems like the goal is more-or-less just to describe whether SCTs were
sent or not, which seems to me like a string that we can just construct in
devtools rather than going back to the browser to fetch it.

As for the EV part, I'm in a hurry so might be mistaken, but it seems to me that
the Connection tab doesn't currently distinguish EV from non-EV as far as CT
goes. It only uses is_ev to decide whether to display the organization name,
etc. in the string. So it doesn't seem to me that we need to say anything
particularly special about EV/CT in devtools in order to maintain feature
parity.

[lgarron, 2016-01-27 23:05:02]

On 2016/01/27 at 21:59:58, estark wrote:
> +certificate-transparency-chrome
> 
> Sorry for the slow reply, I've been conference-ing. I just skimmed the code
and will be able to take a closer look later, but my initial impression is that
right now the Connection tab doesn't seem to actually describe any policy checks
or anything like that -- it's just a string describing the number of
valid/invalid SCTs that were sent. So to have parity with that, I think it would
be okay to send an enum to devtools (HAS_VALID_SCTS, HAS_INVALID_SCTS,
HAS_UNKNOWN_SCTS, or whatever the values are that we care about) and construct
the string in the devtools based on the enum value.
> 
> At some point, I suspect we might want to be able to say something more
useful, like "This certificate's EV status was stripped because the SCTs were
not diverse enough." I'm right now chewing on a way to expose that, so that we
can send such information to devtools as well. (See
https://codereview.chromium.org/1578993003/ -- the tricky thing is how to deal
with the absence of this information when the resource has been cached.) But for
now, it seems like the goal is more-or-less just to describe whether SCTs were
sent or not, which seems to me like a string that we can just construct in
devtools rather than going back to the browser to fetch it.
> 
> As for the EV part, I'm in a hurry so might be mistaken, but it seems to me
that the Connection tab doesn't currently distinguish EV from non-EV as far as
CT goes. It only uses is_ev to decide whether to display the organization name,
etc. in the string. So it doesn't seem to me that we need to say anything
particularly special about EV/CT in devtools in order to maintain feature
parity.

You're soft of right right that the connection tab logic nominally doesn't
distinguish between EV and DV.
However, when it says "valid" for EV, this implies !CT_COMPLIANCE_FAILED.
Therefore, the current EV_CT_VERIFIED string implicitly means "has enough SCTs
for policy" for EV certs rather than just "has SCTs".

I think the enum is fine, but we need to craft the enum values and DevTools
strings carefully to avoid implying that e.g.:
- HAS_VALID_SCTS doesn't mean "no invalid SCTs" for DV.
- HAS_INVALID_SCTS doesn't mean "no valid SCTs" for EV.
Even so, I'm not completely comfortable that the resulting string will be clear
enough for anyone moderately versed in CT to interpret unambiguously. So far,
the Security panel done a good job of stating the situation outright instead of
using obfuscated summaries.

In any case, do you have objections to iterating over sct_list in
`web_url_loader` to compute such an enum?

[estark, 2016-01-28 06:47:13]

On 2016/01/27 23:05:02, lgarron wrote:
> On 2016/01/27 at 21:59:58, estark wrote:
> > +certificate-transparency-chrome
> > 
> > Sorry for the slow reply, I've been conference-ing. I just skimmed the code
> and will be able to take a closer look later, but my initial impression is
that
> right now the Connection tab doesn't seem to actually describe any policy
checks
> or anything like that -- it's just a string describing the number of
> valid/invalid SCTs that were sent. So to have parity with that, I think it
would
> be okay to send an enum to devtools (HAS_VALID_SCTS, HAS_INVALID_SCTS,
> HAS_UNKNOWN_SCTS, or whatever the values are that we care about) and construct
> the string in the devtools based on the enum value.
> > 
> > At some point, I suspect we might want to be able to say something more
> useful, like "This certificate's EV status was stripped because the SCTs were
> not diverse enough." I'm right now chewing on a way to expose that, so that we
> can send such information to devtools as well. (See
> https://codereview.chromium.org/1578993003/ -- the tricky thing is how to deal
> with the absence of this information when the resource has been cached.) But
for
> now, it seems like the goal is more-or-less just to describe whether SCTs were
> sent or not, which seems to me like a string that we can just construct in
> devtools rather than going back to the browser to fetch it.
> > 
> > As for the EV part, I'm in a hurry so might be mistaken, but it seems to me
> that the Connection tab doesn't currently distinguish EV from non-EV as far as
> CT goes. It only uses is_ev to decide whether to display the organization
name,
> etc. in the string. So it doesn't seem to me that we need to say anything
> particularly special about EV/CT in devtools in order to maintain feature
> parity.
> 
> You're soft of right right that the connection tab logic nominally doesn't
> distinguish between EV and DV.
> However, when it says "valid" for EV, this implies !CT_COMPLIANCE_FAILED.
> Therefore, the current EV_CT_VERIFIED string implicitly means "has enough SCTs
> for policy" for EV certs rather than just "has SCTs".

Not necessarily, because the cert could have been on the EV whitelist. The
presence of the EV indicator implicitly means that the certificate complied with
the EV CT policy, but it doesn't necessarily say anything about the SCTs.

> 
> I think the enum is fine, but we need to craft the enum values and DevTools
> strings carefully to avoid implying that e.g.:
> - HAS_VALID_SCTS doesn't mean "no invalid SCTs" for DV.
> - HAS_INVALID_SCTS doesn't mean "no valid SCTs" for EV.
> Even so, I'm not completely comfortable that the resulting string will be
clear
> enough for anyone moderately versed in CT to interpret unambiguously. So far,
> the Security panel done a good job of stating the situation outright instead
of
> using obfuscated summaries.

I don't quite understand this... in the current patch set you have 5 strings
that you choose from, so why can't the enum values just correspond to those 5
strings? i.e. NO_SCTS, SCTS_VALID_EV, SCTS_VALID, SCTS_INVALID, and SCTS_UNKNOWN
(and presumably document exactly what each of them means)

Though, I would argue that SCTS_VALID_EV is maybe not even useful or necessary
-- or actually even slightly misleading. A site could have only a single valid
SCT -- which by itself is not sufficient for EV -- for a whitelisted cert, so
we'd be saying that the SCTs are sufficient for EV when in fact they're not, and
the site has only retained its EV status because it's whitelisted. The more
general point is that I think we don't currently expose enough information out
of net to be super explain-y about CT stuff: if a cert complied with EV policy,
we don't know why (it could have been whitelisted OR had sufficient SCTs, or
maybe it "complied" because we didn't actually have a policy verifier and so we
didn't check the policy at all), and if a cert didn't comply, we also don't know
why (it could have not had enough SCTs, or not diverse enough).

> 
> In any case, do you have objections to iterating over sct_list in
> `web_url_loader` to compute such an enum?

No, that seems fine to me.

[lgarron, 2016-01-28 20:45:26]

On 2016/01/28 at 06:47:13, estark wrote:
> On 2016/01/27 23:05:02, lgarron wrote:
> > On 2016/01/27 at 21:59:58, estark wrote:
> > > +certificate-transparency-chrome
> > > 
> > > Sorry for the slow reply, I've been conference-ing. I just skimmed the
code
> > and will be able to take a closer look later, but my initial impression is
that
> > right now the Connection tab doesn't seem to actually describe any policy
checks
> > or anything like that -- it's just a string describing the number of
> > valid/invalid SCTs that were sent. So to have parity with that, I think it
would
> > be okay to send an enum to devtools (HAS_VALID_SCTS, HAS_INVALID_SCTS,
> > HAS_UNKNOWN_SCTS, or whatever the values are that we care about) and
construct
> > the string in the devtools based on the enum value.
> > > 
> > > At some point, I suspect we might want to be able to say something more
> > useful, like "This certificate's EV status was stripped because the SCTs
were
> > not diverse enough." I'm right now chewing on a way to expose that, so that
we
> > can send such information to devtools as well. (See
> > https://codereview.chromium.org/1578993003/ -- the tricky thing is how to
deal
> > with the absence of this information when the resource has been cached.) But
for
> > now, it seems like the goal is more-or-less just to describe whether SCTs
were
> > sent or not, which seems to me like a string that we can just construct in
> > devtools rather than going back to the browser to fetch it.
> > > 
> > > As for the EV part, I'm in a hurry so might be mistaken, but it seems to
me
> > that the Connection tab doesn't currently distinguish EV from non-EV as far
as
> > CT goes. It only uses is_ev to decide whether to display the organization
name,
> > etc. in the string. So it doesn't seem to me that we need to say anything
> > particularly special about EV/CT in devtools in order to maintain feature
> > parity.
> > 
> > You're soft of right right that the connection tab logic nominally doesn't
> > distinguish between EV and DV.
> > However, when it says "valid" for EV, this implies !CT_COMPLIANCE_FAILED.
> > Therefore, the current EV_CT_VERIFIED string implicitly means "has enough
SCTs
> > for policy" for EV certs rather than just "has SCTs".
> 
> Not necessarily, because the cert could have been on the EV whitelist. The
presence of the EV indicator implicitly means that the certificate complied with
the EV CT policy, but it doesn't necessarily say anything about the SCTs.
> 
> > 
> > I think the enum is fine, but we need to craft the enum values and DevTools
> > strings carefully to avoid implying that e.g.:
> > - HAS_VALID_SCTS doesn't mean "no invalid SCTs" for DV.
> > - HAS_INVALID_SCTS doesn't mean "no valid SCTs" for EV.
> > Even so, I'm not completely comfortable that the resulting string will be
clear
> > enough for anyone moderately versed in CT to interpret unambiguously. So
far,
> > the Security panel done a good job of stating the situation outright instead
of
> > using obfuscated summaries.
> 
> I don't quite understand this... in the current patch set you have 5 strings
that you choose from, so why can't the enum values just correspond to those 5
strings? i.e. NO_SCTS, SCTS_VALID_EV, SCTS_VALID, SCTS_INVALID, and SCTS_UNKNOWN
(and presumably document exactly what each of them means)
> 
> Though, I would argue that SCTS_VALID_EV is maybe not even useful or necessary
-- or actually even slightly misleading. A site could have only a single valid
SCT -- which by itself is not sufficient for EV -- for a whitelisted cert, so
we'd be saying that the SCTs are sufficient for EV when in fact they're not, and
the site has only retained its EV status because it's whitelisted. The more
general point is that I think we don't currently expose enough information out
of net to be super explain-y about CT stuff: if a cert complied with EV policy,
we don't know why (it could have been whitelisted OR had sufficient SCTs, or
maybe it "complied" because we didn't actually have a policy verifier and so we
didn't check the policy at all), and if a cert didn't comply, we also don't know
why (it could have not had enough SCTs, or not diverse enough).
> 
> > 
> > In any case, do you have objections to iterating over sct_list in
> > `web_url_loader` to compute such an enum?
> 
> No, that seems fine to me.

You're right, I totally forgot about whitelisted EV certs.

What if we just send the counts we send now (valid, invalid, unknown), and
display them without any additional interpretation? (e.g. "0 SCTs", "4 valid
SCTs", "2 valid SCTs, 1 unknown SCT".)

That sounds most useful and unambiguous to me at this point.

[lgarron, 2016-01-28 20:50:51]

On 2016/01/28 at 20:45:26, lgarron wrote:
> On 2016/01/28 at 06:47:13, estark wrote:
> > On 2016/01/27 23:05:02, lgarron wrote:
> > > On 2016/01/27 at 21:59:58, estark wrote:
> > > > +certificate-transparency-chrome
> > > > 
> > > > Sorry for the slow reply, I've been conference-ing. I just skimmed the
code
> > > and will be able to take a closer look later, but my initial impression is
that
> > > right now the Connection tab doesn't seem to actually describe any policy
checks
> > > or anything like that -- it's just a string describing the number of
> > > valid/invalid SCTs that were sent. So to have parity with that, I think it
would
> > > be okay to send an enum to devtools (HAS_VALID_SCTS, HAS_INVALID_SCTS,
> > > HAS_UNKNOWN_SCTS, or whatever the values are that we care about) and
construct
> > > the string in the devtools based on the enum value.
> > > > 
> > > > At some point, I suspect we might want to be able to say something more
> > > useful, like "This certificate's EV status was stripped because the SCTs
were
> > > not diverse enough." I'm right now chewing on a way to expose that, so
that we
> > > can send such information to devtools as well. (See
> > > https://codereview.chromium.org/1578993003/ -- the tricky thing is how to
deal
> > > with the absence of this information when the resource has been cached.)
But for
> > > now, it seems like the goal is more-or-less just to describe whether SCTs
were
> > > sent or not, which seems to me like a string that we can just construct in
> > > devtools rather than going back to the browser to fetch it.
> > > > 
> > > > As for the EV part, I'm in a hurry so might be mistaken, but it seems to
me
> > > that the Connection tab doesn't currently distinguish EV from non-EV as
far as
> > > CT goes. It only uses is_ev to decide whether to display the organization
name,
> > > etc. in the string. So it doesn't seem to me that we need to say anything
> > > particularly special about EV/CT in devtools in order to maintain feature
> > > parity.
> > > 
> > > You're soft of right right that the connection tab logic nominally doesn't
> > > distinguish between EV and DV.
> > > However, when it says "valid" for EV, this implies !CT_COMPLIANCE_FAILED.
> > > Therefore, the current EV_CT_VERIFIED string implicitly means "has enough
SCTs
> > > for policy" for EV certs rather than just "has SCTs".
> > 
> > Not necessarily, because the cert could have been on the EV whitelist. The
presence of the EV indicator implicitly means that the certificate complied with
the EV CT policy, but it doesn't necessarily say anything about the SCTs.
> > 
> > > 
> > > I think the enum is fine, but we need to craft the enum values and
DevTools
> > > strings carefully to avoid implying that e.g.:
> > > - HAS_VALID_SCTS doesn't mean "no invalid SCTs" for DV.
> > > - HAS_INVALID_SCTS doesn't mean "no valid SCTs" for EV.
> > > Even so, I'm not completely comfortable that the resulting string will be
clear
> > > enough for anyone moderately versed in CT to interpret unambiguously. So
far,
> > > the Security panel done a good job of stating the situation outright
instead of
> > > using obfuscated summaries.
> > 
> > I don't quite understand this... in the current patch set you have 5 strings
that you choose from, so why can't the enum values just correspond to those 5
strings? i.e. NO_SCTS, SCTS_VALID_EV, SCTS_VALID, SCTS_INVALID, and SCTS_UNKNOWN
(and presumably document exactly what each of them means)
> > 
> > Though, I would argue that SCTS_VALID_EV is maybe not even useful or
necessary -- or actually even slightly misleading. A site could have only a
single valid SCT -- which by itself is not sufficient for EV -- for a
whitelisted cert, so we'd be saying that the SCTs are sufficient for EV when in
fact they're not, and the site has only retained its EV status because it's
whitelisted. The more general point is that I think we don't currently expose
enough information out of net to be super explain-y about CT stuff: if a cert
complied with EV policy, we don't know why (it could have been whitelisted OR
had sufficient SCTs, or maybe it "complied" because we didn't actually have a
policy verifier and so we didn't check the policy at all), and if a cert didn't
comply, we also don't know why (it could have not had enough SCTs, or not
diverse enough).
> > 
> > > 
> > > In any case, do you have objections to iterating over sct_list in
> > > `web_url_loader` to compute such an enum?
> > 
> > No, that seems fine to me.
> 
> You're right, I totally forgot about whitelisted EV certs.
> 
> What if we just send the counts we send now (valid, invalid, unknown), and
display them without any additional interpretation? (e.g. "0 SCTs", "4 valid
SCTs", "2 valid SCTs, 1 unknown SCT".)
> 
> That sounds most useful and unambiguous to me at this point.

(By "we send now" I mean "in the current patch of this CL".)

[estark, 2016-01-29 00:08:40]

On 2016/01/28 20:45:26, lgarron wrote:
> <snip>
> 
> What if we just send the counts we send now (valid, invalid, unknown), and
> display them without any additional interpretation? (e.g. "0 SCTs", "4 valid
> SCTs", "2 valid SCTs, 1 unknown SCT".)
> 
> That sounds most useful and unambiguous to me at this point.

That sounds fine to me, but I don't have a good intuition yet for what info
developers are looking for when it comes to CT, so maybe check with eranm@ how
that sounds to him.

[lgarron, 2016-01-30 01:35:42]

Description was changed from

==========
Surface Certificate Transparency summaries for certs in the DevTools Security
panel.

BUG=551705
TEST=Do the following:
1) Visit mozilla.org, open the Security panel in DevTools, refresh, click on
"https://www.mozilla.org" under "Main Origin", check that the "Certificate"
section has an "SCTs" value of "Present, valid, and sufficient for Extended
Validation (3 valid SCTs)".
2) Do the same for sha1-2017.badssl.com and check that the value is "Not present
(0 SCTs)".
3) Do the same for selecadm.name and check that the value is "Present and valid
(2 valid SCTs)".
==========

to

==========
Surface Certificate Transparency summaries for certs in the DevTools Security
panel.

BUG=551705
TEST=Do the following:
1) Visit mozilla.org, open the Security panel in DevTools, refresh, click on
"https://www.mozilla.org" under "Main Origin", check that the "Certificate"
section has an "SCTs" value of "3 valid SCTs".
2) Do the same for sha1-2017.badssl.com and check that the value is "0 SCTs".
3) Do the same for selecadm.name and check that the value is "2 valid SCTs".
(This sanity checks that dynamically served SCTs are treated the same as
embedded SCTs.)
==========

[lgarron, 2016-01-30 01:36:53]

Description was changed from

==========
Surface Certificate Transparency summaries for certs in the DevTools Security
panel.

BUG=551705
TEST=Do the following:
1) Visit mozilla.org, open the Security panel in DevTools, refresh, click on
"https://www.mozilla.org" under "Main Origin", check that the "Certificate"
section has an "SCTs" value of "3 valid SCTs".
2) Do the same for sha1-2017.badssl.com and check that the value is "0 SCTs".
3) Do the same for selecadm.name and check that the value is "2 valid SCTs".
(This sanity checks that dynamically served SCTs are treated the same as
embedded SCTs.)
==========

to

==========
Surface SCT (Signed Certificate Timestamp) counts in the Security panel.

BUG=551705
TEST=Do the following:
1) Visit mozilla.org, open the Security panel in DevTools, refresh, click on
"https://www.mozilla.org" under "Main Origin", check that the "Certificate"
section has an "SCTs" value of "3 valid SCTs".
2) Do the same for sha1-2017.badssl.com and check that the value is "0 SCTs".
3) Do the same for selecadm.name and check that the value is "2 valid SCTs".
(This sanity checks that dynamically served SCTs are treated the same as
embedded SCTs.)
==========

[lgarron, 2016-01-30 02:29:08]

pfeldman@: I've removed the string formatting command. Could you review again?

I asked eranm@ if he was happy with sending just the counts of
unknown/invalid/valid, and he was happy with that (the word he used was perfect
;-).

[lgarron, 2016-01-30 02:59:47]

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

[commit-bot: I haz the power, 2016-01-30 03:00:48]

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

[lgarron, 2016-01-30 04:26:03]

Patchset #5 (id:100001) has been deleted

[lgarron, 2016-02-01 22:30:49]

Patchset #5 (id:120001) has been deleted

[lgarron, 2016-02-01 22:31:42]

pfeldman@: I sent this out on the weekend. Friendly ping in case you missed it,
since I'm blocking on this CL.

[pfeldman, 2016-02-01 23:01:12]

https://codereview.chromium.org/1589703002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/1589703002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:889: if
(!details) {
drop {}

https://codereview.chromium.org/1589703002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:890:
return "N/A";
WebInspector.UIString()

https://codereview.chromium.org/1589703002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:900:
return (sctTypeList.length == 0) ? WebInspector.UIString("0 SCTs") :
sctTypeList.join(", ");
sctTypeList.length ? ...

https://codereview.chromium.org/1589703002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebURLResponse.h (right):

https://codereview.chromium.org/1589703002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebURLResponse.h:154: BLINK_PLATFORM_EXPORT
void setSecurityDetails(const WebString& protocol, const WebString& keyExchange,
const WebString& cipher, const WebString& mac, int certId, size_t
numUnknownScts, size_t numInvalidScts, size_t numValidScts);
Introduce WebSecurityDetails struct?

[lgarron, 2016-02-02 03:17:08]

https://codereview.chromium.org/1589703002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/1589703002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:889: if
(!details) {
On 2016/02/01 at 23:01:11, pfeldman wrote:
> drop {}

Done.

https://codereview.chromium.org/1589703002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:890:
return "N/A";
On 2016/02/01 at 23:01:11, pfeldman wrote:
> WebInspector.UIString()

Done. (I've also removed the WebInspector.UIString call in
`displayCertificateDetails()` to compensate.)

https://codereview.chromium.org/1589703002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:900:
return (sctTypeList.length == 0) ? WebInspector.UIString("0 SCTs") :
sctTypeList.join(", ");
On 2016/02/01 at 23:01:11, pfeldman wrote:
> sctTypeList.length ? ...

Done.

It still makes me uncomfortable, though, and I can't find anything in the style
guide about it. What is the reason?

https://codereview.chromium.org/1589703002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebURLResponse.h (right):

https://codereview.chromium.org/1589703002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebURLResponse.h:154: BLINK_PLATFORM_EXPORT
void setSecurityDetails(const WebString& protocol, const WebString& keyExchange,
const WebString& cipher, const WebString& mac, int certId, size_t
numUnknownScts, size_t numInvalidScts, size_t numValidScts);
On 2016/02/01 at 23:01:11, pfeldman wrote:
> Introduce WebSecurityDetails struct?

Done.

[lgarron, 2016-02-03 21:09:19]

pfeldman@: ping

(I know it's only been a day, but I'm hoping to land this and two dependent page
info CLs by Friday.)

[pfeldman, 2016-02-04 00:16:15]

lgtm % nit

https://codereview.chromium.org/1589703002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/1589703002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:796:
table.addRow("Subject", certificateDetails.subject.name);
These all should have WebInspector.UIString wrapper since they are user facing.

[lgarron, 2016-02-04 00:34:53]

On 2016/02/04 at 00:16:15, pfeldman wrote:
> lgtm % nit
> 
>
https://codereview.chromium.org/1589703002/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):
> 
>
https://codereview.chromium.org/1589703002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:796:
table.addRow("Subject", certificateDetails.subject.name);
> These all should have WebInspector.UIString wrapper since they are user
facing.

Thanks, I'll make a followup CL right now to do them all together in a logically
separate change.

[lgarron, 2016-02-04 00:34:54]

The CQ bit was checked by lgarron@chromium.org

[lgarron, 2016-02-04 00:34:54]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1589703002/#ps140001
(title: "Address latest comments.")

[commit-bot: I haz the power, 2016-02-04 00:36:31]

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

[commit-bot: I haz the power, 2016-02-04 01:57:03]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2016-02-04 01:58:17]

Description was changed from

==========
Surface SCT (Signed Certificate Timestamp) counts in the Security panel.

BUG=551705
TEST=Do the following:
1) Visit mozilla.org, open the Security panel in DevTools, refresh, click on
"https://www.mozilla.org" under "Main Origin", check that the "Certificate"
section has an "SCTs" value of "3 valid SCTs".
2) Do the same for sha1-2017.badssl.com and check that the value is "0 SCTs".
3) Do the same for selecadm.name and check that the value is "2 valid SCTs".
(This sanity checks that dynamically served SCTs are treated the same as
embedded SCTs.)
==========

to

==========
Surface SCT (Signed Certificate Timestamp) counts in the Security panel.

BUG=551705
TEST=Do the following:
1) Visit mozilla.org, open the Security panel in DevTools, refresh, click on
"https://www.mozilla.org" under "Main Origin", check that the "Certificate"
section has an "SCTs" value of "3 valid SCTs".
2) Do the same for sha1-2017.badssl.com and check that the value is "0 SCTs".
3) Do the same for selecadm.name and check that the value is "2 valid SCTs".
(This sanity checks that dynamically served SCTs are treated the same as
embedded SCTs.)

Committed: https://crrev.com/9a2730e610983086ba1e2512506f79d682a6e270
Cr-Commit-Position: refs/heads/master@{#373420}
==========

[commit-bot: I haz the power, 2016-02-04 01:58:18]

Patchset 5 (id:??) landed as
https://crrev.com/9a2730e610983086ba1e2512506f79d682a6e270
Cr-Commit-Position: refs/heads/master@{#373420}