| Message ID | 86c60a4f-685e-4157-ad10-6de03bb2eef0@gmx.de |
|---|---|
| State | New |
| Headers | show |
| Series | [Openvpn-devel,v2,1/2] port-share: Normalize IPv4-mapped IPv6 addresses | expand |
Hi, On Mon, Dec 16, 2024 at 01:22:51PM +0100, corubba via Openvpn-devel wrote: > In addition to the custom journal solution, also support the widely > used binary PROXY protocol version 2 to convey the original client > connection parameters to the proxy receiver. This makes the port-share > journal feature more accessable and easier to use, because one doesn't > need a custom integration. > > While this is a spec-compliant sender implementation of the PROXY > protocol, it does not implement it in full. Version 1 was left out > entirely, in favour of the superior and easier-to-implement version 2. > The implementation was also kept minimal with regards to what OpenVPN > supports/requires: Local commands, unix sockets, UDP and TLVs are not > implemented. Are there any reference URLs for the PROXY protocol? If yes, I think it would be good to include a reference into one of the comments - the next person to read that code would then know which document this is based on. Besides this, I'm not sure what to do with these > + ASSERT(4 >= sizeof(src.addr.in4.sin_addr)); > + ASSERT(4 >= sizeof(dst.addr.in4.sin_addr)); > + memcpy(&header[16], &src.addr.in4.sin_addr, sizeof(src.addr.in4.sin_addr)); > + memcpy(&header[20], &dst.addr.in4.sin_addr, sizeof(dst.addr.in4.sin_addr)); ... this is all obviously correct, but it feels heavy handed, and many lines of code... (especially 2x sizeof on the same structure element) I think we can fairly safely assume that an in4.sin_addr will always be 4 bytes, an in4.sin_port will always be 2 bytes, and an IPv6 address will always be 16 bytes... so I wonder if it wouln't be easier to just copy a well-known number of bytes (openvpn_sockaddr is also "always bigger than that", so no read overrun) here. I know that "magic numbers" in code are considered a bad pattern, but these things are really constant length, and the buffer layout *requires* "exactly 4" and "exactly 2" bytes... gert
Hi, On 26.12.24 14:13, Gert Doering wrote: > On Mon, Dec 16, 2024 at 01:22:51PM +0100, corubba via Openvpn-devel wrote: >> In addition to the custom journal solution, also support the widely >> used binary PROXY protocol version 2 to convey the original client >> connection parameters to the proxy receiver. This makes the port-share >> journal feature more accessable and easier to use, because one doesn't >> need a custom integration. >> >> While this is a spec-compliant sender implementation of the PROXY >> protocol, it does not implement it in full. Version 1 was left out >> entirely, in favour of the superior and easier-to-implement version 2. >> The implementation was also kept minimal with regards to what OpenVPN >> supports/requires: Local commands, unix sockets, UDP and TLVs are not >> implemented. > > Are there any reference URLs for the PROXY protocol? If yes, I think it > would be good to include a reference into one of the comments - the next > person to read that code would then know which document this is based on. I had included the link to the spec I used only in the cover letter, but I agree putting it into the commit message is more sensible. > Besides this, I'm not sure what to do with these > >> + ASSERT(4 >= sizeof(src.addr.in4.sin_addr)); >> + ASSERT(4 >= sizeof(dst.addr.in4.sin_addr)); >> + memcpy(&header[16], &src.addr.in4.sin_addr, sizeof(src.addr.in4.sin_addr)); >> + memcpy(&header[20], &dst.addr.in4.sin_addr, sizeof(dst.addr.in4.sin_addr)); > > ... this is all obviously correct, but it feels heavy handed, and many > lines of code... (especially 2x sizeof on the same structure element) > > I think we can fairly safely assume that an in4.sin_addr will always be > 4 bytes, an in4.sin_port will always be 2 bytes, and an IPv6 address will > always be 16 bytes... so I wonder if it wouln't be easier to just copy > a well-known number of bytes (openvpn_sockaddr is also "always bigger > than that", so no read overrun) here. > > I know that "magic numbers" in code are considered a bad pattern, but > these things are really constant length, and the buffer layout *requires* > "exactly 4" and "exactly 2" bytes... I wasn't sure about that either, and in the end opted for the "better safe than sorry" variant. If your better-informed opinion too is that these are "practically constant", then I am happy to oblige. Best regards -- Corubba
diff --git a/doc/man-sections/server-options.rst b/doc/man-sections/server-options.rst index 3fe9862c..5fdd4a22 100644 --- a/doc/man-sections/server-options.rst +++ b/doc/man-sections/server-options.rst @@ -435,6 +435,10 @@ fast hardware. SSL/TLS authentication must be used in this mode. the origin of the connection. Each generated file will be automatically deleted when the proxied connection is torn down. + ``dir`` can be set to the special value ``proxy_protocol_v2`` to make + OpenVPN use the binary PROXY protocol version 2 towards the proxy receiver. + No temporary files will be written in this mode. + Not implemented on Windows. --push option diff --git a/src/openvpn/ps.c b/src/openvpn/ps.c index 36ea63b8..b5d04c5b 100644 --- a/src/openvpn/ps.c +++ b/src/openvpn/ps.c @@ -393,6 +393,107 @@ journal_add(const char *journal_dir, struct proxy_connection *pc, struct proxy_c gc_free(&gc); } +/* + * Send the proxy protocol v2 binary header, so that the receiving + * server knows the true client connection parameters. + */ +static void +send_proxy_protocol_v2_header(const struct proxy_connection *const pc, const struct proxy_connection *const cp) +{ + static const uint8_t PP2_AF_UNSPEC = 0x0, PP2_AF_INET = 0x1, PP2_AF_INET6 = 0x2; + static const uint8_t PP2_PROTO_STREAM = 0x1; + + struct openvpn_sockaddr src, dst; + socklen_t src_len, dst_len; + unsigned char header[52] = { + "\x0D\x0A\x0D\x0A\x00\x0D\x0A\x51\x55\x49\x54\x0A" /* signature */ + "\x21" /* version=2 + command=proxy */ + /* initialize the rest to zero for now */ + }; + uint8_t addr_fam, header_len = 16; + uint16_t addr_len; + + src_len = sizeof(src.addr); + dst_len = sizeof(dst.addr); + if (0 != getpeername(pc->sd, &src.addr.sa, &src_len) + || 0 != getsockname(pc->sd, &dst.addr.sa, &dst_len)) + { + msg(M_WARN, "PORT SHARE PROXY: getting client connection parameters failed"); + src.addr.sa.sa_family = dst.addr.sa.sa_family = AF_UNSPEC; + } + + transform_mapped_v4_sockaddr(&src); + transform_mapped_v4_sockaddr(&dst); + if (src.addr.sa.sa_family != dst.addr.sa.sa_family) + { + msg(M_WARN, "PORT SHARE PROXY: address family mismatch between peer and socket"); + /* src wins, because that is usually the more important info */ + dst.addr.sa.sa_family = src.addr.sa.sa_family; + } + + if (msg_test(D_PS_PROXY_DEBUG)) + { + struct gc_arena gc = gc_new(); + dmsg(D_PS_PROXY_DEBUG, "PORT SHARE PROXY: client connection is %s -> %s", + print_openvpn_sockaddr(&src, &gc), print_openvpn_sockaddr(&dst, &gc)); + gc_free(&gc); + } + + switch (src.addr.sa.sa_family) + { + case AF_INET: + addr_fam = PP2_AF_INET; + addr_len = 12; + ASSERT(4 >= sizeof(src.addr.in4.sin_addr)); + ASSERT(4 >= sizeof(dst.addr.in4.sin_addr)); + memcpy(&header[16], &src.addr.in4.sin_addr, sizeof(src.addr.in4.sin_addr)); + memcpy(&header[20], &dst.addr.in4.sin_addr, sizeof(dst.addr.in4.sin_addr)); + ASSERT(2 >= sizeof(src.addr.in4.sin_port)); + ASSERT(2 >= sizeof(dst.addr.in4.sin_port)); + memcpy(&header[24], &src.addr.in4.sin_port, sizeof(src.addr.in4.sin_port)); + memcpy(&header[26], &dst.addr.in4.sin_port, sizeof(dst.addr.in4.sin_port)); + break; + + case AF_INET6: + addr_fam = PP2_AF_INET6; + addr_len = 36; + ASSERT(16 >= sizeof(src.addr.in6.sin6_addr)); + ASSERT(16 >= sizeof(dst.addr.in6.sin6_addr)); + memcpy(&header[16], &src.addr.in6.sin6_addr, sizeof(src.addr.in6.sin6_addr)); + memcpy(&header[32], &dst.addr.in6.sin6_addr, sizeof(dst.addr.in6.sin6_addr)); + ASSERT(2 >= sizeof(src.addr.in6.sin6_port)); + ASSERT(2 >= sizeof(dst.addr.in6.sin6_port)); + memcpy(&header[48], &src.addr.in6.sin6_port, sizeof(src.addr.in6.sin6_port)); + memcpy(&header[50], &dst.addr.in6.sin6_port, sizeof(dst.addr.in6.sin6_port)); + break; + + /* AF_UNIX is currently not suppported by OpenVPN */ + + default: + addr_fam = PP2_AF_UNSPEC; + addr_len = 0; + break; + } + + const uint8_t proto = PP2_PROTO_STREAM; /* DGRAM is currently not supported by port-share */ + header[13] = (addr_fam << 4) | proto; + + /* TLV is currently not implemented */ + + header_len += addr_len; + const uint16_t addr_len_n = htons(addr_len); + memcpy(&header[14], &addr_len_n, sizeof(addr_len_n)); + + ASSERT(header_len <= sizeof(header)); + const socket_descriptor_t sd = cp->sd; + const int status = send(sd, header, header_len, MSG_NOSIGNAL); + dmsg(D_PS_PROXY_DEBUG, "PORT SHARE PROXY: proxy protocol v2 wrote[%d] %d", (int) sd, status); + if (status < (int) header_len) + { + msg(M_WARN, "PORT SHARE PROXY: failed to send proxy protocol v2 header"); + } +} + /* * Cleanup function, on proxy process exit. */ @@ -488,7 +589,14 @@ proxy_entry_new(struct proxy_connection **list, /* add journal entry */ if (journal_dir) { - journal_add(journal_dir, pc, cp); + if (0 == strcmp("proxy_protocol_v2", journal_dir)) + { + send_proxy_protocol_v2_header(pc, cp); + } + else + { + journal_add(journal_dir, pc, cp); + } } dmsg(D_PS_PROXY_DEBUG, "PORT SHARE PROXY: NEW CONNECTION [c=%d s=%d]", (int)sd_client, (int)sd_server);
In addition to the custom journal solution, also support the widely used binary PROXY protocol version 2 to convey the original client connection parameters to the proxy receiver. This makes the port-share journal feature more accessable and easier to use, because one doesn't need a custom integration. While this is a spec-compliant sender implementation of the PROXY protocol, it does not implement it in full. Version 1 was left out entirely, in favour of the superior and easier-to-implement version 2. The implementation was also kept minimal with regards to what OpenVPN supports/requires: Local commands, unix sockets, UDP and TLVs are not implemented. Signed-off-by: Corubba Smith <corubba@gmx.de> --- doc/man-sections/server-options.rst | 4 + src/openvpn/ps.c | 110 +++++++++++++++++++++++++++- 2 files changed, 113 insertions(+), 1 deletion(-) -- 2.47.1