[Openvpn-devel,v3] Improve management-external-key/cert error handling
Commit Message
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.
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(-)
Comments
Hi,
This one applies cleanly on top of master.
On Mon, Apr 2, 2018 at 7:44 AM, 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.
> 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(-)
>
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 669f941b..b06820ba 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
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.
>
> {
> - 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
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index 8ef68ebd..4d434fa2 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -1330,7 +1330,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
Hi,
On 03-04-18 04:53, Selva Nair wrote:
> 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.
Thanks for catching this before it went into master!
For archiving purposes: this patch is no longer needed because it has
been superseded by the refactoring in "Do not load certificate from
tls_ctx_use_external_private_key()" (Message-Id:
<1536916459-25900-1-git-send-email-steffan.karger@fox-it.com>).
I'll register it like so in patchwork too.
-Steffan
@@ -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, 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
@@ -1330,7 +1330,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;
}