| Message ID | 20180308200848.5908-1-steffan@karger.me |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] Improve management-external-key/cert error handling | expand |
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
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
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; }
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(-)