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

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()

 src/openvpn/buffer.c | 31 ++++++++++++++++++++++++-------
 src/openvpn/buffer.h |  7 ++-----
 src/openvpn/crypto.c | 30 +++++-------------------------
 3 files changed, 31 insertions(+), 37 deletions(-)

Hi,

On 26/07/18 00:08, 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()
> 
>  src/openvpn/buffer.c | 31 ++++++++++++++++++++++++-------
>  src/openvpn/buffer.h |  7 ++-----
>  src/openvpn/crypto.c | 30 +++++-------------------------
>  3 files changed, 31 insertions(+), 37 deletions(-)
> 
> diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c
> index 0972139..20e2b9c 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)

Since you are removing the old comments from both function declaration
and definition, why not adding some little doxygen style doc?


I see you are also changing the way this method works (now it takes only
the filename instead of a fd), but given that we currently use it only
once in our code (for now), I think this change is ok and actually makes
the logic more self-contained (no need for the caller to deal with the
fd at all).

>  {
> -    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);
> +        goto cleanup;
> +    }
> +
> +    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))

nitpikcing: I'd dare to say that in the rest of the code we prefer to
use "func() < 0" as check, to make it more explicit that we are looking
for errors (my personal feeling is that when i see something like your
expression I always need to double check "does this function return
something positive too? what in case of errors?").

On top of that consistency is always good :-)

> +    {
> +        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..180dd56 100644
> --- a/src/openvpn/buffer.h
> +++ b/src/openvpn/buffer.h
> @@ -469,11 +469,8 @@ 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.
> - */
> -void buf_write_string_file(const struct buffer *buf, const char *filename, int fd);
> +/** Write buffer contents to file */
> +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..35bd243 100644
> --- a/src/openvpn/crypto.c
> +++ b/src/openvpn/crypto.c
> @@ -1427,27 +1427,19 @@ 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 +1455,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,17 +1465,8 @@ 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))
> -    {
> -        msg(M_ERR, "Close error on shared secret file %s", filename);
> -    }
> +    buffer_write_file(filename, &out);
>  
>      /* zero memory which held file content (memory will be freed by GC) */
>      buf_clear(&out);
> 

I like this change especially because the writing function now returns a
boolean reporting its success or failure. How about adding some more
checks in the calling chain so that such status can be used?

As of now, if the writing function fails, openvpn will print an error
message, but won't really return "a failure" (as in returning non-zero).
What do you think?


Cheers,

Hi,

Thanks for the review.  Responses inline.

On 02-08-18 11:10, Antonio Quartulli wrote:
> On 26/07/18 00:08, Steffan Karger wrote:
>> diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c
>> index 0972139..20e2b9c 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)
> 
> Since you are removing the old comments from both function declaration
> and definition, why not adding some little doxygen style doc?

In the .h, I added the following doxygen:

+/** Write buffer contents to file */
+bool buffer_write_file(const char *filename, const struct buffer *buf);

I'll expand it to explicitly state the return values, but I don't think
there's much more to say.

> I see you are also changing the way this method works (now it takes only
> the filename instead of a fd), but given that we currently use it only
> once in our code (for now), I think this change is ok and actually makes
> the logic more self-contained (no need for the caller to deal with the
> fd at all).

Yeah, that's the idea :)  I was duplicating the surrounding code and
decided I didn't like that.

>>  {
>> -    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);
>> +        goto cleanup;
>> +    }
>> +
>> +    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))
> 
> nitpikcing: I'd dare to say that in the rest of the code we prefer to
> use "func() < 0" as check, to make it more explicit that we are looking
> for errors (my personal feeling is that when i see something like your
> expression I always need to double check "does this function return
> something positive too? what in case of errors?").
> 
> On top of that consistency is always good :-)

Ok, will change.

>> +    {
>> +        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..180dd56 100644
>> --- a/src/openvpn/buffer.h
>> +++ b/src/openvpn/buffer.h
>> @@ -469,11 +469,8 @@ 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.
>> - */
>> -void buf_write_string_file(const struct buffer *buf, const char *filename, int fd);
>> +/** Write buffer contents to file */
>> +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..35bd243 100644
>> --- a/src/openvpn/crypto.c
>> +++ b/src/openvpn/crypto.c
>> @@ -1427,27 +1427,19 @@ 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 +1455,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,17 +1465,8 @@ 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))
>> -    {
>> -        msg(M_ERR, "Close error on shared secret file %s", filename);
>> -    }
>> +    buffer_write_file(filename, &out);
>>  
>>      /* zero memory which held file content (memory will be freed by GC) */
>>      buf_clear(&out);
>>
> 
> I like this change especially because the writing function now returns a
> boolean reporting its success or failure. How about adding some more
> checks in the calling chain so that such status can be used?
> 
> As of now, if the writing function fails, openvpn will print an error
> message, but won't really return "a failure" (as in returning non-zero).
> What do you think?

Absolutely right, will improve error handling and make openvpn return
non-zero.

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