Re: [Openvpn-devel] [PATCH v2] Support for disabled peer-id

[Steffan Karger]

Hi,

The patch looks functionally ok.  Just one minor remark:

On 09-10-15 16:13, Lev Stipakov wrote:

@@ -103,10 +107,14 @@ multi_get_create_instance_udp (struct multi_context *m, 
bool *floated)
  hash_add_fast (hash, bucket, >real, hv, mi);
  mi->did_real_hash = true;

+ /* In future we might want to use P_DATA_V2 but not need 
peer-id/float functionality */
  for (i = 0; i < m->max_clients; ++i)
{
  if (!m->instances[i])
{
+ /* issued peer-id should fit into 3 bytes to 
avoid wrap and cannot have reserved value 0xFF */
+ ASSERT(i < 0xFF);
+
  mi->context.c2.tls_multi->peer_id = i;
  m->instances[i] = mi;
  break;


Checking against the max peer-id indeed makes sense, but this assert can 
be made simpler by just putting


   ASSERT (m->max-clients < 0xFF);

before the loop.  Additionally, I think we should add a user-friendly 
check in options.c, to give a proper error message on startup when a 
user tries to set --max-clients >= 0xFF.


Finally, why did you put this comment at this location?  Shouldn't it be 
in the place where we *send* the peer-id?  (That is the part that is not 
implemented now, right?)


-Steffan

[Openvpn-devel] [PATCH v2] Support for disabled peer-id

[Lev Stipakov]

v2:
 * Add round brackets for clarity.
 * Rephrase comment.

v1:
 * When peer-id value is 0xFF, server should ignore it and treat packet
in a same way as P_DATA_V1.
 * Make sure that issued peer-id does not exceed 0xFF.
---
 src/openvpn/mudp.c  | 14 +++---
 src/openvpn/multi.c |  3 ++-
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c
index 57118f8..fcbb47d 100644
--- a/src/openvpn/mudp.c
+++ b/src/openvpn/mudp.c
@@ -60,12 +60,16 @@ multi_get_create_instance_udp (struct multi_context *m, 
bool *floated)
   struct hash_bucket *bucket = hash_bucket (hash, hv);
   uint8_t* ptr = BPTR(>top.c2.buf);
   uint8_t op = ptr[0] >> P_OPCODE_SHIFT;
+  bool v2 = (op == P_DATA_V2) && (m->top.c2.buf.len >= (1 + 3));
+  bool peer_id_disabled = false;

   /* make sure buffer has enough length to read opcode (1 byte) and 
peer-id (3 bytes) */
-  if (op == P_DATA_V2 && m->top.c2.buf.len >= (1 + 3))
+  if (v2)
{
  uint32_t peer_id = ntohl(*(uint32_t*)ptr) & 0xFF;
- if ((peer_id < m->max_clients) && (m->instances[peer_id]))
+ peer_id_disabled = (peer_id == 0xFF);
+
+ if (!peer_id_disabled && (peer_id < m->max_clients) && 
(m->instances[peer_id]))
{
  mi = m->instances[peer_id];

@@ -80,7 +84,7 @@ multi_get_create_instance_udp (struct multi_context *m, bool 
*floated)
  }
}
}
-  else
+  if (!v2 || peer_id_disabled)
{
  he = hash_lookup_fast (hash, bucket, , hv);
  if (he)
@@ -103,10 +107,14 @@ multi_get_create_instance_udp (struct multi_context *m, 
bool *floated)
  hash_add_fast (hash, bucket, >real, hv, mi);
  mi->did_real_hash = true;

+ /* In future we might want to use P_DATA_V2 but not need 
peer-id/float functionality */
  for (i = 0; i < m->max_clients; ++i)
{
  if (!m->instances[i])
{
+ /* issued peer-id should fit into 3 bytes to 
avoid wrap and cannot have reserved value 0xFF */
+ ASSERT(i < 0xFF);
+
  mi->context.c2.tls_multi->peer_id = i;
  m->instances[i] = mi;
  break;
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 902c4dc..76f5a44 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -562,7 +562,8 @@ multi_close_instance (struct multi_context *m,
}
 #endif

-  m->instances[mi->context.c2.tls_multi->peer_id] = NULL;
+  if (mi->context.c2.tls_multi->peer_id != 0xFF)
+m->instances[mi->context.c2.tls_multi->peer_id] = NULL;

   schedule_remove_entry (m->schedule, (struct schedule_entry *) mi);

-- 
1.9.1