Re: [PATCH] MEDIUM: proxy_protocol: Send IPv4 addresses when possible
Hello, On Wed, 18 Jul 2018 at 14:30, Willy Tarreau wrote: > > Hi Tim, > > On Wed, Jul 18, 2018 at 01:48:01PM +0200, Tim Düsterhus wrote: > > This would solve the issue for my use case and should not break anything > > (a few UNKNOWNs will become TCP6 then). > > OK. > > > I can rework the patch, if technical changes look good to you. There's a > > ton of memcpy in there to not destroy the data structures needed for the > > actual communication: make_proxy_line() now always operates on a copy of > > `struct connection remote`. > > I don't see why a copy could be needed, given that you should never have > to modify anything in the source. Or maybe it's because it was more > convenient to store the remapped addresses ? In this case I think that > it's still cleaner to have two local struct sockaddr_storage that you > can use for the operations. Another option, which I *thought* we had but > am probably wrong on this, was to first check the source+destination > address classes before deciding what to emit. (or maybe it's done in the > PPv2 format) : > > if (src_is_v4 && dst_is_v4) > /* emit_v4(src, dst) */ > else if (src_is_v6 && dst_is_v6) > /* emit_v6(src, dst) */ > else if (src_is_v6 && dst_is_v4) > /* emit_v6(src, 4to6(dst)) */ > else if (src_is_v4 && dst_is_v6) > /* emit_v6(4to6(src), dst) */ > else > /* emit_unknown()*/ > > So in practice you always need only one temporary sockaddr_storage for > a conversion. > > I'm personally fine with something roughly like this. Lukas, I'm interested > in your opinion on this one, as I *think* it addresses the issues without > introducing new ones. However if I read the pseudo-code correctly, we will still transform a v4 src to v6 src, when the destination is v6? I don't like transforming the address family at all - but I realize the proxy protocol needs a single AF for both source and destination, which means we need to compromise and the proposed solution is the best compromise as far as I can see. Agreed. cheers, lukas
Re: [PATCH] MEDIUM: proxy_protocol: Send IPv4 addresses when possible
Willy, Am 18.07.2018 um 14:30 schrieb Willy Tarreau: >> I can rework the patch, if technical changes look good to you. There's a >> ton of memcpy in there to not destroy the data structures needed for the >> actual communication: make_proxy_line() now always operates on a copy of >> `struct connection remote`. > > I don't see why a copy could be needed, given that you should never have > to modify anything in the source. Or maybe it's because it was more > convenient to store the remapped addresses ? In this case I think that Yes it is more convenient. In my current patch I just converted once in connection.c and thus did not need to make any large logic changes in make_proxy_line_v[12]: Keep the patch as small as possible. > it's still cleaner to have two local struct sockaddr_storage that you > can use for the operations. Another option, which I *thought* we had but > am probably wrong on this, was to first check the source+destination Yes, you do this, but you specifically check whether `src->ss_family == dst->ss_family`, not for every possible combination. > address classes before deciding what to emit. (or maybe it's done in the > PPv2 format) : > > if (src_is_v4 && dst_is_v4) > /* emit_v4(src, dst) */ > else if (src_is_v6 && dst_is_v6) > /* emit_v6(src, dst) */ > else if (src_is_v6 && dst_is_v4) > /* emit_v6(src, 4to6(dst)) */ > else if (src_is_v4 && dst_is_v6) > /* emit_v6(4to6(src), dst) */ > else > /* emit_unknown()*/ > > So in practice you always need only one temporary sockaddr_storage for > a conversion. Correct. Reason I did the copy (as mentioned above) is, that I did not want to touch the actual logic. > I'm personally fine with something roughly like this. Lukas, I'm interested > in your opinion on this one, as I *think* it addresses the issues without > introducing new ones. We could even think about backporting this. > Best regards Tim Düsterhus
Re: [PATCH] MEDIUM: proxy_protocol: Send IPv4 addresses when possible
Hi Tim, On Wed, Jul 18, 2018 at 01:48:01PM +0200, Tim Düsterhus wrote: > This would solve the issue for my use case and should not break anything > (a few UNKNOWNs will become TCP6 then). OK. > I can rework the patch, if technical changes look good to you. There's a > ton of memcpy in there to not destroy the data structures needed for the > actual communication: make_proxy_line() now always operates on a copy of > `struct connection remote`. I don't see why a copy could be needed, given that you should never have to modify anything in the source. Or maybe it's because it was more convenient to store the remapped addresses ? In this case I think that it's still cleaner to have two local struct sockaddr_storage that you can use for the operations. Another option, which I *thought* we had but am probably wrong on this, was to first check the source+destination address classes before deciding what to emit. (or maybe it's done in the PPv2 format) : if (src_is_v4 && dst_is_v4) /* emit_v4(src, dst) */ else if (src_is_v6 && dst_is_v6) /* emit_v6(src, dst) */ else if (src_is_v6 && dst_is_v4) /* emit_v6(src, 4to6(dst)) */ else if (src_is_v4 && dst_is_v6) /* emit_v6(4to6(src), dst) */ else /* emit_unknown()*/ So in practice you always need only one temporary sockaddr_storage for a conversion. I'm personally fine with something roughly like this. Lukas, I'm interested in your opinion on this one, as I *think* it addresses the issues without introducing new ones. We could even think about backporting this. > I could not find a better solution and already thought hard about the > current version. I have some vague memories about this area a long time ago, and believe me, when I have memories of some code parts it's likely that I got a headache on it :-) Thanks! Willy
Re: [PATCH] MEDIUM: proxy_protocol: Send IPv4 addresses when possible
Willy, Am 18.07.2018 um 04:25 schrieb Willy Tarreau: > What I would suggest would be to only "upgrade" the addresses to IPv6 > if either side already is on IPv6, but never downgrade from IPv6 to > IPv4 since v6-mapped v4 addresses can exist on both sides for a valid > reason. > This would solve the issue for my use case and should not break anything (a few UNKNOWNs will become TCP6 then). I can rework the patch, if technical changes look good to you. There's a ton of memcpy in there to not destroy the data structures needed for the actual communication: make_proxy_line() now always operates on a copy of `struct connection remote`. I could not find a better solution and already thought hard about the current version. Best regards Tim Düsterhus
Re: [PATCH] MEDIUM: proxy_protocol: Send IPv4 addresses when possible
Hi! On Tue, Jul 17, 2018 at 01:39:38PM +0200, Lukas Tribus wrote: > Hello Tim, > > > On Fri, 29 Jun 2018 at 21:00, Tim Duesterhus wrote: > > > > This patch changes the sending side of proxy protocol to convert IP > > addresses to IPv4 when possible (and converts them IPv6 otherwise). > > > > Previously the code failed to properly provide information under > > certain circumstances: > > > > 1. haproxy is being accessed using IPv4, http-request set-src sets > >a IPv6 address. > > 2. haproxy is being accessed using IPv6, http-request set-src sets > >a IPv4 address. > > 3. haproxy listens on `::` with v4v6 and is accessed using IPv4: > >It would send a TCP6 line, instead of a proper TCP4 line, because > >the IP addresses are representing as a mapped IPv4 address internally. > > > > Once correctness of this patch has been verified it should be evaluated > > whether it should be backported, as (1) and (2) are bugs. (3) is an > > enhancement. > > Thanks for this, just a comment about nr 3: > > A backend may rely on v4-mapped addresses for various reason, consider > a backend that to simplify its handling of IP addresses only handles > IPv6 and expects IPv4 addresses to be mapped. > Also consider that to send native v4 addresses the admin only has to > make a small adjustment in the bind configuration. > > So since this would be a breaking change, and that the admin can > easily reconfigure the bind line any time, I would advise against this > and vote for maintaining the current behavior (where the bind > configuration controls this behavior). I must say I'm a bit reluctant about this change for the same reasons. What I would suggest would be to only "upgrade" the addresses to IPv6 if either side already is on IPv6, but never downgrade from IPv6 to IPv4 since v6-mapped v4 addresses can exist on both sides for a valid reason. I don't know what ppv2 does in this situation where source and destination are in different classes, because initially it was not expected to happen and this became possible after we introduced set-src :-/ Also I suspect we can have similar issues with unix domain sockets. Let's say we have a listener on /var/haproxy-sockets/foo which accepts a connection on which we do a set-src. I don't really know what happens in this case if we want to send a PP header. Maybe we'd need to improve the PP spec to be able to mention only one side when the other one is unknown, though that would obviously not solve the case Tim tried to address above. Cheers, Willy
Re: [PATCH] MEDIUM: proxy_protocol: Send IPv4 addresses when possible
Hello Tim, On Fri, 29 Jun 2018 at 21:00, Tim Duesterhus wrote: > > This patch changes the sending side of proxy protocol to convert IP > addresses to IPv4 when possible (and converts them IPv6 otherwise). > > Previously the code failed to properly provide information under > certain circumstances: > > 1. haproxy is being accessed using IPv4, http-request set-src sets >a IPv6 address. > 2. haproxy is being accessed using IPv6, http-request set-src sets >a IPv4 address. > 3. haproxy listens on `::` with v4v6 and is accessed using IPv4: >It would send a TCP6 line, instead of a proper TCP4 line, because >the IP addresses are representing as a mapped IPv4 address internally. > > Once correctness of this patch has been verified it should be evaluated > whether it should be backported, as (1) and (2) are bugs. (3) is an > enhancement. Thanks for this, just a comment about nr 3: A backend may rely on v4-mapped addresses for various reason, consider a backend that to simplify its handling of IP addresses only handles IPv6 and expects IPv4 addresses to be mapped. Also consider that to send native v4 addresses the admin only has to make a small adjustment in the bind configuration. So since this would be a breaking change, and that the admin can easily reconfigure the bind line any time, I would advise against this and vote for maintaining the current behavior (where the bind configuration controls this behavior). I assume the X-Forwarded-For header behaves similar in this regard. cheers, lukas
Re: [PATCH] MEDIUM: proxy_protocol: Send IPv4 addresses when possible
Willy, Am 29.06.2018 um 20:59 schrieb Tim Duesterhus: > This patch changes the sending side of proxy protocol to convert IP > addresses to IPv4 when possible (and converts them IPv6 otherwise). > > Previously the code failed to properly provide information under > certain circumstances: > > 1. haproxy is being accessed using IPv4, http-request set-src sets >a IPv6 address. > 2. haproxy is being accessed using IPv6, http-request set-src sets >a IPv4 address. > 3. haproxy listens on `::` with v4v6 and is accessed using IPv4: >It would send a TCP6 line, instead of a proper TCP4 line, because >the IP addresses are representing as a mapped IPv4 address internally. > > Once correctness of this patch has been verified it should be evaluated > whether it should be backported, as (1) and (2) are bugs. (3) is an > enhancement. based on the overall lack of responses I assume that you are busy. I just want to make sure that this patch / bug report did not slip through the cracks. A short acknowledgement that you received it would be great, if you are currently unable to take a deeper look at it. Best regards Tim Düsterhus
[PATCH] MEDIUM: proxy_protocol: Send IPv4 addresses when possible
This patch changes the sending side of proxy protocol to convert IP
addresses to IPv4 when possible (and converts them IPv6 otherwise).
Previously the code failed to properly provide information under
certain circumstances:
1. haproxy is being accessed using IPv4, http-request set-src sets
a IPv6 address.
2. haproxy is being accessed using IPv6, http-request set-src sets
a IPv4 address.
3. haproxy listens on `::` with v4v6 and is accessed using IPv4:
It would send a TCP6 line, instead of a proper TCP4 line, because
the IP addresses are representing as a mapped IPv4 address internally.
Once correctness of this patch has been verified it should be evaluated
whether it should be backported, as (1) and (2) are bugs. (3) is an
enhancement.
---
include/common/standard.h | 6 ++
include/proto/connection.h | 2 +-
src/connection.c | 29 -
src/standard.c | 21 +
4 files changed, 52 insertions(+), 6 deletions(-)
diff --git a/include/common/standard.h b/include/common/standard.h
index 6542759d..eb92b22b 100644
--- a/include/common/standard.h
+++ b/include/common/standard.h
@@ -1047,6 +1047,12 @@ extern void v4tov6(struct in6_addr *sin6_addr, struct
in_addr *sin_addr);
*/
extern int v6tov4(struct in_addr *sin_addr, struct in6_addr *sin6_addr);
+/* Calls v4tov6 on the addr in v4. Copies v4 to v6 v6 already is of type
AF_INET6 */
+extern void sockaddr_v4tov6(struct sockaddr_storage *v6, struct
sockaddr_storage *v4);
+
+/* Calls v6tov4 on the addr in v6. Copies v6 to v4 if conversion fails or v6
already is of type AF_INET */
+extern int sockaddr_v6tov4(struct sockaddr_storage *v4, struct
sockaddr_storage *v6);
+
/* compare two struct sockaddr_storage and return:
* 0 (true) if the addr is the same in both
* 1 (false) if the addr is not the same in both
diff --git a/include/proto/connection.h b/include/proto/connection.h
index 8566736f..4e19756b 100644
--- a/include/proto/connection.h
+++ b/include/proto/connection.h
@@ -46,7 +46,7 @@ void conn_fd_handler(int fd);
/* receive a PROXY protocol header over a connection */
int conn_recv_proxy(struct connection *conn, int flag);
int make_proxy_line(char *buf, int buf_len, struct server *srv, struct
connection *remote);
-int make_proxy_line_v1(char *buf, int buf_len, struct sockaddr_storage *src,
struct sockaddr_storage *dst);
+int make_proxy_line_v1(char *buf, int buf_len, struct connection *remote);
int make_proxy_line_v2(char *buf, int buf_len, struct server *srv, struct
connection *remote);
/* receive a NetScaler Client IP insertion header over a connection */
diff --git a/src/connection.c b/src/connection.c
index 1ea96ae3..e8e48d69 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -888,14 +888,24 @@ int make_proxy_line(char *buf, int buf_len, struct server
*srv, struct connectio
{
int ret = 0;
+ struct connection tmp;
+ if (remote) {
+ memcpy(&tmp, remote, sizeof(tmp));
+
+ if (!sockaddr_v6tov4(&tmp.addr.from, &remote->addr.from) ||
!sockaddr_v6tov4(&tmp.addr.to, &remote->addr.to)) {
+ sockaddr_v4tov6(&tmp.addr.from, &remote->addr.from);
+ sockaddr_v4tov6(&tmp.addr.to, &remote->addr.to);
+ }
+
+ remote = &tmp;
+ }
+
+
if (srv && (srv->pp_opts & SRV_PP_V2)) {
ret = make_proxy_line_v2(buf, buf_len, srv, remote);
}
else {
- if (remote)
- ret = make_proxy_line_v1(buf, buf_len,
&remote->addr.from, &remote->addr.to);
- else
- ret = make_proxy_line_v1(buf, buf_len, NULL, NULL);
+ ret = make_proxy_line_v1(buf, buf_len, remote);
}
return ret;
@@ -908,10 +918,19 @@ int make_proxy_line(char *buf, int buf_len, struct server
*srv, struct connectio
* TCP6 and "UNKNOWN" formats. If any of or is null, UNKNOWN is
* emitted as well.
*/
-int make_proxy_line_v1(char *buf, int buf_len, struct sockaddr_storage *src,
struct sockaddr_storage *dst)
+int make_proxy_line_v1(char *buf, int buf_len, struct connection *remote)
{
int ret = 0;
+ struct sockaddr_storage null_addr = { .ss_family = 0 };
+ struct sockaddr_storage *src = &null_addr;
+ struct sockaddr_storage *dst = &null_addr;
+
+ if (remote) {
+ src = &remote->addr.from;
+ dst = &remote->addr.to;
+ }
+
if (src && dst && src->ss_family == dst->ss_family && src->ss_family ==
AF_INET) {
ret = snprintf(buf + ret, buf_len - ret, "PROXY TCP4 ");
if (ret >= buf_len)
diff --git a/src/standard.c b/src/standard.c
index ebe043f1..51fd2cc4 100644
--- a/src/standard.c
+++ b/src/standard.c
@@ -2693,6 +2693,27 @@ int v6tov4(struct in_addr *sin_addr, struct in6_addr
*sin6_addr)
return 0;
}
+void sockaddr_v4tov6(struct sockaddr_stora
