1820807 - Make authentication retries redirect to a new channel

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement, P2)

Description

[Valentin Gosu [:valentin] (he/him)]

We can simplify nsHttpChannel quite a bit if we make the authentication retry not use the same nsHttpChannel but instead to an internal redirect to the same URL with the proper credentials.

Comment 1

[Sunil Mayya]

Attachment: WIP: Bug 1820807: redirect channel for auth retries

Comment 2

[Sunil Mayya]

Attachment: WIP: Bug 1820807: change set 2

Depends on D180071

Comment 3

[Sunil Mayya]

Attachment: WIP: Bug 1820807 - add auth header while internal redirect

Depends on D180071

Comment 4

[Sunil Mayya]

Attachment: Bug 1820807 - redirect channel for auth retries. r=#necko

Comment 5

[Sunil Mayya]

Attachment: WIP: Bug 1820807: add tests to check we send auth requests with new channel

Depends on D182698

Comment 6

[Sunil Mayya]

Hey Luca,

With our new implementation, on authentication failures we would send the retry requests with auth headers on the new
(redirected) channel.
Hence, we will be sending the "http-on-modify-request" and "http-on-before-connect" observer notifications for the new channel before sending the request instead of the old channel which received the auth failure status.

We would like to know if this would have any impacts on the webextensions?

Thanks

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Sunil Mayya from comment #6)

Hey Luca,

With our new implementation, on authentication failures we would send the retry requests with auth headers on the new
(redirected) channel.
Hence, we will be sending the "http-on-modify-request" and "http-on-before-connect" observer notifications for the new channel before sending the request instead of the old channel which received the auth failure status.

We would like to know if this would have any impacts on the webextensions?

Thanks

Hi Sunil, I'm forwarding your needinfo to Rob, Rob have been working on that area recently enough and so he may be able to provide that assessment sooner.

Comment 8

[Sunil Mayya]

Hey Rob,
With my changes, we have changed the mechanism of process auth failures (401/407).

Instead of sending the request with authentication information from the same channel we do internal redirect and send the request with authentication information from the new channel. You can find a detailed description about the design here

Unfortunately, there are some impacts on webextensions due to my changes.

We see that test_webRequest_duelingAuth test fails primarily due to the following exception

 0:01.48 pid:3475 JavaScript error: resource://gre/modules/WebRequest.sys.mjs, line 1213: NS_ERROR_UNEXPECTED

The reason for this exception is that nsHttpChannel::Resume returns NS_ERROR_UNEXPECTED.
From Necko logs, I could see that the WebRequest.sys.mjs script is suspending the channel when prompting for auth credentials. On generation of credentials Necko redirects to the new channel.
The script later calls resume on the redirected channel.
Hence, this check in nsHttpChannel fails as the redirected channel was never suspended.

I would need your help in understanding if there are any additional notification you would expect such that the we don't call resume on the redirected channel?
Please let me know if you need any additional logs.
Thank you in advance.

Comment 9

[Rob Wu [:robwu]]

(In reply to Sunil Mayya from comment #6)

With our new implementation, on authentication failures we would send the retry requests with auth headers on the new
(redirected) channel.
Hence, we will be sending the "http-on-modify-request" and "http-on-before-connect" observer notifications for the new channel before sending the request instead of the old channel which received the auth failure status.

We would like to know if this would have any impacts on the webextensions?

As described, the changes may potentially affects the following parts of the API:

• webRequest.onAuthRequired, dispatched by asyncPromptAuth handler registered at http-on-examine-response
• webRequest.onBeforeRequest = "http-on-modify-request".
• webRequest.onBeforeSendHeaders = "http-on-before-connect"

I haven't tested your patch yet, but I did compare the behavior of Firefox and Chrome (and the documentation on MDN and Chrome's developer docs). These four all differ from each other, unfortunately. I described the behavior in https://bugzilla.mozilla.org/show_bug.cgi?id=1847637 along with an extension and instructions to test manually. Give it a try with your patch and show the output if you have questions on the validity of the extension-observed behavior.

(In reply to Sunil Mayya from comment #8)

Hey Rob,
With my changes, we have changed the mechanism of process auth failures (401/407).

Instead of sending the request with authentication information from the same channel we do internal redirect and send the request with authentication information from the new channel. You can find a detailed description about the design here

Not extension-specific, but relevant question: how does the change interact with the internal redirect loop detection logic in HttpBaseChannel::CheckRedirectLimit? Is the extra consumed mInternalRedirectCount budget reasonable under normal situations and in edge cases? The main thing I'd like to know is if users may get dangerously close to the (internal) redirect limit, to the point that there is not enough budget left for a reasonably small number of server-side redirects. There are many ways to authenticate, and I am not sufficiently familiar with all authentication mechanisms to tell what happens. For example, NLTM involves multiple challenge-response messages - how many internal redirects are counted?

(meta comment: when linking Searchfox links, please use Permalink from the sidebar so that the link always points to the same code. I have replaced the original links with permalinks in the quote below to make it easier to follow the discussion in the future)

Unfortunately, there are some impacts on webextensions due to my changes.

We see that test_webRequest_duelingAuth test fails primarily due to the following exception

 0:01.48 pid:3475 JavaScript error: resource://gre/modules/WebRequest.sys.mjs, line 1213: NS_ERROR_UNEXPECTED

The reason for this exception is that nsHttpChannel::Resume returns NS_ERROR_UNEXPECTED.
From Necko logs, I could see that the WebRequest.sys.mjs script is suspending the channel when prompting for auth credentials. On generation of credentials Necko redirects to the new channel.
The script later calls resume on the redirected channel.
Hence, this check in nsHttpChannel fails as the redirected channel was never suspended.

When there is any webRequest.onAuthRequired listener, the implementation suspends a channel before an extension has had any chance to respond with credentials, and that's implemented through the nsIAuthPrompt2 interface:

• suspend: AuthRequestor's asyncPromptAuth (the actual suspend() call is called by runChannelListener - a synchronous method).
• resume: applyChanges's final part responds with the credentials before calling resume (unless the authentication request was already canceled)
  • note channel.resume() there is actually handled by ChannelWrapper::Resume, which forwards to the underlying channel.

The WebRequest logic seems correct, so if you're encountering NS_ERROR_UNEXPECTED in the channel.resume() call, then it means that your patch caused the underlying channel to proceed without accounting for the suspended state. I am not seeing any reference to mCallOnResume in your patch, could that be the issue? See the changes in bug 1407384 for example.

I would need your help in understanding if there are any additional notification you would expect such that the we don't call resume on the redirected channel?

Given the above analysis, I believe that it's not a lack of notification to WebRequest, but a lack of checking the suspended state in the channel.

Comment 10

[Sunil Mayya]

Hello Rob,

Thanks for your inputs on test_webRequest_duelingAuth, I have added the missing checks for channel suspension. This issue has been resolved.

I haven't tested your patch yet, but I did compare the behavior of Firefox and Chrome (and the documentation on MDN and Chrome's developer docs). These four all differ from each other, unfortunately. I described the behavior in https://bugzilla.mozilla.org/show_bug.cgi?id=1847637 along with an extension and instructions to test manually. Give it a try with your patch and show the output if you have questions on the validity of the extension-observed behavior.

Here is the output for various scenarios for my patch:

Bad Credentials:
requestId 305 onBeforeRequest main_frame https://httpbingo.org/basic-auth/user/passwd 19 background.js:27:17
requestId 305 onBeforeSendHeaders main_frame https://httpbingo.org/basic-auth/user/passwd 20 background.js:27:17
requestId 305 onSendHeaders main_frame https://httpbingo.org/basic-auth/user/passwd 21 background.js:27:17
requestId 305 onHeadersReceived main_frame https://httpbingo.org/basic-auth/user/passwd 22 background.js:27:17
requestId 305 onAuthRequired main_frame https://httpbingo.org/basic-auth/user/passwd 23 background.js:27:17
requestId 305 auth suspended background.js:32:21
requestId 305 auth resumed background.js:34:25
[Auth UI appears and user enters wrong credentials]
requestId 305 onBeforeRedirect main_frame https://httpbingo.org/basic-auth/user/passwd 24 background.js:27:17
requestId 305 onBeforeRequest main_frame https://httpbingo.org/basic-auth/user/passwd 25 background.js:27:17
requestId 305 onBeforeSendHeaders main_frame https://httpbingo.org/basic-auth/user/passwd 26 background.js:27:17
requestId 305 onSendHeaders main_frame https://httpbingo.org/basic-auth/user/passwd 27 background.js:27:17
requestId 305 onHeadersReceived main_frame https://httpbingo.org/basic-auth/user/passwd 28 background.js:27:17
requestId 305 onAuthRequired main_frame https://httpbingo.org/basic-auth/user/passwd 29 background.js:27:17
requestId 305 auth suspended background.js:32:21
requestId 305 auth resumed background.js:34:25
[User closes auth UI]
requestId 305 onResponseStarted main_frame https://httpbingo.org/basic-auth/user/passwd 30 background.js:27:17
requestId 305 onCompleted main_frame https://httpbingo.org/basic-auth/user/passwd 31 background.js:27:17

Good Credentials:
requestId 299 onBeforeRequest main_frame https://httpbingo.org/basic-auth/user/passwd 0 background.js:27:17
requestId 299 onBeforeSendHeaders main_frame https://httpbingo.org/basic-auth/user/passwd 1 background.js:27:17
requestId 299 onSendHeaders main_frame https://httpbingo.org/basic-auth/user/passwd 2 background.js:27:17
requestId 299 onHeadersReceived main_frame https://httpbingo.org/basic-auth/user/passwd 3 background.js:27:17
requestId 299 onAuthRequired main_frame https://httpbingo.org/basic-auth/user/passwd 4 background.js:27:17
requestId 299 auth suspended background.js:32:21
requestId 299 auth resumed background.js:34:25
[Auth UI appears and user enters right credentials]
requestId 299 onBeforeRedirect main_frame https://httpbingo.org/basic-auth/user/passwd 5 background.js:27:17
requestId 299 onBeforeRequest main_frame https://httpbingo.org/basic-auth/user/passwd 6 background.js:27:17
requestId 299 onBeforeSendHeaders main_frame https://httpbingo.org/basic-auth/user/passwd 7 background.js:27:17
requestId 299 onSendHeaders main_frame https://httpbingo.org/basic-auth/user/passwd 8 background.js:27:17
requestId 299 onHeadersReceived main_frame https://httpbingo.org/basic-auth/user/passwd 9 background.js:27:17
requestId 299 onResponseStarted main_frame https://httpbingo.org/basic-auth/user/passwd 10 background.js:27:17
requestId 299 onCompleted main_frame https://httpbingo.org/basic-auth/user/passwd 11 background.js:27:17

By comparing the above events without my patch, I see an additional onBeforeRedirect event that is fired after the user enters the credentials.
Please let me know if this breaks anything?

Not extension-specific, but relevant question: how does the change interact with the internal redirect loop detection logic in HttpBaseChannel::CheckRedirectLimit? Is the extra consumed mInternalRedirectCount budget reasonable under normal situations and in edge cases? The main thing I'd like to know is if users may get dangerously close to the (internal) redirect limit, to the point that there is not enough budget left for a reasonably small number of server-side redirects. There are many ways to authenticate, and I am not sufficiently familiar with all authentication mechanisms to tell what happens. For example, NLTM involves multiple challenge-response messages - how many internal redirects are counted?

Thanks for raising this. We had an internal discussion regarding this. With my patch, the number of auth retries is limited for internal redirect limit. We decided to change this to ensure the user auth retries are not limited.

Comment 11

[Rob Wu [:robwu]]

(In reply to Sunil Mayya from comment #10)

By comparing the above events without my patch, I see an additional onBeforeRedirect event that is fired after the user enters the credentials.
Please let me know if this breaks anything?

It is not documented, but neither is the onAuthRequired > onBeforeRequest transition.

IMO introducing the extra onBeforeRedirect event is acceptable, provided that it's covered by unit tests and documented. Currently, onBeforeRedirect is documented to signal server-side redirects, while in reality it also covers more (internal) redirects. That mismatch in documentation and actual behavior has been raised before in bug 1817450, and IMO an acceptable resolution could be to just document this transition.

The onBeforeRedirect event has additional fields in its details. Are extensions able to discern the different kinds of redirects? Please test all potentially relevant details fields in the unit test.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/11fddb3ea9c0
redirect channel for auth retries. r=necko-reviewers,kershaw,valentin

Comment 13

[Cosmin Sabou [:CosminS]]

Backed out for causing xpcshell failures on test_proxy-failover_canceled.js.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel&revision=11fddb3ea9c04a3c48dcbbef5e567c45a6413da2&selectedTaskRun=bsuys8XSSzW8QKtIPoO4rQ.0

Failure logs:

• https://treeherder.mozilla.org/logviewer?job_id=428689894&repo=autoland
• https://treeherder.mozilla.org/logviewer?job_id=428692468&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/2d433a97e604465a59cb81dc73a61869ac504578

Comment 14

[Sunil Mayya]

Attachment: Bug 1820807 - send Caps parameter PHttpTransaction::OnStartRequest message. r=#necko

Depends on D182698

Comment 15

[Phabricator Automation]

Comment on attachment 9352968 [details]
Bug 1820807 - send Caps parameter PHttpTransaction::OnStartRequest message. r=#necko

Revision D188114 was moved to bug 1853025. Setting attachment 9352968 [details] to obsolete.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2fae28575ca8
redirect channel for auth retries. r=necko-reviewers,kershaw,valentin

Comment 17

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2fae28575ca8

Comment 18

[Cosmin Sabou [:CosminS]]

Backed out for causing frequent network-related startup crashes (bug 1853231).

Backout link: https://hg.mozilla.org/mozilla-central/rev/f4b0e304eceb

Comment 19

[Sunil Mayya]

Attachment: Bug 1820807 - Open redirected channel for auth retries in OnStopRequest. r=#necko,valentin,kershaw

Depends on D182698

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/115ae91a6425
redirect channel for auth retries. r=necko-reviewers,kershaw,valentin
https://hg.mozilla.org/integration/autoland/rev/4e90d1ca661c
Open redirected channel for auth retries in OnStopRequest. r=valentin,kershaw,necko-reviewers

Comment 21

[Cristian Tuns]

Backed out for causing leak failures

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | leakcheck | default 101232 bytes leaked (BackstagePass, BrowsingContext, BrowsingContextGroup, BrowsingContextWebProgress, CanonicalBrowsingContext, ...)

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7fecdcc3336
redirect channel for auth retries. r=necko-reviewers,kershaw,valentin
https://hg.mozilla.org/integration/autoland/rev/6a2f9e20f11f
Open redirected channel for auth retries in OnStopRequest. r=valentin,kershaw,necko-reviewers

Comment 23

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/f7fecdcc3336
https://hg.mozilla.org/mozilla-central/rev/6a2f9e20f11f

Comment 24

[Sunil Mayya]

Attachment: Bug 1820807 - Enable pref network.auth.use_redirect_for_retries for early beta. r=#necko

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7af37893aed9
Enable pref network.auth.use_redirect_for_retries for early beta. r=necko-reviewers,kershaw

Comment 26

[Norisz Fay [:noriszfay]]

Backed out for causing failures on nsHttpChannel.cpp

Backout link

Push with failures - bc
Push with failures - xpcshell
Push with failures - bc leaksanitizer

Failure log - bc
Failure log - xpcshell
Failure log - bc leaksanitizer

Comment 27

[Phabricator Automation]

Comment on attachment 9356083 [details]
Bug 1820807 - Enable pref network.auth.use_redirect_for_retries for early beta. r=#necko

Revision D189759 was moved to bug 1856288. Setting attachment 9356083 [details] to obsolete.

Comment 28

[Sunil Mayya]

The feature is available under pref pref network.auth.use_redirect_for_retries (enabled for early beta).