convert *_inline attributes to bool
PATCH v5 1/1 convert *_inline attributes to bool
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.
---
Based on master + [PATCH (master)] reformatting: fix style in crypto*.{c, h}
Changes from v4:
- remove newline accidentally added in v4
Changes from v3:
- some code style adjustment in options.c:check_inline_file()
- move print_if_inline() from misc.c to crypto.c and rename it to print_key_filename()
- make comment of check_file_access_inline() and check_file_access_chroot_inline() doxygen compliant
- remove *is_inline argument in check_inline_file() and use its return value instead
- move declarations of is_inline to narrower scope in options.c
- move return type of plugin_option_list_add() to its own line
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 | 36 ++++-- src/openvpn/crypto.h | 16 ++- src/openvpn/misc.c | 6 +- src/openvpn/misc.h | 3 +- src/openvpn/options.c | 283 ++++++++++++++++++++++++++-------------------- src/openvpn/options.h | 21 ++-- src/openvpn/plugin.c | 5 +- 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 | 92 ++++++++------- src/openvpn/tls_crypt.c | 2 +- src/openvpn/tls_crypt.h | 9 +- 17 files changed, 344 insertions(+), 272 deletions(-)
src/openvpn/crypto.c +24 -12
@@ -1165,27 +1165,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 (&key2, key_inline,RKF_MUST_SUCCEED |RKF_INLINE ); - } -else - { -read_key_file (&key2, key_file,RKF_MUST_SUCCEED ); + flags |=RKF_INLINE ; } +read_key_file (&key2, 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_key_filename (key_file, key_inline)); }/* check for and fix highly unlikely key problems */
@@ -1225,7 +1224,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 +1262,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 (&in, (const uint8_t *)file, size); - error_filename =INLINE_FILE_TAG ; }else {
@@ -1372,7 +1369,9 @@ 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_key_filename (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' (%d/%d/%d bytes found/min/max)" , - error_filename, count, onekeylen, keylen); +print_key_filename (file, flags &RKF_INLINE ), count, onekeylen, + keylen); }if (state !=PARSE_FINISHED ) {msg (M_FATAL ,"Footer text not found in file '%s' (%d/%d/%d bytes found/min/max)" , - error_filename, count, onekeylen, keylen); +print_key_filename (file, flags &RKF_INLINE ), count, onekeylen, + keylen); } }
@@ -1841,4 +1842,15 @@ translate_cipher_name_to_openvpn(const char *cipher_name) { return pair->openvpn_name; } +const char * +print_key_filename (const char *str,bool is_inline) +{ +if (is_inline) + { +return INLINE_FILE_TAG ; + } + +return np (str); +} +#endif /* ENABLE_CRYPTO */
src/openvpn/crypto.h +13 -3
@@ -490,9 +490,8 @@ void key2_print(const struct key2 *k,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 int key_direction,const char *key_name, -const char *opt_name); +bool key_inline,const int key_direction, +const char *key_name,const char *opt_name);/* * Inline functions
@@ -524,6 +523,17 @@ key_ctx_bi_defined(const struct key_ctx_bi *key) || key->decrypt.hmac; } +/** +* To be used when printing a string that may contain inline data. +* +* If "is_inline" is true, return the inline tag. +* If "is_inline" is false and "str" is not NULL, return "str". +* Return the constant string "[NULL]" otherwise. +* +* @param str the original string to return when is_inline is false +* @param is_inline true when str contains an inline data of some sort +*/ +const char *print_key_filename (const char *str,bool is_inline);#endif /* ENABLE_CRYPTO */ #endif /* CRYPTO_H */
src/openvpn/misc.c +3 -3
@@ -1645,12 +1645,12 @@ make_arg_copy(char **p, struct gc_arena *gc) }const char ** -make_extended_arg_array (char **p,struct gc_arena *gc) +make_extended_arg_array (char **p,bool is_inline,struct gc_arena *gc) {const int argc =string_array_len ((const char **)p); -if (!strcmp (p[0 ],INLINE_FILE_TAG ) && argc ==2 ) +if (is_inline) { -return make_inline_array (p[1 ], gc); +return make_inline_array (p[0 ], gc); }else if (argc ==0 ) {
src/openvpn/misc.h +2 -1
@@ -173,7 +173,8 @@ const char **make_env_array(const struct env_set *es,const char **make_arg_array (const char *first,const char *parms,struct gc_arena *gc); -const char **make_extended_arg_array (char **p,struct gc_arena *gc); +const char **make_extended_arg_array (char **p,bool is_inline, +struct gc_arena *gc);/* an analogue to the random() function, but use OpenSSL functions if available */ #ifdef ENABLE_CRYPTO
src/openvpn/options.c +162 -121
510 lines not on this page · raw message
src/openvpn/options.h +11 -10
@@ -468,7 +468,7 @@ struct options#ifdef ENABLE_CRYPTO /* Cipher parms */ const char *shared_secret_file; -const char *shared_secret_file_inline; +bool shared_secret_file_inline;int key_direction;const char *ciphername;bool ncp_enabled;
@@ -506,13 +506,13 @@ struct optionsconst char *tls_export_cert;const char *crl_file; -const char *ca_file_inline; -const char *cert_file_inline; -const char *extra_certs_file_inline; -const char *crl_file_inline; -char *priv_key_file_inline; -const char *dh_file_inline; -const char *pkcs12_file_inline;/* contains the base64 encoding of pkcs12 file */ +bool ca_file_inline; +bool cert_file_inline; +bool extra_certs_file_inline; +bool crl_file_inline; +bool priv_key_file_inline; +bool dh_file_inline; +bool pkcs12_file_inline;int ns_cert_type;/* set to 0, NS_CERT_CHECK_SERVER, or NS_CERT_CHECK_CLIENT */ unsigned remote_cert_ku[MAX_PARMS ];
@@ -559,11 +559,11 @@ struct options/* Shared secret used for TLS control channel authentication */ const char *tls_auth_file; -const char *tls_auth_file_inline; +bool tls_auth_file_inline;/* Shared secret used for TLS control channel authenticated encryption */ const char *tls_crypt_file; -const char *tls_crypt_inline; +bool tls_crypt_inline;/* Allow only one session */ bool single_session;
@@ -640,6 +640,7 @@ struct options#define OPT_P_SOCKFLAGS (1<<26)#define OPT_P_CONNECTION (1<<27)#define OPT_P_PEER_ID (1<<28) +#define OPT_P_INLINE (1<<29)#define OPT_P_DEFAULT (~(OPT_P_INSTANCE|OPT_P_PULL_MODE))
src/openvpn/plugin.c +3 -2
@@ -161,12 +161,13 @@ plugin_option_list_new(struct gc_arena *gc) }bool -plugin_option_list_add (struct plugin_option_list *list,char **p,struct gc_arena *gc) +plugin_option_list_add (struct plugin_option_list *list,char **p, +struct gc_arena *gc) {if (list->n <MAX_PLUGINS ) {struct plugin_option *o = &list->plugins[list->n++]; - o->argv =make_extended_arg_array (p, gc); + o->argv =make_extended_arg_array (p, false, gc);if (o->argv[0 ]) { o->so_pathname = o->argv[0 ];
src/openvpn/plugin.h +2 -1
@@ -107,7 +107,8 @@ struct plugin_returnstruct plugin_option_list *plugin_option_list_new (struct gc_arena *gc); -bool plugin_option_list_add (struct plugin_option_list *list,char **p,struct gc_arena *gc); +bool plugin_option_list_add (struct plugin_option_list *list,char **p, +struct gc_arena *gc);#ifndef ENABLE_SMALL void plugin_option_list_print (const struct plugin_option_list *list,int msglevel);
src/openvpn/push.c +1 -1
@@ -566,7 +566,7 @@ clone_push_list(struct options *o)void push_options (struct options *o,char **p,int msglevel,struct gc_arena *gc) { -const char **argv =make_extended_arg_array (p, gc); +const char **argv =make_extended_arg_array (p, false, gc);char *opt =print_argv (argv, gc,0 );push_option (o, opt, msglevel); }
src/openvpn/push.h +2 -1
@@ -58,7 +58,8 @@ void clone_push_list(struct options *o);void push_option (struct options *o,const char *opt,int msglevel); -void push_options (struct options *o,char **p,int msglevel,struct gc_arena *gc); +void push_options (struct options *o,char **p,int msglevel, +struct gc_arena *gc);void push_reset (struct options *o);
src/openvpn/ssl.c +3 -3
@@ -542,7 +542,7 @@ tls_version_parse(const char *vstr, const char *extra)*/ static void tls_ctx_reload_crl (struct tls_root_ctx *ssl_ctx,const char *crl_file, -const char *crl_file_inline) +bool crl_file_inline) {/* if something goes wrong with stat(), we'll store 0 as mtime */ platform_stat_t crl_stat = {0 };
@@ -645,8 +645,8 @@ init_ssl(const struct options *options, struct tls_root_ctx *new_ctx) {char *external_certificate =management_query_cert (management, options->management_certificate); -tls_ctx_use_external_private_key (new_ctx,INLINE_FILE_TAG , - external_certificate); +tls_ctx_use_external_private_key (new_ctx, external_certificate, + true);free (external_certificate); } }
src/openvpn/ssl_backend.h +35 -29
137 lines not on this page · raw message
src/openvpn/ssl_common.h +1 -1
@@ -266,7 +266,7 @@ struct tls_optionsint verify_x509_type;const char *verify_x509_name;const char *crl_file; -const char *crl_file_inline; +bool crl_file_inline;int ns_cert_type;unsigned remote_cert_ku[MAX_PARMS ];const char *remote_cert_eku;
src/openvpn/ssl_mbedtls.c +31 -32
166 lines not on this page · raw message
src/openvpn/ssl_openssl.c +45 -47
258 lines not on this page · raw message
src/openvpn/tls_crypt.c +1 -1
@@ -44,7 +44,7 @@ tls_crypt_buf_overhead(void)void tls_crypt_init_key (struct key_ctx_bi *key,const char *key_file, -const char *key_inline,bool tls_server) { +bool key_inline,bool tls_server) {const int key_direction = tls_server ?KEY_DIRECTION_NORMAL :KEY_DIRECTION_INVERSE ;
src/openvpn/tls_crypt.h +5 -4
@@ -92,13 +92,14 @@* * @param key The key context to initialize * @param key_file The file to read the key from (or the inline tag to -* indicate and inline key). -* @param key_inline Array containing (zero-terminated) inline key, or NULL -* if not used. +* indicate and inline key) or an array containing +* (zero-terminated) inline key. +* @param key_inline True if key_file contains an inline key, False +* otherwise. * @param tls_server Must be set to true is this is a TLS server instance. */ void tls_crypt_init_key (struct key_ctx_bi *key,const char *key_file, -const char *key_inline,bool tls_server); +bool key_inline,bool tls_server);/** * Returns the maximum overhead (in bytes) added to the destination buffer by
Signature · 1 line
2.11.0
Hi,
One more real comment and two nitpicks:
src/openvpn/options.c options_postprocess_filechecks(struct options *options) · line 3271
Quoted 6 lines · Antonio Quartulli
src/openvpn/options.c +2
-0@@ -3233,39 +3258,63 @@ options_postprocess_filechecks(struct options *options) + errs |=check_file_access_inline (options->cert_file_inline,CHKACC_FILE , + options->cert_file,R_OK ,"--cert" );
This line has 1 indent too many.
src/openvpn/options.c options_postprocess_filechecks(struct options *options) · line 3275
Quoted 3 lines
src/openvpn/options.c +3
-0+ errs |=check_file_access_inline (options->extra_certs_file,CHKACC_FILE , + options->extra_certs_file,R_OK , +"--extra-certs" );
I think the second one should be options->extra_certs_file_inline?
src/openvpn/ssl.c tls_version_parse(const char *vstr, const char *extra) · line 545
Quoted 8 lines
src/openvpn/ssl.c +1
-1@@ -542,7 +542,7 @@ tls_version_parse(const char *vstr, const char *extra)*/ static void tls_ctx_reload_crl (struct tls_root_ctx *ssl_ctx,const char *crl_file, -const char *crl_file_inline) +bool crl_file_inline)
The doxygen string of the crl_file_inline parameter needs to be updated too (like you did in the header file for the non-static functions).
Apart from these I am ready to give this an ACK.
-Steffan
Quoted 5 lines · Steffan Karger
Hi,
One more real comment and two nitpicks:
On 15-01-17 15:43, Antonio Quartulli wrote:
Quoted 8 lines
src/openvpn/options.c options_postprocess_filechecks(struct options *options) · line 3271
Quoted 6 lines
src/openvpn/options.c +2
-0@@ -3233,39 +3258,63 @@ options_postprocess_filechecks(struct options *options) + errs |=check_file_access_inline (options->cert_file_inline,CHKACC_FILE , + options->cert_file,R_OK ,"--cert" );This line has 1 indent too many.
ops, thanks.
Quoted 6 lines
src/openvpn/options.c options_postprocess_filechecks(struct options *options) · line 3275
Quoted 4 lines
src/openvpn/options.c +3
-0+ errs |=check_file_access_inline (options->extra_certs_file,CHKACC_FILE , + options->extra_certs_file,R_OK , +"--extra-certs" );I think the second one should be
options->extra_certs_file_inline?
good catch, thanks! Unfortunately in my setup I don't use extra-certs, so I couldn't run this piece of code :(
Having automated unit tests for these features would be really nice :/
Quoted 12 lines
src/openvpn/ssl.c tls_version_parse(const char *vstr, const char *extra) · line 545
Quoted 9 lines
src/openvpn/ssl.c +1
-1@@ -542,7 +542,7 @@ tls_version_parse(const char *vstr, const char *extra)*/ static void tls_ctx_reload_crl (struct tls_root_ctx *ssl_ctx,const char *crl_file, -const char *crl_file_inline) +bool crl_file_inline)The doxygen string of the
crl_file_inlineparameter needs to be updated too (like you did in the header file for the non-static functions).
my bad. will fix.
Quoted 2 lines
Apart from these I am ready to give this an ACK.
Thanks! I'll send a new version soon!
Cheers,
Signature · 1 line
Antonio Quartulli