Improve management-external-key/cert error handling

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 <[email protected]...>

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

src/openvpn/ssl.c +0 -0

index 79b985e..25a7085 100644
@@ -660,18 +660,31 @@ 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))

src/openvpn/ssl.c +15 -5

         }
         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

src/openvpn/ssl_openssl.c +0 -0

index 66d98c5..87f6768 100644

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 <[email protected]...>

---
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(-)

src/openvpn/ssl.c +20 -8

index 1756f8a2..1882db52 100644
@@ -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

src/openvpn/ssl_openssl.c +0 -0

index 66d98c54..87f67680 100644

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 <[email protected]...>

---
v2: error out with M_FATAL as suggested by Selva.
v3: rebase on master (without extra patches)

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

src/openvpn/ssl.c +0 -1

index 669f941b..b06820ba 100644
@@ -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)

src/openvpn/ssl.c +15 -5

         {
-            char *external_certificate = management_query_cert(management,
-                                                               options->management_certificate);
-            tls_ctx_use_external_private_key(new_ctx, INLINE_FILE_TAG,
-                                             external_certificate);
-            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, INLINE_FILE_TAG,
+                                                      external_cert))
+            {
+                msg(M_FATAL, "Failed to initialize management-external-key");
+            }
+
+            free(external_cert);
         }
     }

#endif

src/openvpn/ssl_openssl.c +0 -0

index 8ef68ebd..4d434fa2 100644

But I can't believe I missed this in the last round. This else clause will now get executed not only if options->cert_file is false, but also if its true and the call to tls_ctx_use_external_private_key() succeeds! That would be wrong and is not what we want.

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..