Exclude peer-id from pulled options digest

PATCH New Lev Stipakov 4 participants 9 messages Sep 24, 2016 – Oct 10, 2016

lore

Share & tools

lore

Lev Stipakov

author

2016-09-24 15:51 UTC #0 lore raw

PATCH 1/1 Exclude peer-id from pulled options digest

Peer-id might change on restart and this should not trigger reopening tun.

Trac #649

---
 src/openvpn/push.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

src/openvpn/push.c +24 ~~-14~~

index 71f39c1..6feaa2a 100644
@@ -473,20 +473,30 @@ process_incoming_push_msg (struct context *c,
 				  permission_mask,
 				  option_types_found,
 				  c->c2.es))
-	    switch (c->options.push_continuation)
-	      {
-	      case 0:
-	      case 1:
-		md5_state_update (&c->c2.pulled_options_state, BPTR(&buf_orig), BLEN(&buf_orig));
-		md5_state_final (&c->c2.pulled_options_state, &c->c2.pulled_options_digest);
-	        c->c2.pulled_options_md5_init_done = false;
-		ret = PUSH_MSG_REPLY;
-		break;
-	      case 2:
-		md5_state_update (&c->c2.pulled_options_state, BPTR(&buf_orig), BLEN(&buf_orig));
-		ret = PUSH_MSG_CONTINUATION;
-		break;
-	      }
+	    {
+	      char line[OPTION_PARM_SIZE];
+	      while (buf_parse (&buf_orig, ',', line, sizeof (line)))
+		{
+		  /* peer-id might change on restart and this should not trigger reopening tun */
+		  if (strstr (line, "peer-id ") != line)
+		    {
+		      md_ctx_update (&c->c2.pulled_options_state, (const uint8_t *) line, strlen(line));
+		    }
+		}
+	      switch (c->options.push_continuation)
+		{
+		  case 0:
+		  case 1:
+		    md_ctx_final (&c->c2.pulled_options_state, c->c2.pulled_options_digest.digest);
+		    md_ctx_cleanup (&c->c2.pulled_options_state);
+		    c->c2.pulled_options_md5_init_done = false;
+		    ret = PUSH_MSG_REPLY;
+		    break;
+		  case 2:
+		    ret = PUSH_MSG_CONTINUATION;
+		    break;
+		}
+	    }
 	}
       else if (ch == '\0')
 	{

Signature · 1 line

1.9.1

Lev Stipakov

author

2016-09-24 15:54 UTC #1 lore raw

PATCH 1/1 Exclude peer-id from pulled options digest

Peer-id might change on restart and this should not trigger reopening tun.

Trac #649

---
 src/openvpn/push.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

src/openvpn/push.c +24 ~~-15~~

index a1b999e..d7d54bf 100644
@@ -636,21 +636,30 @@ process_incoming_push_msg (struct context *c,
 				  permission_mask,
 				  option_types_found,
 				  c->c2.es))
-	    switch (c->options.push_continuation)
-	      {
-	      case 0:
-	      case 1:
-		md_ctx_update (&c->c2.pulled_options_state, BPTR(&buf_orig), BLEN(&buf_orig));
-		md_ctx_final (&c->c2.pulled_options_state, c->c2.pulled_options_digest.digest);
-		md_ctx_cleanup (&c->c2.pulled_options_state);
-	        c->c2.pulled_options_md5_init_done = false;
-		ret = PUSH_MSG_REPLY;
-		break;
-	      case 2:
-		md_ctx_update (&c->c2.pulled_options_state, BPTR(&buf_orig), BLEN(&buf_orig));
-		ret = PUSH_MSG_CONTINUATION;
-		break;
-	      }
+	    {
+	      char line[OPTION_PARM_SIZE];
+	      while (buf_parse (&buf_orig, ',', line, sizeof (line)))
+		{
+		  /* peer-id might change on restart and this should not trigger reopening tun */
+		  if (strstr (line, "peer-id ") != line)
+		    {
+		      md_ctx_update (&c->c2.pulled_options_state, (const uint8_t *) line, strlen(line));
+		    }
+		}
+	      switch (c->options.push_continuation)
+		{
+		  case 0:
+		  case 1:
+		    md_ctx_final (&c->c2.pulled_options_state, c->c2.pulled_options_digest.digest);
+		    md_ctx_cleanup (&c->c2.pulled_options_state);
+		    c->c2.pulled_options_md5_init_done = false;
+		    ret = PUSH_MSG_REPLY;
+		    break;
+		  case 2:
+		    ret = PUSH_MSG_CONTINUATION;
+		    break;
+		}
+	    }
 	}
       else if (ch == '\0')
 	{

Signature · 1 line

1.9.1

Steffan Karger

2016-10-04 16:47 UTC #2 lore raw

Hi,

Quoted 4 lines · Lev Stipakov

Peer-id might change on restart and this should not trigger reopening tun.
Trac #649

Feature-ACK.

The same holds for ncp stuff though, so I think we should do the same for cipher, auth and keysize. Unless those change the tun-mtu, hmm... That might need further investigation, so maybe keep that separate from the peer-id issue.

src/openvpn/push.c process_incoming_push_msg (struct context *c, · line 648

Quoted 9 lines

src/openvpn/push.c +9 -0

+	      char line[OPTION_PARM_SIZE];
+	      while (buf_parse (&buf_orig, ',', line, sizeof (line)))
+		{
+		  /* peer-id might change on restart and this should not trigger reopening tun */
+		  if (strstr (line, "peer-id ") != line)
+		    {
+		      md_ctx_update (&c->c2.pulled_options_state, (const uint8_t *) line, strlen(line));
+		    }
+		}

This looks like it should be a separate (static) function 'push_update_digest(state, msg)', or something like that.

src/openvpn/push.c process_incoming_push_msg (struct context *c, · line 654

Quoted 1 line

src/openvpn/push.c +1 -0

+		    md_ctx_cleanup (&c->c2.pulled_options_state);

Are you sure? I don't see a "- md_ctx_cleanup (...)" in the patch
for master (there is in 2.3 though). Do you fix a bug here, or is this a cherry-picking mistake?

-Steffan

Lev Stipakov

author

2016-10-04 19:53 UTC #3 lore raw

PATCH v2 1/1 Exclude peer-id from pulled options digest

v2:
- Use md5_* methods
- Move digest update to separate method

Peer-id might change on restart and this should not trigger reopening tun.

Trac #649

---
 src/openvpn/push.c | 43 +++++++++++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 14 deletions(-)

src/openvpn/push.c +29 ~~-14~~

index 71f39c1..402c158 100644
@@ -407,6 +407,20 @@ push_reset (struct options *o)
 }
 #endif
 
+static void
+push_update_digest(struct md5_state *ctx, struct buffer *buf)
+{
+  char line[OPTION_PARM_SIZE];
+  while (buf_parse (buf, ',', line, sizeof (line)))
+    {
+      /* peer-id might change on restart and this should not trigger reopening tun */
+      if (strstr (line, "peer-id ") != line)
+	{
+	  md5_state_update (ctx, line, strlen(line));
+	}
+    }
+}
+
 int
 process_incoming_push_msg (struct context *c,
 			   const struct buffer *buffer,

@@ -473,20 +487,21 @@ process_incoming_push_msg (struct context *c,
 				  permission_mask,
 				  option_types_found,
 				  c->c2.es))
-	    switch (c->options.push_continuation)
-	      {
-	      case 0:
-	      case 1:
-		md5_state_update (&c->c2.pulled_options_state, BPTR(&buf_orig), BLEN(&buf_orig));
-		md5_state_final (&c->c2.pulled_options_state, &c->c2.pulled_options_digest);
-	        c->c2.pulled_options_md5_init_done = false;
-		ret = PUSH_MSG_REPLY;
-		break;
-	      case 2:
-		md5_state_update (&c->c2.pulled_options_state, BPTR(&buf_orig), BLEN(&buf_orig));
-		ret = PUSH_MSG_CONTINUATION;
-		break;
-	      }
+	    {
+	      push_update_digest (&c->c2.pulled_options_state, &buf_orig);
+	      switch (c->options.push_continuation)
+		{
+		  case 0:
+		  case 1:
+		    md5_state_final (&c->c2.pulled_options_state, &c->c2.pulled_options_digest);
+		    c->c2.pulled_options_md5_init_done = false;
+		    ret = PUSH_MSG_REPLY;
+		    break;
+		  case 2:
+		    ret = PUSH_MSG_CONTINUATION;
+		    break;
+		}
+	    }
 	}
       else if (ch == '\0')
 	{

Signature · 1 line

1.9.1

Steffan Karger

2016-10-05 12:27 UTC #4 lore raw

Quoted 64 lines · Lev Stipakov

v2:
- Use md5_* methods
- Move digest update to separate method

Peer-id might change on restart and this should not trigger reopening tun.

Trac #649

---
 src/openvpn/push.c | 43 +++++++++++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 14 deletions(-)

src/openvpn/push.c +22 ~~-14~~

index 71f39c1..402c158 100644
@@ -407,6 +407,20 @@ push_reset (struct options *o)
 }
 #endif
 
+static void
+push_update_digest(struct md5_state *ctx, struct buffer *buf)
+{
+  char line[OPTION_PARM_SIZE];
+  while (buf_parse (buf, ',', line, sizeof (line)))
+    {
+      /* peer-id might change on restart and this should not trigger reopening tun */
+      if (strstr (line, "peer-id ") != line)
+	{
+	  md5_state_update (ctx, line, strlen(line));
+	}
+    }
+}
+
 int
 process_incoming_push_msg (struct context *c,
 			   const struct buffer *buffer,

@@ -473,20 +487,21 @@ process_incoming_push_msg (struct context *c,
 				  permission_mask,
 				  option_types_found,
 				  c->c2.es))
-	    switch (c->options.push_continuation)
-	      {
-	      case 0:
-	      case 1:
-		md5_state_update (&c->c2.pulled_options_state, BPTR(&buf_orig), BLEN(&buf_orig));
-		md5_state_final (&c->c2.pulled_options_state, &c->c2.pulled_options_digest);
-	        c->c2.pulled_options_md5_init_done = false;
-		ret = PUSH_MSG_REPLY;
-		break;
-	      case 2:
-		md5_state_update (&c->c2.pulled_options_state, BPTR(&buf_orig), BLEN(&buf_orig));
-		ret = PUSH_MSG_CONTINUATION;
-		break;
-	      }
+	    {
+	      push_update_digest (&c->c2.pulled_options_state, &buf_orig);
+	      switch (c->options.push_continuation)
+		{
+		  case 0:
+		  case 1:
+		    md5_state_final (&c->c2.pulled_options_state, &c->c2.pulled_options_digest);
+		    c->c2.pulled_options_md5_init_done = false;

src/openvpn/push.c process_incoming_push_msg (struct context *c, · line 507

Quoted 10 lines

src/openvpn/push.c +7 -0

+		    ret = PUSH_MSG_REPLY;
+		    break;
+		  case 2:
+		    ret = PUSH_MSG_CONTINUATION;
+		    break;
+		}
+	    }
 	}
       else if (ch == '\0')
 	{

Looks good now. ACK.

-Steffan

Gert Doering

2016-10-10 08:19 UTC #5 lore raw

In reply to Lev Stipakov

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

commit 84022030dc2af8606e6a11c3dca1780419e7a534

Author: Lev Stipakov
Date:   Tue Oct 4 22:53:06 2016 +0300

     Exclude peer-id from pulled options digest

     Acked-by: Steffan Karger <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg12598.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Lev Stipakov

author

2016-10-04 20:20 UTC #6 lore raw

PATCH v2 1/1 Exclude peer-id from pulled options digest

In reply to Steffan Karger

v2:
- Move digest update to separate method

Peer-id might change on restart and this should not trigger reopening tun.

Trac #649

---
 src/openvpn/push.c | 45 ++++++++++++++++++++++++++++++---------------
 1 file changed, 30 insertions(+), 15 deletions(-)

src/openvpn/push.c +30 ~~-15~~

index a1b999e..c0c78a0 100644
@@ -597,6 +597,20 @@ process_incoming_push_request (struct context *c)
 }
 #endif
 
+static void
+push_update_digest(md_ctx_t *ctx, struct buffer *buf)
+{
+  char line[OPTION_PARM_SIZE];
+  while (buf_parse (buf, ',', line, sizeof (line)))
+    {
+      /* peer-id might change on restart and this should not trigger reopening tun */
+      if (strstr (line, "peer-id ") != line)
+	{
+	  md_ctx_update (ctx, (const uint8_t *) line, strlen(line));
+	}
+    }
+}
+
 int
 process_incoming_push_msg (struct context *c,
 			   const struct buffer *buffer,

@@ -636,21 +650,22 @@ process_incoming_push_msg (struct context *c,
 				  permission_mask,
 				  option_types_found,
 				  c->c2.es))
-	    switch (c->options.push_continuation)
-	      {
-	      case 0:
-	      case 1:
-		md_ctx_update (&c->c2.pulled_options_state, BPTR(&buf_orig), BLEN(&buf_orig));
-		md_ctx_final (&c->c2.pulled_options_state, c->c2.pulled_options_digest.digest);
-		md_ctx_cleanup (&c->c2.pulled_options_state);
-	        c->c2.pulled_options_md5_init_done = false;
-		ret = PUSH_MSG_REPLY;
-		break;
-	      case 2:
-		md_ctx_update (&c->c2.pulled_options_state, BPTR(&buf_orig), BLEN(&buf_orig));
-		ret = PUSH_MSG_CONTINUATION;
-		break;
-	      }
+	    {
+	      push_update_digest (&c->c2.pulled_options_state, &buf_orig);
+	      switch (c->options.push_continuation)
+		{
+		  case 0:
+		  case 1:
+		    md_ctx_final (&c->c2.pulled_options_state, c->c2.pulled_options_digest.digest);
+		    md_ctx_cleanup (&c->c2.pulled_options_state);
+		    c->c2.pulled_options_md5_init_done = false;
+		    ret = PUSH_MSG_REPLY;
+		    break;
+		  case 2:
+		    ret = PUSH_MSG_CONTINUATION;
+		    break;
+		}
+	    }
 	}
       else if (ch == '\0')
 	{

Signature · 1 line

1.9.1

Steffan Karger

2016-10-05 12:30 UTC #7 lore raw

Quoted 65 lines · Lev Stipakov

v2:
- Move digest update to separate method

Peer-id might change on restart and this should not trigger reopening tun.

Trac #649

---
 src/openvpn/push.c | 45 ++++++++++++++++++++++++++++++---------------
 1 file changed, 30 insertions(+), 15 deletions(-)

src/openvpn/push.c +23 ~~-15~~

index a1b999e..c0c78a0 100644
@@ -597,6 +597,20 @@ process_incoming_push_request (struct context *c)
 }
 #endif
 
+static void
+push_update_digest(md_ctx_t *ctx, struct buffer *buf)
+{
+  char line[OPTION_PARM_SIZE];
+  while (buf_parse (buf, ',', line, sizeof (line)))
+    {
+      /* peer-id might change on restart and this should not trigger reopening tun */
+      if (strstr (line, "peer-id ") != line)
+	{
+	  md_ctx_update (ctx, (const uint8_t *) line, strlen(line));
+	}
+    }
+}
+
 int
 process_incoming_push_msg (struct context *c,
 			   const struct buffer *buffer,

@@ -636,21 +650,22 @@ process_incoming_push_msg (struct context *c,
 				  permission_mask,
 				  option_types_found,
 				  c->c2.es))
-	    switch (c->options.push_continuation)
-	      {
-	      case 0:
-	      case 1:
-		md_ctx_update (&c->c2.pulled_options_state, BPTR(&buf_orig), BLEN(&buf_orig));
-		md_ctx_final (&c->c2.pulled_options_state, c->c2.pulled_options_digest.digest);
-		md_ctx_cleanup (&c->c2.pulled_options_state);
-	        c->c2.pulled_options_md5_init_done = false;
-		ret = PUSH_MSG_REPLY;
-		break;
-	      case 2:
-		md_ctx_update (&c->c2.pulled_options_state, BPTR(&buf_orig), BLEN(&buf_orig));
-		ret = PUSH_MSG_CONTINUATION;
-		break;
-	      }
+	    {
+	      push_update_digest (&c->c2.pulled_options_state, &buf_orig);
+	      switch (c->options.push_continuation)
+		{
+		  case 0:
+		  case 1:
+		    md_ctx_final (&c->c2.pulled_options_state, c->c2.pulled_options_digest.digest);
+		    md_ctx_cleanup (&c->c2.pulled_options_state);
+		    c->c2.pulled_options_md5_init_done = false;

src/openvpn/push.c process_incoming_push_msg (struct context *c, · line 671

Quoted 10 lines

src/openvpn/push.c +7 -0

+		    ret = PUSH_MSG_REPLY;
+		    break;
+		  case 2:
+		    ret = PUSH_MSG_CONTINUATION;
+		    break;
+		}
+	    }
 	}
       else if (ch == '\0')
 	{

ACK

-Steffan

Gert Doering

2016-10-10 08:38 UTC #8 lore raw

In reply to Lev Stipakov

Your patch has been applied to the master branch.

commit 3cf51f613c4d0ac0982826cd2e27e1f34bcd1a83

Author: Lev Stipakov
Date:   Tue Oct 4 23:20:03 2016 +0300

     Exclude peer-id from pulled options digest

     Acked-by: Steffan Karger <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg12599.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering