diff mbox series

[Openvpn-devel,v3,3/7] Add pem_read_key_file variant that allows a random key

Message ID 20190510121114.30468-4-arne@rfc2549.org
State Superseded
Delegated to: David Sommerseth
Headers
Series Auth token patches v3 |

Commit Message

Arne Schwabe May 10, 2019, 2:11 a.m. UTC 
  From: Arne Schwabe <arne@openvpn.net>

This is useful for features that can use enither a persistent
or an ephemeral key.

Patch V2: Move the functionality of generating a random key into a
          separate function that acts as wrapper for pem_read_key_file
---
 src/openvpn/crypto.c | 22 ++++++++++++++++++++++
 src/openvpn/crypto.h | 15 +++++++++++++++
 2 files changed, 37 insertions(+)

Comments

David Sommerseth June 7, 2019, 10:46 a.m. UTC | #1 
On 10/05/2019 14:11, Arne Schwabe wrote:
> From: Arne Schwabe <arne@openvpn.net>
> 
> This is useful for features that can use enither a persistent
> or an ephemeral key.
> 
> Patch V2: Move the functionality of generating a random key into a
>           separate function that acts as wrapper for pem_read_key_file
> ---
>  src/openvpn/crypto.c | 22 ++++++++++++++++++++++
>  src/openvpn/crypto.h | 15 +++++++++++++++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
> index eb56421b..0e17c351 100644
> --- a/src/openvpn/crypto.c
> +++ b/src/openvpn/crypto.c
> @@ -1892,6 +1892,28 @@ cleanup:
>      return;
>  }
>  
> +bool
> +load_pem_file_or_ephemeral_key(struct buffer *key, const char *pem_name,
> +                               const char *key_file, const char *key_inline)
> +{
> +    if (key_file == NULL)

I don't like this approach.  Why can't we have generate_ephemeral_key()
basically doing the keyfile == NULL part below and put the "which key to use"
logic in the only caller I could see in auth_token_init_secret() [added in a
later patch] ?  This makes the code clearer:

-----------------------------------------------------------------

void
auth_token_init_secret(struct key_ctx *key_ctx, const char *key_file,
                       const char *key_inline)
{
   /* .... */
   bool key_loaded = false;
   if (key_file)
   {
	key_loaded = read_pem_key(...);
   }
   else
   {
        key_loaded = generate_ephemeral_key(...);
   }
   if (!key_loaded)
   {
        msg(M_FATAL, ....);
   }
   /* .... */
-----------------------------------------------------------------

You could also make it more compact, but not sure it's worth it here, as these
functions requires a few longer argument lists:

   bool key_loaded = (key_file ? read_pem_key(...)
                               : generate_ephemeral_key(...));


The point is: This makes read_pem_key() and generate_ephemeral_key() do only
one simple and predictable task.  The load_pem_file_or_ephemeral_key() does
one of two things, depending on the argument.  And since I only see we have
only a single caller to the load_pem_file_or_ephemeral_key(), there's nothing
to save on code reuse by wrapping it up like this.


> +    {
> +        msg(M_INFO, "Using random %s.", pem_name);
> +        uint8_t rand[BCAP(key)];
> +        if (!rand_bytes(rand, BCAP(key)))
> +        {
> +            msg(M_WARN, "ERROR: could not generate random key");
> +            return false;
> +        }
> +        buf_write(key, rand, BCAP(key));
> +        return true;
> +    }
> +    else
> +    {
> +        return read_pem_key_file(key, pem_name, key_file, key_inline);
> +    }
> +}
> +
diff mbox series

Patch

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index eb56421b..0e17c351 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -1892,6 +1892,28 @@  cleanup:
     return;
 }
 
+bool
+load_pem_file_or_ephemeral_key(struct buffer *key, const char *pem_name,
+                               const char *key_file, const char *key_inline)
+{
+    if (key_file == NULL)
+    {
+        msg(M_INFO, "Using random %s.", pem_name);
+        uint8_t rand[BCAP(key)];
+        if (!rand_bytes(rand, BCAP(key)))
+        {
+            msg(M_WARN, "ERROR: could not generate random key");
+            return false;
+        }
+        buf_write(key, rand, BCAP(key));
+        return true;
+    }
+    else
+    {
+        return read_pem_key_file(key, pem_name, key_file, key_inline);
+    }
+}
+
 bool
 read_pem_key_file(struct buffer *key, const char *pem_name,
                   const char *key_file, const char *key_inline)
diff --git a/src/openvpn/crypto.h b/src/openvpn/crypto.h
index c5947483..4ae02571 100644
--- a/src/openvpn/crypto.h
+++ b/src/openvpn/crypto.h
@@ -430,6 +430,21 @@  unsigned int crypto_max_overhead(void);
 void
 write_pem_key_file(const char *filename, const char *pem_name);
 
+/**
+ * Read key material from a PEM encoded files into the key structure or if
+ * the filename is not passed generate a ephemeral key
+ *
+ * @param key           the key structure that will hold the key material
+ * @param pem_name      the name used in the pem encoding start/end lines
+ * @param key_file      name of the file to read or NULL if an ephermal key
+ *                      should be generated
+ * @param key_inline    a string holding the data in case of an inline key
+ * @return              true if reading into key was successful or generated
+ */
+bool
+load_pem_file_or_ephemeral_key(struct buffer *key, const char *pem_name,
+                               const char *key_file, const char *key_inline);
+
 /**
  * Read key material from a PEM encoded files into the key structure
  * @param key           the key structure that will hold the key material