Closed Bug 1585236 Opened 5 years ago Closed 5 years ago

Conservative TLS settings prevent connecting to a TLS 1.3-only proxy

Categories

(Core :: Networking: HTTP, defect, P1)

Product:
Core
Component:
Networking: HTTP
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla71
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 + verified
firefox71 --- verified

People

(Reporter: agashlin, Assigned: mayhemer)

References

(Regression)

Details

(Keywords: regression, Whiteboard: [necko-triaged][fpn])

Attachments

(1 file)

Bug 1585236 - Have a preference to not be conservative when conneting a proxy, r=dragana
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.

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.

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.

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.

Component: Application Update → Networking: HTTP
Product: Toolkit → Core
Assignee: nobody → honzab.moz
Priority: -- → P1
Whiteboard: [necko-triaged][fpn]

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

Pushed by honzab.moz@firemni.cz: https://hg.mozilla.org/integration/autoland/rev/efe4efedafd6 Have a preference to not be conservative when conneting a proxy, r=dragana

https://hg.mozilla.org/mozilla-central/rev/efe4efedafd6

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox71: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

Should this proxy fix be uplifted to Firefox Beta (70) or ESR (68)?

status-firefox70: --- → ?
status-firefox-esr68: --- → ?
Flags: needinfo?(honzab.moz)

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

It would be very late for a fix in 70 at this point, are you OK with this riding with 71?

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.

status-firefox70: affected → fix-optional

(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.

Flags: needinfo?(honzab.moz) → needinfo?(lhenry)

OK, can you request uplift to mozilla-release please?

status-firefox70: fix-optional → affected
tracking-firefox70: --- → +
Flags: needinfo?(lhenry)
Flags: needinfo?(honzab.moz)

(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 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

Can you please file a separate bug for this? Thanks.

Flags: needinfo?(honzab.moz) → needinfo?(agashlin)

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:
Attachment #9098372 - Flags: approval-mozilla-release?
Flags: qe-verify+

"Breaks updates" is definitely bad enough for dot release.

tracking-firefox70: + → blocking
Severity: normal → major
QA Whiteboard: [qa-triaged]

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.

(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

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.

Status: RESOLVED → VERIFIED
status-firefox71: fixed → verified
Flags: qe-verify+

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?

tracking-firefox70: blocking → +
Flags: needinfo?(honzab.moz)

(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.

Flags: needinfo?(honzab.moz)

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.

Attachment #9098372 - Flags: approval-mozilla-release? → approval-mozilla-release+

(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 - 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.

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.

Flags: needinfo?(agashlin)
Depends on: 1592628

I've tested using the build from comment 25 . This issue is still reproducible, NI myself to recheck after bug 1592628 lands.

Flags: needinfo?(lorand.janos)

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?

Flags: needinfo?(honzab.moz)
Flags: needinfo?(agashlin)

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.

Flags: needinfo?(honzab.moz)

(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

Flags: needinfo?(agashlin) → needinfo?(lhenry)
Flags: needinfo?(lhenry)

Doesn't sound like this is needed on ESR68, but feel free to nominate for uplift if you feel strongly otherwise.

status-firefox-esr68: affected → wontfix

(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 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

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

Flags: needinfo?(lorand.janos)

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

Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: