New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CURLOPT_SOCKS5_AUTH: allowed methods for SOCKS5 proxy auth #1454

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@kdudka
Collaborator

kdudka commented Apr 28, 2017

If libcurl was built with GSS-API support, it unconditionally advertised GSS-API authentication while connecting to a SOCKS5 proxy. This caused problems in environments with improperly configured Kerberos: a stock libcurl failed to connect, despite libcurl built without GSS-API connected fine using username and password.

This commit introduces the CURLOPT_SOCKS5_AUTH option to control the allowed methods for SOCKS5 authentication at run time.

Note that a new option was preferred over reusing CURLOPT_PROXYAUTH for compatibility reasons because the set of authentication methods allowed by default was different for HTTP and SOCKS5 proxies.

Bug: https://curl.haxx.se/mail/lib-2017-01/0005.html

@mention-bot

This comment has been minimized.

mention-bot commented Apr 28, 2017

@kdudka, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @dfandrich and @yangtse to be potential reviewers.

@kdudka kdudka force-pushed the kdudka:socks5-auth branch from 244a925 to 95d09cd May 19, 2017

@iboukris

This comment has been minimized.

Contributor

iboukris commented May 20, 2017

Hi, I'm not well familiar with socks authentication but from what I could find (RFC 1961 and our code) I think that the 'negotiate' terminology might not be appropriate for socks authentication.
To my understanding, the Negotiate scheme in HTTP authentication means SPNEGO mechanism (RFC 4559) which involves negotiation of actual mechanism to use, but in socks-authentication the default GSS-API mechanism is used (no negotiation).
So I think in socks context we should refer to it as GSS-API authentication and so perhaps we need a new set of auth-methods for socks, like CURL_SOCKS_AUTH_GSSAPI (or perhaps instead, we could add a new CURLAUTH_GSSAPI to be used for socks, which will be internally defined as CURLAUTH_NEGOTIATE, since this is just a terminology issue as we currently do not let the app to chose the mechanism but use the appropriate one according to the protocol, SPNEGO for HTTP Negotiate and the default for socks authentication).

@kdudka

This comment has been minimized.

Collaborator

kdudka commented May 22, 2017

@frenche Hi Isaac, thanks for review! Although I do not know the terminology myself, your suggestion about calling it GSS-API instead of Negotiate makes perfect sense to me. I prefer to use CURLAUTH_GSSAPI as an alias to CURLAUTH_NEGOTIATE to make the constants less likely to misuse. There is no compile-time check to warn users if they pass CURLAUTH_NEGOTIATE to CURLOPT_SOCKS5_AUTH, or vice versa. I am also renaming the --no-socks5-negotiate option to --no-socks5-gssapi to stay consistent.

@kdudka kdudka force-pushed the kdudka:socks5-auth branch 2 times, most recently from a5c9905 to e5cdb56 May 22, 2017

@iboukris

This comment has been minimized.

Contributor

iboukris commented May 27, 2017

Hi, the code changes look ok, however I wonder why the tool option is different from the library one, why can't we have --socks5-basic and --socks5-gssapi like we have --proxy-basic and --proxy-negotiate.

@kdudka

This comment has been minimized.

Collaborator

kdudka commented May 29, 2017

The patch introduces both --socks5-gssapi and --no-socks5-gssapi. It only does not make sense to advertise --socks5-gssapi in the commit message when using gssapi has always been the default action (for curl with gssapi support). As for --no-socks5-basic, nobody has asked for such option so far. If anyone needs it, we can easily introduce the option later on.

@iboukris

This comment has been minimized.

Contributor

iboukris commented May 30, 2017

Currently, with HTTP and Proxy authentication, if no method has been specified it will use libcurl default (basic only), if one specifies --negotiate then it will be the only one allowed, if one specify more than one, like --negotiate --ntlm then both would be allowed.
I think it would be better if socks5 authentication would keep these semantics by being implemented in a similar manner (even if it has different defaults due to backward compatibility reasons).
That is, IMO, --socks5-gssapi should mean only gssapi (unlike the defaults), and in order to get only basic-auth one should use --socks-basic with no need to negate other methods specifically.

@kdudka

This comment has been minimized.

Collaborator

kdudka commented Jun 1, 2017

Good point. I will rework handling of the tool options.

@kdudka

This comment has been minimized.

Collaborator

kdudka commented Jun 19, 2017

Sorry for being silent on this. I have been busy with my newly born daughter. I will resubmit this PR later this week...

kdudka added some commits Mar 8, 2017

socks: use proxy_user instead of proxy_name
... to make it obvious what the data is used for
CURLOPT_SOCKS5_AUTH: allowed methods for SOCKS5 proxy auth
If libcurl was built with GSS-API support, it unconditionally advertised
GSS-API authentication while connecting to a SOCKS5 proxy.  This caused
problems in environments with improperly configured Kerberos: a stock
libcurl failed to connect, despite libcurl built without GSS-API
connected fine using username and password.

This commit introduces the CURLOPT_SOCKS5_AUTH option to control the
allowed methods for SOCKS5 authentication at run time.

Note that a new option was preferred over reusing CURLOPT_PROXYAUTH
for compatibility reasons because the set of authentication methods
allowed by default was different for HTTP and SOCKS5 proxies.

Bug: https://curl.haxx.se/mail/lib-2017-01/0005.html

@kdudka kdudka force-pushed the kdudka:socks5-auth branch from e5cdb56 to 0472f15 Jun 20, 2017

@kdudka

This comment has been minimized.

Collaborator

kdudka commented Jun 20, 2017

@frenche Hi, I have reworked the command-line options per your suggestion. The disadvantage of this approach is that --no-socks5-gssapi used on its own does nothing (similarly to --no-basic).

@coveralls

This comment has been minimized.

coveralls commented Jun 20, 2017

Coverage Status

Coverage decreased (-0.02%) to 73.757% when pulling 0472f15 on kdudka:socks5-auth into 0feb762 on curl:master.

@iboukris

Looks good to me.

@iboukris

This comment has been minimized.

Contributor

iboukris commented Jun 23, 2017

@kdudka

This comment has been minimized.

Collaborator

kdudka commented Jun 23, 2017

@kdudka kdudka closed this in 8924f58 Jun 28, 2017

kdudka added a commit that referenced this pull request Jun 28, 2017

@kdudka kdudka deleted the kdudka:socks5-auth branch Jul 10, 2017

@lock lock bot locked as resolved and limited conversation to collaborators May 13, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.