Hi,
This addition is welcome and the code does the job it promises to do,
but after reviewing the code I would like to propose a different
implementation. The reasons for this are gives as inline replies
below. The alternative patch proposal is attached.
On Thu, Mar 3, 2016 at 9:19 AM, James Yonan <ja...@openvpn.net> wrote:
> Signed-off-by: James Yonan <ja...@openvpn.net>
> ---
> src/openvpn/ssl_verify_polarssl.c | 166
> ++++++++++++++++++++++++++++++++++++++
> src/openvpn/syshead.h | 2 +-
> 2 files changed, 167 insertions(+), 1 deletion(-)
>
> diff --git a/src/openvpn/ssl_verify_polarssl.c
> b/src/openvpn/ssl_verify_polarssl.c
> index 9d0d086..ab693d2 100644
> --- a/src/openvpn/ssl_verify_polarssl.c
> +++ b/src/openvpn/ssl_verify_polarssl.c
> @@ -198,6 +198,172 @@ x509_get_subject(x509_crt *cert, struct gc_arena *gc)
> return subject;
> }
>
> +#ifdef ENABLE_X509_TRACK
> +
> +/* these match NID's in OpenSSL crypto/objects/objects.h */
> +#define NID_undef 0
> +#define NID_sha1 64
> +#define NID_commonName 13
> +#define NID_countryName 14
> +#define NID_localityName 15
> +#define NID_stateOrProvinceName 16
> +#define NID_organizationName 17
> +#define NID_organizationalUnitName 18
> +#define NID_pkcs9_emailAddress 48
Hmm, I don't really like to maintain lists in the source code. If
possible, I would prefer an implementation that fits the polarssl
approach, rather than wrapping polarssl with openssl-like code.
> diff --git a/src/openvpn/syshead.h b/src/openvpn/syshead.h
> index 7e77b6c..25aa69b 100644
> --- a/src/openvpn/syshead.h
> +++ b/src/openvpn/syshead.h
> @@ -634,7 +634,7 @@ socket_defined (const socket_descriptor_t sd)
> /*
> * Enable x509-track feature?
> */
> -#if defined(ENABLE_CRYPTO) && defined (ENABLE_CRYPTO_OPENSSL)
> +#if defined(ENABLE_CRYPTO) && (defined(ENABLE_CRYPTO_OPENSSL) ||
> defined(ENABLE_CRYPTO_POLARSSL))
Since both crypto backends now support --x509-track, let's just get
rid of this define all together :)
Finally, while reviewing I noticed that the --x509-track code in
options.c lives inside #ifdef MANAGEMENT, but there seems to be no
valid reason for this. Let's move it outside of this #ifdef.
The attached patch takes all these remarks into account. The upsides
of my alternative are less code, and no lists to maintain. The
downside is less error reporting. I'm curious to hear what you think
of the alternative implementation.
-Steffan
From 2363205265ebc77c1dab740b56239ca6e78f5d11 Mon Sep 17 00:00:00 2001
From: Steffan Karger <stef...@karger.me>
Date: Sat, 5 Mar 2016 17:08:22 +0100
Subject: [PATCH] Implemented x509-track for PolarSSL.
This patch is a variant of the patch to implement x509-track for
PolarSSL that was sent to openvpn-devel@ by James Yonan
(<1456993146-63968-7-git-send-email-ja...@openvpn.net>). It still uses
some of the original code from James, but proposes a different
implementation.
This patch does the following things differently:
* Do not introduce NID_* defines that need to be maintained. Instead,
just use the short name of the attribute for identification. This
has the advantage that we automatically support everything that
PolarSSL supports, it is less code and we do not have maintain the
list. But the disadvantage is that this approach will not error out
when an unknown attribute name is supplied. PolarSSL (at least 1.3,
I didn't check 2.x) does not provide the functions required to do
that. Instead of erroring out, this implementation will just
silently ignore the unknown --x509-track attribute name.
* Remove the ENABLE_X509_TRACK define completely - it depended just on
ENABLE_CRYPTO anyway.
* Move the --x509-track option parsing out of ENABLE_MANAGEMENT, since
it does not depend on management functionality.
Signed-off-by: Steffan Karger <stef...@karger.me>
---
doc/openvpn.8 | 1 -
src/openvpn/init.c | 2 -
src/openvpn/options.c | 14 +++---
src/openvpn/options.h | 2 -
src/openvpn/ssl_common.h | 2 -
src/openvpn/ssl_verify.c | 16 ++-----
src/openvpn/ssl_verify.h | 6 ---
src/openvpn/ssl_verify_backend.h | 4 --
src/openvpn/ssl_verify_openssl.c | 3 --
src/openvpn/ssl_verify_polarssl.c | 90 +++++++++++++++++++++++++++++++++++++++
src/openvpn/syshead.h | 7 ---
11 files changed, 99 insertions(+), 48 deletions(-)
diff --git a/doc/openvpn.8 b/doc/openvpn.8
index decffc7..76b04f6 100644
--- a/doc/openvpn.8
+++ b/doc/openvpn.8
@@ -5112,7 +5112,6 @@ to save values from full cert chain. Values will be encoded
as X509_<depth>_<attribute>=<value>. Multiple
.B \-\-x509\-track
options can be defined to track multiple attributes.
-Not available with PolarSSL.
.\"*********************************************************
.TP
.B \-\-ns\-cert\-type client|server
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 84fac07..42baf97 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2355,9 +2355,7 @@ do_init_crypto_tls (struct context *c, const unsigned int flags)
to.auth_user_pass_file = options->auth_user_pass_file;
#endif
-#ifdef ENABLE_X509_TRACK
to.x509_track = options->x509_track;
-#endif
#if P2MP
#ifdef ENABLE_CLIENT_CR
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 57f3dc5..564e706 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -612,10 +612,8 @@ static const char usage_message[] =
" of verification.\n"
"--ns-cert-type t: Require that peer certificate was signed with an explicit\n"
" nsCertType designation t = 'client' | 'server'.\n"
-#ifdef ENABLE_X509_TRACK
"--x509-track x : Save peer X509 attribute x in environment for use by\n"
" plugins and management interface.\n"
-#endif
#if defined(ENABLE_CRYPTO_OPENSSL) && OPENSSL_VERSION_NUMBER >= 0x10001000
"--keying-material-exporter label len : Save Exported Keying Material (RFC5705)\n"
" of len bytes (min. 16 bytes) using label in environment for use by plugins.\n"
@@ -4337,13 +4335,6 @@ add_option (struct options *options,
options->management_flags |= MF_CLIENT_AUTH;
}
#endif
-#ifdef ENABLE_X509_TRACK
- else if (streq (p[0], "x509-track") && p[1] && !p[2])
- {
- VERIFY_PERMISSION (OPT_P_GENERAL);
- x509_track_add (&options->x509_track, p[1], msglevel, &options->gc);
- }
-#endif
#ifdef MANAGEMENT_PF
else if (streq (p[0], "management-client-pf") && !p[1])
{
@@ -7026,6 +7017,11 @@ add_option (struct options *options,
}
options->key_method = key_method;
}
+ else if (streq (p[0], "x509-track") && p[1] && !p[2])
+ {
+ VERIFY_PERMISSION (OPT_P_GENERAL);
+ x509_track_add (&options->x509_track, p[1], msglevel, &options->gc);
+ }
#ifdef ENABLE_X509ALTUSERNAME
else if (streq (p[0], "x509-username-field") && p[1] && !p[2])
{
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 8a26e14..ffbe5e1 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -574,9 +574,7 @@ struct options
#endif /* ENABLE_CRYPTO */
-#ifdef ENABLE_X509_TRACK
const struct x509_track *x509_track;
-#endif
/* special state parms */
int foreign_option_index;
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index 334ccb0..a0df0ff 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -308,9 +308,7 @@ struct tls_options
struct man_def_auth_context *mda_context;
#endif
-#ifdef ENABLE_X509_TRACK
const struct x509_track *x509_track;
-#endif
#ifdef ENABLE_CLIENT_CR
const struct static_challenge_info *sci;
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index ea381f8..b373bed 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -393,22 +393,17 @@ verify_peer_cert(const struct tls_options *opt, openvpn_x509_cert_t *peer_cert,
*/
static void
verify_cert_set_env(struct env_set *es, openvpn_x509_cert_t *peer_cert, int cert_depth,
- const char *subject, const char *common_name
-#ifdef ENABLE_X509_TRACK
- , const struct x509_track *x509_track
-#endif
- )
+ const char *subject, const char *common_name,
+ const struct x509_track *x509_track)
{
char envname[64];
char *serial = NULL;
struct gc_arena gc = gc_new ();
/* Save X509 fields in environment */
-#ifdef ENABLE_X509_TRACK
if (x509_track)
x509_setenv_track (x509_track, es, cert_depth, peer_cert);
else
-#endif
x509_setenv (es, cert_depth, peer_cert);
/* export subject name string as environmental variable */
@@ -658,11 +653,8 @@ verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep
session->verify_maxlevel = max_int (session->verify_maxlevel, cert_depth);
/* export certificate values to the environment */
- verify_cert_set_env(opt->es, cert, cert_depth, subject, common_name
-#ifdef ENABLE_X509_TRACK
- , opt->x509_track
-#endif
- );
+ verify_cert_set_env(opt->es, cert, cert_depth, subject, common_name,
+ opt->x509_track);
/* export current untrusted IP */
setenv_untrusted (session);
diff --git a/src/openvpn/ssl_verify.h b/src/openvpn/ssl_verify.h
index d5bf22e..8527415 100644
--- a/src/openvpn/ssl_verify.h
+++ b/src/openvpn/ssl_verify.h
@@ -201,8 +201,6 @@ void verify_user_pass(struct user_pass *up, struct tls_multi *multi,
*/
void verify_final_auth_checks(struct tls_multi *multi, struct tls_session *session);
-#ifdef ENABLE_X509_TRACK
-
struct x509_track
{
const struct x509_track *next;
@@ -212,10 +210,6 @@ struct x509_track
int nid;
};
-void x509_track_add (const struct x509_track **ll_head, const char *name, int msglevel, struct gc_arena *gc);
-
-#endif
-
/*
* Certificate checking for verify_nsCertType
*/
diff --git a/src/openvpn/ssl_verify_backend.h b/src/openvpn/ssl_verify_backend.h
index 17e88fb..9d2057b 100644
--- a/src/openvpn/ssl_verify_backend.h
+++ b/src/openvpn/ssl_verify_backend.h
@@ -150,8 +150,6 @@ char *backend_x509_get_serial_hex (openvpn_x509_cert_t *cert,
*/
void x509_setenv (struct env_set *es, int cert_depth, openvpn_x509_cert_t *cert);
-#ifdef ENABLE_X509_TRACK
-
/*
* Start tracking the given attribute.
*
@@ -189,8 +187,6 @@ void x509_track_add (const struct x509_track **ll_head, const char *name,
void x509_setenv_track (const struct x509_track *xt, struct env_set *es,
const int depth, openvpn_x509_cert_t *x509);
-#endif
-
/*
* Check X.509 Netscape certificate type field, if available.
*
diff --git a/src/openvpn/ssl_verify_openssl.c b/src/openvpn/ssl_verify_openssl.c
index 34e7d16..adf68d8 100644
--- a/src/openvpn/ssl_verify_openssl.c
+++ b/src/openvpn/ssl_verify_openssl.c
@@ -303,8 +303,6 @@ err:
}
-#ifdef ENABLE_X509_TRACK
-
/*
* x509-track implementation -- save X509 fields to environment,
* using the naming convention:
@@ -433,7 +431,6 @@ x509_setenv_track (const struct x509_track *xt, struct env_set *es, const int de
}
gc_free(&gc);
}
-#endif
/*
* Save X509 fields to environment, using the naming convention:
diff --git a/src/openvpn/ssl_verify_polarssl.c b/src/openvpn/ssl_verify_polarssl.c
index 73a9af1..d527133 100644
--- a/src/openvpn/ssl_verify_polarssl.c
+++ b/src/openvpn/ssl_verify_polarssl.c
@@ -37,7 +37,9 @@
#if defined(ENABLE_CRYPTO) && defined(ENABLE_CRYPTO_POLARSSL)
+#include "crypto_polarssl.h"
#include "ssl_verify.h"
+#include <polarssl/asn1.h>
#include <polarssl/error.h>
#include <polarssl/bignum.h>
#include <polarssl/oid.h>
@@ -197,6 +199,94 @@ x509_get_subject(x509_crt *cert, struct gc_arena *gc)
return subject;
}
+static void
+do_setenv_x509 (struct env_set *es, const char *name, char *value, int depth)
+{
+ char *name_expand;
+ size_t name_expand_size;
+
+ string_mod (value, CC_ANY, CC_CRLF, '?');
+ msg (D_X509_ATTR, "X509 ATTRIBUTE name='%s' value='%s' depth=%d", name, value, depth);
+ name_expand_size = 64 + strlen (name);
+ name_expand = (char *) malloc (name_expand_size);
+ check_malloc_return (name_expand);
+ openvpn_snprintf (name_expand, name_expand_size, "X509_%d_%s", depth, name);
+ setenv_str (es, name_expand, value);
+ free (name_expand);
+}
+
+static char *
+asn1_buf_to_c_string(const asn1_buf *orig, struct gc_arena *gc)
+{
+ size_t i;
+ char *val;
+
+ for (i = 0; i < orig->len; ++i)
+ if (orig->p[i] == '\0')
+ return "ERROR: embedded null value";
+ val = gc_malloc(orig->len+1, false, gc);
+ memcpy(val, orig->p, orig->len);
+ val[orig->len] = '\0';
+ return val;
+}
+
+static void
+do_setenv_name(struct env_set *es, const struct x509_track *xt,
+ const x509_crt *cert, int depth, struct gc_arena *gc)
+{
+ const x509_name *xn;
+ for (xn = &cert->subject; xn != NULL; xn = xn->next)
+ {
+ const char *xn_short_name = NULL;
+ if (0 == oid_get_attr_short_name (&xn->oid, &xn_short_name) &&
+ 0 == strcmp (xt->name, xn_short_name))
+ {
+ char *val_str = asn1_buf_to_c_string (&xn->val, gc);
+ do_setenv_x509 (es, xt->name, val_str, depth);
+ }
+ }
+}
+
+void
+x509_track_add (const struct x509_track **ll_head, const char *name, int msglevel, struct gc_arena *gc)
+{
+ struct x509_track *xt;
+ ALLOC_OBJ_CLEAR_GC (xt, struct x509_track, gc);
+ if (*name == '+')
+ {
+ xt->flags |= XT_FULL_CHAIN;
+ ++name;
+ }
+ xt->name = name;
+ xt->next = *ll_head;
+ *ll_head = xt;
+}
+
+void
+x509_setenv_track (const struct x509_track *xt, struct env_set *es, const int depth, x509_crt *cert)
+{
+ struct gc_arena gc = gc_new();
+ while (xt)
+ {
+ if (depth == 0 || (xt->flags & XT_FULL_CHAIN))
+ {
+ if (0 == strcmp(xt->name, "SHA1"))
+ {
+ /* SHA1 fingerprint is not part of X509 structure */
+ unsigned char *sha1_hash = x509_get_sha1_hash(cert, &gc);
+ char *sha1_fingerprint = format_hex_ex(sha1_hash, SHA_DIGEST_LENGTH, 0, 1 | FHE_CAPS, ":", &gc);
+ do_setenv_x509(es, xt->name, sha1_fingerprint, depth);
+ }
+ else
+ {
+ do_setenv_name(es, xt, cert, depth, &gc);
+ }
+ }
+ xt = xt->next;
+ }
+ gc_free(&gc);
+}
+
/*
* Save X509 fields to environment, using the naming convention:
*
diff --git a/src/openvpn/syshead.h b/src/openvpn/syshead.h
index 7e77b6c..1c9248f 100644
--- a/src/openvpn/syshead.h
+++ b/src/openvpn/syshead.h
@@ -632,13 +632,6 @@ socket_defined (const socket_descriptor_t sd)
#endif
/*
- * Enable x509-track feature?
- */
-#if defined(ENABLE_CRYPTO) && defined (ENABLE_CRYPTO_OPENSSL)
-#define ENABLE_X509_TRACK
-#endif
-
-/*
* Is poll available on this platform?
*/
#if defined(HAVE_POLL) && defined(HAVE_SYS_POLL_H)
--
2.5.0
