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 

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

[emdee-is]

@nurupo what's the state of this? Can it be merged?

Mentioned in:
#1682 (comment)

[Green-Sky]

@nurupo can you rebase this pr?

[nurupo]

@Green-Sky done

[iphydf]

We're introducing new data members here. Do we need them to not be owned by default as well?

[nurupo]

That would make the API inconsistent - some things are not owned and require a flag to be set, but others are owned.

[iphydf]

Does it functionally matter? If clients think it's not owned, they will save their pointers. If they enable the flag or happen to know that it is owned, they don't. It's making things more complicated, and 0.3.0 will get rid of this un-owned mode entirely.

[nurupo]

I believe I got this addressed now.

Per the IRC discussion:

<iphy> Maybe name the fields internal_do_not_set_directly_proxy_socks5_username etc
<iphy> No new code should be setting those fields, and we shouldn't build complexity to cover for that case

[nurupo]

This PR is currently blocked waiting on @iphydf to make cimple accept a pointer-to-pointer so that the CI on here passes. I could bypass the need to use a pointer to pointer by rewriting the helper function as a macro instead, but the less we use macros - the better, or at least so I think, as they are harder to grok.

[iphydf]

This PR is currently blocked waiting on @iphydf to make cimple accept a pointer-to-pointer so that the CI on here passes. I could bypass the need to use a pointer to pointer by rewriting the helper function as a macro instead, but the less we use macros - the better, or at least so I think, as they are harder to grok.

I wasn't aware that we had decided that pointer to pointer is the way to go. I'll have another look at the code.