Re: [Openvpn-devel] [PATCH v3] convert *_inline attributes to bool

2017-01-15 Thread Antonio Quartulli 
On Sun, Jan 15, 2017 at 10:36:24AM +0100, Steffan Karger wrote:
> > diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
> > index 944f4c5..9a7745f 100644
> > --- a/src/openvpn/crypto.c
> > +++ b/src/openvpn/crypto.c
> > @@ -36,6 +36,7 @@
> >  #include "crypto.h"
> >  #include "error.h"
> >  #include "integer.h"
> > +#include "misc.h"
> 
> Adding a dependency to misc.o breaks the unit tests.  Just try to run
> 'make check' now (after initializing the cmocka submodule).
> 
> I see that this added dependency is because you added
> 'print_if_inline()' in misc.c.  It seems to be used only to print key
> filenames though.  What about renaming it to print_key_filename() and
> moving it to crypto.c?

Yeah, makes sense. I'll move it to crypto.c

> 
> > +/*
> > + * A wrapper for check_file_access_chroot() that returns false immediately 
> > if
> > + * the file is inline (and therefore there is no access to check)
> > + */
> > +static bool
> > +check_file_access_chroot_inline(bool is_inline, const char *chroot,
> > +const int type, const char *file,
> > +const int mode, const char *opt)
> > +{
> > +if (is_inline)
> > +{
> > +return false;
> > +}
> > +
> > +return check_file_access_chroot(chroot, type, file, mode, opt);
> > +}
> > +
> > +/*
> > + * A wrapper for check_file_access() that returns false immediately if the 
> > file
> > + * is inline (and therefore there is no access to check)
> > + */
> > +static bool
> > +check_file_access_inline(bool is_inline, const int type, const char *file,
> > + const int mode, const char *opt)
> > +{
> > +if (is_inline)
> > +{
> > +return false;
> > +}
> > +
> > +return check_file_access(type, file, mode, opt);
> > +}
> 
> If you start these with /**, rather than /*, they will end up nicely in
> the doxygen.

Yeah .. I usually do it, but this time I forgot :) will fix that.

> 
> >  static bool
> > -check_inline_file(struct in_src *is, char *p[], struct gc_arena *gc)
> > +check_inline_file(struct in_src *is, char *p[], bool *is_inline,
> > +  struct gc_arena *gc)
> >  {
> >  bool ret = false;
> > +
> > +*is_inline = false;
> >  if (p[0] && !p[1])
> >  {
> >  char *arg = p[0];
> >  if (arg[0] == '<' && arg[strlen(arg)-1] == '>')
> >  {
> >  struct buffer close_tag;
> > +*is_inline = true;
> >  arg[strlen(arg)-1] = '\0';
> >  p[0] = string_alloc(arg+1, gc);
> > -p[1] = string_alloc(INLINE_FILE_TAG, gc);
> >  close_tag = alloc_buf(strlen(p[0]) + 4);
> >  buf_printf(_tag, "", p[0]);
> > -p[2] = read_inline_file(is, BSTR(_tag), gc);
> > -p[3] = NULL;
> > +p[1] = read_inline_file(is, BSTR(_tag), gc);
> > +p[2] = NULL;
> >  free_buf(_tag);
> >  ret = true;
> >  }
> 
> This function now returns the same value as is set to *is_line.  I think
> we should just use the return value.  (For some reason the return value
> wasn't used anywhere previously, but let's just do so now.)

eheh nice catch. I will remove the parameter and re-use the return value.

> 
> >  static void
> >  add_option(struct options *options,
> > char *p[],
> > +   bool is_inline,
> > const char *file,
> > int line,
> > const int level,
> > @@ -4506,6 +4561,7 @@ read_config_file(struct options *options,
> >   struct env_set *es)
> >  {
> >  const int max_recursive_levels = 10;
> > +bool is_inline;
> 
> is_inline is only used below, in the if(parse_line()) scope.  I think it
> should be declared in that scope.


oh ok. I am not used to that, so I completely missed it. will do!

> 
> >  FILE *fp;
> >  int line_num;
> >  char line[OPTION_LINE_SIZE+1];
> > @@ -4544,8 +4600,10 @@ read_config_file(struct options *options,
> >  if (parse_line(line + offset, p, SIZE(p), file, line_num, 
> > msglevel, >gc))
> >  {
> >  bypass_doubledash([0]);
> > -check_inline_file_via_fp(fp, p, >gc);
> > -add_option(options, p, file, line_num, level, 
> > msglevel, permission_mask, option_types_found, es);
> > +check_inline_file_via_fp(fp, p, _inline, 
> > >gc);
> > +add_option(options, p, is_inline, file, line_num, 
> > level,
> > +   msglevel, permission_mask, 
> > option_types_found,
> > +   es);
> >  }
> >  }
> >  if (fp != stdin)
> > @@ -4578,6 +4636,7 @@ read_config_string(const char *prefix,
> >  char line[OPTION_LINE_SIZE];
> >  struct buffer multiline;
> >  int line_num = 0;
> > +bool is_inline;
> >  buf_set_read(, (uint8_t *)config,

Re: [Openvpn-devel] [PATCH v3] convert *_inline attributes to bool

2017-01-15 Thread Steffan Karger 
Hi,

Thanks, I like each version better.  Even though it adds more lines than
it removes, quite some of it is because of correcting wrapping and
adding useful comments.  I think it really improves the readability of
the code.  Some (hopefully final) minor comments still though:

On 14-01-17 17:30, Antonio Quartulli wrote:
> Carrying around the INLINE_TAG is not really efficient,
> because it requires a strcmp() to be performed every
> time we want to understand if the data is stored inline
> or not.
> 
> Convert all the *_inline attributes to bool to make the
> logic easier and checks more efficient.
> 
> Signed-off-by: Antonio Quartulli 
> ---
> 
> Based on master + [PATCH (master)] reformatting: fix style in crypto*.{c, h}
> 
> Changes from v2:
> - fix indentation in ssl_openssl.c
> - do not attempt to push inline'd options
> - do not attempt to parse inline'd plugin
> - introduce check_file_access_inline() and check_file_access_chroot_inline()
> - introduce OPT_P_INLINE to specify when an option is allowed to
>   be inline. Options not having this permission will fail to be
>   parsed if is_inline is true.
> 
> 
> Changes from v1:
> - remove the INLINE_TAG from the options parsing logic at all. Now a
>   boolean variable is passed around.
> - add print_if_inline() helper function (to misc.c/h) to make sure we
>   never print the inline data, but only the INLINE tag. Such function
>   checks also for NULL pointers;
> - make sure print_if_inline() is always used when printing possibly
>   inline data;
> - remove the INLINE_TAG from the options parsing logic at all. Now a
>   boolean variable is passed around.
> - fix alignment error in comment;
> - remove CHKACC_INLINE from check_file_access() logic: this function
>   is now not invoked at all in case of inline data.
> 
> 
>  src/openvpn/crypto.c  |  25 +++--
>  src/openvpn/crypto.h  |   5 +-
>  src/openvpn/misc.c|  17 ++-
>  src/openvpn/misc.h|  15 ++-
>  src/openvpn/options.c | 281 
> ++
>  src/openvpn/options.h |  21 ++--
>  src/openvpn/plugin.c  |   6 +-
>  src/openvpn/plugin.h  |   3 +-
>  src/openvpn/push.c|   2 +-
>  src/openvpn/push.h|   3 +-
>  src/openvpn/ssl.c |   6 +-
>  src/openvpn/ssl_backend.h |  64 ++-
>  src/openvpn/ssl_common.h  |   2 +-
>  src/openvpn/ssl_mbedtls.c |  63 +--
>  src/openvpn/ssl_openssl.c |  91 ---
>  src/openvpn/tls_crypt.c   |   2 +-
>  src/openvpn/tls_crypt.h   |   9 +-
>  17 files changed, 343 insertions(+), 272 deletions(-)
> 
> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
> index 944f4c5..9a7745f 100644
> --- a/src/openvpn/crypto.c
> +++ b/src/openvpn/crypto.c
> @@ -36,6 +36,7 @@
>  #include "crypto.h"
>  #include "error.h"
>  #include "integer.h"
> +#include "misc.h"

Adding a dependency to misc.o breaks the unit tests.  Just try to run
'make check' now (after initializing the cmocka submodule).

I see that this added dependency is because you added
'print_if_inline()' in misc.c.  It seems to be used only to print key
filenames though.  What about renaming it to print_key_filename() and
moving it to crypto.c?

> +/*
> + * A wrapper for check_file_access_chroot() that returns false immediately if
> + * the file is inline (and therefore there is no access to check)
> + */
> +static bool
> +check_file_access_chroot_inline(bool is_inline, const char *chroot,
> +const int type, const char *file,
> +const int mode, const char *opt)
> +{
> +if (is_inline)
> +{
> +return false;
> +}
> +
> +return check_file_access_chroot(chroot, type, file, mode, opt);
> +}
> +
> +/*
> + * A wrapper for check_file_access() that returns false immediately if the 
> file
> + * is inline (and therefore there is no access to check)
> + */
> +static bool
> +check_file_access_inline(bool is_inline, const int type, const char *file,
> + const int mode, const char *opt)
> +{
> +if (is_inline)
> +{
> +return false;
> +}
> +
> +return check_file_access(type, file, mode, opt);
> +}

If you start these with /**, rather than /*, they will end up nicely in
the doxygen.

>  static bool
> -check_inline_file(struct in_src *is, char *p[], struct gc_arena *gc)
> +check_inline_file(struct in_src *is, char *p[], bool *is_inline,
> +  struct gc_arena *gc)
>  {
>  bool ret = false;
> +
> +*is_inline = false;
>  if (p[0] && !p[1])
>  {
>  char *arg = p[0];
>  if (arg[0] == '<' && arg[strlen(arg)-1] == '>')
>  {
>  struct buffer close_tag;
> +*is_inline = true;
>  arg[strlen(arg)-1] = '\0';
>  p[0] = string_alloc(arg+1, gc);
> -p[1] = string_alloc(INLINE_FILE_TAG, gc);
>  close_tag = alloc_buf(strlen(p[0]) + 4);
>

[Openvpn-devel] [PATCH v3] convert *_inline attributes to bool

2017-01-14 Thread Antonio Quartulli 
Carrying around the INLINE_TAG is not really efficient,
because it requires a strcmp() to be performed every
time we want to understand if the data is stored inline
or not.

Convert all the *_inline attributes to bool to make the
logic easier and checks more efficient.

Signed-off-by: Antonio Quartulli 
---

Based on master + [PATCH (master)] reformatting: fix style in crypto*.{c, h}

Changes from v2:
- fix indentation in ssl_openssl.c
- do not attempt to push inline'd options
- do not attempt to parse inline'd plugin
- introduce check_file_access_inline() and check_file_access_chroot_inline()
- introduce OPT_P_INLINE to specify when an option is allowed to
  be inline. Options not having this permission will fail to be
  parsed if is_inline is true.


Changes from v1:
- remove the INLINE_TAG from the options parsing logic at all. Now a
  boolean variable is passed around.
- add print_if_inline() helper function (to misc.c/h) to make sure we
  never print the inline data, but only the INLINE tag. Such function
  checks also for NULL pointers;
- make sure print_if_inline() is always used when printing possibly
  inline data;
- remove the INLINE_TAG from the options parsing logic at all. Now a
  boolean variable is passed around.
- fix alignment error in comment;
- remove CHKACC_INLINE from check_file_access() logic: this function
  is now not invoked at all in case of inline data.


 src/openvpn/crypto.c  |  25 +++--
 src/openvpn/crypto.h  |   5 +-
 src/openvpn/misc.c|  17 ++-
 src/openvpn/misc.h|  15 ++-
 src/openvpn/options.c | 281 ++
 src/openvpn/options.h |  21 ++--
 src/openvpn/plugin.c  |   6 +-
 src/openvpn/plugin.h  |   3 +-
 src/openvpn/push.c|   2 +-
 src/openvpn/push.h|   3 +-
 src/openvpn/ssl.c |   6 +-
 src/openvpn/ssl_backend.h |  64 ++-
 src/openvpn/ssl_common.h  |   2 +-
 src/openvpn/ssl_mbedtls.c |  63 +--
 src/openvpn/ssl_openssl.c |  91 ---
 src/openvpn/tls_crypt.c   |   2 +-
 src/openvpn/tls_crypt.h   |   9 +-
 17 files changed, 343 insertions(+), 272 deletions(-)

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 944f4c5..9a7745f 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -36,6 +36,7 @@
 #include "crypto.h"
 #include "error.h"
 #include "integer.h"
+#include "misc.h"
 #include "platform.h"
 
 #include "memdbg.h"
@@ -1165,27 +1166,26 @@ test_crypto(struct crypto_options *co, struct frame 
*frame)
 
 void
 crypto_read_openvpn_key(const struct key_type *key_type, struct key_ctx_bi 
*ctx,
-const char *key_file, const char *key_inline,
+const char *key_file, bool key_inline,
 const int key_direction, const char *key_name,
 const char *opt_name)
 {
 struct key2 key2;
 struct key_direction_state kds;
 char log_prefix[128] = { 0 };
+unsigned int flags = RKF_MUST_SUCCEED;
 
 if (key_inline)
 {
-read_key_file(, key_inline, RKF_MUST_SUCCEED | RKF_INLINE);
-}
-else
-{
-read_key_file(, key_file, RKF_MUST_SUCCEED);
+flags |= RKF_INLINE;
 }
+read_key_file(, key_file, flags);
 
 if (key2.n != 2)
 {
 msg(M_ERR, "File '%s' does not have OpenVPN Static Key format.  Using "
-"free-form passphrase file is not supported anymore.", key_file);
+"free-form passphrase file is not supported anymore.",
+print_if_inline(key_file, key_inline));
 }
 
 /* check for and fix highly unlikely key problems */
@@ -1225,7 +1225,6 @@ read_key_file(struct key2 *key2, const char *file, const 
unsigned int flags)
 struct buffer in;
 int fd, size;
 uint8_t hex_byte[3] = {0, 0, 0};
-const char *error_filename = file;
 
 /* parse info */
 const unsigned char *cp;
@@ -1264,7 +1263,6 @@ read_key_file(struct key2 *key2, const char *file, const 
unsigned int flags)
 /* 'file' is a string containing ascii representation of key */
 size = strlen(file) + 1;
 buf_set_read(, (const uint8_t *)file, size);
-error_filename = INLINE_FILE_TAG;
 }
 else
 {
@@ -1372,7 +1370,8 @@ read_key_file(struct key2 *key2, const char *file, const 
unsigned int flags)
 {
 msg(M_FATAL,
 isprint(c) ? printable_char_fmt : unprintable_char_fmt,
-c, line_num, error_filename, count, onekeylen, keylen);
+c, line_num, print_if_inline(file, flags & RKF_INLINE),
+count, onekeylen, keylen);
 }
 }
 ++line_index;
@@ -1394,14 +1393,16 @@ read_key_file(struct key2 *key2, const char *file, 
const unsigned int flags)
 {
 msg(M_FATAL,
 "Insufficient key material or header text not found in file 
'%s'