cleanup: merge packet_id_alloc_outgoing() into packet_id_write()

PATCH In review Steffan Karger 3 participants 4 messages Dec 18, 2016 – May 5, 2017

Share & tools

Steffan Karger

author

2016-12-18 23:01 UTC #0 lore raw

PATCH 1/1 cleanup: merge packet_id_alloc_outgoing() into packet_id_write()

The functions packet_id_alloc_outgoing() and packet_id_write() were always called in tandem. Instead of forcing the caller to allocate a packet_id_net to do so, merge the two functions. This simplifies the API and reduces the chance on mistakes in the future.

This patch adds unit tests to verify the behaviour of packet_id_write(). Verifying that we assert out correctly required the change to mock_msg.c.

Signed-off-by Steffan Karger <[email protected]...>

---
This patch is for the master branch only.

 src/openvpn/crypto.c                      |  19 ++--
 src/openvpn/packet_id.c                   |  24 ++++-
 src/openvpn/packet_id.h                   |  35 +++----
 src/openvpn/tls_crypt.c                   |   4 +-
 tests/unit_tests/openvpn/Makefile.am      |  16 ++-
 tests/unit_tests/openvpn/mock_msg.c       |  15 ++-
 tests/unit_tests/openvpn/test_packet_id.c | 168 ++++++++++++++++++++++++++++++
 7 files changed, 229 insertions(+), 52 deletions(-)
 create mode 100644 tests/unit_tests/openvpn/test_packet_id.c

src/openvpn/crypto.c +6 ~~-13~~

index 17e6cb3..414bbbf 100644
@@ -84,7 +84,6 @@ openvpn_encrypt_aead(struct buffer *buf, struct buffer work,
 
     /* Prepare IV */
     struct buffer iv_buffer;
-    struct packet_id_net pin;
     uint8_t iv[OPENVPN_MAX_IV_LENGTH] = {0};
     const int iv_len = cipher_ctx_iv_length(ctx->cipher);

@@ -94,8 +93,7 @@ openvpn_encrypt_aead(struct buffer *buf, struct buffer work,
     buf_set_write(&iv_buffer, iv, iv_len);
 
     /* IV starts with packet id to make the IV unique for packet */
-    packet_id_alloc_outgoing(&opt->packet_id.send, &pin, false);
-    ASSERT(packet_id_write(&pin, &iv_buffer, false, false));
+    ASSERT(packet_id_write(&opt->packet_id.send, &iv_buffer, false, false));
 
     /* Remainder of IV consists of implicit part (unique per session) */
     ASSERT(buf_write(&iv_buffer, ctx->implicit_iv, ctx->implicit_iv_len));

@@ -201,23 +199,21 @@ openvpn_encrypt_v1(struct buffer *buf, struct buffer work,
                 /* Put packet ID in plaintext buffer */
                 if (packet_id_initialized(&opt->packet_id))
                 {
-                    struct packet_id_net pin;
-                    packet_id_alloc_outgoing(&opt->packet_id.send, &pin, BOOL_CAST(opt->flags & CO_PACKET_ID_LONG_FORM));
-                    ASSERT(packet_id_write(&pin, buf, BOOL_CAST(opt->flags & CO_PACKET_ID_LONG_FORM), true));
+                    ASSERT(packet_id_write(&opt->packet_id.send, buf,
+                            BOOL_CAST(opt->flags & CO_PACKET_ID_LONG_FORM),
+                            true));
                 }
             }
             else if (cipher_kt_mode_ofb_cfb(cipher_kt))
             {
-                struct packet_id_net pin;
                 struct buffer b;
 
                 /* IV and packet-ID required for this mode. */
                 ASSERT(opt->flags & CO_USE_IV);
                 ASSERT(packet_id_initialized(&opt->packet_id));
 
-                packet_id_alloc_outgoing(&opt->packet_id.send, &pin, true);
                 buf_set_write(&b, iv_buf, iv_size);
-                ASSERT(packet_id_write(&pin, &b, true, false));
+                ASSERT(packet_id_write(&opt->packet_id.send, &b, true, false));
             }
             else /* We only support CBC, CFB, or OFB modes right now */
             {

@@ -266,10 +262,7 @@ openvpn_encrypt_v1(struct buffer *buf, struct buffer work,
         {
             if (packet_id_initialized(&opt->packet_id))
             {
-                struct packet_id_net pin;
-                packet_id_alloc_outgoing(&opt->packet_id.send, &pin,
-                        BOOL_CAST(opt->flags & CO_PACKET_ID_LONG_FORM));
-                ASSERT(packet_id_write(&pin, buf,
+                ASSERT(packet_id_write(&opt->packet_id.send, buf,
                         BOOL_CAST(opt->flags & CO_PACKET_ID_LONG_FORM), true));
             }
             if (ctx->hmac)

src/openvpn/packet_id.c +21 -3

index 64d19c5..e095e2e 100644
@@ -325,12 +325,30 @@ packet_id_read(struct packet_id_net *pin, struct buffer *buf, bool long_form)
     return true;
 }
 
+static void
+packet_id_send_update(struct packet_id_send *p, bool long_form)
+{
+    if (!p->time)
+    {
+        p->time = now;
+    }
+    p->id++;
+    if (!p->id)
+    {
+        ASSERT(long_form);
+        p->time = now;
+        p->id = 1;
+    }
+}
+
 bool
-packet_id_write(const struct packet_id_net *pin, struct buffer *buf, bool long_form, bool prepend)
+packet_id_write(struct packet_id_send *p, struct buffer *buf, bool long_form,
+        bool prepend)
 {
-    packet_id_type net_id = htonpid(pin->id);
-    net_time_t net_time = htontime(pin->time);
+    packet_id_send_update(p, long_form);
 
+    const packet_id_type net_id = htonpid(p->id);
+    const net_time_t net_time = htontime(p->time);
     if (prepend)
     {
         if (long_form)

src/openvpn/packet_id.h +12 ~~-23~~

index d4019ff..63b3dcb 100644
@@ -254,7 +254,18 @@ const char *packet_id_persist_print(const struct packet_id_persist *p, struct gc
 
 bool packet_id_read(struct packet_id_net *pin, struct buffer *buf, bool long_form);
 
-bool packet_id_write(const struct packet_id_net *pin, struct buffer *buf, bool long_form, bool prepend);
+/**
+ * Write a packet ID to buf, and update the packet ID state.
+ *
+ * @param p             Packet ID state.
+ * @param buf           Buffer to write the packet ID too
+ * @param long_form     If true, also update and write time_t to buf
+ * @param prepend       If true, prepend to buffer, otherwise apppend.
+ *
+ * @return true if successful, false otherwise.
+ */
+bool packet_id_write(struct packet_id_send *p, struct buffer *buf,
+        bool long_form, bool prepend);
 
 /*
  * Inline functions.

@@ -304,28 +315,6 @@ packet_id_close_to_wrapping(const struct packet_id_send *p)
     return p->id >= PACKET_ID_WRAP_TRIGGER;
 }
 
-/*
- * Allocate an outgoing packet id.
- * Sequence number ranges from 1 to 2^32-1.
- * In long_form, a time_t is added as well.
- */
-static inline void
-packet_id_alloc_outgoing(struct packet_id_send *p, struct packet_id_net *pin, bool long_form)
-{
-    if (!p->time)
-    {
-        p->time = now;
-    }
-    pin->id = ++p->id;
-    if (!pin->id)
-    {
-        ASSERT(long_form);
-        p->time = now;
-        pin->id = p->id = 1;
-    }
-    pin->time = p->time;
-}
-
 static inline bool
 check_timestamp_delta(time_t remote, unsigned int max_delta)
 {

src/openvpn/tls_crypt.c +1 -3

index 8173467..8d58f5e 100644
@@ -96,9 +96,7 @@ tls_crypt_wrap(const struct buffer *src, struct buffer *dst,
 
     /* Get packet ID */
     {
-        struct packet_id_net pin;
-        packet_id_alloc_outgoing(&opt->packet_id.send, &pin, true);
-        packet_id_write(&pin, dst, true, false);
+        ASSERT(packet_id_write(&opt->packet_id.send, dst, true, false));
     }
 
     dmsg(D_PACKET_CONTENT, "TLS-CRYPT WRAP AD: %s",

tests/unit_tests/openvpn/Makefile.am +15 -1

index fafe6b2..c8d7863 100644
@@ -1,6 +1,9 @@
 AUTOMAKE_OPTIONS = foreign
 
-check_PROGRAMS = argv_testdriver buffer_testdriver
+check_PROGRAMS = \
+    argv_testdriver \
+    buffer_testdriver \
+    packet_id_testdriver
 
 if ENABLE_CRYPTO
 check_PROGRAMS += tls_crypt_testdriver

@@ -27,6 +30,17 @@ buffer_testdriver_SOURCES = test_buffer.c mock_msg.c \
 	$(openvpn_srcdir)/buffer.c \
 	$(openvpn_srcdir)/platform.c
 
+packet_id_testdriver_CFLAGS  = @TEST_CFLAGS@ \
+	-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
+	$(OPTIONAL_CRYPTO_CFLAGS)
+packet_id_testdriver_LDFLAGS = @TEST_LDFLAGS@ \
+	$(OPTIONAL_CRYPTO_LIBS)
+packet_id_testdriver_SOURCES = test_packet_id.c mock_msg.c \
+	$(openvpn_srcdir)/buffer.c \
+	$(openvpn_srcdir)/otime.c \
+	$(openvpn_srcdir)/packet_id.c \
+	$(openvpn_srcdir)/platform.c
+
 tls_crypt_testdriver_CFLAGS  = @TEST_CFLAGS@ \
 	-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
 	$(OPTIONAL_CRYPTO_CFLAGS)

tests/unit_tests/openvpn/mock_msg.c +6 -9

index b6f1a46..573e501 100644
@@ -29,9 +29,12 @@
 #endif
 
 #include <stdarg.h>
-#include <stdbool.h>
+#include <stddef.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <setjmp.h>
+#include <cmocka.h>
+
 
 #include "errlevel.h"
 #include "error.h"

@@ -70,14 +73,8 @@ x_msg(const unsigned int flags, const char *format, ...)
 void
 assert_failed(const char *filename, int line, const char *condition)
 {
-    if (condition)
-    {
-        printf("Assertion failed at %s:%d (%s)", filename, line, condition);
-    }
-    else
-    {
-        printf("Assertion failed at %s:%d", filename, line);
-    }
+    mock_assert(false, condition ? condition : "", filename, line);
+    /* Keep compiler happy.  Should not happen, mock_assert() does not return */
     exit(1);
 }

tests/unit_tests/openvpn/test_packet_id.c new file +168 -0

168 lines not on this page · raw message

Signature · 1 line

2.7.4

Steffan Karger

2017-04-13 09:38 UTC #1 lore raw

Quoted 12 lines · Steffan Karger

The functions packet_id_alloc_outgoing() and packet_id_write() were always called in tandem. Instead of forcing the caller to allocate a packet_id_net to do so, merge the two functions. This simplifies the API and reduces the chance on mistakes in the future.
This patch adds unit tests to verify the behaviour of packet_id_write(). Verifying that we assert out correctly required the change to mock_msg.c.
Signed-off-by Steffan Karger <[email protected]...>
---
This patch is for the master branch only.

Quoted 8 lines

 src/openvpn/crypto.c                      |  19 ++--
 src/openvpn/packet_id.c                   |  24 ++++-
 src/openvpn/packet_id.h                   |  35 +++----
 src/openvpn/tls_crypt.c                   |   4 +-
 tests/unit_tests/openvpn/Makefile.am      |  16 ++-
 tests/unit_tests/openvpn/mock_msg.c       |  15 ++-
 tests/unit_tests/openvpn/test_packet_id.c | 168 ++++++++++++++++++++++++++++++
 7 files changed, 229 insertions(+), 52 deletions(-)

Can I tempt anyone into reviewing this patch? Don't get scared by the size - it's mostly adding tests. The function changes are very small.

-Steffan

Steffan Karger

2017-05-05 17:44 UTC #2 lore raw

PATCH v2 1/1 cleanup: merge packet_id_alloc_outgoing() into packet_id_write()

This patch adds unit tests to verify the behaviour of packet_id_write(). Verifying that we assert out correctly required the change to mock_msg.c.

Signed-off-by Steffan Karger <[email protected]...>

---
v2 - rebased onto current master branch

 src/openvpn/crypto.c                      |  20 ++--
 src/openvpn/packet_id.c                   |  24 ++++-
 src/openvpn/packet_id.h                   |  35 +++----
 src/openvpn/tls_crypt.c                   |   6 +-
 tests/unit_tests/openvpn/Makefile.am      |  13 ++-
 tests/unit_tests/openvpn/mock_msg.c       |  15 ++-
 tests/unit_tests/openvpn/test_packet_id.c | 168 ++++++++++++++++++++++++++++++
 7 files changed, 228 insertions(+), 53 deletions(-)
 create mode 100644 tests/unit_tests/openvpn/test_packet_id.c

src/openvpn/crypto.c +8 ~~-12~~

index 7b00601..f5250ac 100644
@@ -85,7 +85,6 @@ openvpn_encrypt_aead(struct buffer *buf, struct buffer work,
     /* Prepare IV */
     {
         struct buffer iv_buffer;
-        struct packet_id_net pin;
         uint8_t iv[OPENVPN_MAX_IV_LENGTH] = {0};
         const int iv_len = cipher_ctx_iv_length(ctx->cipher);

@@ -94,8 +93,7 @@ openvpn_encrypt_aead(struct buffer *buf, struct buffer work,
         buf_set_write(&iv_buffer, iv, iv_len);
 
         /* IV starts with packet id to make the IV unique for packet */
-        packet_id_alloc_outgoing(&opt->packet_id.send, &pin, false);
-        ASSERT(packet_id_write(&pin, &iv_buffer, false, false));
+        ASSERT(packet_id_write(&opt->packet_id.send, &iv_buffer, false, false));
 
         /* Remainder of IV consists of implicit part (unique per session) */
         ASSERT(buf_write(&iv_buffer, ctx->implicit_iv, ctx->implicit_iv_len));

@@ -195,22 +193,20 @@ openvpn_encrypt_v1(struct buffer *buf, struct buffer work,
                 /* Put packet ID in plaintext buffer */
                 if (packet_id_initialized(&opt->packet_id))
                 {
-                    struct packet_id_net pin;
-                    packet_id_alloc_outgoing(&opt->packet_id.send, &pin, BOOL_CAST(opt->flags & CO_PACKET_ID_LONG_FORM));
-                    ASSERT(packet_id_write(&pin, buf, BOOL_CAST(opt->flags & CO_PACKET_ID_LONG_FORM), true));
+                    ASSERT(packet_id_write(&opt->packet_id.send, buf,
+                                           opt->flags & CO_PACKET_ID_LONG_FORM,
+                                           true));
                 }
             }
             else if (cipher_kt_mode_ofb_cfb(cipher_kt))
             {
-                struct packet_id_net pin;
                 struct buffer b;
 
                 /* packet-ID required for this mode. */
                 ASSERT(packet_id_initialized(&opt->packet_id));
 
-                packet_id_alloc_outgoing(&opt->packet_id.send, &pin, true);
                 buf_set_write(&b, iv_buf, iv_size);
-                ASSERT(packet_id_write(&pin, &b, true, false));
+                ASSERT(packet_id_write(&opt->packet_id.send, &b, true, false));
             }
             else /* We only support CBC, CFB, or OFB modes right now */
             {

@@ -257,9 +253,9 @@ openvpn_encrypt_v1(struct buffer *buf, struct buffer work,
         {
             if (packet_id_initialized(&opt->packet_id))
             {
-                struct packet_id_net pin;
-                packet_id_alloc_outgoing(&opt->packet_id.send, &pin, BOOL_CAST(opt->flags & CO_PACKET_ID_LONG_FORM));
-                ASSERT(packet_id_write(&pin, buf, BOOL_CAST(opt->flags & CO_PACKET_ID_LONG_FORM), true));
+                ASSERT(packet_id_write(&opt->packet_id.send, buf,
+                        BOOL_CAST(opt->flags & CO_PACKET_ID_LONG_FORM),
+                        true));
             }
             if (ctx->hmac)
             {

src/openvpn/packet_id.c +21 -3

index e34c228..5175fb0 100644
@@ -325,12 +325,30 @@ packet_id_read(struct packet_id_net *pin, struct buffer *buf, bool long_form)
     return true;
 }
 
+static void
+packet_id_send_update(struct packet_id_send *p, bool long_form)
+{
+    if (!p->time)
+    {
+        p->time = now;
+    }
+    p->id++;
+    if (!p->id)
+    {
+        ASSERT(long_form);
+        p->time = now;
+        p->id = 1;
+    }
+}
+
 bool
-packet_id_write(const struct packet_id_net *pin, struct buffer *buf, bool long_form, bool prepend)
+packet_id_write(struct packet_id_send *p, struct buffer *buf, bool long_form,
+        bool prepend)
 {
-    packet_id_type net_id = htonpid(pin->id);
-    net_time_t net_time = htontime(pin->time);
+    packet_id_send_update(p, long_form);
 
+    const packet_id_type net_id = htonpid(p->id);
+    const net_time_t net_time = htontime(p->time);
     if (prepend)
     {
         if (long_form)

src/openvpn/packet_id.h +12 ~~-23~~

index ecc25a6..109e56a 100644
@@ -254,7 +254,18 @@ const char *packet_id_persist_print(const struct packet_id_persist *p, struct gc
 
 bool packet_id_read(struct packet_id_net *pin, struct buffer *buf, bool long_form);
 
-bool packet_id_write(const struct packet_id_net *pin, struct buffer *buf, bool long_form, bool prepend);
+/**
+ * Write a packet ID to buf, and update the packet ID state.
+ *
+ * @param p             Packet ID state.
+ * @param buf           Buffer to write the packet ID too
+ * @param long_form     If true, also update and write time_t to buf
+ * @param prepend       If true, prepend to buffer, otherwise apppend.
+ *
+ * @return true if successful, false otherwise.
+ */
+bool packet_id_write(struct packet_id_send *p, struct buffer *buf,
+        bool long_form, bool prepend);
 
 /*
  * Inline functions.

@@ -304,28 +315,6 @@ packet_id_close_to_wrapping(const struct packet_id_send *p)
     return p->id >= PACKET_ID_WRAP_TRIGGER;
 }
 
-/*
- * Allocate an outgoing packet id.
- * Sequence number ranges from 1 to 2^32-1.
- * In long_form, a time_t is added as well.
- */
-static inline void
-packet_id_alloc_outgoing(struct packet_id_send *p, struct packet_id_net *pin, bool long_form)
-{
-    if (!p->time)
-    {
-        p->time = now;
-    }
-    pin->id = ++p->id;
-    if (!pin->id)
-    {
-        ASSERT(long_form);
-        p->time = now;
-        pin->id = p->id = 1;
-    }
-    pin->time = p->time;
-}
-
 static inline bool
 check_timestamp_delta(time_t remote, unsigned int max_delta)
 {

src/openvpn/tls_crypt.c +1 -5

index e2fdbed..e47d25c 100644
@@ -98,11 +98,7 @@ tls_crypt_wrap(const struct buffer *src, struct buffer *dst,
          format_hex(BPTR(src), BLEN(src), 80, &gc));
 
     /* Get packet ID */
-    {
-        struct packet_id_net pin;
-        packet_id_alloc_outgoing(&opt->packet_id.send, &pin, true);
-        packet_id_write(&pin, dst, true, false);
-    }
+    ASSERT(packet_id_write(&opt->packet_id.send, dst, true, false));
 
     dmsg(D_PACKET_CONTENT, "TLS-CRYPT WRAP AD: %s",
          format_hex(BPTR(dst), BLEN(dst), 0, &gc));

tests/unit_tests/openvpn/Makefile.am +12 -1

index b902b20..5d7123e 100644
@@ -3,7 +3,7 @@ AUTOMAKE_OPTIONS = foreign
 check_PROGRAMS=
 
 if HAVE_LD_WRAP_SUPPORT
-check_PROGRAMS += argv_testdriver buffer_testdriver
+check_PROGRAMS += argv_testdriver buffer_testdriver packet_id_testdriver
 endif
 
 if ENABLE_CRYPTO

@@ -31,6 +31,17 @@ buffer_testdriver_SOURCES = test_buffer.c mock_msg.c \
 	$(openvpn_srcdir)/buffer.c \
 	$(openvpn_srcdir)/platform.c
 
+packet_id_testdriver_CFLAGS  = @TEST_CFLAGS@ \
+	-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
+	$(OPTIONAL_CRYPTO_CFLAGS)
+packet_id_testdriver_LDFLAGS = @TEST_LDFLAGS@ \
+	$(OPTIONAL_CRYPTO_LIBS)
+packet_id_testdriver_SOURCES = test_packet_id.c mock_msg.c \
+	$(openvpn_srcdir)/buffer.c \
+	$(openvpn_srcdir)/otime.c \
+	$(openvpn_srcdir)/packet_id.c \
+	$(openvpn_srcdir)/platform.c
+
 tls_crypt_testdriver_CFLAGS  = @TEST_CFLAGS@ \
 	-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
 	$(OPTIONAL_CRYPTO_CFLAGS)

tests/unit_tests/openvpn/mock_msg.c +6 -9

index eb0d5e9..060588f 100644
@@ -29,9 +29,12 @@
 #endif
 
 #include <stdarg.h>
-#include <stdbool.h>
+#include <stddef.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <setjmp.h>
+#include <cmocka.h>
+
 
 #include "errlevel.h"
 #include "error.h"

@@ -70,14 +73,8 @@ x_msg(const unsigned int flags, const char *format, ...)
 void
 assert_failed(const char *filename, int line, const char *condition)
 {
-    if (condition)
-    {
-        printf("Assertion failed at %s:%d (%s)", filename, line, condition);
-    }
-    else
-    {
-        printf("Assertion failed at %s:%d", filename, line);
-    }
+    mock_assert(false, condition ? condition : "", filename, line);
+    /* Keep compiler happy.  Should not happen, mock_assert() does not return */
     exit(1);
 }

tests/unit_tests/openvpn/test_packet_id.c new file +168 -0

168 lines not on this page · raw message

Signature · 1 line

2.7.4

Gert Doering

2017-05-05 19:11 UTC #3 lore raw

ACK, thanks, plus

20:39 <@dazo> cron2: you can add ACK on me too on that one, I believe it is the
same we've already reviewed

"Stared at code", plus "passed all my client tests" plus "adds cmocka tests which are really useful for that stuff" (and they pass :) ).

.. and this is a real improvement to the code - it just removes lines, and intermediate variables. Too much magic, that is not needed.

Your patch has been applied to the master and release/2.4 branch.

Backporting to release/2.3 is not a trivial matter of "there is whitespace", but it seems infrastructure is missing (CRYPT_ERROR()), and the grand unified reformatting is too much for git to grok the differences in context. It should be fairly easy to do by hand, though.

commit a87e1431baccd49a9344cfc63ab7446c4317fa2f (master) commit 653d39192297880b4fbc6a53cce6f8692782e1b7 (release/2.4)

Author: Steffan Karger
Date:   Fri May 5 19:44:51 2017 +0200

     cleanup: merge packet_id_alloc_outgoing() into packet_id_write()

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

--
kind regards,

Gert Doering