feat: Add username/password SOCKS5 auth

[nurupo]

Fixes #1682

Not tested. Can someone with a SOCKS5 proxy with username/password test this?

I'm not sure how to prevent APIDSL from generating tox_options_set_proxy_socks5_username_length() and tox_options_set_proxy_socks5_password_length() function declarations. Those seem pointless as the user already sets the length in tox_options_set_proxy_socks5_username() and tox_options_set_proxy_socks5_password(). Savedata has the same issue with a pointless tox_options_set_savedata_length().

---

This change is 

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[nurupo]

Related RFCs:

• https://tools.ietf.org/html/rfc1928
• https://tools.ietf.org/html/rfc1929

[cryptogospod]

@nurupo Does this still need testing? I'm willing to test it.

[nurupo]

Yep, so far no one has tested it as far as I'm aware.

[nurupo]

Fixed a couple of issues I have noticed after reviewing with a fresh set of eyes.

[cryptogospod]

I managed to verify that SOCKS5 authenticated connections work. Currently I've only tested with toxic, but I will test qTox as soon as possible and post another update here.

[cryptogospod]

Update: I managed to verify that SOCKS5 authenticated connections work in qTox too. When using correct credentials, the client connects to a TCP relay and all traffic goes through it. In the case of incorrect credentials or lack of credentials, qTox fails to connect. In all cases, I noticed this in the log: net/updatecheck.cpp:137 : Warning: Failed to check for update: QNetworkReply::ProxyAuthenticationRequiredError.

EDIT (comment from IRC):
anthonybilinski: re: that log, qTox will need to update our http version check to use the same user/pass. unrelated to toxcore

[nurupo]

Cool, thanks for testing.

In all cases, I noticed this in the log: net/updatecheck.cpp:137 : Warning: Failed to check for update: QNetworkReply::ProxyAuthenticationRequiredError

This is a qTox-specific thing, it says nothing about toxcore or this PR working or non-working.

Would be great if someone else could test this too, I'm not too sure if @cryptogospod tested it correctly. The way it should have been tested, that QNetworkReply::ProxyAuthenticationRequiredError shouldn't have triggered.

Since @cryptogospod has asked this on IRC, I want to point it out here too: for this to be merged, there are a few more steps that need to be done:

• I would like someone else to test it
• APIDSL needs to be fixed so that it doesn't generate tox_options_set_proxy_socks5_username_length() and tox_options_set_proxy_socks5_password_length() functions
• Someone has to review and approve this PR

[anthonybilinski]

@nurupo I walked through testing with qTox with @cryptogospod at the time, the diff was adding the calls to the new API here: https://github.com/qTox/qTox/blob/da5c165f4116dce736def63b1b917523b848db09/src/core/toxoptions.cpp#L133.

The QNetworkReply::ProxyAuthenticationRequiredError was triggered from https://github.com/qTox/qTox/blob/da5c165f4116dce736def63b1b917523b848db09/src/net/updatecheck.cpp#L137 when qTox tries to check its release version over https, because it's loading its proxy info from settings, but not the proxy password which was just hacked in to toxoptions.

Agreed that the error doesn't say anything about toxcore, but I do think that @cryptogospod's testing showing that toxcore either connects or doesn't connect when using the proxy with the correct or incorrect password shows password support's working.

[nurupo]

Ok, makes sense and also explains why QNetworkReply::ProxyAuthenticationRequiredError was triggered when I didn't expect it to. I had a different diff in mind for the purpose of testing - hardcode to have toxcore always connect to a proxy, i.e. it would do so even with the proxy in qTox settings turned off, which why I was surprised to see that error.

That's a valid way to test it too, so it sounds like it was tested correctly.

[nurupo]

So, as for the state of this PR, per the checklist in #1683 (comment):

• "I would like someone else to test it" -- I think it has been tested enough at this point
• "APIDSL needs to be fixed so that it doesn't generate tox_options_set_proxy_socks5_username_length() and tox_options_set_proxy_socks5_password_length() functions" -- since we don't use APIDSL anymore I just dropped those, so that's no longer an issue
• "Someone has to review and approve this PR" -- that still needs to be done

[nurupo]

Also, I'm thinking of replacing the two functions:

void tox_options_set_proxy_socks5_username(Tox_Options *options,
  const uint8_t username[TOX_MAX_PROXY_SOCKS5_USERNAME_LENGTH], size_t length);
void tox_options_set_proxy_socks5_password(Tox_Options *options,
  const uint8_t password[TOX_MAX_PROXY_SOCKS5_PASSWORD_LENGTH], size_t length);

with one:

void tox_options_set_proxy_socks5_username_password(Tox_Options *options,
  const uint8_t username[TOX_MAX_PROXY_SOCKS5_USERNAME_LENGTH], size_t username_length,
  const uint8_t password[TOX_MAX_PROXY_SOCKS5_PASSWORD_LENGTH], size_t password_length);

Since it doesn't make sense to have one set but not the other. Even the proxy code skips doing username/password authentication if one of the username or the password (or both) is not set.

Username and password would still have two separate getter functions though, so it's kind of asymmetric that way -- one setter but two getters. This does make the API clearer, in my opinion, but I wonder if such asymmetry is an issue for @iphydf as far as his binding generating code goes.

Opinions?

[nurupo]

Note that SOCKS5 protocol has a lot more authentication methods than just the two of None and Username/Password. So in theory it would make sense to add some sort of Tox_Proxy_Socks5_Authentication_Method enum and a setter/getter for Tox_Options for the user to select which authentication method they want to use. However, I don't really foresee us supporting any more than just these two authentication methods, so with that in mind, the current API is sufficient and I don't think there is a need to introduce any Tox_Proxy_Socks5_Authentication_Method enum. The way it works: if a user doesn't set username/password -- it's assumed that no auth is used, and if they are set -- username/password auth is used.

Another thing to note is that there is no support for multiple authentication method negotiation. A SOCKS5 client can tell to a SOCKS5 server that it supports multiple authentication methods, e.g. None, Username/Password, etc. and the server then replies with which one it wants the client to use. Currently toxcore says to the proxy that it supports only one authentication method, either None or Username/Password, depending on whether username/password is set by the user or not. It's unusual for the user not to know what authentication method the proxy they are trying to connect to supports, so I figured it doesn't make much sense to support this.

[emdee-is]

@nurupo Thanks - I agree the simplest case covers corporate proxies which I feel is the target of this, and perhaps some Tor users.

What has to be done to get this merged? 2 failing, and 1 expected checks?
Does it need a review from someone?

[emdee-is]

Does it deserve a couple of lines in docs/TCP_Network.txt ? A cut-n-paste of what you just wrote, plus a little on proxies?

This changes Tox_Options so I'll need to change my ctypes wrapper when it lands. It would be nice if it lands in a new release so I can change my wrapper depending on the release version.

Is there any kind of "what's new in release" document that gets issued with new releases to note the change in Tox_Options? I don't see a ChangeLog.

Should this be tagged with "API Break"?

[emdee-is]

FWIW there's a partially resurected echobot in Python at https://git.plastiras.org/emdee/toxygen_wrapper/src/branch/main/src/toxygen_wrapper/toxygen_echo.py that is socks aware (partly username/passwd) if it helps you for testing.

I don't know Tox AV well enough to fix the AV part of the echobot, but I presume the AV code was working. Alex Baker's echobot is not SOCKSified, nor is it every likely to be alexbakker/EchoBot#9.

It's from PyTox https://github.com/aitjcize/PyTox/raw/master/examples/echo.py ported to my toxygen_wrapper bindings.

[emdee-is]

tox_options_set_proxy_socks5_username should be tox_options_set_proxy_username i feel but you decide; the proxy user and password should also work for an http proxy, also common in corporate settings.

it's seriously unlikely/impossible that you would have http and socks proxies going on at the same time in the same Tox, so you might as well double use the username/password code if you added http passsword proxies later.

I've seen corporate settings with both http and socks proxies with the same username/password (NTLM) but very different network behaviours: the http proxy was MITMed.

[robinlinden]

I know these used to be generated, but should we really add new deprecated-at-the-time-of-creation macros?

[nurupo]

I wonder.

We have also recently changed the API from using *buffer to buffer[SIZE_CONSTANT] in #2518, e.g.

bool tox_bootstrap(Tox *tox, const char *host, uint16_t port, const uint8_t public_key[TOX_PUBLIC_KEY_SIZE], Tox_Err_Bootstrap *error);

Why are we making the API functions use deprecated macros?

Are we actually keeping the macros now, not deprecating them?

@iphydf ?

Anyway, I made the API changes such that they would match what the rest of the API does, that is: define constants and use them for array length in function declarations. You can see those used in the function declarations:

void tox_options_set_proxy_socks5_username(Tox_Options *options, const uint8_t username[TOX_MAX_PROXY_SOCKS5_USERNAME_LENGTH], size_t length);

void tox_options_set_proxy_socks5_password(Tox_Options *options, const uint8_t password[TOX_MAX_PROXY_SOCKS5_PASSWORD_LENGTH], size_t length);

I think it makes sense to add the macros, even if they are deprecated at the creation, in order to keep the API coherent. Currently everything has macros. Once we decide to remove the macros, we can remove them all at once and nothing will have macros. That way there is no in-between state where some things have macros but some don't. (Please point out if we are already in the in-between state where some of the added APIs don't define macros, in which case I will remove the macros too. I checked that the ngc defines macros, so figured we are still in the all-macro state, but I might have missed something.)

[Green-Sky]

The arrays are not always of max size, or might not be arrays at all.

[nurupo]

@Green-Sky I don't understand what you are saying, can you re-phrase that or provide an example of what do you mean?

[Green-Sky]

If I am not mistaken, then the size is supposed to be exact, so you would have to provide a max sized buffer (if your code is also checked by cimple).

so something like

const char* socks_host = "localhost";

would be strongly discoraged.

[nurupo]

@Green-Sky If you are saying that an API user can pass uint8_t * for the array and that uint8_t array[SIZE] doesn't mean that the array the user passes is of exactly SIZE length (in fact, if you do sizeof(array) on a function with uint8_t array[SIZE] argument, you will notice that it's equal to sizeof(uint8_t *)) -- then yes, that's how it indeed works. The SIZE in uint8_t array[SIZE] is merely for the documentation purposes.

Still, I don't understand what you meant by stating the obvious. Is there some issue in here? Do you want something in the PR to be changed? Please be more explicit!

[nurupo]

@Green-Sky Oh, your 2nd comment didn't show up for me until I refreshed the page just now.

It seems like you are thinking of uint8_t array[static SIZE] instead of uint8_t array[SIZE]. It's the static variant that promises for the array to be at least SIZE in length. The non-static variant doesn't make any promises, it's equivalent to uint8_t array[]. Please correct if I'm wrong though, it might be me who is confusing them.

[nurupo]

Ok, I think I understand what you mean. I just checked the API and we have the size specified only for arrays that should always be of that exact size. So it's a cimple thing, not a C standard thing?

Alright, I will be removing those size specifiers then, thanks for letting me know and bearing with my confusion, @Green-Sky.

[nurupo]

@Green-Sky is this better? You can see the diff as r2 vs r3 in Reviewable (assuming it shows the same revisions to everyone).

[Green-Sky]

@nurupo sorry, this noti got burried in my github inbox.

I just checked the API and we have the size specified only for arrays that should always be of that exact size. So it's a cimple thing, not a C standard thing?

Yea, that is what I meant. We should probably specify when we talk about the toxcore c-dialect cimple. :)

[nurupo]

Regarding #1683 (comment), after a discussion with @iphydf on IRC it was decided to keep the username and password setter functions separate, as a single setter function setting both would be bad for binding generation. (An alternative could have been an opaque username-password-socks5-auth struct with a separate constructior/destructor/setter/getter functions, which we would set on Tox_Options, but it seemed a bit excessive).