Re: [Openvpn-devel] [PATCH] Peer-id patch v7

[Gert Doering]

Hi,

On Thu, Nov 27, 2014 at 05:08:09PM +0200, Lev Stipakov wrote:
> As for the second question - hard to say. If we make it opt-in, we
> probably will need to announce this feature loudly to make users aware
> of that. From the other side, it is not inconceivable to assume that
> someone might not want this. Maybe we could make it opt-out and have
> "-no-peer-id" config option?

Definitely not "a new option", as we already have --float :-)

But I can see the argument about opt-in/opt-out - I found it a useful
thing to explicitely enable the feature (so, no surprises here) but if
there is not enough advertising, people won't be able to benefit 
from it...

gert

-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


pgpu84QrDvLKn.pgp
Description: PGP signature

Re: [Openvpn-devel] [PATCH] Peer-id patch v7

[Lev Stipakov]

Hello,

Currently it should be safe, since multi_create_instance returns NULL
if amount of clients >= max_clients. In this case we won't reach that
"for" loop thanks to "if (mi)" check.

But it probably won't harm to assert if we've reached "for" loop and
could not find available "instance" item. I can implement that.

As for the second question - hard to say. If we make it opt-in, we
probably will need to announce this feature loudly to make users aware
of that. From the other side, it is not inconceivable to assume that
someone might not want this. Maybe we could make it opt-out and have
"-no-peer-id" config option?

-Lev

2014-11-27 16:22 GMT+02:00 Gert Doering :
> Hi,
>
> On Sun, Nov 23, 2014 at 05:17:11PM +0200, Lev Stipakov wrote:
>> Changes in v7:
>> A few nitpicks.
>
> Went in, and has just been pushed.  Time for your dance now :-)
>
> Question to you:
>
>> @@ -75,6 +101,16 @@ multi_get_create_instance_udp (struct multi_context *m)
>>   {
>> hash_add_fast (hash, bucket, >real, hv, mi);
>> mi->did_real_hash = true;
>> +
>> +   for (i = 0; i < m->max_clients; ++i)
>> + {
>> +   if (!m->instances[i])
>> + {
>> +   mi->context.c2.tls_multi->peer_id = i;
>> +   m->instances[i] = mi;
>> +   break;
>> + }
>> + }
>
> Is this code "safe"?  That is, if max_clients is set too low, and you have
> more than this number of clients trying to connect, what will happen?
>
> Will the server check this case before this loop, or will it just "not
> find a free instance" and leave peer_id as "0"?
>
> Maybe we should add an ASSERT() here, to be sure that whatever other pieces
> of the code do, this will always end up in a defined state  (as a separate
> patch).
>
>
>
> Second, question to the group.  Right now, this code is active unconditionally
> on the server side - what if someone does not want this behaviour, for
> whatever reason?  Shall we make it depend on --float (which right now
> doesn't do anything for a --server OpenVPN server)?
>
> This should be a fairly small change, I think, just never send the client
> a "peer-id " push-reply (so it won't use T_PACKET_V2), and check
> "if (options.float)" before checking whether it's a floated client...
> so the question is more along the line "is this a useful thing to do,
> nor not?".
>
> gert
> --
> USENET is *not* the non-clickable part of WWW!
>//www.muc.de/~gert/
> Gert Doering - Munich, Germany g...@greenie.muc.de
> fax: +49-89-35655025g...@net.informatik.tu-muenchen.de



-- 
-Lev

Re: [Openvpn-devel] [PATCH] Peer-id patch v7

[Gert Doering]

Hi,

On Sun, Nov 23, 2014 at 05:17:11PM +0200, Lev Stipakov wrote:
> Changes in v7:
> A few nitpicks.

Went in, and has just been pushed.  Time for your dance now :-)

Question to you:

> @@ -75,6 +101,16 @@ multi_get_create_instance_udp (struct multi_context *m)
>   {
> hash_add_fast (hash, bucket, >real, hv, mi);
> mi->did_real_hash = true;
> +
> +   for (i = 0; i < m->max_clients; ++i)
> + {
> +   if (!m->instances[i])
> + {
> +   mi->context.c2.tls_multi->peer_id = i;
> +   m->instances[i] = mi;
> +   break;
> + }
> + }

Is this code "safe"?  That is, if max_clients is set too low, and you have
more than this number of clients trying to connect, what will happen?

Will the server check this case before this loop, or will it just "not
find a free instance" and leave peer_id as "0"?

Maybe we should add an ASSERT() here, to be sure that whatever other pieces
of the code do, this will always end up in a defined state  (as a separate
patch).



Second, question to the group.  Right now, this code is active unconditionally
on the server side - what if someone does not want this behaviour, for
whatever reason?  Shall we make it depend on --float (which right now 
doesn't do anything for a --server OpenVPN server)?

This should be a fairly small change, I think, just never send the client
a "peer-id " push-reply (so it won't use T_PACKET_V2), and check 
"if (options.float)" before checking whether it's a floated client...  
so the question is more along the line "is this a useful thing to do, 
nor not?".

gert
-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


pgp9uijzH9E6v.pgp
Description: PGP signature

[Openvpn-devel] [PATCH applied] Re: Add client-only support for peer-id.

[Gert Doering]

Your patch has been applied to the release/2.3 branch.

commit 0e1fd33247460bdfa65d306e8bcdd3cbafed8b73
Author: Gert Doering
List-Post: openvpn-devel@lists.sourceforge.net
Date:   Sun Nov 23 20:17:30 2014 +0100

 Add client-only support for peer-id.

 Signed-off-by: Gert Doering 
 Acked-by: Steffan Karger 
 Message-Id: <1416770250-92680-1-git-send-email-g...@greenie.muc.de>
 URL: http://article.gmane.org/gmane.network.openvpn.devel/9274


--
kind regards,

Gert Doering

[Openvpn-devel] [PATCH applied] Re: Peer-id patch v7

[Gert Doering]

ACK from me as well.

Your patch has been applied to the master branch (and a client-only version
of this has been applied to release/2.3 - separate mail coming).

commit 65eedc353349d2967fc03c54da807727e416e1b0 (master)

Author: Lev Stipakov
List-Post: openvpn-devel@lists.sourceforge.net
Date:   Sun Nov 23 17:17:11 2014 +0200

 Peer-id patch v7

 Acked-by: Steffan Karger 
 Acked-by: Gert Doering 
 Message-Id: <1416755831-21250-1-git-send-email-lstipa...@gmail.com>
 URL: http://article.gmane.org/gmane.network.openvpn.devel/9270
 Signed-off-by: Gert Doering 


--
kind regards,

Gert Doering