[Openvpn-devel] Improve management-external-key/cert error handling

Message ID 20180308200848.5908-1-steffan@karger.me
State Superseded
Headers show
Series
  • [Openvpn-devel] Improve management-external-key/cert error handling
Related show

Commit Message

Steffan Karger March 8, 2018, 8:08 p.m.
Check the return values of management_query_cert() and
tls_ctx_use_external_private_key(), and error out with a more descriptive
error message.  To do so, we make the openssl-backed implementation of
tls_ctx_use_external_private_key() not throw fatal error anymore.

(And fix line wrapping while touching this code.)

Signed-off-by: Steffan Karger <steffan@karger.me>
---
v2: error out with M_FATAL as suggested by Selva.

 src/openvpn/ssl.c         | 28 ++++++++++++++++++++--------
 src/openvpn/ssl_openssl.c |  2 +-
 2 files changed, 21 insertions(+), 9 deletions(-)

Comments

Selva Nair March 9, 2018, 3:38 a.m. | #1
Hi,

I wanted to give this a quick test, but it doesn't apply.

It seems you have patch 116 (Antonio's "inline-tag changed to bool"
patch) in your local repo.

By the way, the M_FATAL after management_query_cert() looks like a
regression. One problem with these FATAL exits is that it makes it
hard for the GUI to gracefully handle when user presses cancel on some
dialogs. Anyway that's beyond this patch... So just saying..

On Thu, Mar 8, 2018 at 3:08 PM, Steffan Karger <steffan@karger.me> wrote:
> Check the return values of management_query_cert() and
> tls_ctx_use_external_private_key(), and error out with a more descriptive
> error message.  To do so, we make the openssl-backed implementation of
> tls_ctx_use_external_private_key() not throw fatal error anymore.
>
> (And fix line wrapping while touching this code.)
>
> Signed-off-by: Steffan Karger <steffan@karger.me>
> ---
> v2: error out with M_FATAL as suggested by Selva.
>
>  src/openvpn/ssl.c         | 28 ++++++++++++++++++++--------
>  src/openvpn/ssl_openssl.c |  2 +-
>  2 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 1756f8a2..1882db52 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -660,18 +660,30 @@ init_ssl(const struct options *options, struct tls_root_ctx *new_ctx)
>      else if ((options->management_flags & MF_EXTERNAL_KEY)
>               && (options->cert_file || options->management_flags & MF_EXTERNAL_CERT))
>      {
> -        if (options->cert_file)
> +        if (options->cert_file
> +            && 0 != tls_ctx_use_external_private_key(new_ctx,
> +                                                     options->cert_file,
> +                                                     options->cert_file_inline))
>          {
> -            tls_ctx_use_external_private_key(new_ctx, options->cert_file,
> -                                             options->cert_file_inline);
> +            msg(M_FATAL, "Failed to initialize management-external-key");
>          }
>          else
>          {
> -            char *external_certificate = management_query_cert(management,
> -                                                               options->management_certificate);
> -            tls_ctx_use_external_private_key(new_ctx, external_certificate,
> -                                             true);
> -            free(external_certificate);
> +            char *external_cert = management_query_cert(
> +                            management, options->management_certificate);
> +
> +            if (!external_cert)
> +            {
> +                msg(M_FATAL, "Failed to initialize management-external-cert");
> +            }
> +
> +            if (0 != tls_ctx_use_external_private_key(new_ctx, external_cert,
> +                                                      true))
> +            {
> +                msg(M_FATAL, "Failed to initialize management-external-key");
> +            }
> +
> +            free(external_cert);
>          }
>      }
>  #endif
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index 66d98c54..87f67680 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -1327,7 +1327,7 @@ tls_ctx_use_external_private_key(struct tls_root_ctx *ctx,
>      return 0;
>
>  err:
> -    crypto_msg(M_FATAL, "Cannot enable SSL external private key capability");
> +    crypto_msg(M_WARN, "Cannot enable SSL external private key capability");
>      return 1;
>  }
>

Selva

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Steffan Karger April 2, 2018, 11:50 a.m. | #2
Hi,

On 9 March 2018 at 04:38, Selva Nair <selva.nair@gmail.com> wrote:
> I wanted to give this a quick test, but it doesn't apply.
>
> It seems you have patch 116 (Antonio's "inline-tag changed to bool"
> patch) in your local repo.

Oops, you're right - this was based on top of my local working branch,
which indeed includes Antonio's inline-tag cleanup patch. Just sent a
version based on current public master.

> By the way, the M_FATAL after management_query_cert() looks like a
> regression. One problem with these FATAL exits is that it makes it
> hard for the GUI to gracefully handle when user presses cancel on some
> dialogs. Anyway that's beyond this patch... So just saying..

Not really a regression, right?  Previously, management_query_cert()
would return NULL, which would cause
tls_ctx_use_external_private_key() to ASSERT out, because
tls_ctx_load_cert_file_and_copy() would not be able to return a valid
"cert" pointer.

But you're definitely right that the error handling over the
management interface could benefit greatly from some rework. (Beyond
this patch, indeed.)

-Steffan

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Patch

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 1756f8a2..1882db52 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -660,18 +660,30 @@  init_ssl(const struct options *options, struct tls_root_ctx *new_ctx)
     else if ((options->management_flags & MF_EXTERNAL_KEY)
              && (options->cert_file || options->management_flags & MF_EXTERNAL_CERT))
     {
-        if (options->cert_file)
+        if (options->cert_file
+            && 0 != tls_ctx_use_external_private_key(new_ctx,
+                                                     options->cert_file,
+                                                     options->cert_file_inline))
         {
-            tls_ctx_use_external_private_key(new_ctx, options->cert_file,
-                                             options->cert_file_inline);
+            msg(M_FATAL, "Failed to initialize management-external-key");
         }
         else
         {
-            char *external_certificate = management_query_cert(management,
-                                                               options->management_certificate);
-            tls_ctx_use_external_private_key(new_ctx, external_certificate,
-                                             true);
-            free(external_certificate);
+            char *external_cert = management_query_cert(
+                            management, options->management_certificate);
+
+            if (!external_cert)
+            {
+                msg(M_FATAL, "Failed to initialize management-external-cert");
+            }
+
+            if (0 != tls_ctx_use_external_private_key(new_ctx, external_cert,
+                                                      true))
+            {
+                msg(M_FATAL, "Failed to initialize management-external-key");
+            }
+
+            free(external_cert);
         }
     }
 #endif
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 66d98c54..87f67680 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -1327,7 +1327,7 @@  tls_ctx_use_external_private_key(struct tls_root_ctx *ctx,
     return 0;
 
 err:
-    crypto_msg(M_FATAL, "Cannot enable SSL external private key capability");
+    crypto_msg(M_WARN, "Cannot enable SSL external private key capability");
     return 1;
 }