Re: [Openvpn-devel] [PATCH 2/4] socket: introduce INDIRECT transport protocol abstraction
>>> +++ b/src/openvpn/forward.c
>>> @@ -2150,6 +2150,11 @@ io_wait_dowork(struct context *c, const unsigned int
>>> flags)
>>> {
>>> int i;
>>> c->c2.event_set_status = 0;
>>> +#ifdef ENABLE_PLUGIN
>>> +c->c2.event_set_status |=
>>> +(socket_indirect_pump(c->c2.link_socket, esr, )
>>> & 3)
>>
>> The & 3 looks like some defined should be used instead.
>
> I think this is simply about fetching the last two bits..not much fuzz
> here..this is mostly because of how event_set_status works. But will see
> if I find a meaningful name.
>
INDIRECT_FLAG_MASK or something.
>>> bool
>>> @@ -3167,6 +3236,10 @@ proto_is_net(int proto)
>>> bool
>>> proto_is_dgram(int proto)
>>> {
>>> +if (proto_is_indirect(proto))
>>> +{
>>> +return true;
>>> +}
>>> return proto_is_udp(proto);
>>> }
>>
>> I think here need to be a good explaination why indirect is dgram but
>> not udp and also proto_is_dgram and proto_is_udp need to get some
>> comment to explain their difference in usage as this is now different.
>
> I wouldn't say is_udp and is_dgram are now different, but is_dgram has
> been extended to account for the indirect case as well.
>
is_dgram and is_udp return different values for INDIRECT, so they are
different. I would like some comment why that is. Without having looked
deeper I suspect the one function is used to determine the wire format
and the other is used for determing the actual outer protocol. Just
adding that as comment in the is_dgram function would be nice (double
check first).
>>
>>>
>>> @@ -3301,6 +3374,18 @@ proto_remote(int proto, bool remote)
>>> return "TCPv4_CLIENT";
>>> }
>>>
>>> +#ifdef ENABLE_PLUGIN
>>> +if (proto == PROTO_INDIRECT)
>>> +{
>>> +/* FIXME: the string reported here should match the actual
>>> transport
>>> + * protocol being used, however in this function we have no
>>> knowledge of
>>> + * what protocol is exactly being used by the transport-plugin.
>>> + * Therefore we simply return INDIRECT for now.
>>> + */
>>> +return "INDIRECT";
>>> +}
>>> +#endif
>>
>> From reading the code this function is only used in creating the string
>> in options_string which needs to match between peers. As the plugin
>> emulates UDP/DGRAM behaviour I think we should instead return here the
>> same as a real UDP protocol.
>
> well, the problem here is that if I use transport-plugin1 and you use
> transport-plugin2 these strings will match between client and server
> even though they should not (because the protocols being used are
> actually different).
If we get to the point that the cleartext inside the openvpn tunnel is
compared the methods are compatible. And if you have a client on the one
side that has plugin in it and a proxy+openvpn server on the other side,
it should also work.
> I don't think INDIRECT is UDP only. That is just how it prefers to be
> treated, but could be non-UDP as well, I think. (need to double check)
But for this check it is only important that the wire format is the TCP
or UDP wire format. Not the actual protocol being used.
>
>>
>>>
>>> +#ifdef ENABLE_PLUGIN
>>> +
>>> +int link_socket_read_indirect(struct link_socket *sock,
>>> + struct buffer *buf,
>>> + struct link_socket_actual *from);
>>> +
>>> +int link_socket_write_indirect(struct link_socket *sock,
>>> + struct buffer *buf,
>>> + struct link_socket_actual *from);
>>> +
>>> +bool proto_is_indirect(int proto);
>>> +
>>
>> This prototype definition looks a bit weird here. I see not reason why
>> the real proto_is_indirect cannot be here
>
> you mean the proto_is_indirect() implementation should be here in the
> header? Why?
It is already in the header. Having an inline implementation AND a
prototype in the same header is what I am complaining about.
Arne
signature.asc
Description: OpenPGP digital signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 2/4] socket: introduce INDIRECT transport protocol abstraction
Hi,
On 23/01/2019 03:22, Arne Schwabe wrote:
> Am 30.12.18 um 12:28 schrieb Antonio Quartulli:
>> From: Robin Tarsiger
>>
>> This new transport protocol is used to tell the core code that traffic
>> should not be directly processed, but should rather be rerouted to a
>> transport plugin. It is basically an abstraction as it does not say tell
>> the code how to process the data, but simply forces its redirection to
>> the external code.
>>
>> Signed-off-by: Robin Tarsiger
>> [anto...@openvpn.net: refactored commits, restyled code]
>> ---
>> src/openvpn/forward.c | 5 ++
>> src/openvpn/socket.c| 146 ++--
>> src/openvpn/socket.h| 70 +++
>> src/openvpn/transport.h | 5 ++
>> 4 files changed, 222 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
>> index 0a90fff0..a7092c7e 100644
>> --- a/src/openvpn/forward.c
>> +++ b/src/openvpn/forward.c
>> @@ -2150,6 +2150,11 @@ io_wait_dowork(struct context *c, const unsigned int
>> flags)
>> {
>> int i;
>> c->c2.event_set_status = 0;
>> +#ifdef ENABLE_PLUGIN
>> +c->c2.event_set_status |=
>> +(socket_indirect_pump(c->c2.link_socket, esr, )
>> & 3)
>
> The & 3 looks like some defined should be used instead.
I think this is simply about fetching the last two bits..not much fuzz
here..this is mostly because of how event_set_status works. But will see
if I find a meaningful name.
>
>
>
>> if (addr->ai_protocol == IPPROTO_UDP || addr->ai_socktype == SOCK_DGRAM)
>> {
>> sock->sd = create_socket_udp(addr, sock->sockflags);
>> @@ -2279,7 +2320,11 @@ link_socket_init_phase2(struct link_socket *sock,
>> }
>>
>> /* If socket has not already been created create it now */
>> -if (sock->sd == SOCKET_UNDEFINED)
>> +if (sock->sd == SOCKET_UNDEFINED
>> +#ifdef ENABLE_PLUGIN
>> +&& !sock->indirect
>> +#endif
>> +)
>> {
>> /* If we have no --remote and have still not figured out the
>> * protocol family to use we will use the first of the bind */
>> @@ -2300,7 +2345,11 @@ link_socket_init_phase2(struct link_socket *sock,
>> }
>>
>> /* Socket still undefined, give a warning and abort connection */
>> -if (sock->sd == SOCKET_UNDEFINED)
>> +if (sock->sd == SOCKET_UNDEFINED
>> +#ifdef ENABLE_PLUGIN
>> +&& !sock->indirect
>> +#endif
>> +)
>
> Better use the inline funtcion proto_is_indirect (or similar
> sock_is_indirect) that always returns false here and ifdef in header
> than to add the ifdefs inline in the code.
agreed! I am all for that! will change
>
>
>> bool
>> @@ -3167,6 +3236,10 @@ proto_is_net(int proto)
>> bool
>> proto_is_dgram(int proto)
>> {
>> +if (proto_is_indirect(proto))
>> +{
>> +return true;
>> +}
>> return proto_is_udp(proto);
>> }
>
> I think here need to be a good explaination why indirect is dgram but
> not udp and also proto_is_dgram and proto_is_udp need to get some
> comment to explain their difference in usage as this is now different.
I wouldn't say is_udp and is_dgram are now different, but is_dgram has
been extended to account for the indirect case as well.
>
>>
>> @@ -3301,6 +3374,18 @@ proto_remote(int proto, bool remote)
>> return "TCPv4_CLIENT";
>> }
>>
>> +#ifdef ENABLE_PLUGIN
>> +if (proto == PROTO_INDIRECT)
>> +{
>> +/* FIXME: the string reported here should match the actual transport
>> + * protocol being used, however in this function we have no
>> knowledge of
>> + * what protocol is exactly being used by the transport-plugin.
>> + * Therefore we simply return INDIRECT for now.
>> + */
>> +return "INDIRECT";
>> +}
>> +#endif
>
> From reading the code this function is only used in creating the string
> in options_string which needs to match between peers. As the plugin
> emulates UDP/DGRAM behaviour I think we should instead return here the
> same as a real UDP protocol.
well, the problem here is that if I use transport-plugin1 and you use
transport-plugin2 these strings will match between client and server
even though they should not (because the protocols being used are
actually different).
I don't think INDIRECT is UDP only. That is just how it prefers to be
treated, but could be non-UDP as well, I think. (need to double check)
>
>>
>> +#ifdef ENABLE_PLUGIN
>> +
>> +int link_socket_read_indirect(struct link_socket *sock,
>> + struct buffer *buf,
>> + struct link_socket_actual *from);
>> +
>> +int link_socket_write_indirect(struct link_socket *sock,
>> + struct buffer *buf,
>> + struct link_socket_actual *from);
>> +
>> +bool
Re: [Openvpn-devel] [PATCH 2/4] socket: introduce INDIRECT transport protocol abstraction
Am 30.12.18 um 12:28 schrieb Antonio Quartulli:
> From: Robin Tarsiger
>
> This new transport protocol is used to tell the core code that traffic
> should not be directly processed, but should rather be rerouted to a
> transport plugin. It is basically an abstraction as it does not say tell
> the code how to process the data, but simply forces its redirection to
> the external code.
>
> Signed-off-by: Robin Tarsiger
> [anto...@openvpn.net: refactored commits, restyled code]
> ---
> src/openvpn/forward.c | 5 ++
> src/openvpn/socket.c| 146 ++--
> src/openvpn/socket.h| 70 +++
> src/openvpn/transport.h | 5 ++
> 4 files changed, 222 insertions(+), 4 deletions(-)
>
> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
> index 0a90fff0..a7092c7e 100644
> --- a/src/openvpn/forward.c
> +++ b/src/openvpn/forward.c
> @@ -2150,6 +2150,11 @@ io_wait_dowork(struct context *c, const unsigned int
> flags)
> {
> int i;
> c->c2.event_set_status = 0;
> +#ifdef ENABLE_PLUGIN
> +c->c2.event_set_status |=
> +(socket_indirect_pump(c->c2.link_socket, esr, ) &
> 3)
The & 3 looks like some defined should be used instead.
> if (addr->ai_protocol == IPPROTO_UDP || addr->ai_socktype == SOCK_DGRAM)
> {
> sock->sd = create_socket_udp(addr, sock->sockflags);
> @@ -2279,7 +2320,11 @@ link_socket_init_phase2(struct link_socket *sock,
> }
>
> /* If socket has not already been created create it now */
> -if (sock->sd == SOCKET_UNDEFINED)
> +if (sock->sd == SOCKET_UNDEFINED
> +#ifdef ENABLE_PLUGIN
> +&& !sock->indirect
> +#endif
> +)
> {
> /* If we have no --remote and have still not figured out the
> * protocol family to use we will use the first of the bind */
> @@ -2300,7 +2345,11 @@ link_socket_init_phase2(struct link_socket *sock,
> }
>
> /* Socket still undefined, give a warning and abort connection */
> -if (sock->sd == SOCKET_UNDEFINED)
> +if (sock->sd == SOCKET_UNDEFINED
> +#ifdef ENABLE_PLUGIN
> +&& !sock->indirect
> +#endif
> +)
Better use the inline funtcion proto_is_indirect (or similar
sock_is_indirect) that always returns false here and ifdef in header
than to add the ifdefs inline in the code.
> bool
> @@ -3167,6 +3236,10 @@ proto_is_net(int proto)
> bool
> proto_is_dgram(int proto)
> {
> +if (proto_is_indirect(proto))
> +{
> +return true;
> +}
> return proto_is_udp(proto);
> }
I think here need to be a good explaination why indirect is dgram but
not udp and also proto_is_dgram and proto_is_udp need to get some
comment to explain their difference in usage as this is now different.
>
> @@ -3301,6 +3374,18 @@ proto_remote(int proto, bool remote)
> return "TCPv4_CLIENT";
> }
>
> +#ifdef ENABLE_PLUGIN
> +if (proto == PROTO_INDIRECT)
> +{
> +/* FIXME: the string reported here should match the actual transport
> + * protocol being used, however in this function we have no
> knowledge of
> + * what protocol is exactly being used by the transport-plugin.
> + * Therefore we simply return INDIRECT for now.
> + */
> +return "INDIRECT";
> +}
> +#endif
From reading the code this function is only used in creating the string
in options_string which needs to match between peers. As the plugin
emulates UDP/DGRAM behaviour I think we should instead return here the
same as a real UDP protocol.
>
> +#ifdef ENABLE_PLUGIN
> +
> +int link_socket_read_indirect(struct link_socket *sock,
> + struct buffer *buf,
> + struct link_socket_actual *from);
> +
> +int link_socket_write_indirect(struct link_socket *sock,
> + struct buffer *buf,
> + struct link_socket_actual *from);
> +
> +bool proto_is_indirect(int proto);
> +
This prototype definition looks a bit weird here. I see not reason why
the real proto_is_indirect cannot be here
> +#else /* ifdef ENABLE_PLUGIN */
> +
> +static int
> +link_socket_read_indirect(struct link_socket *sock,
> +
> +static int
> +link_socket_write_indirect(struct link_socket *sock,
> +static bool
> +proto_is_indirect(int proto)
I think functions implemented in the header should have the inline
keyword. At least the rest of the header files do it that way.
Arne
signature.asc
Description: OpenPGP digital signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 2/4] socket: introduce INDIRECT transport protocol abstraction
From: Robin Tarsiger
This new transport protocol is used to tell the core code that traffic
should not be directly processed, but should rather be rerouted to a
transport plugin. It is basically an abstraction as it does not say tell
the code how to process the data, but simply forces its redirection to
the external code.
Signed-off-by: Robin Tarsiger
[anto...@openvpn.net: refactored commits, restyled code]
---
src/openvpn/forward.c | 5 ++
src/openvpn/socket.c| 146 ++--
src/openvpn/socket.h| 70 +++
src/openvpn/transport.h | 5 ++
4 files changed, 222 insertions(+), 4 deletions(-)
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 0a90fff0..a7092c7e 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -2150,6 +2150,11 @@ io_wait_dowork(struct context *c, const unsigned int
flags)
{
int i;
c->c2.event_set_status = 0;
+#ifdef ENABLE_PLUGIN
+c->c2.event_set_status |=
+(socket_indirect_pump(c->c2.link_socket, esr, ) & 3)
+<< socket_shift;
+#endif
for (i = 0; i < status; ++i)
{
const struct event_set_return *e = [i];
diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index db944245..b548ab7a 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -41,6 +41,7 @@
#include "manage.h"
#include "openvpn.h"
#include "forward.h"
+#include "transport.h"
#include "memdbg.h"
@@ -49,6 +50,9 @@ const int proto_overhead[] = { /* indexed by PROTO_x */
IPv4_UDP_HEADER_SIZE, /* IPv4 */
IPv4_TCP_HEADER_SIZE,
IPv4_TCP_HEADER_SIZE,
+#ifdef ENABLE_PLUGIN
+INDIRECT_HEADER_SIZE,
+#endif
IPv6_UDP_HEADER_SIZE, /* IPv6 */
IPv6_TCP_HEADER_SIZE,
IPv6_TCP_HEADER_SIZE,
@@ -1103,9 +1107,46 @@ bind_local(struct link_socket *sock, const sa_family_t
ai_family)
}
}
+#ifdef ENABLE_PLUGIN
+
+static void
+create_socket_indirect(struct link_socket *sock, sa_family_t ai_family)
+{
+struct addrinfo *bind_addresses = NULL;
+if (sock->bind_local)
+{
+bind_addresses = sock->info.lsa->bind_local;
+}
+
+sock->indirect = transport_bind(sock->info.plugins,
+sock->info.transport_plugin_argv,
+ai_family,
+bind_addresses);
+}
+
+bool
+proto_is_indirect(int proto)
+{
+return proto == PROTO_INDIRECT;
+}
+
+#else /* ifdef ENABLE_PLUGIN */
+
+static void
+create_socket_indirect(struct link_socket *sock, sa_family_t ai_family)
+{
+}
+
+#endif /* ENABLE_PLUGIN */
+
static void
create_socket(struct link_socket *sock, struct addrinfo *addr)
{
+if (proto_is_indirect(sock->info.proto))
+{
+create_socket_indirect(sock, addr->ai_family);
+}
+
if (addr->ai_protocol == IPPROTO_UDP || addr->ai_socktype == SOCK_DGRAM)
{
sock->sd = create_socket_udp(addr, sock->sockflags);
@@ -2279,7 +2320,11 @@ link_socket_init_phase2(struct link_socket *sock,
}
/* If socket has not already been created create it now */
-if (sock->sd == SOCKET_UNDEFINED)
+if (sock->sd == SOCKET_UNDEFINED
+#ifdef ENABLE_PLUGIN
+&& !sock->indirect
+#endif
+)
{
/* If we have no --remote and have still not figured out the
* protocol family to use we will use the first of the bind */
@@ -2300,7 +2345,11 @@ link_socket_init_phase2(struct link_socket *sock,
}
/* Socket still undefined, give a warning and abort connection */
-if (sock->sd == SOCKET_UNDEFINED)
+if (sock->sd == SOCKET_UNDEFINED
+#ifdef ENABLE_PLUGIN
+&& !sock->indirect
+#endif
+)
{
msg(M_WARN, "Could not determine IPv4/IPv6 protocol");
sig_info->signal_received = SIGUSR1;
@@ -2338,7 +2387,10 @@ link_socket_init_phase2(struct link_socket *sock,
}
}
-phase2_set_socket_flags(sock);
+if (sock->sd != SOCKET_UNDEFINED)
+{
+phase2_set_socket_flags(sock);
+}
linksock_print_addr(sock);
done:
@@ -2362,6 +2414,14 @@ link_socket_close(struct link_socket *sock)
const int gremlin = 0;
#endif
+#ifdef ENABLE_PLUGIN
+if (sock->indirect)
+{
+sock->indirect->vtab->close(sock->indirect);
+sock->indirect = NULL;
+}
+#endif
+
if (socket_defined(sock->sd))
{
#ifdef _WIN32
@@ -3143,16 +3203,25 @@ static const struct proto_names proto_names[] = {
{"tcp-server", "TCP_SERVER", AF_UNSPEC, PROTO_TCP_SERVER},
{"tcp-client", "TCP_CLIENT", AF_UNSPEC, PROTO_TCP_CLIENT},
{"tcp","TCP", AF_UNSPEC, PROTO_TCP},
+#ifdef ENABLE_PLUGIN
+{"indirect", "INDIRECT", AF_UNSPEC, PROTO_INDIRECT},
+#endif
/* force IPv4 */
{"udp4", "UDPv4",
