Support duplicate x509 field values in environment

PATCH In review Steffan Karger 3 participants 4 messages Nov 28, 2015 – Nov 29, 2015

Share & tools

Steffan Karger

author

2015-11-28 10:03 UTC #0 lore raw

PATCH 1/1 Support duplicate x509 field values in environment

As reported in trac #387, an x509 DN can contain duplicate fields. Previously, we would overwrite any previous field value with a new one if we would process a second same-name field. Now, instead, append _$N, starting at N=1 to the name for each consequent field to export all fields to the enviroment.

Signed-off-by Steffan Karger <[email protected]...>

---
 Changes.rst                       |  7 +++++++
 src/openvpn/misc.c                | 32 ++++++++++++++++++++++++++++++++
 src/openvpn/misc.h                |  7 +++++++
 src/openvpn/ssl_verify_openssl.c  |  2 +-
 src/openvpn/ssl_verify_polarssl.c |  2 +-
 5 files changed, 48 insertions(+), 2 deletions(-)

Changes.rst +7 -0

index 41629bd..a791ca3 100644
@@ -36,6 +36,13 @@ LZ4 Compression
 
 User-visible Changes
 --------------------
+- For certificate DNs with duplicate fields, e.g. "OU=one,OU=two", both fields
+  are now exported to the environment, where each second and later occurrence
+  of a field get _$N appended to it's field name, starting at N=1.  For the
+  example above, that would result in e.g. X509_0_OU=one, X509_0_OU_1=two.
+  Note that this breaks setups that rely on the fact that OpenVPN would
+  previously (incorrectly) only export the last occurence of a field.
+
 - proto udp and proto tcp specify to use IPv4 and IPv6. The new
   options proto udp4 and tcp4 specify to use IPv4 only.

src/openvpn/misc.c +32 -0

index 0f04d09..1c5b455 100644
@@ -607,6 +607,16 @@ env_set_add (struct env_set *es, const char *str)
   env_set_add_nolock (es, str);
 }
 
+const char*
+env_set_get (struct env_set *es, const char *name)
+{
+  struct env_item *item = es->list;
+  while (item && !env_string_equal(item->string, name)) {
+      item = item->next;
+  }
+  return item ? item->string : NULL;
+}
+
 void
 env_set_print (int msglevel, const struct env_set *es)
 {

@@ -741,6 +751,28 @@ setenv_str_safe (struct env_set *es, const char *name, const char *value)
     msg (M_WARN, "setenv_str_safe: name overflow");
 }
 
+void setenv_str_incr(struct env_set *es, const char *name, const char *value)
+{
+  unsigned int counter = 1;
+  const size_t tmpname_len = strlen(name) + 5;
+  char *tmpname = gc_malloc(tmpname_len, true, NULL); /* 3 digits counter max */
+  memcpy(tmpname, name, strlen(name));
+  while (NULL != env_set_get(es, tmpname) && counter < 1000)
+    {
+      ASSERT (openvpn_snprintf (tmpname, tmpname_len, "%s_%u", name, counter));
+      counter++;
+    }
+  if (counter < 1000)
+    {
+      setenv_str (es, tmpname, value);
+    }
+  else
+    {
+      msg (D_TLS_DEBUG_MED, "Too many same-name env variables, ignoring: %s", name);
+    }
+  free (tmpname);
+}
+
 void
 setenv_del (struct env_set *es, const char *name)
 {

src/openvpn/misc.h +7 -0

index ec2e417..d8b2c18 100644
@@ -133,6 +133,12 @@ void setenv_str (struct env_set *es, const char *name, const char *value);
 void setenv_str_safe (struct env_set *es, const char *name, const char *value);
 void setenv_del (struct env_set *es, const char *name);
 
+/**
+ * Store the supplied name value pair in the env_set.  If the variable with the
+ * supplied name  already exists, append _N to the name, starting at N=1.
+ */
+void setenv_str_incr(struct env_set *es, const char *name, const char *value);
+
 void setenv_int_i (struct env_set *es, const char *name, const int value, const int i);
 void setenv_str_i (struct env_set *es, const char *name, const char *value, const int i);

@@ -142,6 +148,7 @@ struct env_set *env_set_create (struct gc_arena *gc);
 void env_set_destroy (struct env_set *es);
 bool env_set_del (struct env_set *es, const char *str);
 void env_set_add (struct env_set *es, const char *str);
+const char* env_set_get (struct env_set *es, const char *name);
 
 void env_set_print (int msglevel, const struct env_set *es);

src/openvpn/ssl_verify_openssl.c +1 -1

index bf53522..d014f9d 100644
@@ -448,7 +448,7 @@ x509_setenv (struct env_set *es, int cert_depth, openvpn_x509_cert_t *peer_cert)
 	  objbuf);
       string_mod (name_expand, CC_PRINT, CC_CRLF, '_');
       string_mod ((char*)buf, CC_PRINT, CC_CRLF, '_');
-      setenv_str (es, name_expand, (char*)buf);
+      setenv_str_incr (es, name_expand, (char*)buf);
       free (name_expand);
       OPENSSL_free (buf);
     }

src/openvpn/ssl_verify_polarssl.c +1 -1

index 62818ad..a2e6a8e 100644
@@ -245,7 +245,7 @@ x509_setenv (struct env_set *es, int cert_depth, openvpn_x509_cert_t *cert)
 	/* Check both strings, set environment variable */
 	string_mod (name_expand, CC_PRINT, CC_CRLF, '_');
 	string_mod ((char*)s, CC_PRINT, CC_CRLF, '_');
-	setenv_str (es, name_expand, (char*)s);
+	setenv_str_incr (es, name_expand, (char*)s);
 
 	name = name->next;
     }

Signature · 1 line

2.5.0

Selva Nair

2015-11-29 05:29 UTC #1 lore raw

Hi,

Quoted 6 lines · Steffan Karger

As reported in trac #387, an x509 DN can contain duplicate fields. Previously, we would overwrite any previous field value with a new one if we would process a second same-name field. Now, instead, append _$N, starting at N=1 to the name for each consequent field to export all fields to the enviroment.

A useful change and clean code. A couple of places could benefit from const qualifiers, though:

--- a/src/openvpn/misc.c

src/openvpn/misc.c env_set_add (struct env_set *es, const char *str) · line 612

Quoted 8 lines

+++ b/src/openvpn/misc.c

src/openvpn/misc.c +2 -0

@@ -607,6 +607,16 @@ env_set_add (struct env_set *es, const char *str)
   env_set_add_nolock (es, str);
 }
 
+const char*
+env_set_get (struct env_set *es, const char *name)

... (const struct env_set *es,.... ) <to be matched in misc.h>

Review env_set_add (struct env_set *es, const char *str) · line 614

Quoted 2 lines

(quoted diff) +1 -0

+  struct env_item *item = es->list;

const struct env_item *item = ...

+ while (item && !envstring_equal(item->string, name)) {

src/openvpn/misc.c env_set_add (struct env_set *es, const char *str)

Quoted 5 lines

src/openvpn/misc.c +4 -0

+      item = item->next;
+  }
+  return item ? item->string : NULL;
+}

Thanks,

Selva

Steffan Karger

author

2015-11-29 09:43 UTC #2 lore raw

Hi,

Quoted 2 lines · Selva Nair

A useful change and clean code. A couple of places could benefit from const qualifiers, though

You're absolutely right. Thanks. Attached a v2 patch that adds the suggested const qualifiers.

Now that I was looking at my own code again, I also realized a strcpy() makes more sense in setenv_str_incr() than the memcpy() from the previous patch. (Both work just fine, but this should be easier to read.)

-Steffan

From 44a5af585953d5384d3bbd64e55c1de6343919d8 Mon Sep 17 00:00:00 2001
From: Steffan Karger <[email protected]>
Date: Sun, 29 Nov 2015 10:39:24 +0100
Subject: [PATCH] Support duplicate x509 field values in environment

v2 - make better use of const qualifiers in env_set_get(), and use strcpy()
instead of memcpy() in setenv_str_incr()

Signed-off-by Steffan Karger <[email protected]>

---
 Changes.rst                       |  7 +++++++
 src/openvpn/misc.c                | 32 ++++++++++++++++++++++++++++++++
 src/openvpn/misc.h                |  7 +++++++
 src/openvpn/ssl_verify_openssl.c  |  2 +-
 src/openvpn/ssl_verify_polarssl.c |  2 +-
 5 files changed, 48 insertions(+), 2 deletions(-)

Changes.rst +7 -0

index c674da5..e712f63 100644
@@ -36,6 +36,13 @@ LZ4 Compression
 
 User-visible Changes
 --------------------
+- For certificate DNs with duplicate fields, e.g. "OU=one,OU=two", both fields
+  are now exported to the environment, where each second and later occurrence
+  of a field get _$N appended to it's field name, starting at N=1.  For the
+  example above, that would result in e.g. X509_0_OU=one, X509_0_OU_1=two.
+  Note that this breaks setups that rely on the fact that OpenVPN would
+  previously (incorrectly) only export the last occurence of a field.
+
 - proto udp and proto tcp specify to use IPv4 and IPv6. The new
   options proto udp4 and tcp4 specify to use IPv4 only.

src/openvpn/misc.c +32 -0

index 0f04d09..6bf46ce 100644
@@ -607,6 +607,16 @@ env_set_add (struct env_set *es, const char *str)
   env_set_add_nolock (es, str);
 }
 
+const char*
+env_set_get (const struct env_set *es, const char *name)
+{
+  const struct env_item *item = es->list;
+  while (item && !env_string_equal(item->string, name)) {
+      item = item->next;
+  }
+  return item ? item->string : NULL;
+}
+
 void
 env_set_print (int msglevel, const struct env_set *es)
 {

@@ -741,6 +751,28 @@ setenv_str_safe (struct env_set *es, const char *name, const char *value)
     msg (M_WARN, "setenv_str_safe: name overflow");
 }
 
+void setenv_str_incr(struct env_set *es, const char *name, const char *value)
+{
+  unsigned int counter = 1;
+  const size_t tmpname_len = strlen(name) + 5; /* 3 digits counter max */
+  char *tmpname = gc_malloc(tmpname_len, true, NULL);
+  strcpy(tmpname, name);
+  while (NULL != env_set_get(es, tmpname) && counter < 1000)
+    {
+      ASSERT (openvpn_snprintf (tmpname, tmpname_len, "%s_%u", name, counter));
+      counter++;
+    }
+  if (counter < 1000)
+    {
+      setenv_str (es, tmpname, value);
+    }
+  else
+    {
+      msg (D_TLS_DEBUG_MED, "Too many same-name env variables, ignoring: %s", name);
+    }
+  free (tmpname);
+}
+
 void
 setenv_del (struct env_set *es, const char *name)
 {

src/openvpn/misc.h +7 -0

index ec2e417..7686399 100644
@@ -133,6 +133,12 @@ void setenv_str (struct env_set *es, const char *name, const char *value);
 void setenv_str_safe (struct env_set *es, const char *name, const char *value);
 void setenv_del (struct env_set *es, const char *name);
 
+/**
+ * Store the supplied name value pair in the env_set.  If the variable with the
+ * supplied name  already exists, append _N to the name, starting at N=1.
+ */
+void setenv_str_incr(struct env_set *es, const char *name, const char *value);
+
 void setenv_int_i (struct env_set *es, const char *name, const int value, const int i);
 void setenv_str_i (struct env_set *es, const char *name, const char *value, const int i);

@@ -142,6 +148,7 @@ struct env_set *env_set_create (struct gc_arena *gc);
 void env_set_destroy (struct env_set *es);
 bool env_set_del (struct env_set *es, const char *str);
 void env_set_add (struct env_set *es, const char *str);
+const char* env_set_get (const struct env_set *es, const char *name);
 
 void env_set_print (int msglevel, const struct env_set *es);

src/openvpn/ssl_verify_openssl.c +1 -1

index bf53522..d014f9d 100644
@@ -448,7 +448,7 @@ x509_setenv (struct env_set *es, int cert_depth, openvpn_x509_cert_t *peer_cert)
 	  objbuf);
       string_mod (name_expand, CC_PRINT, CC_CRLF, '_');
       string_mod ((char*)buf, CC_PRINT, CC_CRLF, '_');
-      setenv_str (es, name_expand, (char*)buf);
+      setenv_str_incr (es, name_expand, (char*)buf);
       free (name_expand);
       OPENSSL_free (buf);
     }

src/openvpn/ssl_verify_polarssl.c +1 -1

index 62818ad..a2e6a8e 100644
@@ -245,7 +245,7 @@ x509_setenv (struct env_set *es, int cert_depth, openvpn_x509_cert_t *cert)
 	/* Check both strings, set environment variable */
 	string_mod (name_expand, CC_PRINT, CC_CRLF, '_');
 	string_mod ((char*)s, CC_PRINT, CC_CRLF, '_');
-	setenv_str (es, name_expand, (char*)s);
+	setenv_str_incr (es, name_expand, (char*)s);
 
 	name = name->next;
     }

Signature · 1 line

2.5.0

Gert Doering

2015-11-29 13:05 UTC #3 lore raw

ACK on the changes v1->v2, keeping Selva's ACK for v1.

Your patch has been applied to the master branch.

commit 13b585e8a4c6f9681ff23bc7fb0af71ce9d0162f

Author: Steffan Karger
Date:   Sun Nov 29 10:39:24 2015 +0100

     Support duplicate x509 field values in environment

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Selva Nair <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/10654
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering