diff mbox series

[Openvpn-devel,04/28] Add documentation for swap_hmac function

Message ID 20220422134038.3801239-5-arne@rfc2549.org
State Accepted
Headers
Series Stateless three-way handshake and control channel improvements |

Commit Message

Arne Schwabe April 22, 2022, 3:40 a.m. UTC 
  Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/ssl.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

Comments

Gert Doering April 22, 2022, 9:05 a.m. UTC | #1 
Hi,

On Fri, Apr 22, 2022 at 03:40:33PM +0200, Arne Schwabe wrote:
> +/**
> + * Move a packet authentication HMAC + related fields to or from the front
> + * of the buffer so it can be processed by encrypt/decrypt.
> + *
> + * Turning the on wire format that starts with the opcode to a format
> + * that starts with the hmac
> + * e.g. "onwire" [opcode + packet id] [hmac] [remainder of packed]
> + *
> + *
> + *    "internal" [hmac] [opcode + packet id] [remainer of packet]
> + *

I was about to merge this as "it is easy, and does not change code", but
if we add documentation, it should be correct :-) - and this schematic
does not match my understanding of the actual code - which seems to
swap

  [opcode + session id] [hmac + packet id]

could you double check that?  The size of the "things it swaps" is

         /* hmac + packet_id (8 bytes) */
         const int hmac_size = hmac_ctx_size(ctx->hmac) + packet_id_size(true);
 
         /* opcode + session_id */
         const int osid_size = 1 + SID_SIZE;

... so that would make it "[hmac + packet id]" not "[hmac]" and
"[opcode + packet id]"...?!

gert
Gert Doering April 24, 2022, 8:10 a.m. UTC | #2 
Hi,

On Fri, Apr 22, 2022 at 09:05:34PM +0200, Gert Doering wrote:
> I was about to merge this as "it is easy, and does not change code", but
> if we add documentation, it should be correct :-) - and this schematic
> does not match my understanding of the actual code - which seems to
> swap
> 
>   [opcode + session id] [hmac + packet id]
> 
> could you double check that?  The size of the "things it swaps" is

Ah, found it.  This comment gets changed in 05/28, to the values I 
also figured out.

So, ACK on this one, will merge together with 05/ ASAPish (maybe I'll
move the "fix comment hunk" from 05 to 04).

gert
Gert Doering April 24, 2022, 10:41 a.m. UTC | #3 
Acked-by: Gert Doering <gert@greenie.muc.de>

I have taken the hunks from 05/28 that relate to swap_hmac (change
the comment, add byte sizes to the "osid_size" comment) into this
one, so this one is now factually correct, and 05(bis)/28 does no longer
contain "swap_hmac" related changes.

As this brings no code change, I've just verified that the comment
matches my understanding of the function (it now does), not tested
anything.

Your patch has been applied to the master branch.

commit e061ec5800af746cb86ccbb9b571e1cb18f8ad99
Author: Arne Schwabe
Date:   Fri Apr 22 15:40:33 2022 +0200

     Add documentation for swap_hmac function

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20220422134038.3801239-5-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24177.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 4ebf5acc2..f58f3b727 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1366,10 +1366,7 @@  tls_multi_free(struct tls_multi *multi, bool clear)
 }
 
 
-/*
- * Move a packet authentication HMAC + related fields to or from the front
- * of the buffer so it can be processed by encrypt/decrypt.
- */
+
 
 /*
  * Dependent on hmac size, opcode size, and session_id size.
@@ -1377,6 +1374,23 @@  tls_multi_free(struct tls_multi *multi, bool clear)
  */
 #define SWAP_BUF_SIZE 256
 
+/**
+ * Move a packet authentication HMAC + related fields to or from the front
+ * of the buffer so it can be processed by encrypt/decrypt.
+ *
+ * Turning the on wire format that starts with the opcode to a format
+ * that starts with the hmac
+ * e.g. "onwire" [opcode + packet id] [hmac] [remainder of packed]
+ *
+ *
+ *    "internal" [hmac] [opcode + packet id] [remainer of packet]
+ *
+ *  @param buf      the buffer the swap operation is executed on
+ *  @param incoming determines the direction of the swap
+ *  @param co       crypto options, determines the hmac to use in the swap
+ *
+ *  @return         if the swap was successful (buf was large enough)
+ */
 static bool
 swap_hmac(struct buffer *buf, const struct crypto_options *co, bool incoming)
 {