Conservative TLS settings prevent connecting to a TLS 1.3-only proxy
Categories
(Core :: Networking: HTTP, defect, P1)
Tracking
()
People
(Reporter: agashlin, Assigned: mayhemer)
References
(Regression)
Details
(Keywords: regression, Whiteboard: [necko-triaged][fpn])
Attachments
(1 file)
47 bytes,
text/x-phabricator-request
|
lizzard
:
approval-mozilla-release+
|
Details | Review |
It seems like setting beConservative
on the channel used for update checking is causing failures with the Firefox Private Network proxy, which seems to only accept TLS 1.3. I have a more complete writeup in the See Also link.
Essentially, I'm not sure if it's intended behavior for this flag to affect the connection made to the proxy, rather than the connection being made over the proxy.
It doesn't seem to happen every time, but it's reproducible. I suspect sometimes it is reusing an existing connection to the proxy? I notice that if I first go into about:addons and check for updates there, subsequent browser update checks via the About dialog will work.
I'm not 100% sure I've diagnosed this correctly, but removing the flag seems to fix updates, so there must be some relationship.
Comment 1•5 years ago
|
||
App update isn't the only consumer so bugs should also be filed for the following two components that also use beConservative
https://searchfox.org/mozilla-central/source/toolkit/modules/ServiceRequest.jsm#49
https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/ProductAddonChecker.jsm#360
https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/ProductAddonChecker.jsm#129
Reporter | ||
Comment 2•5 years ago
|
||
ServiceRequest
is used in a half dozen places, so that might also hit some trouble. But the addon update check seems to work, and what's more after that has worked, browser updates also work, so this mightt be something specific about how the browser update service is setting up the channel.
Reporter | ||
Comment 3•5 years ago
|
||
On further inspection, it looks like the addon update check is actually failing, lots of errors logged. Weird then that it seems to un-break the update service somehow.
I don't know if I want to open bugs all around if there's an underlying network issue behind all of this, but maybe this isn't the right component.
Comment 4•5 years ago
|
||
In that case it would be best to either move this over to a proxy or networking component. If the consumers need bugs it would be best to have individual bugs for each of the components that use beConservative.
Reporter | ||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 5•5 years ago
|
||
This is funny - the problem was there since ever. The production proxy was always accepting only TLS 1.3 (as just confirmed by Andrew Plunk).
This needs a platform change. I would propose a preference, def=false, that would allow ignoring BE_CONSERVATIVE for proxy connections. The addon would flip it to allow TLS1.3 with the proxy even for conservative requests. This would be an upliftable and simple enough change.
The more proper fix would be to fail over to the same proxy with enforcing conservative approach if errors between the browser and a proxy caused by middleboxes were well detectable... BE_CONSERVATIVE was introduced in bug 1321783 as a precaution for "fancy protocols" being mangled by middleboxes.
Note that for the current impl of this flag, there is no preference to flip, we hard-set the flag and hard-check it here and here:
https://searchfox.org/mozilla-central/rev/23f836a71cfe961373c8bd0d0219ec60a64b3c8f/netwerk/protocol/http/nsHttpConnectionMgr.cpp#4088-4091
https://searchfox.org/mozilla-central/rev/23f836a71cfe961373c8bd0d0219ec60a64b3c8f/security/manager/ssl/nsNSSIOLayer.cpp#2031-2038
Assignee | ||
Comment 6•5 years ago
|
||
Comment 8•5 years ago
|
||
bugherder |
Comment 9•5 years ago
|
||
Should this proxy fix be uplifted to Firefox Beta (70) or ESR (68)?
Updated•5 years ago
|
Updated•5 years ago
|
Reporter | ||
Comment 10•5 years ago
|
||
I've been testing with the network.http.proxy.respect-be-conservative
pref set to false
. The update check succeeds after I set it (yay!), but when Firefox is restarted it doesn't always take effect; sometimes (about half the time?) I'll see the update check fail until I flip the pref again.
This can be seen, with Firefox Private Network v11 active, by setting app.update.log
to true
and opening the About dialog, this message will be logged to the browser console:
AUS:SVC Checker:onError - request.status: 2153394078
AUS:SVC Checker:onError - Getting secInfo failed.
AUS:SVC getStatusTextFromCode - transfer error: Update XML file malformed (200), default code: 200
Comment 11•5 years ago
|
||
It would be very late for a fix in 70 at this point, are you OK with this riding with 71?
Comment 12•5 years ago
|
||
Marking fix-optional as this isn't a new regression. If there is a strong argument to take a patch for 70 or in a 70 dot release, please let me know.
Assignee | ||
Comment 13•5 years ago
|
||
(In reply to Chris Peterson [:cpeterson] from comment #9)
Should this proxy fix be uplifted to Firefox Beta (70) or ESR (68)?
(Assuming this is what I've been ni? for)
It depends. This breaks updates when on the sec proxy, as we don't have invalid check for system requests · Issue #140 · mozilla/secure-proxy. So, this should be uplifted to the branch we ship proxy on, so probably up to the current release, which is 70 right now. Sorry I didn't answer sooner, was offline the last week, while there still were a chance to get this innocent fix to beta before merging.
The fix is really very simple and clear and with default preferences doesn't effect any functionality, but fixes a major issue for FPN. So, I think it's worth getting this to a dot release for 70.
Comment 14•5 years ago
|
||
OK, can you request uplift to mozilla-release please?
Updated•5 years ago
|
Assignee | ||
Comment 15•5 years ago
|
||
(In reply to Adam Gashlin (he/him) [:agashlin] from comment #10)
I've been testing with the
network.http.proxy.respect-be-conservative
pref set tofalse
. The update check succeeds after I set it (yay!), but when Firefox is restarted it doesn't always take effect; sometimes (about half the time?) I'll see the update check fail until I flip the pref again.This can be seen, with Firefox Private Network v11 active, by setting
app.update.log
totrue
and opening the About dialog, this message will be logged to the browser console:AUS:SVC Checker:onError - request.status: 2153394078 AUS:SVC Checker:onError - Getting secInfo failed. AUS:SVC getStatusTextFromCode - transfer error: Update XML file malformed (200), default code: 200
Can you please file a separate bug for this? Thanks.
Assignee | ||
Comment 16•5 years ago
|
||
Comment on attachment 9098372 [details]
Bug 1585236 - Have a preference to not be conservative when conneting a proxy, r=dragana
Beta/Release Uplift Approval Request
- User impact if declined: This is a prerequisite for the FPN addon. W/o this platform fix, with the FPN enabled, Firefox update check or download request (and some other less important system requests) will fail without any fail-over possibility and no visible user notification, to say the least.
- Is this code covered by automated tests?: No
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: Yes
- If yes, steps to reproduce: see comment 10
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): or zero. with the default pref values this doesn't change the behavior at all. the patch on its own is very simple, so there is no risk for crash/leak/functionality problem. the patch was baked enough.
- String changes made/needed:
Assignee | ||
Updated•5 years ago
|
Comment 17•5 years ago
|
||
"Breaks updates" is definitely bad enough for dot release.
Updated•5 years ago
|
Updated•5 years ago
|
Reporter | ||
Comment 18•5 years ago
|
||
I've mentioned this elsewhere but it should be in here as well:
I don't think the original bug permanently breaks updates. Eventually the check will succeed, though I have not yet been able to determine exactly why. (Is there a preexisting proxied connection to aus5.mozilla.org that the update.xml check can reuse? Or just a preexisting proxy connection at all? Maybe there are multiple FPN endpoints, some of which allow TLS 1.2?)
Even if the update check never succeeds, after 10 checks (app.update.backgroundMaxErrors) there will be a prompt to manually check for a new version.
Assignee | ||
Comment 19•5 years ago
|
||
(In reply to Adam Gashlin (he/him) [:agashlin] from comment #18)
I've mentioned this elsewhere but it should be in here as well:
I don't think the original bug permanently breaks updates. Eventually the check will succeed, though I have not yet been able to determine exactly why. (Is there a preexisting proxied connection to aus5.mozilla.org that the update.xml check can reuse?
Hmm.. we isolate connections according the be-conservative flag:
nsHttpConnectionInfo.h - mozsearch
But this could be bypassed with h2 coalescing. I would have to look into the code, it's too messy to say for sure just like that, but it's likely the cause.
Or just a preexisting proxy connection at all?
as above.
Maybe there are multiple FPN endpoints, some of which allow TLS 1.2?)
no, all nodes are 1.3 only according the info I have
Comment 20•5 years ago
|
||
This issue is no longer reproducible on Firefox Beta 71.0b4, using Windows 10 x64, MacOS 10.14.2, Linux Ubuntu 16.04. The errors mentioned above are no longer displayed in the browser console. Marking this issue as verified.
Comment 21•5 years ago
|
||
Since this doesn't permanently break updates I'll stop calling it a blocking issue. Do you think we should uplift the fix to release?
Assignee | ||
Comment 22•5 years ago
|
||
(In reply to Liz Henry (:lizzard) from comment #21)
Since this doesn't permanently break updates I'll stop calling it a blocking issue. Do you think we should uplift the fix to release?
I still think it's worth doing. The patch is simple, at default doesn't change anything - no effect on non-users of FPN. We can't make things worse: if there is a middlebox in the local network that disrupts the connection to TLS1.3-only proxies, we are at the same state as w/o the patch: can't connect the proxy at all, not just for update checks. As we know browser sessions can be short, the chance for an update check to reuse a non-conservative connection through the proxy is thus lowered.
Comment 23•5 years ago
|
||
Comment on attachment 9098372 [details]
Bug 1585236 - Have a preference to not be conservative when conneting a proxy, r=dragana
OK for uplift to m-r for 70.1.
Reporter | ||
Comment 24•5 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #19)
(In reply to Adam Gashlin (he/him) [:agashlin] from comment #18)
Is there a preexisting proxied connection to aus5.mozilla.org that the update.xml check can reuse?
Hmm.. we isolate connections according the be-conservative flag:
nsHttpConnectionInfo.h - mozsearchBut this could be bypassed with h2 coalescing. I would have to look into the code, it's too messy to say for sure just like that, but it's likely the cause.
I think I do recall seeing HTTP 2 mentioned when the update worked, I may have caused it by navigating directly to aus5.mozilla.org first.
Comment 25•5 years ago
|
||
bugherder uplift |
Comment 26•5 years ago
|
||
I've tested using the build from comment 25 . This issue is still reproducible, NI myself to recheck after bug 1592628 lands.
Comment 27•5 years ago
|
||
Honza, Adam, is this definitely resolved? Do we have a way to verify that it is fixed in 70.0.1 and users are no longer affected or have updated as expected?
Assignee | ||
Comment 28•5 years ago
|
||
I can reproduce bug 1592628, but it is hard to diagnose using a local build that adds some necessary logging as it doesn't make the early update check, obviously.
I will land a patch with added logging and retry with that when it gets to Nightly, in bug 1592628.
Assignee | ||
Comment 29•5 years ago
|
||
(In reply to Liz Henry (:lizzard) from comment #27)
Honza, Adam, is this definitely resolved? Do we have a way to verify that it is fixed in 70.0.1 and users are no longer affected or have updated as expected?
Confirmed broken - please see https://bugzilla.mozilla.org/show_bug.cgi?id=1592628#c2
Updated•5 years ago
|
Comment 30•5 years ago
|
||
Doesn't sound like this is needed on ESR68, but feel free to nominate for uplift if you feel strongly otherwise.
Comment 31•5 years ago
|
||
(In reply to Adam Gashlin (he/him) [:agashlin] from comment #10)
I've been testing with the
network.http.proxy.respect-be-conservative
pref set tofalse
. The update check succeeds after I set it (yay!), but when Firefox is restarted it doesn't always take effect; sometimes (about half the time?) I'll see the update check fail until I flip the pref again.This can be seen, with Firefox Private Network v11 active, by setting
app.update.log
totrue
and opening the About dialog, this message will be logged to the browser console:AUS:SVC Checker:onError - request.status: 2153394078 AUS:SVC Checker:onError - Getting secInfo failed. AUS:SVC getStatusTextFromCode - transfer error: Update XML file malformed (200), default code: 200
I took the same steps, using Firefox 70.0 Release, and Firefox Private Network v17 active with the following message in the browser console:
AUS:SVC isServiceInstalled - returning true
AUS:SVC shouldUseService - returning true
AUS:SVC getCanStageUpdates - able to stage updates using the service
Updated•5 years ago
|
Reporter | ||
Comment 32•5 years ago
|
||
Thanks Lorand!
For reference, the log message that indicates the update check went through successfully would be
AUS:SVC Checker:onLoad - request completed downloading document
followed by
AUS:SVC Checker:onLoad - number of updates available: 0
Updated•3 years ago
|
Description
•