1794464 - Allow HTTP authentication in proxy.onRequest

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, enhancement, P3)

Description

[erosman]

Preamble

The bug was filed subsequent to a discussion with Valentin in #necko:mozilla.org and instruction to file a bug under https://bugzilla.mozilla.org/enter_bug.cgi?product=Core&component=Networking

API was restricted due to the request in D29825:

can you restrict this only for https proxies?
Why exactly should this be limited to https proxies only?
Because we limit any new stuff to https only.

Background

Reference: Bug 1545420 & D29825

Originally, proxy.onRequest was created to process proxy forwarding and authenticating.

• The API can handle HTTP/HTTPS/SOCKS4/SOCKS5 proxy forwarding
• The API can also provide authentication for SOCKS5 (unique to Firefox), as well as HTTPS proxies

At the moment, proxying using proxy.onRequest involves:

• Forwarding to SOCKS4 proxies (no authentication as it is not supported)
• Forwarding & authenticating withing the same process to SOCKS5 proxies
• Forwarding & authenticating withing the same process to HTTPS proxies as basic authentication header e.g. Proxy-Authorization: Basic YWxhZGRpbjpvcGVuc2VzYW1l
• Forwarding to HTTP proxies

As a result, in case of HTTP proxies, forwarding and authenticating can not be performed within the same process and additional listeners (and permissions) are needed.

webRequest.onAuthRequired

Proxy authorization
If an extension has the "webRequest", "webRequestBlocking", "proxy", and "<all_urls>" permissions, then it will be able to use onAuthRequired to supply credentials for proxy authorization (but not for normal web authorization).

browser.webRequest.onAuthRequired.addListener() 
browser.webRequest.onCompleted.addListener()
browser.webRequest.onErrorOccurred.addListener()

Performance Comparison:

• Forwarding to proxy with Proxy-Authorization headers in one process (as is the case with SOCKS5 & HTTPS)
  ----- vs -----
• Forwarding to the proxy (HTTP)
• Adding a listeners to wait for HTTP 407
• Replying to the HTTP 407 after retrieving user/pass

If HTTP authentication header was also permitted in proxy.onRequest, then there would not be any need to resort to a separate API (actually 3 APIs) simply to handle sending authentication for HTTP proxies.

In fact, the other APIs eventually end up sending the authentication as Proxy-Authorization: Basic YWxhZGRpbjpvcGVuc2VzYW1l header anyway.

Currently, proxy.onRequest disables setting proxyAuthorizationHeader for HTTP in
https://searchfox.org/mozilla-central/source/toolkit/components/extensions/ProxyChannelFilter.jsm#173
https://searchfox.org/mozilla-central/source/toolkit/components/extensions/ProxyChannelFilter.sys.mjs#167
(updated URL 2024-01-23)

  proxyAuthorizationHeader(proxyData) {
    let { proxyAuthorizationHeader, type } = proxyData;
    if (proxyAuthorizationHeader === undefined) {
      return;
    }
    if (typeof proxyAuthorizationHeader !== "string") {
      throw new ExtensionError(
        `ProxyInfoData: Invalid proxy server authorization header: "${proxyAuthorizationHeader}"`
      );
    }
    if (type !== "https") {
      throw new ExtensionError(
        `ProxyInfoData: ProxyAuthorizationHeader requires type "https"`
      );
    }
  },

Request

Above limit was requested in D29825. If there are no underlying issues, then the removal of lines 183-187 should enhance the proxy.onRequest and improve the overall performance by processing everything within the same process.

Comment 1

[erosman]

Other notes on API

The followings are not part of the above request and are added only as additional material on the API (that can be looked into while on the subject):

• Is passing proxyDNS necessary for HTTP/HTTPS?
• Username/password is not supported in SOCKS4. Is it necessary to pass it for SCOSK4?

https://searchfox.org/mozilla-central/source/toolkit/components/extensions/ProxyChannelFilter.jsm#141

  proxyDNS(proxyData) {
    let { proxyDNS, type } = proxyData;
    if (proxyDNS !== undefined) {
      if (typeof proxyDNS !== "boolean") {
        throw new ExtensionError(
          `ProxyInfoData: Invalid proxyDNS value: "${proxyDNS}"`
        );
      }
      if (
        proxyDNS &&
        type !== PROXY_TYPES.SOCKS &&
        type !== PROXY_TYPES.SOCKS4
      ) {
        throw new ExtensionError(
          `ProxyInfoData: proxyDNS can only be true for SOCKS proxy servers`
        );
      }
    }
  },

https://searchfox.org/mozilla-central/source/toolkit/components/extensions/ProxyChannelFilter.jsm#237

    if (type === PROXY_TYPES.SOCKS || type === PROXY_TYPES.SOCKS4) {
      proxyInfo = lazy.ProxyService.newProxyInfoWithAuth(
        type,
        host,
        port,
        username,
        password,
        proxyAuthorizationHeader,
        connectionIsolationKey,
        proxyDNS ? TRANSPARENT_PROXY_RESOLVES_HOST : 0,
        failoverTimeout ? failoverTimeout : PROXY_TIMEOUT_SEC,
        failoverProxy
      );
    } else {
      proxyInfo = lazy.ProxyService.newProxyInfo(
        type,
        host,
        port,
        proxyAuthorizationHeader,
        connectionIsolationKey,
        proxyDNS ? TRANSPARENT_PROXY_RESOLVES_HOST : 0,
        failoverTimeout ? failoverTimeout : PROXY_TIMEOUT_SEC,
        failoverProxy
      );
    }

Comment 2

[Shane Caraveo (:mixedpuppy)]

This is a limitation in the networking code, not extensions, per https://phabricator.services.mozilla.com/D29825#881054

Comment 3

[Kershaw Chang [:kershaw]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #2)

This is a limitation in the networking code, not extensions, per https://phabricator.services.mozilla.com/D29825#881054

My apologies not seeing that.
I've put this in our internal queue to get this fixed soon.

Comment 4

[BMO Automation]

Moving bug to Core/Networking: Proxy.

Comment 5

[BMO Automation]

Moving bug to Core/Networking: Proxy

Comment 6

[Manuel Bucher [:manuel]]

Adding matrix chat from #necko:mozilla.org (2024-01-23):

erosman:

Any chance of working on Bug 1794464?

There might not be much (if any) work needed by the necko except agreeing to remove the restriction.
WebExtension API can then be updated (remove one throw) which should be easy enough.

It is causing complication for example... https://github.com/foxyproxy/browser-extension/issues/76#issuecomment-1906398346

manuel:

Do you feel comfortable preparing the patch yourself? The priority P3 indicates that we would accept patches enabling the behavior. It is currently not on our radar of bugs that we work on, though. I think this is the fastest way of getting it fixed would be to write the patch.
It seems simple and a good bug candidate for a first bug to work on. I'd mentor you if you need help.

erosman:

If I understand correctly, AFA proxy.onRequest, all that is needed is the removal of https://searchfox.org/mozilla-central/source/toolkit/components/extensions/ProxyChannelFilter.sys.mjs#177-181

I have never done a patch for Firefox, and I am concerned about testing which is compiling and providing test cases. These are processes that I am not familiar with. Otherwise, providing a PR with those 5 lines removed is not difficult.

manuel:

Test would make the patch harder indeed. Not sure right now which ones exists. Will check tomorrow. And I'm not sure if we still need to exclude SOCKS4(/5) proxies there. But will also take a look tomorrow.

erosman:

proxyAuthorizationHeader is sent for the HTTP/S types as...

`'Basic ' + btoa(username + ':' + password)

I don't know how the user/pass is sent for SOCKS.

I didn't take a look yet how the current test situation looks like and how hard creating tests would be. Adding the conversation from matrix, because it is hard to find for me and I am better at keeping track of stuff in bugzilla.

Comment 7

[erosman]

Summary

• Browsers already provide credential subsequent to 407 Proxy Authentication Required request to HTTP & HTTPS requesters
• webRequest.onAuthRequired provides a browser extension API for the above
• proxy.onRequest should also be permitted to provide authentication to both HTTP & HTTPS requesters like above

Patch

• There should not be a need to change anything AFA the underlying core:networking code is concerned
• Remove the following lines from ProxyChannelFilter.sys.mjs#177-180

    if (type !== "https") {
      throw new ExtensionError(
        `ProxyInfoData: ProxyAuthorizationHeader requires type "https"`
      );

• Remove HTTP test from test_proxy_info_results.js#186-198

    {
      proxy: [
        {
          type: "http",
          host: "foo.bar",
          port: 3128,
          proxyAuthorizationHeader: "test",
        },
      ],
      expected: {
        error: 'ProxyInfoData: ProxyAuthorizationHeader requires type "https"',
      },
    },

• If needed, add an HTTP test after line test_proxy_info_results.js#449

      proxy: [
        {
          type: "http",
          host: "foo.bar",
          port: 3128,
          proxyAuthorizationHeader: "test",
          connectionIsolationKey: "key",
        },
      ],
      expected: {
        proxyInfo: {
          host: "foo.bar",
          port: "3128",
          type: "http",
          proxyAuthorizationHeader: "test",
          connectionIsolationKey: "key",
        },
      },
    },

Comment 8

[Manuel Bucher [:manuel]]

Attachment: Bug 1794464 - Allow HTTP authentication in proxy.onRequest r=#necko

This removes the restriction put in place in Bug 1545420. Auth is
currently already possible, but more inconvenient. Lift the restriction
to make Addon development more pleasant.

Comment 9

[Rob Wu [:robwu]]

This ended up being an extension-only change, so moving components and added dev-doc-needed for when this lands.

Comment 10

[erosman]

MDN

Filed Add HTTP to proxy.ProxyInfo 'proxyAuthorizationHeader'

connectionIsolationKey

• Is connectionIsolationKey affected? Does it require a change to HTTP/HTTPS as part of this bug as well or not? (I don't see a restriction)
• Should MDN doc reflect a change under connectionIsolationKey?

  An optional key used for additional isolation of this proxy connection. Applies to HTTPS proxies only.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/745e254ec119
Allow HTTP authentication in proxy.onRequest r=extension-reviewers,robwu

Comment 12

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/745e254ec119

Comment 13

[rbloor]

Documentation update in content repository PR #32536 "proxy.ProxyInfo.proxyAuthorizationHeader HTTP support". (This requirement is also represented in content repository issue #32475 "Add HTTP to proxy.ProxyInfo 'proxyAuthorizationHeader'".)

Comment 14

[Rob Wu [:robwu]]

(In reply to erosman from comment #10)

MDN

Filed Add HTTP to proxy.ProxyInfo 'proxyAuthorizationHeader'

connectionIsolationKey

• Is connectionIsolationKey affected? Does it require a change to HTTP/HTTPS as part of this bug as well or not? (I don't see a restriction)
• Should MDN doc reflect a change under connectionIsolationKey?

  An optional key used for additional isolation of this proxy connection. Applies to HTTPS proxies only.

In Manuel's patch I see a test adding "http" type and connectionIsolationKey (diff source), but I think that it is more copy-paste than an intentional change, since connectionIsolationKey key seems independent of the proxyAuthorizationHeader feature.

Nevertheless, when I trace the use of connectionIsolationKey, it ends up at nsHttpConnectionInfo::BuildHashKey.

I see that the comment was inadvertently added as part of an unrelated change, in https://github.com/mdn/content/pull/17958/files. I'll comment on Github to get the documentation fixed.