Skip to contentSkip to conversation

convert *_inline attributes to bool

PATCH Superseded Antonio Quartulli 1 participant 2 messages Jan 23, 2017 – May 8, 2017 1 message not in the archive

v6 of 8 Superseded Latest is v8 →

lore
Share & tools
lore
#0 lore raw

PATCH v6 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 v5:
- fix function invocation alignment in options.c:options_postprocess_filechecks()
- fix typ0 in function invocation in options.c:options_postprocess_filechecks()
- fix doxygen comment for function tls_ctx_reload_crl() in ssl.c

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         |  14 +--
 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, 348 insertions(+), 276 deletions(-)
src/openvpn/crypto.c +24 -12
index f10478d..4b46fb1 100644
@@ -1168,27 +1168,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 */
@@ -1228,7 +1227,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;
@@ -1267,7 +1265,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
     {
@@ -1375,7 +1372,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;
@@ -1397,14 +1396,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);
         }
     }
 
@@ -1847,4 +1848,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
index 64a6460..389e8da 100644
@@ -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
@@ -525,6 +524,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
index a2f45b6..e320aaf 100644
@@ -1652,12 +1652,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
index 16be621..c44a2b1 100644
@@ -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
index dc63aed..a14f2ab 100644
@@ -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 options
     const 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
index e530c0c..0c5036a 100644
@@ -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
index 4ded529..938d6b3 100644
@@ -107,7 +107,8 @@ struct plugin_return
 
 struct 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
index c9c04a6..4d09207 100644
@@ -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
index 86900c8..1f95266 100644
@@ -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 +7 -7
index 86450fe..977f4bf 100644
@@ -537,14 +537,14 @@ tls_version_parse(const char *vstr, const char *extra)
  * - the CRL file was passed inline
  * - the CRL file was not modified since the last (re)load
  *
- * @param ssl_ctx       The TLS context to use when reloading the CRL
- * @param crl_file      The file name to load the CRL from, or
- *                      "[[INLINE]]" in the case of inline files.
- * @param crl_inline    A string containing the CRL
+ * @param ssl_ctx           The TLS context to use when reloading the CRL
+ * @param crl_file          The file name to load the CRL from, or a string
+ *                          containing the CRL in case of inline files.
+ * @param crl_file_inline   True if crl_file is an inline file.
  */
 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};
@@ -647,8 +647,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
index 9a16d77..75b129f 100644
@@ -266,7 +266,7 @@ struct tls_options
     int 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
index 804c279..95e1fca 100644
@@ -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
index 47f75d0..801127b 100644
@@ -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

A message that is not in the archive (sent off-list or not yet received): [email protected]...

#2 lore raw

Hi,

Quoted 1 line · Steffan Karger

[cut]

Quoted 10 lines

ACK, no more nagging from me, thanks!

Now someone needs to go and review "[PATCH (master)] reformatting: fix style in crypto*.{c, h}"[1], because this patch needs that one ;)

-Steffan

[1]
https://www.mail-archive.com/[email protected]/msg13854.html
// <[email protected]...>

Now that "[PATCH (master)] reformatting: fix style in crypto*.{c, h}” has been merged, maybe this patch here could be considered for inclusion? :)

Cheers,

--
Antonio Quartulli