Closed Bug 1794464 Opened 2 years ago Closed 2 months ago

Allow HTTP authentication in proxy.onRequest

Categories

(WebExtensions :: Request Handling, enhancement, P3)

enhancement

Tracking

(firefox125 fixed)

RESOLVED FIXED
125 Branch
Tracking Status
firefox125 --- fixed

People

(Reporter: eros_uk, Assigned: manuel, NeedInfo)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [necko-triaged])

Attachments

(1 file)

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.

Component: Networking → Request Handling
Product: Core → WebExtensions

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
      );
    }

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

Component: Request Handling → Networking
Product: WebExtensions → Core

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

Severity: -- → N/A
Priority: -- → P2
Whiteboard: [necko-triaged][necko-priority-review]
Blocks: necko-proxy
Priority: P2 → P3
Whiteboard: [necko-triaged][necko-priority-review] → [necko-triaged]

Moving bug to Core/Networking: Proxy.

Component: Networking → Networking: Proxy

Moving bug to Core/Networking: Proxy

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.

Flags: needinfo?(manuel)

Summary

Patch

    if (type !== "https") {
      throw new ExtensionError(
        `ProxyInfoData: ProxyAuthorizationHeader requires type "https"`
      );
    {
      proxy: [
        {
          type: "http",
          host: "foo.bar",
          port: 3128,
          proxyAuthorizationHeader: "test",
        },
      ],
      expected: {
        error: 'ProxyInfoData: ProxyAuthorizationHeader requires type "https"',
      },
    },
      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",
        },
      },
    },

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.

Assignee: nobody → manuel
Status: NEW → ASSIGNED

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

Component: Networking: Proxy → Request Handling
Keywords: dev-doc-needed
Product: Core → WebExtensions

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.

Flags: needinfo?(rob)
Flags: needinfo?(manuel)
Pushed by mbucher@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/745e254ec119
Allow HTTP authentication in proxy.onRequest r=extension-reviewers,robwu
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 125 Branch

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'".)

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: