Closed
Bug 1313511
Opened 8 years ago
Closed 7 years ago
Certificate Transparency UI for network monitor security tab
Categories
(DevTools :: Netmonitor, defect, P2)
Tracking
(firefox61 fixed)
RESOLVED
FIXED
Firefox 61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: jryans, Assigned: vincent)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
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.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → vi.le
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 years ago
|
||
Hello,
I'll work on this :-) I have a protoype that seems to work.
Vincent
Hi Vincent - looks like you're at step 4 here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction#Step_4_-_Get_your_code_reviewed
Assignee | ||
Comment 4•7 years ago
|
||
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
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
I submitted the patch. The pref 'security.pki.certificate_transparency.mode' must be set to 1 to activate the certificate transparency checks.
Comment 7•7 years ago
|
||
mozreview-review |
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
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 11•7 years ago
|
||
mozreview-review |
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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•7 years ago
|
||
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 14•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•7 years ago
|
||
mozreview-review-reply |
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 18•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 19•7 years ago
|
||
Assignee | ||
Comment 20•7 years ago
|
||
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)
Comment 21•7 years ago
|
||
(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)
Comment 22•7 years ago
|
||
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)
Assignee | ||
Comment 23•7 years ago
|
||
Honza,
if the tests failures are unrelated, is it possible to merge this?
Vincent
Flags: needinfo?(odvarko)
Comment 24•7 years ago
|
||
(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)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(vi.le)
Keywords: checkin-needed
Assignee | ||
Comment 25•7 years ago
|
||
Done :-)
Comment 26•7 years ago
|
||
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
Comment 27•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Comment 28•7 years ago
|
||
Some localizers mentioned the fact both strings containing "SCTs records" may need to use "SCT records" instead. Thoughts?
Assignee | ||
Comment 29•7 years ago
|
||
(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
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•