Re: [PR] SOCKS4(A)
Hi.
On 02.10.20 13:54, Christopher Faulet wrote:
Le 02/10/2020 à 08:58, Willy Tarreau a écrit :
So if anyone currently uses socks4 to talk to servers, I suggest you
run a quick test on 2.2 or 2.3 to see if health checks continue to work
over socks4 or not, in which case it's likely you'll be able to provide
an easier reproducer that will allow to fix the problem. This will save
everyone time and protect our eyeballs by keeping them away from this
blinking patch.
There is indeed a bug. The flag CO_FL_SOCKS4 is set after the connect()
for tcp-checks, making the health-checks though a socks4 proxy fail.
Here is a patch to fix this bug. I will push it very soon.
Remains the support of the SOCKS4A in Alex patches. But I will let anyone
motivated by this part working on it :)
I was curious how much the patch really change and have took the time to
adopt the patch.
What I have seen is that he current code mixes tabs and white spaces also
the goto intentions. I think this is because of the evolution of the code ;-)
Attached the patch which I have cleaned from the formatings.
The code builds but I have not tested it if it works.
The main patch is 001 the other two are just to remove double definition
of functions.
Regards
Aleks
>From 249f3e2467f3957e4af786829d5bb585de7f2df9 Mon Sep 17 00:00:00 2001
From: Alex
Date: Sun, 4 Oct 2020 03:09:13 +0200
Subject: [PATCH 3/3] Sock4(A) original patch from alex v3
---
include/haproxy/connection.h | 34 --
1 file changed, 34 deletions(-)
diff --git a/include/haproxy/connection.h b/include/haproxy/connection.h
index e06de3c67..2189ab8c1 100644
--- a/include/haproxy/connection.h
+++ b/include/haproxy/connection.h
@@ -150,40 +150,6 @@ static inline void conn_prepare_new_for_socks4(struct connection *conn, struct s
}
}
-static inline void conn_set_domain(struct connection *conn, const char *domain)
-{
- conn_free_domain(conn);
- if (domain) {
- size_t len = strlen(domain) + 1;
- conn->requested_domain = malloc(len);
- if (!conn->requested_domain) {
- /* TODO: Handle malloc error */
- }
-
- memcpy(conn->requested_domain, domain, len);
- }
-}
-
-static int is_server_fake_address(struct server *srv)
-{
- return (srv->flags & SRV_F_SOCKS4_PROXY_FAILED_RESOLVE);
-}
-
-static inline void conn_set_domain_from_server(struct connection *conn, struct server *srv)
-{
- if (is_server_fake_address(srv))
- conn_set_domain(conn, srv->hostname);
-}
-
-static inline void conn_prepare_new_for_socks4(struct connection *conn, struct server *srv)
-{
- if (srv && (srv->flags & SRV_F_SOCKS4_PROXY)) {
- conn->send_proxy_ofs = 1;
- conn->flags |= CO_FL_SOCKS4;
- conn_set_domain_from_server(conn, srv);
- }
-}
-
/* Calls the close() function of the transport layer if any and if not done
* yet, and clears the CO_FL_XPRT_READY flag. However this is not done if the
* CO_FL_XPRT_TRACKED flag is set, which allows logs to take data from the
--
2.25.1
>From 9f9f4c33d15b3c15b37d54152b141bfc0e345ed2 Mon Sep 17 00:00:00 2001
From: Alex
Date: Sun, 4 Oct 2020 03:00:29 +0200
Subject: [PATCH 2/3] Sock4(A) original patch from alex v2
---
include/haproxy/connection.h | 9 -
1 file changed, 9 deletions(-)
diff --git a/include/haproxy/connection.h b/include/haproxy/connection.h
index c1eec84de..e06de3c67 100644
--- a/include/haproxy/connection.h
+++ b/include/haproxy/connection.h
@@ -150,15 +150,6 @@ static inline void conn_prepare_new_for_socks4(struct connection *conn, struct s
}
}
-static inline void conn_free_domain(struct connection *conn)
-{
- if (conn->requested_domain)
- {
- free(conn->requested_domain);
- conn->requested_domain = NULL;
- }
-}
-
static inline void conn_set_domain(struct connection *conn, const char *domain)
{
conn_free_domain(conn);
--
2.25.1
>From 294196a787f7345d42553d531d39906ef7a7468b Mon Sep 17 00:00:00 2001
From: Alex
Date: Sun, 4 Oct 2020 02:34:15 +0200
Subject: [PATCH 1/3] Sock4(A) original patch from alex
---
include/haproxy/connection-t.h | 3 +-
include/haproxy/connection.h | 90 ++
include/haproxy/fake_host.h| 24 +
include/haproxy/server-t.h | 1 +
src/backend.c | 17 +++
src/connection.c | 80 +-
src/server.c | 30
src/tcpcheck.c | 3 ++
8 files changed, 215 insertions(+), 33 deletions(-)
create mode 100644 include/haproxy/fake_host.h
diff --git a/include/haproxy/connection-t.h b/include/haproxy/connection-t.h
index 9caa2ca49..669f97c12 100644
--- a/include/haproxy/connection-t.h
+++ b/include/haproxy/connection-t.h
@@ -490,7 +490,8 @@ struct connection {
struct sockaddr_storage *dst; /* destination address (pool), when known, otherwise NULL */
char *proxy_authority; /* Value of authority TLV received via PROXYv2 */
uint8_t proxy_authority_len; /* Length of authority TLV received via PROXYv2 */
- struct
Re: [PR] SOCKS4(A)
Le 02/10/2020 à 08:58, Willy Tarreau a écrit :
So if anyone currently uses socks4 to talk to servers, I suggest you
run a quick test on 2.2 or 2.3 to see if health checks continue to work
over socks4 or not, in which case it's likely you'll be able to provide
an easier reproducer that will allow to fix the problem. This will save
everyone time and protect our eyeballs by keeping them away from this
blinking patch.
There is indeed a bug. The flag CO_FL_SOCKS4 is set after the connect() for
tcp-checks, making the health-checks though a socks4 proxy fail. Here is a patch
to fix this bug. I will push it very soon.
Remains the support of the SOCKS4A in Alex patches. But I will let anyone
motivated by this part working on it :)
--
Christopher Faulet
>From 18c4c8b281c0a5ee9b345dadbb13c1559f0c254b Mon Sep 17 00:00:00 2001
From: Christopher Faulet
Date: Fri, 2 Oct 2020 13:41:55 +0200
Subject: [PATCH] BUG/MINOR: tcpcheck: Set socks4 and send-proxy flags before
the connect call
Since the health-check refactoring in the 2.2, the checks through a socks4 proxy
are broken. To fix this bug, CO_FL_SOCKS4 flag must be set on the connection
before calling the connect() callback function because this flags is checked to
use the right destination address. The same is done for the CO_FL_SEND_PROXY
flag for a consistency purpose.
This patch must be backported to 2.2.
---
src/tcpcheck.c | 35 ++-
1 file changed, 18 insertions(+), 17 deletions(-)
diff --git a/src/tcpcheck.c b/src/tcpcheck.c
index 5bd237ad6..b9ef3802b 100644
--- a/src/tcpcheck.c
+++ b/src/tcpcheck.c
@@ -1073,6 +1073,24 @@ enum tcpcheck_eval_ret tcpcheck_eval_connect(struct check *check, struct tcpchec
conn_prepare(conn, proto, xprt);
cs_attach(cs, check, _conn_cb);
+ if ((connect->options & TCPCHK_OPT_SOCKS4) && s && (s->flags & SRV_F_SOCKS4_PROXY)) {
+ conn->send_proxy_ofs = 1;
+ conn->flags |= CO_FL_SOCKS4;
+ }
+ else if ((connect->options & TCPCHK_OPT_DEFAULT_CONNECT) && s && s->check.via_socks4 && (s->flags & SRV_F_SOCKS4_PROXY)) {
+ conn->send_proxy_ofs = 1;
+ conn->flags |= CO_FL_SOCKS4;
+ }
+
+ if (connect->options & TCPCHK_OPT_SEND_PROXY) {
+ conn->send_proxy_ofs = 1;
+ conn->flags |= CO_FL_SEND_PROXY;
+ }
+ else if ((connect->options & TCPCHK_OPT_DEFAULT_CONNECT) && s && s->check.send_proxy && !(check->state & CHK_ST_AGENT)) {
+ conn->send_proxy_ofs = 1;
+ conn->flags |= CO_FL_SEND_PROXY;
+ }
+
status = SF_ERR_INTERNAL;
next = get_next_tcpcheck_rule(check->tcpcheck_rules, rule);
if (proto && proto->connect) {
@@ -1102,23 +1120,6 @@ enum tcpcheck_eval_ret tcpcheck_eval_connect(struct check *check, struct tcpchec
else if ((connect->options & TCPCHK_OPT_DEFAULT_CONNECT) && s && s->check.alpn_str)
ssl_sock_set_alpn(conn, (unsigned char *)s->check.alpn_str, s->check.alpn_len);
#endif
- if ((connect->options & TCPCHK_OPT_SOCKS4) && s && (s->flags & SRV_F_SOCKS4_PROXY)) {
- conn->send_proxy_ofs = 1;
- conn->flags |= CO_FL_SOCKS4;
- }
- else if ((connect->options & TCPCHK_OPT_DEFAULT_CONNECT) && s && s->check.via_socks4 && (s->flags & SRV_F_SOCKS4_PROXY)) {
- conn->send_proxy_ofs = 1;
- conn->flags |= CO_FL_SOCKS4;
- }
-
- if (connect->options & TCPCHK_OPT_SEND_PROXY) {
- conn->send_proxy_ofs = 1;
- conn->flags |= CO_FL_SEND_PROXY;
- }
- else if ((connect->options & TCPCHK_OPT_DEFAULT_CONNECT) && s && s->check.send_proxy && !(check->state & CHK_ST_AGENT)) {
- conn->send_proxy_ofs = 1;
- conn->flags |= CO_FL_SEND_PROXY;
- }
if (conn_ctrl_ready(conn) && (connect->options & TCPCHK_OPT_LINGER)) {
/* Some servers don't like reset on close */
--
2.26.2
Re: [PR] SOCKS4(A)
On Fri, Oct 02, 2020 at 01:51:36PM +0500, ??? wrote: > vtc is not very well suited for complex things like frontend + socks + > backend I disagree. It's possible to send/receive hex, we don't need more. We can simply have a server which first receives the socks4 preface, then the request, then sends back the socks4 preface and the response. It's enough to do the job fine and detect breakage. > I'm thinking of "multi container github actions" (it is announced, but we > never tried it yet) I don't know how that works, but I don't see how having multiple containers helps doing things that we couldn't do in a single one. But let's see once someone has time for this :-) Willy
Re: [PR] SOCKS4(A)
пт, 2 окт. 2020 г. в 12:58, Willy Tarreau : > On Fri, Oct 02, 2020 at 12:49:02PM +0500, ??? wrote: > > > Alex also sent me a piece of config supposed to reproduce the socks4 > > > regression regarding the checks, but I haven't tested yet, and since it > > > seems to include some private parts, it will have to be cleaned up > first. > > > > > > > > > can we add a ticket for that ? > > I asked him to add one but instead he sent me his config. Feel free to do > so if you want so that we don't forget. In the worst case we'll close it > if it was a false alarm. > > > looks like we can add automated testing based on that (like low priority > > weekly job on github) > > Or it could simply be yet-another-vtc. > vtc is not very well suited for complex things like frontend + socks + backend I'm thinking of "multi container github actions" (it is announced, but we never tried it yet) > > Willy >
Re: [PR] SOCKS4(A)
On Fri, Oct 02, 2020 at 12:49:02PM +0500, ??? wrote: > > Alex also sent me a piece of config supposed to reproduce the socks4 > > regression regarding the checks, but I haven't tested yet, and since it > > seems to include some private parts, it will have to be cleaned up first. > > > > > can we add a ticket for that ? I asked him to add one but instead he sent me his config. Feel free to do so if you want so that we don't forget. In the worst case we'll close it if it was a false alarm. > looks like we can add automated testing based on that (like low priority > weekly job on github) Or it could simply be yet-another-vtc. Willy
Re: [PR] SOCKS4(A)
пт, 2 окт. 2020 г. в 11:58, Willy Tarreau : > Guys, > > quick note, Alex contacted me offline saying his PR will be deleted > soon, asked me to save it before this happens (which I did) and not to > be contacted anymore (which I totally respect and kindly ask anyone else > to respect). > > He insisted that socks4 with tcp-checks is currently broken, which is > perfectly possible. It's impossible to figure anything from his mind- > blowing patch set anyway, since more than 95% of it is pure obfuscation > achieved using automatic and bogus reindent by an editor obviously not > configured for the C language, making the patches huge and extremely > unreadable, as tiny changes are probably hidden there. The only way to > figure the changes would actually be to compile and compare the binaries > to figure what changed! The remaining 5% seem to be debug code, hacks > such as reserving some hard-coded IP addresses to flag certain features, > and some features I don't even understand given that the code goes back > and forth along the patches and changes do not even match what the title > suggests. Also it was apparently only built on 32-bit as I spotted a few > "%u" with a size_t. Some sample fetches seem to have been removed by > accident during the refactoring. I have the series still available as > an mbox if someone ever wants to dig into it by curiosity or to learn > how not to contribute to any project, but quite frankly, whoever deployed > this in production must be extremely embarrassed now, knowing they will > never be able to apply the tiniest fix on top of this anymore, but hey, > we cannot save the world from fools and it's not our job :-/ > > Alex also sent me a piece of config supposed to reproduce the socks4 > regression regarding the checks, but I haven't tested yet, and since it > seems to include some private parts, it will have to be cleaned up first. > can we add a ticket for that ? looks like we can add automated testing based on that (like low priority weekly job on github) > > So if anyone currently uses socks4 to talk to servers, I suggest you > run a quick test on 2.2 or 2.3 to see if health checks continue to work > over socks4 or not, in which case it's likely you'll be able to provide > an easier reproducer that will allow to fix the problem. This will save > everyone time and protect our eyeballs by keeping them away from this > blinking patch. > > Thanks, > Willy >
Re: [PR] SOCKS4(A)
Guys, quick note, Alex contacted me offline saying his PR will be deleted soon, asked me to save it before this happens (which I did) and not to be contacted anymore (which I totally respect and kindly ask anyone else to respect). He insisted that socks4 with tcp-checks is currently broken, which is perfectly possible. It's impossible to figure anything from his mind- blowing patch set anyway, since more than 95% of it is pure obfuscation achieved using automatic and bogus reindent by an editor obviously not configured for the C language, making the patches huge and extremely unreadable, as tiny changes are probably hidden there. The only way to figure the changes would actually be to compile and compare the binaries to figure what changed! The remaining 5% seem to be debug code, hacks such as reserving some hard-coded IP addresses to flag certain features, and some features I don't even understand given that the code goes back and forth along the patches and changes do not even match what the title suggests. Also it was apparently only built on 32-bit as I spotted a few "%u" with a size_t. Some sample fetches seem to have been removed by accident during the refactoring. I have the series still available as an mbox if someone ever wants to dig into it by curiosity or to learn how not to contribute to any project, but quite frankly, whoever deployed this in production must be extremely embarrassed now, knowing they will never be able to apply the tiniest fix on top of this anymore, but hey, we cannot save the world from fools and it's not our job :-/ Alex also sent me a piece of config supposed to reproduce the socks4 regression regarding the checks, but I haven't tested yet, and since it seems to include some private parts, it will have to be cleaned up first. So if anyone currently uses socks4 to talk to servers, I suggest you run a quick test on 2.2 or 2.3 to see if health checks continue to work over socks4 or not, in which case it's likely you'll be able to provide an easier reproducer that will allow to fix the problem. This will save everyone time and protect our eyeballs by keeping them away from this blinking patch. Thanks, Willy
Re: [PR] SOCKS4(A)
чт, 1 окт. 2020 г. в 17:25, Willy Tarreau : > On Thu, Oct 01, 2020 at 01:45:05PM +0200, Nicolas CARPi wrote: > > Yes Willy, we want to know! Why the hell you keep using C and don't > > switch to C++? > > Seriously, do you know any single more tasteless language ? Even > brainfuck is more readable. > > > Better yet, why don't you rewrite it in Rust. > > 1) because there's no need for it > 2) because I'm not interested > 3) because I don't use it > 4) because you don't replace 20 years of work in an eyeblink. Every single >code rewrite loses lots of functionalities, claims they were broken and >declares their absence the new standard. > while all the above is correct, do not forget that using functions simplifies things a lot > > And all this is true for $WHATEVER_LANGUAGE. By the way, a few years ago > I wrote an April's fool claiming we'd rewrite haproxy in Lua, and got > some private reponses from people who found this was an awesome idea :-) > > > The part where he explains to you how you should use functions is GOLD! > > :D > > Agreed :-) > > Willy > >
Re: [PR] SOCKS4(A)
On Thu, Oct 01, 2020 at 01:45:05PM +0200, Nicolas CARPi wrote: > Yes Willy, we want to know! Why the hell you keep using C and don't > switch to C++? Seriously, do you know any single more tasteless language ? Even brainfuck is more readable. > Better yet, why don't you rewrite it in Rust. 1) because there's no need for it 2) because I'm not interested 3) because I don't use it 4) because you don't replace 20 years of work in an eyeblink. Every single code rewrite loses lots of functionalities, claims they were broken and declares their absence the new standard. And all this is true for $WHATEVER_LANGUAGE. By the way, a few years ago I wrote an April's fool claiming we'd rewrite haproxy in Lua, and got some private reponses from people who found this was an awesome idea :-) > The part where he explains to you how you should use functions is GOLD! > :D Agreed :-) Willy
Re: [PR] SOCKS4(A)
Yes Willy, we want to know! Why the hell you keep using C and don't switch to C++? Better yet, why don't you rewrite it in Rust. The part where he explains to you how you should use functions is GOLD! :D Have a nice day everyone and thanks for the laughs alex! ~Nico On 01 Oct, Willy Tarreau wrote: > Hello, > > On Wed, Sep 30, 2020 at 10:19:32PM +0200, PR Bot wrote: > > Dear list! > > > > Author: alexzk1 > > Number of patches: 30 > > > > This is an automated relay of the Github pull request: > >SOCKS4(A) > (...) > > Description: > >So I completely understand that you will dislike what my formatter did > >there, but you can just reject. I don't care as it was paid update. > > I think it could win the contest of the funniest ever contribution > received over the last two decades! > > First, I failed to spot a single patch that does something related to what > the 2-to-8 words "commit message" claimed and this is not in any way the > formatter's fault. Second, claiming "I don't care if you pick it anyway > since I was paid to do it" makes me wonder "who's crazy enough to pay for > this, and probably to go as far as deploying it?". I mean, I'm not sure > they realized they'll have to pay you to do it again and again after each > fix that gets applied to the main tree. Well done at least if that worked :-) > > >So what it do: > >1. adds support for socks4A, if it cannot resolve > >dns localy it does ..read file "fake_host.h" > >2. you forgot to > >copy-paste setup from "backend.c" to "tcpcheck.c" for socks4. So since > >2.1 it does not do check over socks. > >I fixed that and removed > >copy-paste by doing function. > > If you have a fix to provide, please point it in this ocean of random > patches, because I can't find it. Or better, just explain what the > problem is so that someone can fix it properly :-/ > > >Also it is generic rule, if any > >piece of code met twice -> do a function. 90% of your current can be > >changed in that way. > > Excellent advice, thank you! > > >3. and at then end I have rhetoric question: > >if you invented there virtual functions and virtual table, why the > >hell you keep using C and don't switch to C++ ? > > Hehe good one :-) > > Willy > -- ~Nico
Re: [PR] SOCKS4(A)
Hello, On Wed, Sep 30, 2020 at 10:19:32PM +0200, PR Bot wrote: > Dear list! > > Author: alexzk1 > Number of patches: 30 > > This is an automated relay of the Github pull request: >SOCKS4(A) (...) > Description: >So I completely understand that you will dislike what my formatter did >there, but you can just reject. I don't care as it was paid update. I think it could win the contest of the funniest ever contribution received over the last two decades! First, I failed to spot a single patch that does something related to what the 2-to-8 words "commit message" claimed and this is not in any way the formatter's fault. Second, claiming "I don't care if you pick it anyway since I was paid to do it" makes me wonder "who's crazy enough to pay for this, and probably to go as far as deploying it?". I mean, I'm not sure they realized they'll have to pay you to do it again and again after each fix that gets applied to the main tree. Well done at least if that worked :-) >So what it do: >1. adds support for socks4A, if it cannot resolve >dns localy it does ..read file "fake_host.h" >2. you forgot to >copy-paste setup from "backend.c" to "tcpcheck.c" for socks4. So since >2.1 it does not do check over socks. >I fixed that and removed >copy-paste by doing function. If you have a fix to provide, please point it in this ocean of random patches, because I can't find it. Or better, just explain what the problem is so that someone can fix it properly :-/ >Also it is generic rule, if any >piece of code met twice -> do a function. 90% of your current can be >changed in that way. Excellent advice, thank you! >3. and at then end I have rhetoric question: >if you invented there virtual functions and virtual table, why the >hell you keep using C and don't switch to C++ ? Hehe good one :-) Willy
[PR] SOCKS4(A)
Dear list! Author: alexzk1 Number of patches: 30 This is an automated relay of the Github pull request: SOCKS4(A) Patch title(s): trying to resolve server domain using SOCKS4A added htonl for IP fixed access to non-existing address more checks moved domen dump to backend fixed wrong paste in wrong file added error message updated error text made socks4 to use fake 10.10.10.10 moved struct socks4_request back to header as it could have alignment there added domain to debug out removed htonl call for fake ip fixed user_length added explicit #pragma pack added debug out of socks4a header added back htonl fixed userid, domain must follow it, not replace made single block send for socks4a updated debug text fixed "correction" removed extra if ()?: changed dumper to use unsigned char* updated debug text removed closing domain string in connection updated debug texts, added domain set to tcp check updated conn_set_domain to allow nullptr which just frees then, moved conn_set_domain in tcp_check as it can be couple cases updated debug text moved fake host definitions to separated file, made connection check for fake IP not conditional to debug updated debug output more text update Link: https://github.com/haproxy/haproxy/pull/883 Edit locally: wget https://github.com/haproxy/haproxy/pull/883.patch && vi 883.patch Apply locally: curl https://github.com/haproxy/haproxy/pull/883.patch | git am - Description: So I completely understand that you will dislike what my formatter did there, but you can just reject. I don't care as it was paid update. So what it do: 1. adds support for socks4A, if it cannot resolve dns localy it does ..read file "fake_host.h" 2. you forgot to copy-paste setup from "backend.c" to "tcpcheck.c" for socks4. So since 2.1 it does not do check over socks. I fixed that and removed copy-paste by doing function. Also it is generic rule, if any piece of code met twice -> do a function. 90% of your current can be changed in that way. 3. and at then end I have rhetoric question: if you invented there virtual functions and virtual table, why the hell you keep using C and don't switch to C++ ? Instructions: This github pull request will be closed automatically; patch should be reviewed on the haproxy mailing list (haproxy@formilux.org). Everyone is invited to comment, even the patch's author. Please keep the author and list CCed in replies. Please note that in absence of any response this pull request will be lost.
