Minor AEAD patch cleanup

PATCH In review Steffan Karger 3 participants 7 messages Feb 21, 2016 – Feb 22, 2016

Share & tools

Steffan Karger

author

2016-02-21 01:08 UTC #0 lore raw

PATCH 1/2 Minor AEAD patch cleanup

* Remove stale function declaration.
This slipped into the AEAD cipher modes patch, but the function is now implemented as a static function is ssl.c.
* Add ASSERT() to ensure frame is not NULL.
* Fix "ENCRYPT TO" log message in openvpn_encrypt_aead().

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

---
 src/openvpn/crypto.c |  5 +++--
 src/openvpn/crypto.h | 11 -----------
 2 files changed, 3 insertions(+), 13 deletions(-)

src/openvpn/crypto.c +3 -2

index e8ab27a..6d9c119 100644
@@ -161,10 +161,10 @@ openvpn_encrypt_aead (struct buffer *buf, struct buffer work,
   /* Write authentication tag */
   ASSERT (cipher_ctx_get_tag (ctx->cipher, mac_out, mac_len));
 
-  dmsg (D_PACKET_CONTENT, "ENCRYPT TO: %s", format_hex (BPTR (buf), BLEN (buf), 80, &gc));
-
   *buf = work;
 
+  dmsg (D_PACKET_CONTENT, "ENCRYPT TO: %s", format_hex (BPTR (buf), BLEN (buf), 80, &gc));
+
 cleanup:
   gc_free (&gc);
   return;

@@ -397,6 +397,7 @@ openvpn_decrypt_aead (struct buffer *buf, struct buffer work,
   gc_init (&gc);
 
   ASSERT (opt);
+  ASSERT (frame);
   ASSERT (buf->len > 0);
   ASSERT (ctx->cipher);
   ASSERT (cipher_kt_mode_aead (cipher_kt));

src/openvpn/crypto.h +0 ~~-11~~

index 14b6ab7..d3e08c1 100644
@@ -311,17 +311,6 @@ void free_key_ctx (struct key_ctx *ctx);
 
 void free_key_ctx_bi (struct key_ctx_bi *ctx);
 
-/**
- * Set an implicit IV for a key context.
- *
- * @param ctx	The key context to update
- * @param iv	The implicit IV to load into ctx
- * @param len	The length (in bytes) of iv
- */
-bool key_ctx_set_implicit_iv (struct key_ctx *ctx, const uint8_t *iv,
-    size_t len);
-
-
 
 /**************************************************************************/
 /** @name Functions for performing security operations on data channel packets

Signature · 1 line

2.5.0

Steffan Karger

author

2016-02-21 01:08 UTC #1 lore raw

PATCH 2/2 Clean up get_tls_handhake_key()

This function contained *much* more code than required. This commit cleans up the function:
* Merge the handling of inline and non-inline code.
* Don't double-check key.2, must_have_n_keys() already checks that
* Remove stale references to 'passphrase' - we no longer support those

This commit should not change any behaviour except for log messages.

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

---
 src/openvpn/crypto.c | 44 ++++++++------------------------------------
 1 file changed, 8 insertions(+), 36 deletions(-)

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

index 6d9c119..308b886 100644
@@ -1080,57 +1080,29 @@ test_crypto (struct crypto_options *co, struct frame* frame)
 void
 get_tls_handshake_key (const struct key_type *key_type,
 		       struct key_ctx_bi *ctx,
-		       const char *passphrase_file,
+		       const char *key_file,
 		       const int key_direction,
 		       const unsigned int flags)
 {
-  if (passphrase_file && key_type->hmac_length)
+  if (key_file)
     {
       struct key2 key2;
       struct key_direction_state kds;
 
       if (flags & GHK_INLINE)
 	{
-	  /* key was specified inline, key text is in passphrase_file */
-	  read_key_file (&key2, passphrase_file, RKF_INLINE|RKF_MUST_SUCCEED);
-
-	  /* succeeded? */
-	  if (key2.n == 2)
-	    msg (M_INFO, "Control Channel Authentication: tls-auth using INLINE static key file");
-	  else
-	    msg (M_FATAL, "INLINE tls-auth file lacks the requisite 2 keys");
+	  read_key_file (&key2, key_file, RKF_INLINE|RKF_MUST_SUCCEED);
 	}
       else
-      {
-	/* first try to parse as an OpenVPN static key file */
-	read_key_file (&key2, passphrase_file, 0);
-
-	/* succeeded? */
-	if (key2.n == 2)
-	  {
-	    msg (M_INFO,
-		 "Control Channel Authentication: using '%s' as a " PACKAGE_NAME " static key file",
-		 passphrase_file);
-	  }
-	else
-	  {
-	    CLEAR (key2);
-
-	    /* failed, now bail out */
+	{
+	  read_key_file (&key2, key_file, RKF_MUST_SUCCEED);
+	}
 
-	    msg (M_ERR,
-		 "Control Channel Authentication: File '%s' does not have OpenVPN Static Key format. "
-		 "Using free-form passphrase file is not supported anymore",
-		 passphrase_file);
-	  }
-      }
       /* handle key direction */
-
       key_direction_state_init (&kds, key_direction);
-      must_have_n_keys (passphrase_file, "tls-auth", &key2, kds.need_keys);
-
-      /* initialize hmac key in both directions */
+      must_have_n_keys (key_file, "tls-auth", &key2, kds.need_keys);
 
+      /* initialize key in both directions */
       init_key_ctx (&ctx->encrypt, &key2.keys[kds.out_key], key_type, OPENVPN_OP_ENCRYPT,
 		    "Outgoing Control Channel Authentication");
       init_key_ctx (&ctx->decrypt, &key2.keys[kds.in_key], key_type, OPENVPN_OP_DECRYPT,

Signature · 1 line

2.5.0

Gert Doering

2016-02-21 12:28 UTC #6 lore raw

In reply to Steffan Karger

ACK "straightforward enough" ;-)

Your patch has been applied to the master branch.

commit b3560c98d7e4877f3ff6283dde1751654e6f7d6d

Author: Steffan Karger
Date:   Sun Feb 21 02:08:11 2016 +0100

     Minor AEAD patch cleanup

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/11233
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Arne Schwabe

2016-02-22 13:50 UTC #2 lore raw

Re: [Openvpn-devel] [PATCH 2/2] Clean up get_tls_handhake_key()

In reply to Steffan Karger

Am 21.02.16 um 02:08 schrieb Steffan Karger:

Quoted 9 lines

This function contained *much* more code than required. This commit cleans up the function:
* Merge the handling of inline and non-inline code.
* Don't double-check key.2, must_have_n_keys() already checks that
* Remove stale references to 'passphrase' - we no longer support those
This commit should not change any behaviour except for log messages.

I would like to keep that error message about free-form files in 2.4 since 2.4 is the first version not to warn but also to bail out on this change with an error message.
We can change the error message not have the filename in it, something like "tls-auth option does have not static key format ... "

Otherwise it gets an ACK from me.

Arne

Steffan Karger

author

2016-02-22 14:24 UTC #3 lore raw

PATCH v2 2/2 Clean up get_tls_handhake_key()

This function has *much* more code than required. This commit cleans up the function:
* Merge the handling of inline and non-inline code.
* Don't double-check key.2, since must_have_n_keys() already does that (but keep the message about dropped passphrase support in 2.4).
* Remove stale references to 'passphrase' - we no longer support those

This commit should not change any behaviour except for log messages.

v2: Leave message about dropped passphrase support in place - this option
was dropped in 2.4, so it is indeed better to be clear about it.

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

---
 src/openvpn/crypto.c | 46 ++++++++++++----------------------------------
 1 file changed, 12 insertions(+), 34 deletions(-)

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

index 6d9c119..bd86679 100644
@@ -1080,57 +1080,35 @@ test_crypto (struct crypto_options *co, struct frame* frame)
 void
 get_tls_handshake_key (const struct key_type *key_type,
 		       struct key_ctx_bi *ctx,
-		       const char *passphrase_file,
+		       const char *key_file,
 		       const int key_direction,
 		       const unsigned int flags)
 {
-  if (passphrase_file && key_type->hmac_length)
+  if (key_file)
     {
       struct key2 key2;
       struct key_direction_state kds;
 
       if (flags & GHK_INLINE)
 	{
-	  /* key was specified inline, key text is in passphrase_file */
-	  read_key_file (&key2, passphrase_file, RKF_INLINE|RKF_MUST_SUCCEED);
-
-	  /* succeeded? */
-	  if (key2.n == 2)
-	    msg (M_INFO, "Control Channel Authentication: tls-auth using INLINE static key file");
-	  else
-	    msg (M_FATAL, "INLINE tls-auth file lacks the requisite 2 keys");
+	  read_key_file (&key2, key_file, RKF_INLINE|RKF_MUST_SUCCEED);
 	}
       else
-      {
-	/* first try to parse as an OpenVPN static key file */
-	read_key_file (&key2, passphrase_file, 0);
+	{
+	  read_key_file (&key2, key_file, RKF_MUST_SUCCEED);
+	}
 
-	/* succeeded? */
-	if (key2.n == 2)
+	if (key2.n != 2)
 	  {
-	    msg (M_INFO,
-		 "Control Channel Authentication: using '%s' as a " PACKAGE_NAME " static key file",
-		 passphrase_file);
+	    msg (M_ERR, "Control Channel Authentication: File '%s' does not "
+		"have OpenVPN Static Key format.  Using free-form passphrase "
+		"file is not supported anymore.", key_file);
 	  }
-	else
-	  {
-	    CLEAR (key2);
-
-	    /* failed, now bail out */
-
-	    msg (M_ERR,
-		 "Control Channel Authentication: File '%s' does not have OpenVPN Static Key format. "
-		 "Using free-form passphrase file is not supported anymore",
-		 passphrase_file);
-	  }
-      }
       /* handle key direction */
-
       key_direction_state_init (&kds, key_direction);
-      must_have_n_keys (passphrase_file, "tls-auth", &key2, kds.need_keys);
-
-      /* initialize hmac key in both directions */
+      must_have_n_keys (key_file, "tls-auth", &key2, kds.need_keys);
 
+      /* initialize key in both directions */
       init_key_ctx (&ctx->encrypt, &key2.keys[kds.out_key], key_type, OPENVPN_OP_ENCRYPT,
 		    "Outgoing Control Channel Authentication");
       init_key_ctx (&ctx->decrypt, &key2.keys[kds.in_key], key_type, OPENVPN_OP_DECRYPT,

Signature · 1 line

2.5.0

Arne Schwabe

2016-02-22 14:59 UTC #4 lore raw

Re: [Openvpn-devel] [PATCH 2/2 v2] Clean up get_tls_handhake_key()

Am 22.02.16 um 15:24 schrieb Steffan Karger:

Quoted 11 lines

This function has *much* more code than required. This commit cleans up the function:
* Merge the handling of inline and non-inline code.
* Don't double-check key.2, since must_have_n_keys() already does that (but keep the message about dropped passphrase support in 2.4).
* Remove stale references to 'passphrase' - we no longer support those
This commit should not change any behaviour except for log messages.
v2: Leave message about dropped passphrase support in place - this option
was dropped in 2.4, so it is indeed better to be clear about it.

ACK. Thanks for keeping the log message.

Arne

Gert Doering

2016-02-22 15:21 UTC #5 lore raw

[Openvpn-devel] [PATCH applied] Re: Clean up get_tls_handhake_key()

In reply to Steffan Karger

Your patch has been applied to the master branch.

commit 463ea2bc90555efdcd8b2591a18aad2d47eb1d25

Author: Steffan Karger
Date:   Mon Feb 22 15:24:06 2016 +0100

     Clean up get_tls_handhake_key()

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Arne Schwabe <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/11238
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering