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

[Steffan Karger]

Hi,

On 9 March 2018 at 04:38, Selva Nair  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
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

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

[Selva Nair]

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  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 
> ---
> 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
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

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

[Steffan Karger]

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 
---
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;
 }
 
-- 
2.15.1


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

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

[Selva Nair]

Hi,

On Sun, Mar 4, 2018 at 6:17 AM, Steffan Karger  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 
> ---
>  src/openvpn/ssl.c | 29 +
>  src/openvpn/ssl_openssl.c |  2 +-
>  2 files changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 79b985e..25a7085 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -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))
>  {
> -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_WARN, "Failed to initialize management-external-key");

Don't we need a M_FATAL here -- similar to the M_FATAL for the same
failure below?

> +goto err;

Then this could be removed as well.

>  }
>  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 66d98c5..87f6768 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
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

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

[Steffan Karger]

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 
---
 src/openvpn/ssl.c | 29 +
 src/openvpn/ssl_openssl.c |  2 +-
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 79b985e..25a7085 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -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))
 {
-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_WARN, "Failed to initialize management-external-key");
+goto err;
 }
 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 66d98c5..87f6768 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;
 }
 
-- 
2.7.4


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel