Welcome! Log In Create A New Profile

Advanced

[PATCH] MEDIUM: proxy_protocol: Send IPv4 addresses when possible

Posted by 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.
---
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 <src> or <dst> 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_storage *v6, struct sockaddr_storage *v4) {
+ if (v4->ss_family == AF_INET) {
+ v6->ss_family = AF_INET6;
+ v4tov6(&((struct sockaddr_in6 *) v6)->sin6_addr, &((struct sockaddr_in *) v4)->sin_addr);
+ return;
+ }
+
+ memcpy(v6, v4, sizeof(struct sockaddr_storage));
+}
+
+int sockaddr_v6tov4(struct sockaddr_storage *v4, struct sockaddr_storage *v6) {
+ if (v6->ss_family == AF_INET6 && v6tov4(&((struct sockaddr_in *) v4)->sin_addr, &((struct sockaddr_in6 *) v6)->sin6_addr)) {
+ v4->ss_family = AF_INET;
+ return 1;
+ }
+
+ memcpy(v4, v6, sizeof(struct sockaddr_storage));
+
+ return v4->ss_family == AF_INET;
+}
+
/* 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
--
2.18.0
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
Hello Tim,


On Fri, 29 Jun 2018 at 21:00, Tim Duesterhus <[email protected]> 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
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 <[email protected]> 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
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
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
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
Hello,


On Wed, 18 Jul 2018 at 14:30, Willy Tarreau <[email protected]> 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
Willy,

attached is an updated patch that:

1. Only converts the addresses to IPv6 if at least one of them is IPv6.
But it does not convert them to IPv4 if both of them can be converted to IPv4.
2. Does not copy the whole `struct connection`, but performs the conversion inside
`make_proxy_line_v?`.

I'm not sure whether I like this better than my first attempt at it. Proxy protocol
v2 was rather easy to modify, but proxy protocol v1 required a complete restructuring
to not create a new case for each of the 4 address combinations (44, 46, 64, 66).

I performed a manual test running using both send-proxy as well as send-proxy-v2 inside
of valgrind. It sent the expected values. Valgrind did not report any memory corruption
or memory leaks.

So I believe this patch is good, but you want to double check my logic. Especially inside
`make_proxy_line_v1`.

Apply with `git am --scissors` to automatically cut the commit message.
-- >8 --

http-request set-src possibly creates a situation where src and dst
are from different address families. Convert both addresses to IPv6
to avoid a PROXY UNKNOWN.

This patch should be backported to haproxy 1.8.
---
src/connection.c | 173 +++++++++++++++++++++++++++--------------------
1 file changed, 98 insertions(+), 75 deletions(-)

diff --git a/src/connection.c b/src/connection.c
index 4b1e066e..8826706f 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -964,73 +964,71 @@ int make_proxy_line(char *buf, int buf_len, struct server *srv, struct connectio
int make_proxy_line_v1(char *buf, int buf_len, struct sockaddr_storage *src, struct sockaddr_storage *dst)
{
int ret = 0;
-
- 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)
- return 0;
-
- /* IPv4 src */
- if (!inet_ntop(src->ss_family, &((struct sockaddr_in *)src)->sin_addr, buf + ret, buf_len - ret))
- return 0;
-
- ret += strlen(buf + ret);
+ char * protocol;
+ char src_str[MAX(INET_ADDRSTRLEN, INET6_ADDRSTRLEN)];
+ char dst_str[MAX(INET_ADDRSTRLEN, INET6_ADDRSTRLEN)];
+ in_port_t src_port;
+ in_port_t dst_port;
+
+ if ( !src
+ || !dst
+ || (src->ss_family != AF_INET && src->ss_family != AF_INET6)
+ || (dst->ss_family != AF_INET && dst->ss_family != AF_INET6)) {
+ /* unknown family combination */
+ ret = snprintf(buf, buf_len, "PROXY UNKNOWN\r\n");
if (ret >= buf_len)
return 0;

- buf[ret++] = ' ';
-
- /* IPv4 dst */
- if (!inet_ntop(dst->ss_family, &((struct sockaddr_in *)dst)->sin_addr, buf + ret, buf_len - ret))
- return 0;
+ return ret;
+ }

- ret += strlen(buf + ret);
- if (ret >= buf_len)
+ /* IPv4 for both src and dst */
+ if (src->ss_family == AF_INET && dst->ss_family == AF_INET) {
+ protocol = "TCP4";
+ if (!inet_ntop(AF_INET, &((struct sockaddr_in *)src)->sin_addr, src_str, sizeof(src_str)))
return 0;
-
- /* source and destination ports */
- ret += snprintf(buf + ret, buf_len - ret, " %u %u\r\n",
- ntohs(((struct sockaddr_in *)src)->sin_port),
- ntohs(((struct sockaddr_in *)dst)->sin_port));
- if (ret >= buf_len)
+ src_port = ((struct sockaddr_in *)src)->sin_port;
+ if (!inet_ntop(AF_INET, &((struct sockaddr_in *)dst)->sin_addr, dst_str, sizeof(dst_str)))
return 0;
+ dst_port = ((struct sockaddr_in *)dst)->sin_port;
}
- else if (src && dst && src->ss_family == dst->ss_family && src->ss_family == AF_INET6) {
- ret = snprintf(buf + ret, buf_len - ret, "PROXY TCP6 ");
- if (ret >= buf_len)
- return 0;
-
- /* IPv6 src */
- if (!inet_ntop(src->ss_family, &((struct sockaddr_in6 *)src)->sin6_addr, buf + ret, buf_len - ret))
- return 0;
+ /* IPv6 for at least one of src and dst */
+ else {
+ struct in6_addr tmp;

- ret += strlen(buf + ret);
- if (ret >= buf_len)
- return 0;
+ protocol = "TCP6";

- buf[ret++] = ' ';
+ if (src->ss_family == AF_INET) {
+ /* Convert src to IPv6 */
+ v4tov6(&tmp, &((struct sockaddr_in *)src)->sin_addr);
+ src_port = ((struct sockaddr_in *)src)->sin_port;
+ }
+ else {
+ tmp = ((struct sockaddr_in6 *)src)->sin6_addr;
+ src_port = ((struct sockaddr_in6 *)src)->sin6_port;
+ }

- /* IPv6 dst */
- if (!inet_ntop(dst->ss_family, &((struct sockaddr_in6 *)dst)->sin6_addr, buf + ret, buf_len - ret))
+ if (!inet_ntop(AF_INET6, &tmp, src_str, sizeof(src_str)))
return 0;

- ret += strlen(buf + ret);
- if (ret >= buf_len)
- return 0;
+ if (dst->ss_family == AF_INET) {
+ /* Convert dst to IPv6 */
+ v4tov6(&tmp, &((struct sockaddr_in *)dst)->sin_addr);
+ dst_port = ((struct sockaddr_in *)dst)->sin_port;
+ }
+ else {
+ tmp = ((struct sockaddr_in6 *)dst)->sin6_addr;
+ dst_port = ((struct sockaddr_in6 *)dst)->sin6_port;
+ }

- /* source and destination ports */
- ret += snprintf(buf + ret, buf_len - ret, " %u %u\r\n",
- ntohs(((struct sockaddr_in6 *)src)->sin6_port),
- ntohs(((struct sockaddr_in6 *)dst)->sin6_port));
- if (ret >= buf_len)
- return 0;
- }
- else {
- /* unknown family combination */
- ret = snprintf(buf, buf_len, "PROXY UNKNOWN\r\n");
- if (ret >= buf_len)
+ if (!inet_ntop(AF_INET6, &tmp, dst_str, sizeof(dst_str)))
return 0;
}
+
+ ret = snprintf(buf, buf_len, "PROXY %s %s %s %u %u\r\n", protocol, src_str, dst_str, ntohs(src_port), ntohs(dst_port));
+ if (ret >= buf_len)
+ return 0;
+
return ret;
}

@@ -1071,35 +1069,60 @@ int make_proxy_line_v2(char *buf, int buf_len, struct server *srv, struct connec
dst = &remote->addr.to;
}

- if (src && dst && src->ss_family == dst->ss_family && src->ss_family == AF_INET) {
- if (buf_len < PP2_HDR_LEN_INET)
- return 0;
- hdr->ver_cmd = PP2_VERSION | PP2_CMD_PROXY;
- hdr->fam = PP2_FAM_INET | PP2_TRANS_STREAM;
- hdr->addr.ip4.src_addr = ((struct sockaddr_in *)src)->sin_addr.s_addr;
- hdr->addr.ip4.dst_addr = ((struct sockaddr_in *)dst)->sin_addr.s_addr;
- hdr->addr.ip4.src_port = ((struct sockaddr_in *)src)->sin_port;
- hdr->addr.ip4.dst_port = ((struct sockaddr_in *)dst)->sin_port;
- ret = PP2_HDR_LEN_INET;
- }
- else if (src && dst && src->ss_family == dst->ss_family && src->ss_family == AF_INET6) {
- if (buf_len < PP2_HDR_LEN_INET6)
- return 0;
- hdr->ver_cmd = PP2_VERSION | PP2_CMD_PROXY;
- hdr->fam = PP2_FAM_INET6 | PP2_TRANS_STREAM;
- memcpy(hdr->addr.ip6.src_addr, &((struct sockaddr_in6 *)src)->sin6_addr, 16);
- memcpy(hdr->addr.ip6.dst_addr, &((struct sockaddr_in6 *)dst)->sin6_addr, 16);
- hdr->addr.ip6.src_port = ((struct sockaddr_in6 *)src)->sin6_port;
- hdr->addr.ip6.dst_port = ((struct sockaddr_in6 *)dst)->sin6_port;
- ret = PP2_HDR_LEN_INET6;
- }
- else {
+ /* At least one of src or dst is not of AF_INET or AF_INET6 */
+ if ( !src
+ || !dst
+ || (src->ss_family != AF_INET && src->ss_family != AF_INET6)
+ || (dst->ss_family != AF_INET && dst->ss_family != AF_INET6)) {
if (buf_len < PP2_HDR_LEN_UNSPEC)
return 0;
hdr->ver_cmd = PP2_VERSION | PP2_CMD_LOCAL;
hdr->fam = PP2_FAM_UNSPEC | PP2_TRANS_UNSPEC;
ret = PP2_HDR_LEN_UNSPEC;
}
+ else {
+ /* IPv4 for both src and dst */
+ if (src->ss_family == AF_INET && dst->ss_family == AF_INET) {
+ if (buf_len < PP2_HDR_LEN_INET)
+ return 0;
+ hdr->ver_cmd = PP2_VERSION | PP2_CMD_PROXY;
+ hdr->fam = PP2_FAM_INET | PP2_TRANS_STREAM;
+ hdr->addr.ip4.src_addr = ((struct sockaddr_in *)src)->sin_addr.s_addr;
+ hdr->addr.ip4.src_port = ((struct sockaddr_in *)src)->sin_port;
+ hdr->addr.ip4.dst_addr = ((struct sockaddr_in *)dst)->sin_addr.s_addr;
+ hdr->addr.ip4.dst_port = ((struct sockaddr_in *)dst)->sin_port;
+ ret = PP2_HDR_LEN_INET;
+ }
+ /* IPv6 for at least one of src and dst */
+ else {
+ struct in6_addr tmp;
+
+ if (buf_len < PP2_HDR_LEN_INET6)
+ return 0;
+ hdr->ver_cmd = PP2_VERSION | PP2_CMD_PROXY;
+ hdr->fam = PP2_FAM_INET6 | PP2_TRANS_STREAM;
+ if (src->ss_family == AF_INET) {
+ v4tov6(&tmp, &((struct sockaddr_in *)src)->sin_addr);
+ memcpy(hdr->addr.ip6.src_addr, &tmp, 16);
+ hdr->addr.ip6.src_port = ((struct sockaddr_in *)src)->sin_port;
+ }
+ else {
+ memcpy(hdr->addr.ip6.src_addr, &((struct sockaddr_in6 *)src)->sin6_addr, 16);
+ hdr->addr.ip6.src_port = ((struct sockaddr_in6 *)src)->sin6_port;
+ }
+ if (dst->ss_family == AF_INET) {
+ v4tov6(&tmp, &((struct sockaddr_in *)dst)->sin_addr);
+ memcpy(hdr->addr.ip6.dst_addr, &tmp, 16);
+ hdr->addr.ip6.src_port = ((struct sockaddr_in *)src)->sin_port;
+ }
+ else {
+ memcpy(hdr->addr.ip6.dst_addr, &((struct sockaddr_in6 *)dst)->sin6_addr, 16);
+ hdr->addr.ip6.dst_port = ((struct sockaddr_in6 *)dst)->sin6_port;
+ }
+
+ ret = PP2_HDR_LEN_INET6;
+ }
+ }

if (srv->pp_opts & SRV_PP_V2_CRC32C) {
uint32_t zero_crc32c = 0;
--
2.18.0
Hi Tim,

On Fri, Jul 27, 2018 at 06:46:13PM +0200, Tim Duesterhus wrote:
> Willy,
>
> attached is an updated patch that:
>
> 1. Only converts the addresses to IPv6 if at least one of them is IPv6.
> But it does not convert them to IPv4 if both of them can be converted to IPv4.
> 2. Does not copy the whole `struct connection`, but performs the conversion inside
> `make_proxy_line_v?`.
>
> I'm not sure whether I like this better than my first attempt at it. Proxy protocol
> v2 was rather easy to modify, but proxy protocol v1 required a complete restructuring
> to not create a new case for each of the 4 address combinations (44, 46, 64, 66).

In my opinion the code resulting from this approach is cleaner and safer
than the code it replaces. I've looked carefully at it (v1 and v2) and am
fine with it. I personally prefer to use the unhandled case in the "else"
part to avoid maintaining two sets of conditions but here it's fine because
these conditions are easy enough to enumerate.

Thus I'm merging it and backporting it to 1.8 since it also makes sense to
address this issue there.

Thanks!
Willy
Sorry, only registered users may post in this forum.

Click here to login