Accept SOCKS credentials in proxyInfo object

RESOLVED FIXED in Firefox 45

Status

()

Core
Networking
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: arthuredelstein, Assigned: arthuredelstein)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

(Whiteboard: [tor])

Attachments

(1 attachment, 3 obsolete attachments)

Created attachment 8655622 [details] [diff] [review]
0001-Accept-RFC1929-SOCKS-credentials-in-proxyInfo.patch

Tor Browser uses SOCKs credentials (RFC 1929) when communicating with the Tor process. For each new connection, a proxyFilter has the opportunity to assign a new SOCKS username/password via the proxyInfo object. We would like to propose upstreaming this patch to mozilla-central.

The attached patch includes Tor Browser's implementation for SOCKS authentication. In this patch, the proxyInfo object is checked for the presence of a SOCKS username/password before connects to the SOCKS server.

This patch does not include a user interface for SOCKS auth, nor a database of credentials for different servers, as discussed in 122752, comment 80.
Attachment #8655622 - Flags: review?(michal.novotny)
(Assignee)

Comment 1

2 years ago
(The last sentence should have linked to bug 122752, comment 80.)
Comment on attachment 8655622 [details] [diff] [review]
0001-Accept-RFC1929-SOCKS-credentials-in-proxyInfo.patch

Review of attachment 8655622 [details] [diff] [review]:
-----------------------------------------------------------------

So, if I understand correctly the patch is mostly the same as in bug 122752. As I already wrote, user/pass for normal socks proxy (i.e. not Tor) should be obtained from some auth manager or user prompt once we find out in nsSOCKSSocketInfo that the proxy supports it. I.e. user/pass in nsIProxyInfo will be used only by Tor browser. Let's see what Patrick thinks about this change. I'll review the socks part once he review all the proxy changes.
Attachment #8655622 - Flags: review?(michal.novotny) → review?(mcmanus)
Comment on attachment 8655622 [details] [diff] [review]
0001-Accept-RFC1929-SOCKS-credentials-in-proxyInfo.patch

Review of attachment 8655622 [details] [diff] [review]:
-----------------------------------------------------------------

overall I'm ok with the notion that we have an optional set of default bearer credentials that get sent on first use.. and its even ok that its part of nsIProxyInfo. That ought to work for both socks and http proxies though.. as for where to store those credentials, I'm not sure. prefs seems wonky, but I think you should ask sworkman to help figure out if that's right.

::: netwerk/base/nsIProtocolProxyService.idl
@@ +140,4 @@
>                                in nsIProxyInfo aFailoverProxy);
>  
>      /**
> +     * This method may be called to construct a nsIProxyInfo instance for

you didn't bump the uuid

@@ +161,5 @@
> +     * @param aFailoverProxy
> +     *        Specifies the next proxy to try if this proxy fails.  This
> +     *        parameter may be null.
> +     */
> +    nsIProxyInfo newSOCKSProxyInfo(in AUTF8String aHost, in long aPort,

why isn't this just newProxyInfo2 with auth information added to the old one?

::: netwerk/base/nsIProxyInfo.idl
@@ +51,4 @@
>    readonly attribute unsigned long resolveFlags;
>  
>    /**
> +   * Specifies a SOCKS5 username.

probly not socks5 only, right?

::: netwerk/base/nsProtocolProxyService.cpp
@@ +628,5 @@
>      if (!pref || !strcmp(pref, PROXY_PREF("socks_port")))
>          proxy_GetIntPref(prefBranch, PROXY_PREF("socks_port"), mSOCKSProxyPort);
>  
> +    if (!pref || !strcmp(pref, PROXY_PREF("socks_username")))
> +        proxy_GetStringPref(prefBranch, PROXY_PREF("socks_username"), mSOCKSProxyUsername);

I'm not sure about storing credentials in prefs.. I would want to find someone to help with that design. Maybe ni sworkman.

but as to the prefs themselves, again - not socks specific.

::: netwerk/protocol/http/nsHttpConnectionInfo.cpp
@@ +167,5 @@
>          mHashKey.Append(')');
> +        mHashKey.Append('[');
> +        mHashKey.Append(ProxyUsername());
> +        mHashKey.Append(':');
> +        mHashKey.Append(ProxyPassword());

this hashkey will end up in a bunch of debug logging. not sure if that's a problem, but I'll note it.
Attachment #8655622 - Flags: review?(mcmanus)
(Assignee)

Comment 4

2 years ago
Created attachment 8671156 [details] [diff] [review]
0001-Accept-RFC1929-SOCKS-credentials-in-proxyInfo.patch

(In reply to Patrick McManus [:mcmanus] from comment #3)
> Comment on attachment 8655622 [details] [diff] [review]
> 0001-Accept-RFC1929-SOCKS-credentials-in-proxyInfo.patch
> 
> Review of attachment 8655622 [details] [diff] [review]:
> -----------------------------------------------------------------
[...]
> ::: netwerk/base/nsIProtocolProxyService.idl
> @@ +140,4 @@
> >                                in nsIProxyInfo aFailoverProxy);
> >  
> >      /**
> > +     * This method may be called to construct a nsIProxyInfo instance for
> 
> you didn't bump the uuid

Done.

> @@ +161,5 @@
> > +     * @param aFailoverProxy
> > +     *        Specifies the next proxy to try if this proxy fails.  This
> > +     *        parameter may be null.
> > +     */
> > +    nsIProxyInfo newSOCKSProxyInfo(in AUTF8String aHost, in long aPort,
> 
> why isn't this just newProxyInfo2 with auth information added to the old one?

For most proxies, auth info isn't needed, so it's convenient to have a function without them, rather than requiring the caller to provide empty strings.

> ::: netwerk/base/nsIProxyInfo.idl
> @@ +51,4 @@
> >    readonly attribute unsigned long resolveFlags;
> >  
> >    /**
> > +   * Specifies a SOCKS5 username.
> 
> probly not socks5 only, right?

We (Tor Browser) only need a username/password for the SOCKS proxy. I could try to implement username/password for the HTTP proxy, but would it be better to simply throw a "not implemented" exception instead, on the YAGNI principle?

> ::: netwerk/base/nsProtocolProxyService.cpp
> @@ +628,5 @@
> >      if (!pref || !strcmp(pref, PROXY_PREF("socks_port")))
> >          proxy_GetIntPref(prefBranch, PROXY_PREF("socks_port"), mSOCKSProxyPort);
> >  
> > +    if (!pref || !strcmp(pref, PROXY_PREF("socks_username")))
> > +        proxy_GetStringPref(prefBranch, PROXY_PREF("socks_username"), mSOCKSProxyUsername);
> 
> I'm not sure about storing credentials in prefs.. I would want to find
> someone to help with that design. Maybe ni sworkman.

These prefs are not needed by Tor, so I have removed them.

> but as to the prefs themselves, again - not socks specific.
> 
> ::: netwerk/protocol/http/nsHttpConnectionInfo.cpp
> @@ +167,5 @@
> >          mHashKey.Append(')');
> > +        mHashKey.Append('[');
> > +        mHashKey.Append(ProxyUsername());
> > +        mHashKey.Append(':');
> > +        mHashKey.Append(ProxyPassword());
> 
> this hashkey will end up in a bunch of debug logging. not sure if that's a
> problem, but I'll note it.

Good point -- I suppose we could sha256-hash these if you think it is worth it.
Attachment #8655622 - Attachment is obsolete: true
Flags: needinfo?(mcmanus)
> > > +     * @param aFailoverProxy
> > > +     *        Specifies the next proxy to try if this proxy fails.  This
> > > +     *        parameter may be null.
> > > +     */
> > > +    nsIProxyInfo newSOCKSProxyInfo(in AUTF8String aHost, in long aPort,
> > 
> > why isn't this just newProxyInfo2 with auth information added to the old one?
> 
> For most proxies, auth info isn't needed, so it's convenient to have a
> function without them, rather than requiring the caller to provide empty
> strings.

so the function without them would be newProxyInfo and the function with them would be newProxyInfo2 (or newProxyInfoWithAuth())

> 
> > ::: netwerk/base/nsIProxyInfo.idl
> > @@ +51,4 @@
> > >    readonly attribute unsigned long resolveFlags;
> > >  
> > >    /**
> > > +   * Specifies a SOCKS5 username.
> > 
> > probly not socks5 only, right?
> 
> We (Tor Browser) only need a username/password for the SOCKS proxy. I could
> try to implement username/password for the HTTP proxy, but would it be
> better to simply throw a "not implemented" exception instead, on the YAGNI
> principle?
> 

so I think throwing not implemented is fine, but for a different reason - http auth requires more parameters (digest/ntlm/basic) etc..

> > 
> > ::: netwerk/protocol/http/nsHttpConnectionInfo.cpp
> > @@ +167,5 @@
> > >          mHashKey.Append(')');
> > > +        mHashKey.Append('[');
> > > +        mHashKey.Append(ProxyUsername());
> > > +        mHashKey.Append(':');
> > > +        mHashKey.Append(ProxyPassword());
> > 
> > this hashkey will end up in a bunch of debug logging. not sure if that's a
> > problem, but I'll note it.
> 
> Good point -- I suppose we could sha256-hash these if you think it is worth
> it.


I think that makes sense - thanks!
Flags: needinfo?(mcmanus)
(Assignee)

Comment 6

2 years ago
Created attachment 8671760 [details] [diff] [review]
0001-Bug-1200802-Accept-RFC1929-SOCKS-credentials-in-prox.patch

(In reply to Patrick McManus [:mcmanus] from comment #5)
> > > > +     * @param aFailoverProxy
> > > > +     *        Specifies the next proxy to try if this proxy fails.  This
> > > > +     *        parameter may be null.
> > > > +     */
> > > > +    nsIProxyInfo newSOCKSProxyInfo(in AUTF8String aHost, in long aPort,
> > > 
> > > why isn't this just newProxyInfo2 with auth information added to the old one?
> > 
> > For most proxies, auth info isn't needed, so it's convenient to have a
> > function without them, rather than requiring the caller to provide empty
> > strings.
> 
> so the function without them would be newProxyInfo and the function with
> them would be newProxyInfo2 (or newProxyInfoWithAuth())

OK, I have called it newProxyInfoWithAuth().

> > 
> > > ::: netwerk/base/nsIProxyInfo.idl
> > > @@ +51,4 @@
> > > >    readonly attribute unsigned long resolveFlags;
> > > >  
> > > >    /**
> > > > +   * Specifies a SOCKS5 username.
> > > 
> > > probly not socks5 only, right?
> > 
> > We (Tor Browser) only need a username/password for the SOCKS proxy. I could
> > try to implement username/password for the HTTP proxy, but would it be
> > better to simply throw a "not implemented" exception instead, on the YAGNI
> > principle?
> > 
> 
> so I think throwing not implemented is fine, but for a different reason -
> http auth requires more parameters (digest/ntlm/basic) etc..

OK, done.

> > > 
> > > ::: netwerk/protocol/http/nsHttpConnectionInfo.cpp
> > > @@ +167,5 @@
> > > >          mHashKey.Append(')');
> > > > +        mHashKey.Append('[');
> > > > +        mHashKey.Append(ProxyUsername());
> > > > +        mHashKey.Append(':');
> > > > +        mHashKey.Append(ProxyPassword());
> > > 
> > > this hashkey will end up in a bunch of debug logging. not sure if that's a
> > > problem, but I'll note it.
> > 
> > Good point -- I suppose we could sha256-hash these if you think it is worth
> > it.
> 
> 
> I think that makes sense - thanks!

I've added sha256 hashing to the password.
Attachment #8671156 - Attachment is obsolete: true
Attachment #8671760 - Flags: review?(mcmanus)
Comment on attachment 8671760 [details] [diff] [review]
0001-Bug-1200802-Accept-RFC1929-SOCKS-credentials-in-prox.patch

Review of attachment 8671760 [details] [diff] [review]:
-----------------------------------------------------------------

I'm ready to r+ this - thanks!

BUT before you check it in, will you do some regression testing of http proxies with and witout basic auth on them. quid would be fine here. It would be good if you tried both configuring explicitly and via pac. Our automated testing of real proxies is almost non existant (partly because most of our infrastructure uses a fake proxy and getting the two to coexist has been more trouble than its worth).
Attachment #8671760 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 8

2 years ago
Comment on attachment 8671760 [details] [diff] [review]
0001-Bug-1200802-Accept-RFC1929-SOCKS-credentials-in-prox.patch

(In reply to Patrick McManus [:mcmanus] from comment #7)
> Comment on attachment 8671760 [details] [diff] [review]
> 0001-Bug-1200802-Accept-RFC1929-SOCKS-credentials-in-prox.patch
> 
> Review of attachment 8671760 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm ready to r+ this - thanks!
> 
> BUT before you check it in, will you do some regression testing of http
> proxies with and witout basic auth on them. quid would be fine here. It
> would be good if you tried both configuring explicitly and via pac. Our
> automated testing of real proxies is almost non existant (partly because
> most of our infrastructure uses a fake proxy and getting the two to coexist
> has been more trouble than its worth).

Thanks for the review!

As requested, I ran manual regression tests using squid3. I set it up using the following config file:

  auth_param digest program /usr/lib/squid3/digest_file_auth -c /etc/squid3/passwords
  auth_param digest realm proxy
  acl authenticated proxy_auth REQUIRED
  http_access allow authenticated
  http_port 3128

I entered a proxy user and password with

  htdigest -c /etc/squid3/passwords proxy user

Then I used the Firefox dialog at
  about:preferences > Advanced > Network > Connection Settings
to set the Proxy to "Manual proxy configuration", where I entered an HTTP Proxy with 127.0.0.1:3128.

When I attempted to browse to a web page, I was presented with a password prompt. Entering the correct password resulted in the page successfully loading, and I could monitor pages loading by tailing the squid3 access log file:

  tail -f /var/log/squid3/access.log

I then restarted Firefox and tested the following minimal PAC file (instead of Manual proxy configuration):

  function FindProxyForURL(url, host) {
    return "PROXY 127.0.0.1:3128";
  }

And I got the same behavior: an authentication prompt, followed by normal proxyied connections.

So the next step is to see what :michal thinks of the SOCKS part of this patch.
Attachment #8671760 - Flags: review?(michal.novotny)
Comment on attachment 8671760 [details] [diff] [review]
0001-Bug-1200802-Accept-RFC1929-SOCKS-credentials-in-prox.patch

Review of attachment 8671760 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the assertion fixed

::: netwerk/socket/nsSOCKSIOLayer.cpp
@@ +619,4 @@
>  
>      LOGDEBUG(("socks4: checking connection reply"));
> + 
> +    if (mDataLength != 8) {

Why did you change the assertion to a normal if check? This should never happen so we shouldn't try to handle such situation. If it ever happens then ReadFromSocket() doesn't work correctly and assertion is the best way to get notified about it. Please change it back on all places including the new code.

@@ +656,5 @@
>      mDataLength = Buffer<BUFFER_SIZE>(mData)
>                    .WriteUint8(0x05) // version -- 5
> +                  .WriteUint8(0x01) // # of auth methods -- 1
> +                  // Use authenticate iff we have a proxy username.
> +                  .WriteUint8(mProxyUsername.IsEmpty() ? 0x00 : 0x02)

See my comment #80 in bug 122752. I'm not going to block the review on this, but expect this change to happen if we ever support normal socks proxy authentication.
Attachment #8671760 - Flags: review?(michal.novotny) → review+
(Assignee)

Comment 10

2 years ago
Created attachment 8691771 [details] [diff] [review]
0001-Bug-1200802-Accept-RFC1929-SOCKS-credentials-in-prox.patch

(In reply to Michal Novotny (:michal) from comment #9)
> Comment on attachment 8671760 [details] [diff] [review]
> 0001-Bug-1200802-Accept-RFC1929-SOCKS-credentials-in-prox.patch
> 
> Review of attachment 8671760 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the assertion fixed

Thank you for the review.

> ::: netwerk/socket/nsSOCKSIOLayer.cpp
> @@ +619,4 @@
> >  
> >      LOGDEBUG(("socks4: checking connection reply"));
> > + 
> > +    if (mDataLength != 8) {
> 
> Why did you change the assertion to a normal if check? This should never
> happen so we shouldn't try to handle such situation. If it ever happens then
> ReadFromSocket() doesn't work correctly and assertion is the best way to get
> notified about it. Please change it back on all places including the new
> code.

Good point -- I changed them back and used asserts in similar places in the new code.

> @@ +656,5 @@
> >      mDataLength = Buffer<BUFFER_SIZE>(mData)
> >                    .WriteUint8(0x05) // version -- 5
> > +                  .WriteUint8(0x01) // # of auth methods -- 1
> > +                  // Use authenticate iff we have a proxy username.
> > +                  .WriteUint8(mProxyUsername.IsEmpty() ? 0x00 : 0x02)
> 
> See my comment #80 in bug 122752. I'm not going to block the review on this,
> but expect this change to happen if we ever support normal socks proxy
> authentication.

That sounds reasonable to me.

Attaching the new version, with original author's name. Here are the try server results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ebc410e5c7e
Attachment #8671760 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 11

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/76cee391a698
Keywords: checkin-needed

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/76cee391a698
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45

Comment 13

2 years ago
(In reply to Carsten Book [:Tomcat] from comment #12)
> https://hg.mozilla.org/mozilla-central/rev/76cee391a698
This changeset lists as author: Ben Bucksch <ben.bucksch@beonex.com>
Shouldn't the author be: Arthur Edelstein ?
Flags: needinfo?(cbook)

Updated

2 years ago
Assignee: nobody → arthuredelstein

Updated

2 years ago
Depends on: 1228422

Comment 14

2 years ago
Oops
Flags: needinfo?(cbook)

Comment 15

a year ago
I just tried Firefox 45.  It does not support authentication for SOCKS5 - there is no option to enter username/password, nor does any dialog box pop up to prompt you for the username/password - or am I missing something?

Where do you enter the SOCKS5 username/password on FF 45?

How do I populate the ProxyInfo object with my username/password?
Philip Chee, I'm the original author of the patch, so that's fair.
Tomcat, thanks for crediting me in the git commit!
Arthur, thanks for picking up my patch and bringing it to land.
Ben
alpha3, as mentioned above, there is no UI for this patch. My patch had at least an about:config preference socks_username and socks_password, but it was removed here in comment 4. So, this feature currently can only be used from an XUL extension using the XPCOM API.

Comment 18

a year ago
@benB: ok thanks --- so how do us mere mortals get the XUL extension working... is there an add-on or something?  Or is there some code you can share?

@Arthur Edelstein --- any chance we can reinstate the about:config preference socks_username and socks_password?  This would make this patch more a lot more useful to the general public.

Look forward to seeing FF 46.

Updated

10 months ago
Blocks: 1260929

Comment 19

6 months ago
(In reply to Ben Bucksch (:BenB) from comment #17)
> alpha3, as mentioned above, there is no UI for this patch. My patch had at
> least an about:config preference socks_username and socks_password, but it
> was removed here in comment 4. So, this feature currently can only be used
> from an XUL extension using the XPCOM API.

@benB: Is this XPCOM API available in FF50? I am getting error when trying in FF50 developer edition:

const Cc = Components.classes, Ci = Components.interfaces
protocolProxyService = Cc["@mozilla.org/network/protocol-proxy-service;1"].getService(Ci.nsIProtocolProxyService);
protocolProxyService.newProxyInfoWithAuth('socks', 'host', '888', 'user', 'pass', 0, 0, null)
[Exception... "JavaScript component does not have a method named: "newProxyInfoWithAuth"'JavaScript component does not have a method named: "newProxyInfoWithAuth"' when calling method: [nsIProtocolProxyService::newProxyInfoWithAuth]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame :: debugger eval code :: <TOP_LEVEL> :: line 1"  data: no]

Comment 20

6 months ago
(In reply to sevenever from comment #19)

> Is this XPCOM API available in FF50? I am getting error when trying
> in FF50 developer edition:

[Exception... "JavaScript component does not have a method named: "newProxyInfoWithAuth"'JavaScript component does not have a method named: "newProxyInfoWithAuth"' when calling method: [nsIProtocolProxyService::newProxyInfoWithAuth]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"

Confirmed with 52.0a2 (2016-12-15) (64-bit) Firefox Developer Edition Aurora Update Channel.
You need to log in before you can comment on or make changes to this bug.