1554218 - Handle 407 from a HTTP/2 proxy

Status: RESOLVED FIXED

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

Description

[Honza Bambas (:mayhemer)]

We just fail with NS_ERROR_NET_RESET, the channel doesn't even know there was 407.

Comment 1

[Honza Bambas (:mayhemer)]

Note that this happens only when we are connecting to an https end point. We do pop-up the dialog when connecting a plain end server.

Comment 2

[Nhi Nguyen (:nhi)]

Honza, do we need this for skyline?

Comment 3

[Honza Bambas (:mayhemer)]

(In reply to Nhi Nguyen (:nhi) from comment #2)

Honza, do we need this for skyline?

Absolutely not.

Comment 5

[Yegor]

(In reply to Honza Bambas (:mayhemer) from comment #3)

(In reply to Nhi Nguyen (:nhi) from comment #2)

Honza, do we need this for skyline?

Absolutely not.

Absolutely not? Why? This is a major problem, that effectively makes the proxy API useless unless you downgrade to http/1.1 (or use open proxies which is a terrible practice). We're forced to drop H2 support now for all Firefox users of our extension. This will reduce performance without the benefits of H2.

Comment 6

[Honza Bambas (:mayhemer)]

It's not needed for one of our projects (skyline codename). But it's definitely a major bug we want to fix - tho, we lived with it a long time already. In other words, I can't guarantee it will be fixed for Fx70.

Comment 7

[Yegor]

Latest Nightly appears to exhibit the same issues, so no fix there.

Comment 8

[Yegor]

Is a fix for this issue planned? We had to deploy a separate proxy server instance on all our machines, which uses http/1.1 instead of H2, just to support Firefox at this point.

A huge step backwards, imo....

Comment 10

[Dimas]

If this bug is the responsable of 1468870 can it be prioritized? Thx

Comment 11

[Mike Kaply [:mkaply]]

This is apparently affecting enterprises due to the whatsapp issue.

Comment 12

[Romain Testard [:RT]]

With a large share of our desktop users inside corporate environments this seems to be a serious issue likely leading to churn, especially since Whatsapp relies on it.
Dragana, do you have a rough idea of when this could be prioritized?

Comment 13

[Dragana Damjanovic [:dragana]]

Nhi, can you prioritize this?

Comment 14

[dekimsey]

We encountered this issue in our forwarding proxy as well. During testing we found:
68.6.0esr (works)
72.0.1 (fails)
74.0 (fails)

We didn't try bother trying to bisect past what was immediately available on our desktops. Just adding this information in case anyone else runs into it.

Comment 15

[Honza Bambas (:mayhemer)]

For h1 we get the 407 response as a plain response transparently from the HTTP proxy. The HTTP response code and all response headers (either for a plain http request or the CONNECT request for an https request) bubble up to the http channel for handling.

For h2 when doing a plain http request, we get the response again as a transparent plain response and handle it the same way as in h1 case.

For h2 when doing a CONENCT request for an https request, the 407 response is captured here. We need to pass the response headers and the code along with the mDrivingTransaction. I'm quite sure we can't do these two steps when we receive 407.

Comment 16

[Honza Bambas (:mayhemer)]

You can use my testing proxy to work with this. Just change the config for "authenticate": false to "authenticate": "Basic realm='foo'".

Comment 17

[Junior [inactive]]

For h2 when doing a CONENCT request for an https request, the 407 response is captured here. We need to pass the response headers and the code along with the mDrivingTransaction. I'm quite sure we can't do these two steps when we receive 407.

mDrivingTransaction can take the response headers from it's mTunnelProvider (i.e., Http2Session::mFlatHTTPResponseHeaders)
For the 407 case, we'll force the stream to plain text here, hence we won't set the secondary TLS.
We still need to initiate the transaction again to let the new nsHttpConnection close the nsHttpTransaction (the request itself, not the SpdyConnectionTransaction.

We're doing some redundant CONNECT, which might be a followup issue.
When we activate the second nsHttpConnection, we fail here and close the transaction, which eventually trigger nsHttpChannel::OnStartRequest

Therefore, my plan is to check in nsHttpTransaction::Close.
If it's 407 from tunnel, then parse the flattenHeader from tunnelProvider to let the nsHttpChannel take it.
Close the transaction with reason NS_OK (at least close the pipeOut) to make the nsHttpChannel process the response, instead of fallback
We don't want to reuse this nsHttpConnection, so make sure we release the connection here given the reason might be overwritten.

Comment 18

[Junior [inactive]]

Did comment 17 make sense, Honza?

Comment 19

[Junior [inactive]]

Attachment: Bug 1554218 - pass headers to nsHttpChannel for H2 proxy returning 407

Comment 20

[Junior [inactive]]

Let's discuss in patch.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e09151714b98
pass headers to nsHttpChannel for H2 proxy returning 407 r=mayhemer,necko-reviewers

Comment 22

[Arthur Iakab [arthur_iakab]]

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

Comment 23

[Dimas]

Thx! Is it possible to have this fix in 68 ESR?

Comment 24

[Honza Bambas (:mayhemer)]

(In reply to Dimas from comment #23)

Thx! Is it possible to have this fix in 68 ESR?

No. But there will soon be ESR 78.

Comment 25

[Mike Kaply [:mkaply]]

No. But there will soon be ESR 78.

Why do you say this? Is it not portable? Do you consider it risky?

Comment 26

[Honza Bambas (:mayhemer)]

It doesn't meet ESR back-porting requirements. We only backport major stability and security patches. This issue has been on ESR for a long time and the next release (in few weeks) will be the new ESR major release, containing this change.

I don't think drivers will approve this for the current ESR 68 branch, specially because it's a new unbaked code.

Comment 27

[Mike Kaply [:mkaply]]

It doesn't meet ESR back-porting requirements. We only backport major stability and security patches. This issue has been on ESR for a long time and the next release (in few weeks) will be the new ESR major release, containing this change.

Those aren't all the criteria. If something is impacting enterprises, we consider it.

And the next ESR is two months away. :)

Right now we're prioritizing fixes that work from home, and this has been an issue because of whatsapp.

I don't think drivers will approve this for the current ESR 68 branch, specially because it's a new unbaked code.

Let's let it bake for a few days and see.

Comment 28

[Honza Bambas (:mayhemer)]

(In reply to Mike Kaply [:mkaply] from comment #27)

It doesn't meet ESR back-porting requirements. We only backport major stability and security patches. This issue has been on ESR for a long time and the next release (in few weeks) will be the new ESR major release, containing this change.

Those aren't all the criteria. If something is impacting enterprises, we consider it.

And the next ESR is two months away. :)

Right now we're prioritizing fixes that work from home, and this has been an issue because of whatsapp.

I don't think drivers will approve this for the current ESR 68 branch, specially because it's a new unbaked code.

Let's let it bake for a few days and see.

Good reasoning, thanks! I'll nominate. This can be left a+'ed for a while before actually landing.

Comment 29

[Honza Bambas (:mayhemer)]

Comment on attachment 9139900 [details]
Bug 1554218 - pass headers to nsHttpChannel for H2 proxy returning 407

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Total inability or very hard to authenticate to more and more used HTTP/2 proxies.
• User impact if declined: as above
• Fix Landed on Version: 77
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a new change and may need some baking at least on Nightly (not sure if it will be sufficient coverage, tho) but it's also quite simple and straightforward, having an automated test.
• String or UUID changes made by this patch: none

Comment 30

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

We're building the RC for 68.8esr on Monday. It's too late in the cycle to take this for the upcoming releases. Leaving the approval request set for evaluation next cycle after this has had more time to bake with a wider audience.

Comment 31

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Also, comment 14 suggests that ESR68 isn't affected by this issue? Can someone please clarify?

Comment 32

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Comment on attachment 9139900 [details]
Bug 1554218 - pass headers to nsHttpChannel for H2 proxy returning 407

Also, this patch doesn't graft cleanly to ESR68. It'll need a rebased patch if it's to be uplifted there.

Comment 33

[Junior [inactive]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #31)

Also, comment 14 suggests that ESR68 isn't affected by this issue? Can someone please clarify?

ESR 68 is affected.
However, one can works around by establish a HTTP (not HTTPS) connection, and we'll have the auth for the future connections.
That could be something like captive portal, automatically prompting the auth box.

I'll prepare the ESR patch when I get a chance

Comment 34

[Junior [inactive]]

Dependent patches need to be uplift as well.
Let's wait for esr approval to avoid possible time-wasting.

Comment 35

[Junior [inactive]]

I'm working on the patches
Bug 1602502 and Bug 1579049 are dependent bugs which need to be uplifted as well

Comment 36

[Junior [inactive]]

(In reply to Junior from comment #35)

I'm working on the patches
Bug 1602502 and Bug 1579049 are dependent bugs which need to be uplifted as well

Bug 1611472 as well

Comment 37

[Junior [inactive]]

Attachment: (ESR68) Bug 1579049 - P1: Map Http error response codes to gecko error codes r=mayhemer,valentin

Comment 38

[Junior [inactive]]

Attachment: (ESR68) Bug 1579049 - Part2: Expose proxy response error code in nsIProxiedChannel.idl r=mayhemer

Comment 39

[Junior [inactive]]

Attachment: (ESR68) Bug 1611472 - Add missing error codes r=mayhemer

Comment 40

[Junior [inactive]]

Attachment: (ESR68) Bug 1554218 - pass headers to nsHttpChannel for H2 proxy returning 407 r=mayhemer,necko-reviewers

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Total inability or very hard to authenticate to more and more used HTTP/2 proxies.
User impact if declined: see above
Fix Landed on Version: 77
Risk to taking this patch (and alternatives if risky): low.
This is a new change and may need some baking at least on Nightly (not sure if it will be sufficient coverage, tho) but it's also quite simple and straightforward, having an automated test in nightly.

Note that the test in ESR68 is disabled (since the file is enabled in 71, see bug 1556037)

4 patches to be uplifted from comment 37 to comment 40

Comment 41

[Junior [inactive]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=427c7774c320eb780dbde1fbb53fb4a8f6307545

Comment 42

[Junior [inactive]]

ping for Attachment 9144193 [details] [diff] esr uplift request

Comment 43

[Alexandru Trif, Desktop QA [:atrif]]

Hi, are there any STR that manual QA can use to verify this? Thank you!

Comment 44

[Junior [inactive]]

(In reply to Alexandru Trif, QA [:atrif] from comment #43)

Hi, are there any STR that manual QA can use to verify this? Thank you!

Follow comment 16. Thanks.

Comment 45

[Alexandru Trif, Desktop QA [:atrif]]

Thank you!

Comment 46

[Bogdan Maris, Desktop QA]

Using Firefox 74.0 and 76.0 I followed the steps from comment 16 in order to reproduce the initial issue but the Auth form appears and no 407 error is thrown, maybe I'm missing something.

What I did:

• Installed everything from Install section from GitHub
• Installed the cert http2-ca.pem
• Added data:text/javascript,function FindProxyForURL() { return "HTTPS localhost:3000"; } in Connection Settings from Fx.
• Visited 0.0.0.0:3000
  Authentication pop-up launches and hitting Ok will open a Raw Json window containing {"title":"request error","description":"Error: socket hang up"}

This was the same for Fx 77.0b3 as well. I'm sure I'm missing something, Junior can you please offer some guidance please?

Comment 47

[Junior [inactive]]

• Visited 0.0.0.0:3000

Please visit an HTTPS site.
Note that once your auth is cached, you'll not hit the issue.
In that case you need to clean the profile.

The auth dialog might popup by other non-HTTPS connections as well, e.g., telemetry ping, dom push.
To reproduce the issue, simply cancel the dialog which is not from your navigation of an HTTPS site.

Comment 48

[Bogdan Maris, Desktop QA]

Thanks Junior for your help! I was able to reproduce the initial issue using Firefox 76.0, and verified it fixed using latest Firefox Beta 77.0b4.

Comment 49

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

This looks like a lot of changes to be taking on an ESR branch this late in its lifecycle (ESR78 is right around the corner in less than 2 months). Do we have any evidence of this breaking enterprise users in the wild suggesting this having high urgency for uplift?

Comment 50

[Mike Kaply [:mkaply]]

The issue was whatapp breakage, but with the number of patches and dependent bugs, I understand your concern.

I'll let Junior speak to the risk.

Comment 51

[Junior [inactive]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #49)

This looks like a lot of changes to be taking on an ESR branch this late in its lifecycle (ESR78 is right around the corner in less than 2 months). Do we have any evidence of this breaking enterprise users in the wild suggesting this having high urgency for uplift?

For the level of risk, please see comment 40.
It needs some testing If you ask me.
I rebased those patches, but my current toolchain can't build ESR68. I use treeherder to verify it builds.
On the other hand, we have xpcshell-test in nightly, but not ESR68 since its node version is old.

And this is another work-from-home scenario under COVID-19 that numbers of enterprise relies on proxy.
But I don't have evidences other than whatsapp

Comment 52

[Junior [inactive]]

Comment on attachment 9144193 [details] [diff] [review]
(ESR68) Bug 1554218 - pass headers to nsHttpChannel for H2 proxy returning 407 r=mayhemer,necko-reviewers

Let's wait for next ESR since I'm not able to provide more evidence.

Comment 53

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Agreed that we should let this ride 77+ and ESR78 absent any strong evidence that enterprises are being disproportionately affected by this problem.