diff mbox series

[Openvpn-devel,v2,2/6] Allow pem_read_key_file to generate a random key

Message ID 20190122150333.1061-2-arne@rfc2549.org
State Superseded
Headers
Series [Openvpn-devel,v2,1/6] Rename tls_crypt_v2_read_keyfile into generic pem_read_key_file |

Commit Message

Arne Schwabe Jan. 22, 2019, 4:03 a.m. UTC 
  From: Arne Schwabe <arne@openvpn.net>

This is useful for features that can use either a persistent
or an ephemeral key.
---
 src/openvpn/crypto.c    | 23 ++++++++++++++++++++---
 src/openvpn/crypto.h    |  4 +++-
 src/openvpn/tls_crypt.c |  5 +++--
 3 files changed, 26 insertions(+), 6 deletions(-)

Comments

David Sommerseth Feb. 8, 2019, 1:30 a.m. UTC | #1 
On 22/01/2019 16:03, Arne Schwabe wrote:
> From: Arne Schwabe <arne@openvpn.net>
> 
> This is useful for features that can use either a persistent
> or an ephemeral key.

Agreed, this is valuable.  But I generally don't like "hiding" this feature
behind an argument to {read,write}_pem_key_file().  I prefer having functions
doing explicit things, not "I can do this, or this, or this" depending on the
arguments given.

Rather add a generate_random_pem_key() which does just that.

> ---
>  src/openvpn/crypto.c    | 23 ++++++++++++++++++++---
>  src/openvpn/crypto.h    |  4 +++-
>  src/openvpn/tls_crypt.c |  5 +++--
>  3 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
> index ff9dbfdc..68a28dee 100644
> --- a/src/openvpn/crypto.c
> +++ b/src/openvpn/crypto.c
> @@ -1885,13 +1885,30 @@ cleanup:
>  
>  bool
>  read_pem_key_file(struct buffer *key, const char *pem_name,
> -                  const char *key_file, const char *key_inline)
> +                  const char *key_file, const char *key_inline,
> +                  bool allow_random)
>  {
>      bool ret = false;
>      struct buffer key_pem = { 0 };
>      struct gc_arena gc = gc_new();
>  
> -    if (strcmp(key_file, INLINE_FILE_TAG))
> +
> +    if (allow_random && 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");
> +        }
> +        else
> +        {
> +            buf_write(key, rand, BCAP(key));
> +            ret = true;
> +        }

The if (rand_bytes(rand, BCAP(key))) logic could be improved when splitting it
into a separate function.

    if (!rand_bytes(rand, BCAP(key)))
    {
         msg(...);
	return false;
    }
    buf_write(...);
    return true;


[...]

> -    if (strcmp(key_file, INLINE_FILE_TAG))
> +    if (key_file && strcmp(key_file, INLINE_FILE_TAG))

Is this fixing a bug?  I'd recommend putting such fixes in a separate commit.
David Sommerseth Feb. 15, 2019, 9:40 a.m. UTC | #2 
On 08/02/2019 15:50, Arne Schwabe wrote:
> Am 08.02.19 um 13:30 schrieb David Sommerseth:
>> On 22/01/2019 16:03, Arne Schwabe wrote:
>>> From: Arne Schwabe <arne@openvpn.net>
>>>
>>> This is useful for features that can use either a persistent
>>> or an ephemeral key.
>> Agreed, this is valuable.  But I generally don't like "hiding" this feature
>> behind an argument to {read,write}_pem_key_file().  I prefer having functions
>> doing explicit things, not "I can do this, or this, or this" depending on the
>> arguments given.
>>
>> Rather add a generate_random_pem_key() which does just that.
>>
> That name would completely confusing that is not what it does. It is
> more really load a key from a file or generate one on the fly.
> 
> Should add a small wrapper function that is something
> load_key_or_random? I also don't really want to rename the function
> read_pem_key_file as that is the main function of it. Return the
> ephermal key is just for function that can work with that.

What I find confusing is:


     if (!read_pem_key_file(&client_key, tls_crypt_v2_cli_pem_name,
                            key_file, key_inline, false))

Flip that 'false' to 'true' and you get a different behaviour if key_file == NULL.

In this case I would prefer to have the "which key will I use" logic in
auth_token_init_secret().  In the following patch 3/6, you have this:

     if (!read_pem_key_file(&server_secret_key, auth_token_pem_name,
                            key_file, key_inline, true))

I would prefer:

    if (NULL == key_file)
    {
        generate_key_file(...);
	....
    }
    else if (!read_pem_key_file(&server_secret_key, auth_token_pem_name,
                            key_file, key_inline))
    {
       ....
    }

This is much more explicit in the behaviour when reading the code, you don't
need to parse through the arguments of read_pem_key_file() to understand how
it behaves.

This will also not requiring touching tls_crypt_v2_init_client_key() or
read_pem_key_file() at all.  So this change would even be less intrusive.


>>> -    if (strcmp(key_file, INLINE_FILE_TAG))
>>> +    if (key_file && strcmp(key_file, INLINE_FILE_TAG))
>> Is this fixing a bug?  I'd recommend putting such fixes in a separate commit.
> 
> No, key_file can now be null since in this case we generate the random
> key material.

Fine, then it makes sense where it is :)
diff mbox series

Patch

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index ff9dbfdc..68a28dee 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -1885,13 +1885,30 @@  cleanup:
 
 bool
 read_pem_key_file(struct buffer *key, const char *pem_name,
-                  const char *key_file, const char *key_inline)
+                  const char *key_file, const char *key_inline,
+                  bool allow_random)
 {
     bool ret = false;
     struct buffer key_pem = { 0 };
     struct gc_arena gc = gc_new();
 
-    if (strcmp(key_file, INLINE_FILE_TAG))
+
+    if (allow_random && 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");
+        }
+        else
+        {
+            buf_write(key, rand, BCAP(key));
+            ret = true;
+        }
+        goto cleanup;
+    }
+    else if (strcmp(key_file, INLINE_FILE_TAG))
     {
         key_pem = buffer_read_from_file(key_file, &gc);
         if (!buf_valid(&key_pem))
@@ -1914,7 +1931,7 @@  read_pem_key_file(struct buffer *key, const char *pem_name,
 
     ret = true;
 cleanup:
-    if (strcmp(key_file, INLINE_FILE_TAG))
+    if (key_file && strcmp(key_file, INLINE_FILE_TAG))
     {
         buf_clear(&key_pem);
     }
diff --git a/src/openvpn/crypto.h b/src/openvpn/crypto.h
index 09f7bb25..dfbfb38b 100644
--- a/src/openvpn/crypto.h
+++ b/src/openvpn/crypto.h
@@ -436,11 +436,13 @@  write_pem_key_file(const char *filename, const char *pem_name);
  * @param pem_name      the name used in the pem encoding start/end lines
  * @param key_file      name of the file to read
  * @param key_inline    a string holding the data in case of an inline key
+ * @param allow_random  allow generating a random key if no file is provided
  * @return              true if reading into key was successful
  */
 bool
 read_pem_key_file(struct buffer *key, const char *pem_name,
-                  const char *key_file, const char *key_inline);
+                  const char *key_file, const char *key_inline,
+                  bool allow_random);
 
 /* Minimum length of the nonce used by the PRNG */
 #define NONCE_SECRET_LEN_MIN 16
diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
index d6a82252..baa193c3 100644
--- a/src/openvpn/tls_crypt.c
+++ b/src/openvpn/tls_crypt.c
@@ -301,7 +301,7 @@  tls_crypt_v2_init_client_key(struct key_ctx_bi *key, struct buffer *wkc_buf,
                                          + TLS_CRYPT_V2_MAX_WKC_LEN);
 
     if (!read_pem_key_file(&client_key, tls_crypt_v2_cli_pem_name,
-                           key_file, key_inline))
+                           key_file, key_inline, false))
     {
         msg(M_FATAL, "ERROR: invalid tls-crypt-v2 client key format");
     }
@@ -326,8 +326,9 @@  tls_crypt_v2_init_server_key(struct key_ctx *key_ctx, bool encrypt,
     struct buffer srv_key_buf;
 
     buf_set_write(&srv_key_buf, (void *)&srv_key, sizeof(srv_key));
+
     if (!read_pem_key_file(&srv_key_buf, tls_crypt_v2_srv_pem_name,
-                           key_file, key_inline))
+                           key_file, key_inline, false))
     {
         msg(M_FATAL, "ERROR: invalid tls-crypt-v2 server key format");
     }