| Message ID | 20211214165928.30676-8-selva.nair@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | External key provider for use with OpenSSL 3 | expand |
Am 14.12.21 um 17:59 schrieb selva.nair@gmail.com: > From: Selva Nair <selva.nair@gmail.com> > > - Add a function to set as sign_op during key import. The > function passes the signature request to management interface, > and returns the result to the provider. > > v2 changes: Method to do digest added to match the changes in > the provider signature callback. > TODO: > - Allow passing the undigested message to management interface > - Add pkcs1 DigestInfo header when required > > Signed-off-by: Selva Nair <selva.nair@gmail.com> > Acked-By: Arne Schwabe <arne@rfc2549.org>
Compile and client tested on 1.1.1 and 3.0.1.
Glancing at the code related to management_external_key() does
not make me very happy... too many build time variants.
Maybe we should look into "external key is only supported with
OpenSSL 3.0.1+ builds" for 2.7 and get rid of all the #ifdef'ed
code there...
Your patch has been applied to the master branch.
commit 199df03bf57339661a853cb764ea41a0c8349b95
Author: Selva Nair
Date: Tue Dec 14 11:59:17 2021 -0500
Enable signing via provider for management-external-key
Signed-off-by: Selva Nair <selva.nair@gmail.com>
Acked-by: Arne Schwabe <arne@rfc2549.org>
Message-Id: <20211214165928.30676-8-selva.nair@gmail.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg23428.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
Hi, On Thu, Jan 20, 2022 at 10:18 AM Gert Doering <gert@greenie.muc.de> wrote: > Compile and client tested on 1.1.1 and 3.0.1. > > Glancing at the code related to management_external_key() does > not make me very happy... too many build time variants. "Happiness" is never a word that comes to mind while reading OpenVPN code :) ... Even at our snail's pace, 2.7 may be out before we can break free of OpenSSL 1, LibreSSL xyz etc. An option may be to require OpenSSL 3+ or similar for external keys, or at least for management-external-key. That feature is really used by only a few platforms (only Android for now?). Although it's a nifty option that could potentially be leveraged to remove pkcs11-helper, CNG etc out of OpenVPN core. Selva <div dir="ltr"><div>Hi,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jan 20, 2022 at 10:18 AM Gert Doering <<a href="mailto:gert@greenie.muc.de" target="_blank">gert@greenie.muc.de</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Compile and client tested on 1.1.1 and 3.0.1.<br> <br> Glancing at the code related to management_external_key() does<br> not make me very happy... too many build time variants. </blockquote><div><br></div><div>"Happiness" is never a word that comes to mind while reading OpenVPN code :)</div><div>...<br></div><div><br></div><div>Even at our snail's pace, 2.7 may be out before we can break free of OpenSSL 1, LibreSSL xyz etc. An option may be to require OpenSSL 3+ or similar for external keys, or at least for management-external-key. </div><div><br></div><div>That feature is really used by only a few platforms (only Android for now?). Although it's a nifty option that could potentially be leveraged to remove pkcs11-helper, CNG etc out of OpenVPN core.</div><div><br></div><div>Selva</div></div></div>
Hi, On Thu, Jan 20, 2022 at 11:32:40AM -0500, Selva Nair wrote: > On Thu, Jan 20, 2022 at 10:18 AM Gert Doering <gert@greenie.muc.de> wrote: > > > Compile and client tested on 1.1.1 and 3.0.1. > > > > Glancing at the code related to management_external_key() does > > not make me very happy... too many build time variants. > > > "Happiness" is never a word that comes to mind while reading OpenVPN code :) > ... Oh, some of the code paths are really nice these days :-) - but the #ifdef maze regarding SSL libraries / crypto features is getting truly annoying. > Even at our snail's pace, 2.7 may be out before we can break free of > OpenSSL 1, LibreSSL xyz etc. An option may be to require OpenSSL 3+ or > similar for external keys, or at least for management-external-key. > > That feature is really used by only a few platforms (only Android for > now?). That was my idea - since only Windows and Android use the "xkey" code paths today (as far as I understand), make 3.0.1 a hard requirement for Windows and Android, and disable --management-external-key for older SSL builds. Maybe this is a bit too drastic, but it would reduce code paths to be maintained and tested quite a bit. For Windows and Android, we bundle the SSL library to be used anyway, so we do not need to care what the OS might bring along. > Although it's a nifty option that could potentially be leveraged to > remove pkcs11-helper, CNG etc out of OpenVPN core. Whatever reduces #ifdef and library dependencies :-) gert
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c index 23c74f55..8f0281b1 100644 --- a/src/openvpn/ssl_openssl.c +++ b/src/openvpn/ssl_openssl.c @@ -1169,7 +1169,7 @@ end: } -#ifdef ENABLE_MANAGEMENT +#if defined(ENABLE_MANAGEMENT) && !defined(HAVE_XKEY_PROVIDER) /* encrypt */ static int @@ -1470,7 +1470,9 @@ err: return 0; } #endif /* OPENSSL_VERSION_NUMBER > 1.1.0 dev && !defined(OPENSSL_NO_EC) */ +#endif /* ENABLE_MANAGEMENT && !HAVE_XKEY_PROVIDER */ +#ifdef ENABLE_MANAGEMENT int tls_ctx_use_management_external_key(struct tls_root_ctx *ctx) { diff --git a/src/openvpn/xkey_common.h b/src/openvpn/xkey_common.h index 5bda5e30..608afe99 100644 --- a/src/openvpn/xkey_common.h +++ b/src/openvpn/xkey_common.h @@ -67,10 +67,13 @@ typedef struct { * * @returns 1 on success, 0 on error. * - * The data in tbs is just the digest with no DigestInfo header added. This is + * If sigalg.op = "Sign", the data in tbs is the digest. If sigalg.op = "DigestSign" + * it is the message that the backend should hash wih appropriate hash algorithm before + * signing. In the former case no DigestInfo header is added to tbs. This is * unlike the deprecated RSA_sign callback which provides encoded digest. * For RSA_PKCS1 signatures, the external signing function must encode the digest - * before signing. The digest algorithm used is passed in the sigalg structure. + * before signing. The digest algorithm used (or to be used) is passed in the sigalg + * structure. */ typedef int (XKEY_EXTERNAL_SIGN_fn)(void *handle, unsigned char *sig, size_t *siglen, const unsigned char *tbs, size_t tbslen, diff --git a/src/openvpn/xkey_helper.c b/src/openvpn/xkey_helper.c index 51cfb12b..aac78a2c 100644 --- a/src/openvpn/xkey_helper.c +++ b/src/openvpn/xkey_helper.c @@ -32,6 +32,8 @@ #include "error.h" #include "buffer.h" #include "xkey_common.h" +#include "manage.h" +#include "base64.h" #ifdef HAVE_XKEY_PROVIDER @@ -48,6 +50,31 @@ static const char *const props = XKEY_PROV_PROPS; XKEY_EXTERNAL_SIGN_fn xkey_management_sign; +/** helper to compute digest */ +static int +xkey_digest(const unsigned char *src, size_t srclen, unsigned char *buf, + size_t *buflen, const char *mdname) +{ + dmsg(D_LOW, "In xkey_digest"); + EVP_MD *md = EVP_MD_fetch(NULL, mdname, NULL); /* from default context */ + if (!md) + { + msg(M_WARN, "WARN: xkey_digest: MD_fetch failed for <%s>", mdname); + return 0; + } + + unsigned int len = (unsigned int) *buflen; + if (EVP_Digest(src, srclen, buf, &len, md, NULL) != 1) + { + msg(M_WARN, "WARN: xkey_digest: EVP_Digest failed"); + return 0; + } + EVP_MD_free(md); + + *buflen = len; + return 1; +} + /** * Load external key for signing via management interface. * The public key must be passed in by the caller as we may not @@ -94,13 +121,88 @@ xkey_load_management_key(OSSL_LIB_CTX *libctx, EVP_PKEY *pubkey) return pkey; } -/* not yet implemented */ +/** + * Signature callback for xkey_provider with management-external-key + * + * @param handle Unused -- may be null + * @param sig On successful return signature is in sig. + * @param siglen On entry *siglen has length of buffer sig, + * on successful return size of signature + * @param tbs hash or message to be signed + * @param tbslen len of data in dgst + * @param sigalg extra signature parameters + * + * @return signature length or -1 on error. + */ int xkey_management_sign(void *unused, unsigned char *sig, size_t *siglen, const unsigned char *tbs, size_t tbslen, XKEY_SIGALG alg) { - msg(M_FATAL, "FATAL ERROR: A sign callback for this key is not implemented."); - return 0; + (void) unused; + char alg_str[128]; + unsigned char buf[EVP_MAX_MD_SIZE]; /* for computing digest if required */ + size_t buflen = sizeof(buf); + + if (!strcmp(alg.op, "DigestSign")) + { + dmsg(D_LOW, "xkey_management_sign: computing digest"); + if (xkey_digest(tbs, tbslen, buf, &buflen, alg.mdname)) + { + tbs = buf; + tbslen = buflen; + alg.op = "Sign"; + } + else + { + return 0; + } + } + + if (!strcmp(alg.keytype, "EC")) + { + strncpynt(alg_str, "ECDSA", sizeof(alg_str)); + } + /* else assume RSA key */ + else if (!strcmp(alg.padmode, "pkcs1")) + { + strncpynt(alg_str, "RSA_PKCS1_PADDING", sizeof(alg_str)); + } + else if (!strcmp(alg.padmode, "none")) + { + strncpynt(alg_str, "RSA_NO_PADDING", sizeof(alg_str)); + } + else if (!strcmp(alg.padmode, "pss")) + { + openvpn_snprintf(alg_str, sizeof(alg_str), "%s,hashalg=%s,saltlen=%s", + "RSA_PKCS1_PSS_PADDING", alg.mdname,alg.saltlen); + } + else { + msg(M_NONFATAL, "Unsupported RSA padding mode in signature request<%s>", + alg.padmode); + return 0; + } + dmsg(D_LOW, "xkey management_sign: requesting sig with algorithm <%s>", alg_str); + + char *in_b64 = NULL; + char *out_b64 = NULL; + int len = -1; + + int bencret = openvpn_base64_encode(tbs, (int) tbslen, &in_b64); + + if (management && bencret > 0) + { + out_b64 = management_query_pk_sig(management, in_b64, alg_str); + } + if (out_b64) + { + len = openvpn_base64_decode(out_b64, sig, (int) *siglen); + } + free(in_b64); + free(out_b64); + + *siglen = (len > 0) ? len : 0; + + return (*siglen > 0); } #endif /* HAVE_XKEY_PROVIDER */