Closed Bug 1313511 Opened 8 years ago Closed 6 years ago

Certificate Transparency UI for network monitor security tab

Categories

(DevTools :: Netmonitor, defect, P2)

Product:
DevTools
Component:
Netmonitor
Version:
49 Branch
Type:
defect

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: jryans, Assigned: vincent)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1313511 - Add certificate transparency status in the netmonitor security tab;
59 bytes, text/x-review-board-request
Honza
: review+
Details 
In bug 1305289, the page info window now has support for showing certificate transparency status.

It seems useful to show some version of this info in the Network Monitor's security tab as well.
Yes, good catch, thanks for the report.

Honza
Priority: -- → P2
Assignee: nobody → vi.le
Status: NEW → ASSIGNED
Hello,

I'll work on this :-) I have a protoype that seems to work.

Vincent
Hello David,

Thank you for your help. I will submit a patch when the code will be clean enough and the tests are updated.

Vincent
I submitted the patch. The pref 'security.pki.certificate_transparency.mode' must be set to 1 to activate the certificate transparency checks.
Comment on attachment 8954911 [details]
Bug 1313511 - Add certificate transparency status in the netmonitor security tab;

https://reviewboard.mozilla.org/r/224074/#review230160

Thanks for working on this!

Looks good to me just one comment. Can we put the new 'Certificate transparency' field under 'Certificate' group?

The structure would be like as follows:

- Connection
- Host
+ Certificate
  - Issued To
  - Issued By
  - Period of Validity
  - Fingerprints
  - Transparency


Honza
Attachment #8954911 - Flags: review?(odvarko)
Thank you for the review :-)

That's done. I have also changed the translation labels to be more consistent with the rest of the translation strings in the security tab.
Comment on attachment 8954911 [details]
Bug 1313511 - Add certificate transparency status in the netmonitor security tab;

https://reviewboard.mozilla.org/r/224074/#review230902

I am seeing a failure when running the test locally (Win10)

Overall Summary
mochitest-browser: 39/40
  TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_security-details.js | Label has the expected value. - Got <Not Available>, expected Not enough SCTs
Stack trace:
    chrome://mochikit/content/browser-test.js:test_is:1271
    chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_security-details.js:null:82
    Tester_execTest@chrome://mochikit/content/browser-test.js:1058:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:953:9
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59


I am using --verify arg when running the test
(you might also want to use --headless):

mach test devtools/client/netmonitor/test/browser_net_security-details.js --verify --headless


Honza
Attachment #8954911 - Flags: review?(odvarko)
Thank you for the tip! I did not know about this so I was just running 'mach test devtools/client/netmonitor/test/browser_net_security-details.js' and it did not report any errors. I changed the test so it passes with --verify.

Vincent
Comment on attachment 8954911 [details]
Bug 1313511 - Add certificate transparency status in the netmonitor security tab;

https://reviewboard.mozilla.org/r/224074/#review233156

Looks good, the test passes for me now.

Please make sure to rebase the patch on top o m-c (there is a conflict)

There is also one inline comment to resolve.

Honza

::: devtools/client/netmonitor/test/browser_net_security-details.js:13
(Diff revision 4)
>   */
>  
>  add_task(function* () {
>    let { tab, monitor } = yield initNetMonitor(CUSTOM_GET_URL);
>    let { document, store, windowRequire } = monitor.panelWin;
> +  Services.prefs.setIntPref("security.pki.certificate_transparency.mode", 1);

Please use pushPref, like here:
https://searchfox.org/mozilla-central/rev/99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/devtools/client/netmonitor/test/browser_net_cached-status.js#12

This ensures that the pref will auto-revert its value after the test finishes.

And move it at the very beggining of the test, like as follows:

add_task(async function () {
  await pushPref("security.pki.certificate_transparency.mode", 1);

  let { tab, monitor } = await initNetMonitor(CUSTOM_GET_URL);
  
  ...
Attachment #8954911 - Flags: review?(odvarko)
Comment on attachment 8954911 [details]
Bug 1313511 - Add certificate transparency status in the netmonitor security tab;

https://reviewboard.mozilla.org/r/224074/#review233156

I have rebased the patch against latest m-c :-)

> Please use pushPref, like here:
> https://searchfox.org/mozilla-central/rev/99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/devtools/client/netmonitor/test/browser_net_cached-status.js#12
> 
> This ensures that the pref will auto-revert its value after the test finishes.
> 
> And move it at the very beggining of the test, like as follows:
> 
> add_task(async function () {
>   await pushPref("security.pki.certificate_transparency.mode", 1);
> 
>   let { tab, monitor } = await initNetMonitor(CUSTOM_GET_URL);
>   
>   ...

Done, thanks!
Comment on attachment 8954911 [details]
Bug 1313511 - Add certificate transparency status in the netmonitor security tab;

https://reviewboard.mozilla.org/r/224074/#review233380

Thanks!

Honza
Attachment #8954911 - Flags: review?(odvarko) → review+
Honza, I'm not sure about how to interpret the failures in the try push. The mochitests are mosly passing but sometimes they are failures. It seems to me that they are not related to the security panel but I can be wrong. What do you think?

Vincent
Flags: needinfo?(odvarko)
(In reply to Vincent Lequertier from comment #20)
> Honza, I'm not sure about how to interpret the failures in the try push. The
> mochitests are mosly passing but sometimes they are failures. It seems to me
> that they are not related to the security panel but I can be wrong. What do
> you think?
Agree, these seem to be unrelated.

Patrick, there are many failures related to webaudioeditor:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1ddc849d2b21b7aca08ba52859358265d7539f9&selectedJob=168023941

..and also animation:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1ddc849d2b21b7aca08ba52859358265d7539f9&selectedJob=168026976

Is this known?

Honza
Flags: needinfo?(odvarko) → needinfo?(pbrosset)
Yes, there's currently a pretty frequent intermittent in the animation inspector bug 1445291, which is being investigated already.
As for the webaudio, I don't have any idea. I don't think this is related to webaudio in fact, it only happens on the JSDCov platform which is where we run our code coverage system. This might be related.
Flags: needinfo?(pbrosset)
Honza,

if the tests failures are unrelated, is it possible to merge this?

Vincent
Flags: needinfo?(odvarko)
(In reply to Vincent Lequertier from comment #23)
> Honza,
> 
> if the tests failures are unrelated, is it possible to merge this?
Yes, this can land.

You need to properly mark this bug as checkin-needed, so one of the sheriffs can land it for you

1) Click the blue Edit bug button at the top
2) Go to the Tracking section and put `checkin-needed` flag into Keywords field
3) Submit


Thanks for working on this!
Honza
Flags: needinfo?(odvarko) → needinfo?(vi.le)
Flags: needinfo?(vi.le)
Keywords: checkin-needed
Done :-)
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0dbf4c26fad9
Add certificate transparency status in the netmonitor security tab; r=Honza
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0dbf4c26fad9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Some localizers mentioned the fact both strings containing "SCTs records" may need to use "SCT records" instead. Thoughts?
(In reply to Ton [:Tonnes] from comment #28)
> Some localizers mentioned the fact both strings containing "SCTs records"
> may need to use "SCT records" instead. Thoughts?

Good catch! I created this bug to fix the mistake: https://bugzilla.mozilla.org/show_bug.cgi?id=1453435

Vincent
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: