Closed Bug 1564221 Opened 2 months ago Closed Last month

Make nsITransportSecurityInfo builtinclass

Categories

(Core :: Security: PSM, task, P1)

task

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: barret, Assigned: barret)

References

(Blocks 1 open bug)

Details

(Whiteboard: [psm-assigned])

Attachments

(9 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

nsITransportSecurityInfo is presently not a builtinclass interface since several tests create a JS implemented nsITransportSecurityInfo in order to manually modify the cert. This not being builtinclass blocks

After discussing this with :keeler, the way forward is to

  1. Add a contract ID for nsITransportSecurityInfo so that tests that only need the existence of this object can create one.
  2. Add support to the test TLS server for setting the headers that these tests are testing.
  3. Translate all the tests manually testing headers to use the machinery provided in (2).
Priority: -- → P1
Whiteboard: [psm-assigned]

After further discussion, we've decided to not do (2) since neither side of the connection actually speaks HTTP. Instead, the tests will be updated to use the callback provided by add_connection_test.

There is now a contract ID for nsITransportSecurityInfo, allowing
mozilla::psm::TransportSecurityInfo instances to be created from JS. Tests
using a JS-implemented nsITransportSecurityInfo that were not modifying,
e.g., the serverCert attribute have been updated to create a
mozilla::psm::TransportSecurityInfo via the contract.

As part of making nsITranportSecurityInfo builtinclass, we can no longer use
JS-implemented nsITransportSecurityInfo instances in test cases. This patch
migrates test_forget_about_site_security_headers.js to useadd_connection_test()to get a validnsITransportSecurityInfo` instance for
the unit tests.

To make this work, we also need default-ee cert and keys, as well as an
alternate.key (required as the subject key for
a.pinning2.example.com-pinningroot.pem) in test_pinning_dynamic, or the
tests will fail due to certificate errors.

Depends on D40346

As part of making nsITranportSecurityInfo builtinclass, we can no longer use
JS-implemented nsITransportSecurityInfo instances in test cases. This patch
migrates test_ocsp_must_staple.js to use add_connection_test() to get a
valid nsITransportSecurityInfo instance for the unit tests.

Depends on D40347

As part of making nsITranportSecurityInfo builtinclass, we can no longer use
JS-implemented nsITransportSecurityInfo instances in test cases. This patch
migrates test_pinning_header_parsing.js to use add_connection_test() to get
a valid nsITransportSecurityInfo instance for the unit tests.

Depends on D40348

As part of making nsITranportSecurityInfo builtinclass, we can no longer
use JS-implemented nsITransportSecurityInfo instances in test cases.
This patch migrates test_sss_enumerate.js to use add_connection_test() to
get a valid nsITransportSecurityInfo instance for the unit tests.

Depends on D40349

As part of making nsITranportSecurityInfo builtinclass, we can no longer use
JS-implemented nsITransportSecurityInfo instances in test cases. This patch
migrates test_sss_originAttributes.js to use add_connection_test() to get a
valid nsITransportSecurityInfo instance for the unit tests.

Depends on D40350

As part of making nsITranportSecurityInfo builtinclass, we can no longer
use JS-implemented nsITransportSecurityInfo instances in test cases.
This patch migrates test_sss_resetState.js to use add_connection_test() to
get a valid nsITransportSecurityInfo instance for the unit tests.

Depends on D40351

There are no longer any consumers of the JS-implemented
FakeTransportSecurityInfo class, so it can be removed. That removes the last
JS-implemented nsITransportSecurityInfo instance and it therefore can be
marked builtinclass.

Depends on D40352

The MockSecurityInfo instances in the patched devtools tests are not actually
being used as nsITransportSecurityInfo instances; while QueryInterface
methods were generated for the them, these were never called. Additionally, the
methods they are being passed to are not XPCOM-defined and therefore do not
strictly require nsITransportSecurityInfo.

Pushed by brennie@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bc933f236434
Add a contract ID for nsITransportSecurityInfo r=keeler
https://hg.mozilla.org/integration/autoland/rev/dc76eeb3a74a
Do not use FakeTransportSecurityInfo in test_forget_about_site_security_headers.js r=keeler
https://hg.mozilla.org/integration/autoland/rev/02a324e713d6
Do not use FakeTransportSecurityInfo in test_ocsp_must_staple.js r=keeler
https://hg.mozilla.org/integration/autoland/rev/a140da3467e0
Do not use FakeTransportSecurityInfo in test_pinning_header_parsing.js r=keeler
https://hg.mozilla.org/integration/autoland/rev/31f730109760
Do not use FakeTransportSecurityInfo in test_sss_enumerate.js r=keeler
https://hg.mozilla.org/integration/autoland/rev/ad7a644c5a8d
Do not use FakeTransportSecurityInfo in test_sss_originAttributes.js r=keeler
https://hg.mozilla.org/integration/autoland/rev/8c3157ad3ac9
Do not use FakeTransportSecurityInfo in test_sss_resetState.js r=keeler
https://hg.mozilla.org/integration/autoland/rev/12d1607c1415
Make nsITransportSecurityInfo builtinclass r=keeler
https://hg.mozilla.org/integration/autoland/rev/36e33a3b59f0
Remove QueryInterface parameter from MockSecurityInfo in devtools tests r=ochameau

Latest patch should address these failures.

Flags: needinfo?(brennie)
Pushed by brennie@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4cd81a6d3b25
Add a contract ID for nsITransportSecurityInfo r=keeler
https://hg.mozilla.org/integration/autoland/rev/e804c46a9541
Do not use FakeTransportSecurityInfo in test_forget_about_site_security_headers.js r=keeler
https://hg.mozilla.org/integration/autoland/rev/63d578684d29
Do not use FakeTransportSecurityInfo in test_ocsp_must_staple.js r=keeler
https://hg.mozilla.org/integration/autoland/rev/ede7219b9a9e
Do not use FakeTransportSecurityInfo in test_pinning_header_parsing.js r=keeler
https://hg.mozilla.org/integration/autoland/rev/6cd17e69d1c7
Do not use FakeTransportSecurityInfo in test_sss_enumerate.js r=keeler
https://hg.mozilla.org/integration/autoland/rev/a1947eef7d86
Do not use FakeTransportSecurityInfo in test_sss_originAttributes.js r=keeler
https://hg.mozilla.org/integration/autoland/rev/aaa8ffb687f2
Do not use FakeTransportSecurityInfo in test_sss_resetState.js r=keeler
https://hg.mozilla.org/integration/autoland/rev/0efeb9b1f5fa
Make nsITransportSecurityInfo builtinclass r=keeler
https://hg.mozilla.org/integration/autoland/rev/bcae1e55fc27
Remove QueryInterface parameter from MockSecurityInfo in devtools tests r=ochameau,nchevobbe

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&fromchange=bcae1e55fc27a2df59f94f233cf661d1c62fb87e&tochange=eb80ebd4ac1d91d04e842658f75860d859f4dd11&searchStr=devtools&selectedJob=262340222

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=262340222&repo=autoland&lineNumber=13477

Backout link: https://hg.mozilla.org/integration/autoland/rev/4270a51c13610f43de1bec53fa717a1620524cd5

task 2019-08-19T21:01:02.491Z] 21:01:02 INFO - TEST-PASS | devtools/client/netmonitor/test/browser_net_security-redirect.js | There were two requests due to redirect. -
[task 2019-08-19T21:01:02.491Z] 21:01:02 INFO - Buffered messages finished
[task 2019-08-19T21:01:02.493Z] 21:01:02 INFO - TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_security-redirect.js | Initial request was marked insecure for domain column. -
[task 2019-08-19T21:01:02.494Z] 21:01:02 INFO - Stack trace:
[task 2019-08-19T21:01:02.494Z] 21:01:02 INFO - chrome://mochikit/content/browser-test.js:test_ok:1576
[task 2019-08-19T21:01:02.494Z] 21:01:02 INFO - chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_security-redirect.js:null:39
[task 2019-08-19T21:01:02.495Z] 21:01:02 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1346
[task 2019-08-19T21:01:02.495Z] 21:01:02 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1381
[task 2019-08-19T21:01:02.496Z] 21:01:02 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1209
[task 2019-08-19T21:01:02.496Z] 21:01:02 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
[task 2019-08-19T21:01:02.496Z] 21:01:02 INFO - TEST-PASS | devtools/client/netmonitor/test/browser_net_security-redirect.js | Redirected request was marked secure for domain column. -
[task 2019-08-19T21:01:02.497Z] 21:01:02 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-08-19T21:01:02.497Z] 21:01:02 INFO - TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_security-redirect.js | Initial request was marked insecure for URL column. -
[task 2019-08-19T21:01:02.498Z] 21:01:02 INFO - Stack trace:
[task 2019-08-19T21:01:02.498Z] 21:01:02 INFO - chrome://mochikit/content/browser-test.js:test_ok:1576
[task 2019-08-19T21:01:02.498Z] 21:01:02 INFO - chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_security-redirect.js:null:49
[task 2019-08-19T21:01:02.499Z] 21:01:02 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1346
[task 2019-08-19T21:01:02.499Z] 21:01:02 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1381
[task 2019-08-19T21:01:02.499Z] 21:01:02 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1209
[task 2019-08-19T21:01:02.500Z] 21:01:02 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803

Flags: needinfo?(brennie)
Pushed by brennie@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/15994e94ce79
Add a contract ID for nsITransportSecurityInfo r=keeler
https://hg.mozilla.org/integration/autoland/rev/1877f9c9aeeb
Do not use FakeTransportSecurityInfo in test_forget_about_site_security_headers.js r=keeler
https://hg.mozilla.org/integration/autoland/rev/d01f8050aa3b
Do not use FakeTransportSecurityInfo in test_ocsp_must_staple.js r=keeler
https://hg.mozilla.org/integration/autoland/rev/09b75b688829
Do not use FakeTransportSecurityInfo in test_pinning_header_parsing.js r=keeler
https://hg.mozilla.org/integration/autoland/rev/72e97b86ce0b
Do not use FakeTransportSecurityInfo in test_sss_enumerate.js r=keeler
https://hg.mozilla.org/integration/autoland/rev/efd936e4cafd
Do not use FakeTransportSecurityInfo in test_sss_originAttributes.js r=keeler
https://hg.mozilla.org/integration/autoland/rev/1023f2ecd9b5
Do not use FakeTransportSecurityInfo in test_sss_resetState.js r=keeler
https://hg.mozilla.org/integration/autoland/rev/6cfbe7c8ad5f
Make nsITransportSecurityInfo builtinclass r=keeler
https://hg.mozilla.org/integration/autoland/rev/acd7b8cc02ab
Remove QueryInterface parameter from MockSecurityInfo in devtools tests r=ochameau,nchevobbe

Trivial fix for this, rolled up into last commit.

Flags: needinfo?(brennie)
You need to log in before you can comment on or make changes to this bug.