>>> +++ 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, &status)
>>> & 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
