[Openvpn-devel] Patch: Export NotBefore and NotAfter items to the environment in client-connect

  We're considering to use shorter-lived client certificates for our VPN
users. In an effort to prevent negative impact for our staff due to
expired certificates, we 'd like to keep track of imminent expiration
of certificates in the client-connect script (which we're using anyway
to check is the certificate matches the user id). Many certificate
attributes are passed to the script, but not the "NotAfter" and
"NotBefore" attributes.

The attached patch adds these to the mix.

Rolf
  

On 16/08/2019 10:27, Rolf Fokkens via Openvpn-devel wrote:
> We're considering to use shorter-lived client certificates for our VPN
> users. In an effort to prevent negative impact for our staff due to
> expired certificates, we 'd like to keep track of imminent expiration
> of certificates in the client-connect script (which we're using anyway
> to check is the certificate matches the user id). Many certificate
> attributes are passed to the script, but not the "NotAfter" and
> "NotBefore" attributes.
> 
> The attached patch adds these to the mix.

This gets a Feature-ACK from me.  This is useful information, and something
other users in the community have asked for earlier too.  But there are a few
things here before starting to dive into the details.

First of all, we want to have patches first into git master, and then we need
to discuss in the community if this feature is something we want to backport
to the 2.4 release.  After a new release has stabilized (which 2.4 has), we
are quite reluctant to add new features to those releases.

Another thing is that I think it would be valuable to also print this
information into the logs as well.  The X509_get_notBefore() value is probably
not so important unless that has a value which is in the future.  The
X509_get_notAfter() is fine to always log, but would be nice if it would come
a M_WARN log entry if it has expired.

To achieve this logging feature, setenv_ASN1_TIME() would need to be
refactored a bit - possibly by returning a string as well as "is now() after
the time stamp?" bool flag.  The "printing" could happen to a gc_arena
allocated buffer (which is available in verify_cert_set_env()).  The logging
should probably already happen in verify_cert(), which also has its own
gc_arena.  There are various alternatives to avoid doing the ASN1_TIME_print()
preparations and processing multiple times (for logging and setenv), but I
don't have a clear idea right now what could be a reasonable approach.

And lastly, this code will break compilation if using
./configure --with-crypto-library=mbedtls ... This should also be improved.

Other than that, the code looks reasonable at first glance (I have not compile
tested it yet)
  

On Fri, 2019-08-16 at 13:45 +0200, David Sommerseth wrote:
> This gets a Feature-ACK from me.  This is useful information, and

> something

> other users in the community have asked for earlier too.  But there

> are a few

> things here before starting to dive into the details.

> 

> First of all, we want to have patches first into git master, and then

> we need

> to discuss in the community if this feature is something we want to

> backport

> to the 2.4 release.  After a new release has stabilized (which 2.4

> has), we

> are quite reluctant to add new features to those releases.


I started off by creating a pull request: 
https://github.com/OpenVPN/openvpn/pull/129

During creation of the pull request I was pointed to the openvpn-devel
list, so I attached the patch there too. That one was based on 2.4,
because that's what we're using and how we're testing (and using) the
patch.

> Another thing is that I think it would be valuable to also print this

> information into the logs as well.  The X509_get_notBefore() value is

> probably

> not so important unless that has a value which is in the future.  The

> X509_get_notAfter() is fine to always log, but would be nice if it

> would come

> a M_WARN log entry if it has expired.

> 

> To achieve this logging feature, setenv_ASN1_TIME() would need to be

> refactored a bit - possibly by returning a string as well as "is

> now() after

> the time stamp?" bool flag.  The "printing" could happen to a

> gc_arena

> allocated buffer (which is available in verify_cert_set_env()).  The

> logging

> should probably already happen in verify_cert(), which also has its

> own

> gc_arena.  There are various alternatives to avoid doing the

> ASN1_TIME_print()

> preparations and processing multiple times (for logging and setenv),

> but I

> don't have a clear idea right now what could be a reasonable

> approach.

> 

> And lastly, this code will break compilation if using

> ./configure --with-crypto-library=mbedtls ... This should also be

> improved.

> 


I updated my pull request based on your feedback. I'm not sure if I
correcty understood the structure of the software, but I think it's a
decent attempt.

- The notAfter information is in the logs now (appended to the "VERIFY
OK" lines)
- Warnings are issued if the now is before notBefore of after notAfter
- openssl specifics are moved to ssl_verify_openssl.c.
ssl_verify_mbedtls.c has a dummy equivalent which should make openvpn
both compile and run.

Attached you'll find the updated patch too.
diff -ruN openvpn-2.4.7.orig/src/openvpn/ssl_verify_backend.h openvpn-2.4.7/src/openvpn/ssl_verify_backend.h
--- openvpn-2.4.7.orig/src/openvpn/ssl_verify_backend.h	2019-02-20 13:28:23.000000000 +0100
+++ openvpn-2.4.7/src/openvpn/ssl_verify_backend.h	2019-08-17 13:35:38.832574389 +0200
@@ -267,4 +267,25 @@
  */
 bool tls_verify_crl_missing(const struct tls_options *opt);
 
+/*
+ * Get certificate notBefore and notAfter attributes
+ *
+ * @param cert          Certificate to retrieve attributes from
+ * @param notsize       Size of char buffers for notbefore and notafter
+ * @param notbefore     Charachter representation of notBefore attribute
+ * @param cmpbefore     Compare notBefore with "now"; > 0 if notBefore in the past
+ * @param notafter      Character representation of notAfter attribute
+ * @param cmpafter      Compare notAfter  with "now"; > 0 if notAfter  in the past
+ *
+ * On failing to retrieve notBefore attributes:
+ *   - notbefore[0] = '\0'
+ *   - cmpbefore    = 0
+ *
+ * On failing to retrieve notAfter attributes:
+ *   - notafter[0] = '\0'
+ *   - cmpafter    = 0
+ */
+
+void x509_get_validity(openvpn_x509_cert_t *cert, int notsize, char *notbefore, int *cmpbefore, char *notafter, int *cmpafter);
+
 #endif /* SSL_VERIFY_BACKEND_H_ */
diff -ruN openvpn-2.4.7.orig/src/openvpn/ssl_verify.c openvpn-2.4.7/src/openvpn/ssl_verify.c
--- openvpn-2.4.7.orig/src/openvpn/ssl_verify.c	2019-02-20 13:28:23.000000000 +0100
+++ openvpn-2.4.7/src/openvpn/ssl_verify.c	2019-08-17 13:39:43.229250136 +0200
@@ -447,6 +447,17 @@
     return SUCCESS;
 }
 
+static void
+setenv_validity (struct env_set *es, char *envprefix, int depth, char *dt)
+{
+    char varname[32];
+
+    if (!dt[0]) return;
+
+    openvpn_snprintf(varname, sizeof(varname), "%s_%d", envprefix, depth);
+    setenv_str(es, varname, dt);
+}
+
 /*
  * Export the subject, common_name, and raw certificate fields to the
  * environment for later verification by scripts and plugins.
@@ -673,6 +684,8 @@
     char common_name[TLS_USERNAME_LEN+1] = {0}; /* null-terminated */
     const struct tls_options *opt;
     struct gc_arena gc = gc_new();
+    char notbefore_buf[32], notafter_buf[32];
+    int notbefore_cmp, notafter_cmp;
 
     opt = session->opt;
     ASSERT(opt);
@@ -767,6 +780,13 @@
     /* export current untrusted IP */
     setenv_untrusted(session);
 
+    x509_get_validity(cert, sizeof (notbefore_buf), notbefore_buf, &notbefore_cmp, notafter_buf, &notafter_cmp);
+    setenv_validity (opt->es, "tls_notbefore", cert_depth, notbefore_buf);
+    setenv_validity (opt->es, "tls_notafter",  cert_depth, notafter_buf);
+
+    if (notbefore_cmp < 0) msg(M_WARN, "Certificate notBefore (%s)", notbefore_buf);
+    if (notafter_cmp  > 0) msg(M_WARN, "Certificate notAfter (%s)",  notafter_buf);
+
     /* If this is the peer's own certificate, verify it */
     if (cert_depth == 0 && SUCCESS != verify_peer_cert(opt, cert, subject, common_name))
     {
@@ -806,7 +826,8 @@
         }
     }
 
-    msg(D_HANDSHAKE, "VERIFY OK: depth=%d, %s", cert_depth, subject);
+    msg(D_HANDSHAKE, "VERIFY OK: depth=%d, %s, notAfter=%s", cert_depth, subject,
+        (notafter_buf[0] ? notafter_buf : "-"));
     session->verified = true;
     ret = SUCCESS;
 
diff -ruN openvpn-2.4.7.orig/src/openvpn/ssl_verify_mbedtls.c openvpn-2.4.7/src/openvpn/ssl_verify_mbedtls.c
--- openvpn-2.4.7.orig/src/openvpn/ssl_verify_mbedtls.c	2019-02-20 13:28:23.000000000 +0100
+++ openvpn-2.4.7/src/openvpn/ssl_verify_mbedtls.c	2019-08-17 13:23:46.250827837 +0200
@@ -550,4 +550,14 @@
     return false;
 }
 
+void
+x509_get_validity(mbedtls_x509_crt *cert, int notsize, char *notbefore, int *cmpbefore, char *notafter, int *cmpafter)
+{
+    notbefore[0] = '\0';
+    notafter[0]  = '\0';
+
+    *cmpbefore = 0;
+    *cmpafter  = 0;
+}
+
 #endif /* #if defined(ENABLE_CRYPTO) && defined(ENABLE_CRYPTO_MBEDTLS) */
diff -ruN openvpn-2.4.7.orig/src/openvpn/ssl_verify_openssl.c openvpn-2.4.7/src/openvpn/ssl_verify_openssl.c
--- openvpn-2.4.7.orig/src/openvpn/ssl_verify_openssl.c	2019-02-20 13:28:23.000000000 +0100
+++ openvpn-2.4.7/src/openvpn/ssl_verify_openssl.c	2019-08-17 13:36:58.222439208 +0200
@@ -802,4 +802,35 @@
     return true;
 }
 
+static int
+get_ASN1_TIME(const ASN1_TIME *asn1_time, char *dt, int dtsize, int *cmpnow)
+{
+    BIO *mem;
+    int ret, pday, psec;
+
+    mem = BIO_new(BIO_s_mem());
+    if ((ret = ASN1_TIME_print (mem, asn1_time))) {
+        dt[BIO_read(mem, dt, dtsize-1)] = '\0';
+    }
+    BIO_free(mem);
+    if (!ret) goto fail;
+
+    if (!ASN1_TIME_diff(&pday, &psec, asn1_time, NULL)) goto fail;
+    *cmpnow = (pday ? pday : psec);
+
+    return 1;
+
+fail:
+    dt[0] = '\0';
+    *cmpnow = 0;
+    return 0;
+}
+
+void
+x509_get_validity(X509 *cert, int notsize, char *notbefore, int *cmpbefore, char *notafter, int *cmpafter)
+{
+    get_ASN1_TIME(X509_get_notBefore(cert), notbefore, notsize, cmpbefore);
+    get_ASN1_TIME(X509_get_notAfter(cert),  notafter,  notsize, cmpafter);
+}
+
 #endif /* defined(ENABLE_CRYPTO) && defined(ENABLE_CRYPTO_OPENSSL) */
  

Hi Rolf,

I know this is old....but...

Is this something you'd consider resending based on current master?
Would you also have any chance of testing it again after rebase?

Cheers,

On 17/08/2019 14:12, Rolf Fokkens via Openvpn-devel wrote:
> On Fri, 2019-08-16 at 13:45 +0200, David Sommerseth wrote:
>> This gets a Feature-ACK from me.  This is useful information, and
>> something
>> other users in the community have asked for earlier too.  But there
>> are a few
>> things here before starting to dive into the details.
>>
>> First of all, we want to have patches first into git master, and then
>> we need
>> to discuss in the community if this feature is something we want to
>> backport
>> to the 2.4 release.  After a new release has stabilized (which 2.4
>> has), we
>> are quite reluctant to add new features to those releases.
> 
> I started off by creating a pull request: 
> https://github.com/OpenVPN/openvpn/pull/129
> 
> During creation of the pull request I was pointed to the openvpn-devel
> list, so I attached the patch there too. That one was based on 2.4,
> because that's what we're using and how we're testing (and using) the
> patch.
> 
>> Another thing is that I think it would be valuable to also print this
>> information into the logs as well.  The X509_get_notBefore() value is
>> probably
>> not so important unless that has a value which is in the future.  The
>> X509_get_notAfter() is fine to always log, but would be nice if it
>> would come
>> a M_WARN log entry if it has expired.
>>
>> To achieve this logging feature, setenv_ASN1_TIME() would need to be
>> refactored a bit - possibly by returning a string as well as "is
>> now() after
>> the time stamp?" bool flag.  The "printing" could happen to a
>> gc_arena
>> allocated buffer (which is available in verify_cert_set_env()).  The
>> logging
>> should probably already happen in verify_cert(), which also has its
>> own
>> gc_arena.  There are various alternatives to avoid doing the
>> ASN1_TIME_print()
>> preparations and processing multiple times (for logging and setenv),
>> but I
>> don't have a clear idea right now what could be a reasonable
>> approach.
>>
>> And lastly, this code will break compilation if using
>> ./configure --with-crypto-library=mbedtls ... This should also be
>> improved.
>>
> 
> I updated my pull request based on your feedback. I'm not sure if I
> correcty understood the structure of the software, but I think it's a
> decent attempt.
> 
> - The notAfter information is in the logs now (appended to the "VERIFY
> OK" lines)
> - Warnings are issued if the now is before notBefore of after notAfter
> - openssl specifics are moved to ssl_verify_openssl.c.
> ssl_verify_mbedtls.c has a dummy equivalent which should make openvpn
> both compile and run.
> 
> Attached you'll find the updated patch too.
> 
> 
> 
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>