Closed
Bug 1325309
Opened 7 years ago
Closed 7 years ago
TEST-UNEXPECTED-FAIL | services/common/tests/unit/test_blocklist_pinning.js
Categories
(Thunderbird :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 53.0
People
(Reporter: jorgk-bmo, Assigned: jorgk-bmo)
References
Details
(Keywords: intermittent-failure)
Attachments
(1 file, 1 obsolete file)
2.18 KB,
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=a844bb9580da7b60946d7592d772c7ef5b79da71&selectedJob=55358 TEST-UNEXPECTED-FAIL | services/common/tests/unit/test_blocklist_pinning.js | xpcshell return code: 1 TEST-UNEXPECTED-FAIL | services/common/tests/unit/test_blocklist_pinning.js | test_something - [test_something : 111] false == true PROCESS | 10248 | FATAL ERROR: AsyncShutdown timeout in profile-before-change Conditions: [{"name":"Sqlite.jsm shutdown blocker","state":{"description":"Waiting for connections to close","state":[{"name":"kinto.sqlite#1: waiting for shutdown","state":{"identifier":"kinto.sqlite#1","isCloseRequested":false,"hasDbConn":true,"hasInProgressTransaction":false,"pendingStatements":0,"statementCounter":4},"filename":"resource://gre/modules/Sqlite.jsm","lineNumber":259,"stack":["reso Caused by new test created in bug 1306470: https://hg.mozilla.org/mozilla-central/rev/3e31e8cddf83 Mark, can you please advise us on what we need to do to get this test working for the Thunderbird Xpcshell test suite.
Flags: needinfo?(mgoodwin)
Assignee | ||
Comment 1•7 years ago
|
||
Looking at https://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-win32/1482354112/comm-central_win7_ix_test-xpcshell-bm112-tests1-windows-build0.txt.gz: WARNING - TEST-UNEXPECTED-FAIL | services/common/tests/unit/test_blocklist_pinning.js | test_something - [test_something : 111] false == So it fails on line 111 of the test which is ... === // ensure our pins are all missing before we start ok(!sss.isSecureHost(sss.HEADER_HPKP, "one.example.com", 0)); ok(!sss.isSecureHost(sss.HEADER_HPKP, "two.example.com", 0)); ok(!sss.isSecureHost(sss.HEADER_HPKP, "three.example.com", 0)); // Test an empty db populates let result = yield PinningPreloadClient.maybeSync(2000, Date.now()); let connection = yield FirefoxAdapter.openConnection({path: KINTO_STORAGE_PATH}); // Open the collection, verify it's been populated: // Our test data has a single record; it should be in the local collection let collection = do_get_kinto_collection(connection, COLLECTION_NAME); let list = yield collection.list(); do_check_eq(list.data.length, 1); // check that a pin exists for one.example.com ok(sss.isSecureHost(sss.HEADER_HPKP, "one.example.com", 0)); <== line 111 ===
Comment 2•7 years ago
|
||
From a bit of poking around, I think the issue here is that this is a test for a key pinning feature - which (suprisingly, perhaps?) seems to be disabled on Thunderbird. security.cert_pinning.enforcement_level has a default value of 0 - which is pinningDisabled So possible some possible fixes are (in no particular order): 1) Enable pinning for Thunderbird 2) Disable the test for Thunderbird 3) Modify the test to enable pinning for the duration of the test I don't know which of the above is the most appropriate for your needs but please let me know if I can help at all with any of these.
Flags: needinfo?(mgoodwin)
Assignee | ||
Comment 3•7 years ago
|
||
Mark, thanks for your comment. With this patch, setting the preference, the test passes: mozilla/mach xpcshell-test services/common/tests/unit/test_blocklist_pinning.js Setting the preference can be found in IM https://dxr.mozilla.org/comm-central/rev/31a0301ffb6abc35496db987a1b0b0a66a28e805/im/app/profile/all-instantbird.js#223 and was most probably copied form here: https://dxr.mozilla.org/comm-central/rev/c36fbe84042debef0a5d58b7fc88185b401762ce/mozilla/browser/app/profile/firefox.js#1227 Any chance to improve the comment? 1 = allow MITM for certificate pinning checks. MITM is "man in the middle", right? - so this reads: allow "man in the middle" for certificate pinning checks. Really, we want to allow a man in the middle?? I thought the idea is to make the man in the middle's life as hard as possible.
Comment 4•7 years ago
|
||
(In reply to Jorg K (GMT+1) from comment #3) > Created attachment 8821160 [details] [diff] [review] > 1325309-cert_pinning.patch > > Mark, thanks for your comment. > > With this patch, setting the preference, the test passes: > mozilla/mach xpcshell-test > services/common/tests/unit/test_blocklist_pinning.js > > Setting the preference can be found in IM > https://dxr.mozilla.org/comm-central/rev/ > 31a0301ffb6abc35496db987a1b0b0a66a28e805/im/app/profile/all-instantbird. > js#223 > and was most probably copied form here: > https://dxr.mozilla.org/comm-central/rev/ > c36fbe84042debef0a5d58b7fc88185b401762ce/mozilla/browser/app/profile/firefox. > js#1227 > > Any chance to improve the comment? > 1 = allow MITM for certificate pinning checks. > > MITM is "man in the middle", right? - so this reads: > allow "man in the middle" for certificate pinning checks. > > Really, we want to allow a man in the middle?? I thought the idea is to make > the man in the middle's life as hard as possible. Yes, MITM is "man in the middle". We could improve the comment, I guess. To explain, this pref corresponds to the values of the PinningMode enum (see https://dxr.mozilla.org/mozilla-central/source/security/certverifier/CertVerifier.h#151). The intent of the pinningAllowUserCAMITM default is to allow users with local roots to have all certs issued by that local root accepted by the browser. Common use-cases for this include: 1) Antivirus software (it's common for A/V software to run a local MITM proxy which dynamically creates certificates for the sites a user is visiting) 2) Web filters. It's common for company networks to have a device which MITMs users' TLS traffic. 3) Security tools. Web sec. testing tools (like OWASP ZAP) will terminate TLS to allow you to interactively inspect and modify traffic.
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to Mark Goodwin [:mgoodwin] from comment #4) > Yes, MITM is "man in the middle". We could improve the comment, I guess. Care to make a suggestion? How about: 1 = allow local proxy (man in the middle) for certificate pinning checks.
Comment 6•7 years ago
|
||
Comment on attachment 8821160 [details] [diff] [review] 1325309-cert_pinning.patch Review of attachment 8821160 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. If you do improve the comment, please do it in all places!
Attachment #8821160 -
Flags: review?(aleth) → review+
Assignee | ||
Comment 7•7 years ago
|
||
OK, let's go with: 1 = allow "Man In The Middle" (local proxy, web filter, etc.) for certificate pinning checks.
Assignee | ||
Comment 8•7 years ago
|
||
I'll push this in a little while when the current push turns green.
Attachment #8821160 -
Attachment is obsolete: true
Attachment #8821189 -
Flags: review+
Assignee | ||
Comment 9•7 years ago
|
||
https://hg.mozilla.org/comm-central/rev/cfe5bca5b5255cdddf4e474a8fa8bc5d94af1e70
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
You need to log in
before you can comment on or make changes to this bug.
Description
•