[Openvpn-devel] Log serial number of revoked certificate

Message ID 20200727171750.26657-1-themiron@yandex-team.ru
State Accepted
Headers show
Series
  • [Openvpn-devel] Log serial number of revoked certificate
Related show

Commit Message

Vladislav Grishenko July 27, 2020, 5:17 p.m.
As it appears commit 767e4c56becbfeea525e4695a810593f373883cd "Log
serial number of revoked certificate" hasn't survive refactoring
of CRL handling.

In most of situations admin of OpenVPN server needs to know which
particular certificate is used by client.
In the case when certificate is valid, environment variable can be
used for that but once it is revoked, no user scripts are invoked
so there is no way to get serial number, only subject is logged.

Let's log certificate serial in case it is revoked and additionally
log certificate depth & subject in crl-verify "dir" mode for better
consistency with crl file (non-dir) mode.

Signed-off-by: Vladislav Grishenko <themiron@yandex-team.ru>
---
 src/openvpn/ssl_verify.c         | 7 ++++---
 src/openvpn/ssl_verify_mbedtls.c | 5 +++--
 src/openvpn/ssl_verify_openssl.c | 5 +++--
 3 files changed, 10 insertions(+), 7 deletions(-)

Comments

Lev Stipakov Aug. 5, 2020, 8:29 a.m. | #1
Hi,

Compiled and tested on Ubuntu 20.04, looks good.

A few nit-picks:

> +verify_check_crl_dir(const char *crl_dir, int cert_depth, openvpn_x509_cert_t *cert, char *subject)

The last parameter could benefit from const to indicate that function
is not going to modify it.


> -        msg(D_HANDSHAKE, "VERIFY CRL: certificate serial number %s is revoked", serial);
> +        msg(D_HANDSHAKE, "VERIFY CRL: depth=%d, %s, serial=%s is revoked",
> +            cert_depth, subject, serial);

Since you are modifying this line, could you add a NULL check to
serial to and pass
something like "<not available>" in this case?


> +            msg(D_TLS_ERRORS, "VERIFY ERROR: depth=%d, subject=%s, serial=%s: %s",
> +                cert_depth, subject, serial ? serial : "", errstr);

I would use "<not available>" in NULL case, otherwise the error
message becomes funny.


> +        msg(D_TLS_ERRORS, "VERIFY ERROR: depth=%d, error=%s: %s, serial=%s",
>              X509_STORE_CTX_get_error_depth(ctx),
>              X509_verify_cert_error_string(X509_STORE_CTX_get_error(ctx)),
> -            subject);
> +            subject, serial ? serial : "");

Same as above.
Vladislav Grishenko Aug. 5, 2020, 8:32 a.m. | #2
Hi, Lev
Thanks for review, I'll make improvements in V2. 

--
Best Regards, Vladislav Grishenko

-----Original Message-----
From: Lev Stipakov <lstipakov@gmail.com> 
Sent: Wednesday, August 5, 2020 1:29 PM
To: Vladislav Grishenko <themiron@yandex-team.ru>
Cc: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Subject: Re: [Openvpn-devel] [PATCH] Log serial number of revoked certificate

Hi,

Compiled and tested on Ubuntu 20.04, looks good.

A few nit-picks:

> +verify_check_crl_dir(const char *crl_dir, int cert_depth, 
> +openvpn_x509_cert_t *cert, char *subject)

The last parameter could benefit from const to indicate that function is not going to modify it.


> -        msg(D_HANDSHAKE, "VERIFY CRL: certificate serial number %s is revoked", serial);
> +        msg(D_HANDSHAKE, "VERIFY CRL: depth=%d, %s, serial=%s is revoked",
> +            cert_depth, subject, serial);

Since you are modifying this line, could you add a NULL check to serial to and pass something like "<not available>" in this case?


> +            msg(D_TLS_ERRORS, "VERIFY ERROR: depth=%d, subject=%s, serial=%s: %s",
> +                cert_depth, subject, serial ? serial : "", errstr);

I would use "<not available>" in NULL case, otherwise the error message becomes funny.


> +        msg(D_TLS_ERRORS, "VERIFY ERROR: depth=%d, error=%s: %s, 
> + serial=%s",
>              X509_STORE_CTX_get_error_depth(ctx),
>              X509_verify_cert_error_string(X509_STORE_CTX_get_error(ctx)),
> -            subject);
> +            subject, serial ? serial : "");

Same as above.

Patch

diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 844bc57d..07745514 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -599,7 +599,7 @@  cleanup:
  * check peer cert against CRL directory
  */
 static result_t
-verify_check_crl_dir(const char *crl_dir, openvpn_x509_cert_t *cert)
+verify_check_crl_dir(const char *crl_dir, int cert_depth, openvpn_x509_cert_t *cert, char *subject)
 {
     result_t ret = FAILURE;
     char fn[256];
@@ -616,7 +616,8 @@  verify_check_crl_dir(const char *crl_dir, openvpn_x509_cert_t *cert)
     fd = platform_open(fn, O_RDONLY, 0);
     if (fd >= 0)
     {
-        msg(D_HANDSHAKE, "VERIFY CRL: certificate serial number %s is revoked", serial);
+        msg(D_HANDSHAKE, "VERIFY CRL: depth=%d, %s, serial=%s is revoked",
+            cert_depth, subject, serial);
         goto cleanup;
     }
 
@@ -758,7 +759,7 @@  verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep
     {
         if (opt->ssl_flags & SSLF_CRL_VERIFY_DIR)
         {
-            if (SUCCESS != verify_check_crl_dir(opt->crl_file, cert))
+            if (SUCCESS != verify_check_crl_dir(opt->crl_file, cert_depth, cert, subject))
             {
                 goto cleanup;
             }
diff --git a/src/openvpn/ssl_verify_mbedtls.c b/src/openvpn/ssl_verify_mbedtls.c
index fd31bbbd..e9982e41 100644
--- a/src/openvpn/ssl_verify_mbedtls.c
+++ b/src/openvpn/ssl_verify_mbedtls.c
@@ -68,6 +68,7 @@  verify_callback(void *session_obj, mbedtls_x509_crt *cert, int cert_depth,
         int ret = 0;
         char errstr[512] = { 0 };
         char *subject = x509_get_subject(cert, &gc);
+        char *serial = backend_x509_get_serial(cert, &gc);
 
         ret = mbedtls_x509_crt_verify_info(errstr, sizeof(errstr)-1, "", *flags);
         if (ret <= 0 && !openvpn_snprintf(errstr, sizeof(errstr),
@@ -82,8 +83,8 @@  verify_callback(void *session_obj, mbedtls_x509_crt *cert, int cert_depth,
 
         if (subject)
         {
-            msg(D_TLS_ERRORS, "VERIFY ERROR: depth=%d, subject=%s: %s",
-                cert_depth, subject, errstr);
+            msg(D_TLS_ERRORS, "VERIFY ERROR: depth=%d, subject=%s, serial=%s: %s",
+                cert_depth, subject, serial ? serial : "", errstr);
         }
         else
         {
diff --git a/src/openvpn/ssl_verify_openssl.c b/src/openvpn/ssl_verify_openssl.c
index ff14db23..20095cf7 100644
--- a/src/openvpn/ssl_verify_openssl.c
+++ b/src/openvpn/ssl_verify_openssl.c
@@ -71,6 +71,7 @@  verify_callback(int preverify_ok, X509_STORE_CTX *ctx)
     {
         /* get the X509 name */
         char *subject = x509_get_subject(current_cert, &gc);
+        char *serial = backend_x509_get_serial(current_cert, &gc);
 
         if (!subject)
         {
@@ -89,10 +90,10 @@  verify_callback(int preverify_ok, X509_STORE_CTX *ctx)
         }
 
         /* Remote site specified a certificate, but it's not correct */
-        msg(D_TLS_ERRORS, "VERIFY ERROR: depth=%d, error=%s: %s",
+        msg(D_TLS_ERRORS, "VERIFY ERROR: depth=%d, error=%s: %s, serial=%s",
             X509_STORE_CTX_get_error_depth(ctx),
             X509_verify_cert_error_string(X509_STORE_CTX_get_error(ctx)),
-            subject);
+            subject, serial ? serial : "");
 
         ERR_clear_error();