| Message ID | 65530ded0938659345877e870f49d2ad4b9768ae.camel@target-holding.nl |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [Openvpn-devel] Patch: Export NotBefore and NotAfter items to the environment in client-connect | expand |
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, ¬before_cmp, notafter_buf, ¬after_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 >
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-15 20:57:29.803381111 +0200 @@ -448,6 +448,25 @@ } /* + * Export ASN1_TIME items to the environment + */ +static void +setenv_ASN1_TIME(struct env_set *es, char *envname, int envnamesize, + char *envprefix, int depth, const ASN1_TIME *asn1_time) +{ + char timestamp[32]; + BIO *mem; + + mem = BIO_new(BIO_s_mem()); + if (ASN1_TIME_print (mem, asn1_time)) { + timestamp[BIO_read(mem, timestamp, sizeof(timestamp)-1)] = '\0'; + openvpn_snprintf(envname, envnamesize, "%s_%d", envprefix, depth); + setenv_str(es, envname, timestamp); + } + BIO_free(mem); +} + +/* * Export the subject, common_name, and raw certificate fields to the * environment for later verification by scripts and plugins. */ @@ -505,6 +524,12 @@ openvpn_snprintf(envname, sizeof(envname), "tls_serial_hex_%d", cert_depth); setenv_str(es, envname, serial); + setenv_ASN1_TIME(es, envname, sizeof(envname), "tls_notbefore", cert_depth, + X509_get_notBefore(peer_cert)); + + setenv_ASN1_TIME(es, envname, sizeof(envname), "tls_notafter", cert_depth, + X509_get_notAfter(peer_cert)); + gc_free(&gc); }