[Openvpn-devel,v5,1/7] Introduce buffer_write_file()

Message ID 1533542553-7383-1-git-send-email-steffan.karger@fox-it.com
State Accepted
Headers show
Series [Openvpn-devel,v5,1/7] Introduce buffer_write_file() | expand

Commit Message

Steffan Karger Aug. 5, 2018, 10:02 p.m. UTC
Rewrite buf_write_string_file to buffer_write_file, which is simpler to
use and can deal with not-null-terminated strings.  Mostly implemented so
this can be easily reused for tls-crypt-v2 (client) key files.

Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
---
v3: split change out of "generate client key", reuse in write_key_file()
v4: improve doxygen and error handling (thanks to ordex)
v5: do not print close error if open fails (thanks to ordex)

 src/openvpn/buffer.c | 31 ++++++++++++++++++++++++-------
 src/openvpn/buffer.h | 12 ++++++++----
 src/openvpn/crypto.c | 33 ++++++---------------------------
 src/openvpn/crypto.h |  5 +++++
 src/openvpn/init.c   |  4 ++++
 5 files changed, 47 insertions(+), 38 deletions(-)

Comments

tincanteksup Aug. 6, 2018, 2:20 a.m. UTC | #1
Hi,

One typo inline


On 06/08/18 09:02, Steffan Karger wrote:
> Rewrite buf_write_string_file to buffer_write_file, which is simpler to
> use and can deal with not-null-terminated strings.  Mostly implemented so
> this can be easily reused for tls-crypt-v2 (client) key files.
> 
> Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
> ---
> v3: split change out of "generate client key", reuse in write_key_file()
> v4: improve doxygen and error handling (thanks to ordex)
> v5: do not print close error if open fails (thanks to ordex)
> 
>   src/openvpn/buffer.c | 31 ++++++++++++++++++++++++-------
>   src/openvpn/buffer.h | 12 ++++++++----
>   src/openvpn/crypto.c | 33 ++++++---------------------------
>   src/openvpn/crypto.h |  5 +++++
>   src/openvpn/init.c   |  4 ++++
>   5 files changed, 47 insertions(+), 38 deletions(-)
> 
> diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c
> index 0972139..7630ff7 100644
> --- a/src/openvpn/buffer.c
> +++ b/src/openvpn/buffer.c
> @@ -343,16 +343,33 @@ convert_to_one_line(struct buffer *buf)
>       }
>   }
>   
> -/* NOTE: requires that string be null terminated */
> -void
> -buf_write_string_file(const struct buffer *buf, const char *filename, int fd)
> +bool
> +buffer_write_file(const char *filename, const struct buffer *buf)
>   {
> -    const int len = strlen((char *) BPTR(buf));
> -    const int size = write(fd, BPTR(buf), len);
> -    if (size != len)
> +    bool ret = false;
> +    int fd = platform_open(filename, O_CREAT | O_TRUNC | O_WRONLY,
> +                           S_IRUSR | S_IWUSR);
> +    if (fd == -1)
> +    {
> +        msg(M_ERRNO, "Cannot open file '%s' for write", filename);
> +        return false;
> +    }
> +
> +    const int size = write(fd, BPTR(buf), BLEN(buf));
> +    if (size != BLEN(buf))
>       {
> -        msg(M_ERR, "Write error on file '%s'", filename);
> +        msg(M_ERRNO, "Write error on file '%s'", filename);
> +        goto cleanup;
> +    }
> +
> +    ret = true;
> +cleanup:
> +    if (close(fd) < 0)
> +    {
> +        msg(M_ERRNO, "Close error on file %s", filename);
> +        ret = false;
>       }
> +    return ret;
>   }
>   
>   /*
> diff --git a/src/openvpn/buffer.h b/src/openvpn/buffer.h
> index 6b6025e..704129a 100644
> --- a/src/openvpn/buffer.h
> +++ b/src/openvpn/buffer.h
> @@ -469,11 +469,15 @@ const char *skip_leading_whitespace(const char *str);
>   
>   void string_null_terminate(char *str, int len, int capacity);
>   
> -/*
> - * Write string in buf to file descriptor fd.
> - * NOTE: requires that string be null terminated.
> +/**
> + * Write buffer contents to file.
> + *
> + * @param filename  The filename to write the buffer to.
> + * @param buf       Thu buffer to write to the file.

Thu --> The


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Antonio Quartulli Aug. 7, 2018, 7:24 p.m. UTC | #2
Hi,

On 06/08/18 16:02, Steffan Karger wrote:
> Rewrite buf_write_string_file to buffer_write_file, which is simpler to
> use and can deal with not-null-terminated strings.  Mostly implemented so
> this can be easily reused for tls-crypt-v2 (client) key files.
> 
> Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>

All looks good now! Thanks!

Acked-by: Antonio Quartulli <antonio@openvpn.net>
Tested-by: Antonio Quartulli <antonio@openvpn.net>

Key generation (success and failure) and basic connection work as expected.
'make check' performed on various platforms, but no buildbot test has
been executed.

Cheers,
Gert Doering Aug. 7, 2018, 9:54 p.m. UTC | #3
Your patch has been applied to the master branch.

(I have not run extra tests - no buildbot explosions are to be expected,
and if Antonio says "key generation has been tested" that is about all
the function does today.  The code looks good, visually :) )

The "Thu" comment has been changed to "The".  No code changes.

commit a8fa167941c548f1cbc82e4bbe5316518df015d4
Author: Steffan Karger
Date:   Mon Aug 6 10:02:33 2018 +0200

     Introduce buffer_write_file()

     Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Tested-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <1533542553-7383-1-git-send-email-steffan.karger@fox-it.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg17371.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Patch

diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c
index 0972139..7630ff7 100644
--- a/src/openvpn/buffer.c
+++ b/src/openvpn/buffer.c
@@ -343,16 +343,33 @@  convert_to_one_line(struct buffer *buf)
     }
 }
 
-/* NOTE: requires that string be null terminated */
-void
-buf_write_string_file(const struct buffer *buf, const char *filename, int fd)
+bool
+buffer_write_file(const char *filename, const struct buffer *buf)
 {
-    const int len = strlen((char *) BPTR(buf));
-    const int size = write(fd, BPTR(buf), len);
-    if (size != len)
+    bool ret = false;
+    int fd = platform_open(filename, O_CREAT | O_TRUNC | O_WRONLY,
+                           S_IRUSR | S_IWUSR);
+    if (fd == -1)
+    {
+        msg(M_ERRNO, "Cannot open file '%s' for write", filename);
+        return false;
+    }
+
+    const int size = write(fd, BPTR(buf), BLEN(buf));
+    if (size != BLEN(buf))
     {
-        msg(M_ERR, "Write error on file '%s'", filename);
+        msg(M_ERRNO, "Write error on file '%s'", filename);
+        goto cleanup;
+    }
+
+    ret = true;
+cleanup:
+    if (close(fd) < 0)
+    {
+        msg(M_ERRNO, "Close error on file %s", filename);
+        ret = false;
     }
+    return ret;
 }
 
 /*
diff --git a/src/openvpn/buffer.h b/src/openvpn/buffer.h
index 6b6025e..704129a 100644
--- a/src/openvpn/buffer.h
+++ b/src/openvpn/buffer.h
@@ -469,11 +469,15 @@  const char *skip_leading_whitespace(const char *str);
 
 void string_null_terminate(char *str, int len, int capacity);
 
-/*
- * Write string in buf to file descriptor fd.
- * NOTE: requires that string be null terminated.
+/**
+ * Write buffer contents to file.
+ *
+ * @param filename  The filename to write the buffer to.
+ * @param buf       Thu buffer to write to the file.
+ *
+ * @return true on success, false otherwise.
  */
-void buf_write_string_file(const struct buffer *buf, const char *filename, int fd);
+bool buffer_write_file(const char *filename, const struct buffer *buf);
 
 /*
  * write a string to the end of a buffer that was
diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 5381ef2..c8d95f3 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -1418,36 +1418,24 @@  read_key_file(struct key2 *key2, const char *file, const unsigned int flags)
     gc_free(&gc);
 }
 
-/*
- * Write key to file, return number of random bits
- * written.
- */
 int
 write_key_file(const int nkeys, const char *filename)
 {
     struct gc_arena gc = gc_new();
 
-    int fd, i;
-    int nbits = 0;
+    int nbits = nkeys * sizeof(struct key) * 8;
 
     /* must be large enough to hold full key file */
     struct buffer out = alloc_buf_gc(2048, &gc);
-    struct buffer nbits_head_text = alloc_buf_gc(128, &gc);
 
     /* how to format the ascii file representation of key */
     const int bytes_per_line = 16;
 
-    /* open key file */
-    fd = platform_open(filename, O_CREAT | O_TRUNC | O_WRONLY, S_IRUSR | S_IWUSR);
-
-    if (fd == -1)
-    {
-        msg(M_ERR, "Cannot open shared secret file '%s' for write", filename);
-    }
-
+    /* write header */
+    buf_printf(&out, "#\n# %d bit OpenVPN static key\n#\n", nbits);
     buf_printf(&out, "%s\n", static_key_head);
 
-    for (i = 0; i < nkeys; ++i)
+    for (int i = 0; i < nkeys; ++i)
     {
         struct key key;
         char *fmt;
@@ -1463,9 +1451,6 @@  write_key_file(const int nkeys, const char *filename)
                             "\n",
                             &gc);
 
-        /* increment random bits counter */
-        nbits += sizeof(key) * 8;
-
         /* write to holding buffer */
         buf_printf(&out, "%s\n", fmt);
 
@@ -1476,16 +1461,10 @@  write_key_file(const int nkeys, const char *filename)
 
     buf_printf(&out, "%s\n", static_key_foot);
 
-    /* write number of bits */
-    buf_printf(&nbits_head_text, "#\n# %d bit OpenVPN static key\n#\n", nbits);
-    buf_write_string_file(&nbits_head_text, filename, fd);
-
     /* write key file, now formatted in out, to file */
-    buf_write_string_file(&out, filename, fd);
-
-    if (close(fd))
+    if(!buffer_write_file(filename, &out))
     {
-        msg(M_ERR, "Close error on shared secret file %s", filename);
+        nbits = -1;
     }
 
     /* zero memory which held file content (memory will be freed by GC) */
diff --git a/src/openvpn/crypto.h b/src/openvpn/crypto.h
index 9417fa1..0dc597f 100644
--- a/src/openvpn/crypto.h
+++ b/src/openvpn/crypto.h
@@ -271,6 +271,11 @@  struct crypto_options
 #define RKF_INLINE       (1<<1)
 void read_key_file(struct key2 *key2, const char *file, const unsigned int flags);
 
+/**
+ * Write nkeys 1024-bits keys to file.
+ *
+ * @returns number of random bits written, or -1 on failure.
+ */
 int write_key_file(const int nkeys, const char *filename);
 
 int read_passphrase_hash(const char *passphrase_file,
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index f432106..2933d55 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1041,6 +1041,10 @@  do_genkey(const struct options *options)
         }
 
         nbits_written = write_key_file(2, options->shared_secret_file);
+        if (nbits_written < 0)
+        {
+            msg(M_FATAL, "Failed to write key file");
+        }
 
         msg(D_GENKEY | M_NOPREFIX,
             "Randomly generated %d bit key written to %s", nbits_written,