From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 98FEFFD3772 for ; Wed, 25 Feb 2026 16:18:10 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 1C7C183F00; Wed, 25 Feb 2026 17:18:09 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=quarantine dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.b="e9Yj7qZz"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 330E683FD4; Wed, 25 Feb 2026 17:18:08 +0100 (CET) Received: from sea.source.kernel.org (sea.source.kernel.org [IPv6:2600:3c0a:e001:78e:0:1991:8:25]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 701EC83F00 for ; Wed, 25 Feb 2026 17:16:42 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=quarantine dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=tempfail smtp.mailfrom=mkorpershoek@kernel.org Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sea.source.kernel.org (Postfix) with ESMTP id D0CF641B34; Wed, 25 Feb 2026 16:16:35 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 23684C116D0; Wed, 25 Feb 2026 16:16:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1772036195; bh=mN4HFg+26zwjQVYRx076H7j15Q0uY8PuLLAugbNGFHU=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=e9Yj7qZzIUG2iPZSemmf5jKD6RJyylceJmvSf7hI/+72OXTuk1OhIg4dZKMBXjDcs Ul19wf5x1OkM68Pi4ovV2YW6ekfBp3wSyMkCuDMcee0phoB4ZmEkLcxHGQ41z8cQRd Mu1t8GNmdIro1eA4oUJotvC0TRyJVP1f0VVpX7PD3QAHJ6HxSfMhbkrxyamQqn81BG iUVWjO1c/HeaQYAW+S0WW3bGLjddqJWUDBn1X35WV7gROkKO9K0ED4hY3Jawsl0VRt woY6OGMfQZN3P/NmlARthC+2GZAup5lrGMYhrYRU71YbSekPiPVo93t2E7gzDTVWQP pFFWdPtuFI2Ow== From: Mattijs Korpershoek To: Eddie Kovsky , Mattijs Korpershoek Cc: Eddie Kovsky , Tom Rini , Tobias Olausson , Paul HENRYS , Simon Glass , Jan Stancek , Enric Balletbo i Serra , a.fatoum@pengutronix.de, mark.kettenis@xs4all.nl, u-boot@lists.denx.de Subject: Re: [PATCH v3] Add support for OpenSSL Provider API In-Reply-To: References: <20260120164524.253188-1-ekovsky@redhat.com> <87ikckmbbi.fsf@kernel.org> Date: Wed, 25 Feb 2026 17:16:32 +0100 Message-ID: <87a4ww23z3.fsf@kernel.org> MIME-Version: 1.0 Content-Type: text/plain X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.8 at phobos.denx.de X-Virus-Status: Clean Hi Eddie, On Thu, Feb 19, 2026 at 09:51, Eddie Kovsky wrote: > On 01/29/26, Mattijs Korpershoek wrote: >> Hi Eddie, >> >> Thank you for the patch. >> > > Hi Mattijs > > Thanks for the review. > >> On Tue, Jan 20, 2026 at 09:45, Eddie Kovsky wrote: >> >> > The Engine API has been deprecated since the release of OpenSSL 3.0. End >> > users have been advised to migrate to the new Provider interface. >> > Several distributions have already removed support for engines, which is >> > preventing U-Boot from being compiled in those environments. >> > >> > Add support for the Provider API while continuing to support the existing >> > Engine API on distros shipping older releases of OpenSSL. >> > >> > This is based on similar work contributed by Jan Stancek updating Linux >> > to use the Provider interface. >> > >> > commit 558bdc45dfb2669e1741384a0c80be9c82fa052c >> > Author: Jan Stancek >> > Date: Fri Sep 20 19:52:48 2024 +0300 >> > >> > sign-file,extract-cert: use pkcs11 provider for OPENSSL MAJOR >= 3 >> > >> > The changes have been tested with the FIT signature verification vboot >> > tests on Fedora 42 and Debian 13. All 30 tests pass with both the legacy >> > Engine library installed and with the Provider API. >> > >> > Signed-off-by: Eddie Kovsky >> >> As a follow-up, can we look into reverting/removing >> commit 3a8b919932fd ("tools: avoid OpenSSL deprecation warnings") ? >> > > Yes, we could certainly revert this once we've determined it's no longer > needed for CI. But it's outside the scope of what I'm proposing for this > patch. Yes agreed, this can be done later on and is out of scope for this patch. > >> This looks much better than v2 in my opinion. >> >> Some additional comments below: >> >> > --- >> > Changes in v3: >> > - Removed Kconfig option >> > - Changed macro symbol from CONFIG_OPENSSL_NO_DEPRECATED to >> > USE_PKCS11_PROVIDER or USE_PKCS11_ENGINE >> > v2: https://lore.kernel.org/u-boot/20251027195834.71109-1-ekovsky@redhat.com/ >> > >> > Changes in v2: >> > - Remove default for new Kconfig option >> > - Use #ifdef instead of IS_ENABLED macro >> > - Remove comment after #endif >> > - Remove unrelated checkpatch cleanup of 'sslErr' variable name >> > v1: https://lore.kernel.org/u-boot/20251017171329.255689-1-ekovsky@redhat.com/ >> > --- >> > lib/aes/aes-encrypt.c | 4 +- >> > lib/rsa/rsa-sign.c | 95 ++++++++++++++++++++++++++++++++++++++++++- >> > 2 files changed, 97 insertions(+), 2 deletions(-) >> > >> > diff --git a/lib/aes/aes-encrypt.c b/lib/aes/aes-encrypt.c >> > index 90e1407b4f09..4fc4ce232478 100644 >> > --- a/lib/aes/aes-encrypt.c >> > +++ b/lib/aes/aes-encrypt.c >> > @@ -16,7 +16,9 @@ >> > #include >> > #include >> > #include >> > -#include >> > +#if !defined(OPENSSL_NO_ENGINE) && !defined(OPENSSL_NO_DEPRECATED_3_0) >> > +# include >> > +#endif >> > #include >> > >> > #if OPENSSL_VERSION_NUMBER >= 0x10000000L >> > diff --git a/lib/rsa/rsa-sign.c b/lib/rsa/rsa-sign.c >> > index 0e38c9e802fd..31269db65950 100644 >> > --- a/lib/rsa/rsa-sign.c >> > +++ b/lib/rsa/rsa-sign.c >> > @@ -19,7 +19,47 @@ >> > #include >> > #include >> > #include >> > -#include >> > +#if OPENSSL_VERSION_MAJOR >= 3 >> > +# define USE_PKCS11_PROVIDER >> > +# include >> > +# include >> > +# include >> > +#else >> > +# if !defined(OPENSSL_NO_ENGINE) && !defined(OPENSSL_NO_DEPRECATED_3_0) >> > +# define USE_PKCS11_ENGINE >> > +# include >> > +# endif >> > +#endif >> > + >> > +#ifdef USE_PKCS11_PROVIDER >> > +#define ERR(cond, fmt, ...) \ >> > + do { \ >> > + bool __cond = (cond); \ >> > + drain_openssl_errors(__LINE__, 0); \ >> > + if (__cond) { \ >> > + errx(1, fmt, ## __VA_ARGS__); \ >> > + } \ >> > + } while (0) >> > + >> > +static void drain_openssl_errors(int l, int silent) >> > +{ >> > + const char *file; >> > + char buf[120]; >> > + int e, line; >> > + >> > + if (ERR_peek_error() == 0) >> > + return; >> > + if (!silent) >> > + fprintf(stderr, "At main.c:%d:\n", l); >> > + >> > + while ((e = ERR_peek_error_line(&file, &line))) { >> > + ERR_error_string(e, buf); >> > + if (!silent) >> > + fprintf(stderr, "- SSL %s: %s:%d\n", buf, file, line); >> > + ERR_get_error(); >> > + } >> > +} >> > +#endif >> > >> > static int rsa_err(const char *msg) >> > { >> > @@ -98,6 +138,7 @@ err_cert: >> > * @evpp Returns EVP_PKEY object, or NULL on failure >> > * Return: 0 if ok, -ve on error (in which case *evpp will be set to NULL) >> > */ >> > +#ifdef USE_PKCS11_ENGINE >> > static int rsa_engine_get_pub_key(const char *keydir, const char *name, >> > ENGINE *engine, EVP_PKEY **evpp) >> > { >> > @@ -157,6 +198,7 @@ static int rsa_engine_get_pub_key(const char *keydir, const char *name, >> > >> > return 0; >> > } >> > +#endif >> > >> > /** >> > * rsa_get_pub_key() - read a public key >> > @@ -170,8 +212,10 @@ static int rsa_engine_get_pub_key(const char *keydir, const char *name, >> >> With this change, the ENGINE pointer might be NULL (or undefined). >> Can we please update the documentation comment to reflect this? >> >> For example, we could reword as: >> * @engine Engine to use or NULL when using pcks11 provider >> > > Sure, I can update the comment for v4. >> > static int rsa_get_pub_key(const char *keydir, const char *name, >> > ENGINE *engine, EVP_PKEY **evpp) >> > { >> > +#ifdef USE_PKCS11_ENGINE >> > if (engine) >> > return rsa_engine_get_pub_key(keydir, name, engine, evpp); >> > +#endif >> > return rsa_pem_get_pub_key(keydir, name, evpp); >> > } >> >> Actually, looking even closer at this function, it's seems to be called >> only once. >> >> Why can't we drop this function alltogether and call >> rsa_engine_get_pub_key() / rsa_pem_get_pub_key() directly in >> rsa_add_verify_data() ? >> >> Reason I'm asking: in rsa_add_verify_data(), ENGINE *e is not used when >> we use PROVIDER. It seems weird (and error prone) to pass a NULL pointer >> to a function that does not need that argument >> > > Yes, we could drop rsa_get_pub_key(). It is set up as a helper function and > only called once from rsa_add_verify_data(). > > But I am hesitant to make any changes to the RSA API in this file. I > want to limit the scope of this patch so that the existing code is > unchanged for users of the Engine API. And I think removing this > function would require adding more #ifdefs around the error handling in > rsa_add_verify_data(). I'm fine as well if you keep it this way. > >> > >> > @@ -207,6 +251,37 @@ static int rsa_pem_get_priv_key(const char *keydir, const char *name, >> > return -ENOENT; >> > } >> > >> > +#ifdef USE_PKCS11_PROVIDER >> > + EVP_PKEY *private_key = NULL; >> > + OSSL_STORE_CTX *store; >> > + >> > + if (!OSSL_PROVIDER_try_load(NULL, "pkcs11", true)) >> > + ERR(1, "OSSL_PROVIDER_try_load(pkcs11)"); >> > + if (!OSSL_PROVIDER_try_load(NULL, "default", true)) >> > + ERR(1, "OSSL_PROVIDER_try_load(default)"); >> > + >> > + store = OSSL_STORE_open(path, NULL, NULL, NULL, NULL); >> > + ERR(!store, "OSSL_STORE_open"); >> > + >> > + while (!OSSL_STORE_eof(store)) { >> > + OSSL_STORE_INFO *info = OSSL_STORE_load(store); >> > + >> > + if (!info) { >> > + drain_openssl_errors(__LINE__, 0); >> > + continue; >> > + } >> > + if (OSSL_STORE_INFO_get_type(info) == OSSL_STORE_INFO_PKEY) { >> > + private_key = OSSL_STORE_INFO_get1_PKEY(info); >> > + ERR(!private_key, "OSSL_STORE_INFO_get1_PKEY"); >> > + } >> > + OSSL_STORE_INFO_free(info); >> > + if (private_key) >> > + break; >> > + } >> > + OSSL_STORE_close(store); >> > + >> > + *evpp = private_key; >> > +#else >> > if (!PEM_read_PrivateKey(f, evpp, NULL, path)) { >> > rsa_err("Failure reading private key"); >> > fclose(f); >> > @@ -214,6 +289,7 @@ static int rsa_pem_get_priv_key(const char *keydir, const char *name, >> > } >> > fclose(f); >> > >> > +#endif >> > return 0; >> >> This block should be >> >> fclose(f); >> +#endif >> >> return 0; >> >> (not having a blank line between the fclose and the #endif) >> >> > } > > I'll clean that up in v4. >> > >> > @@ -226,6 +302,7 @@ static int rsa_pem_get_priv_key(const char *keydir, const char *name, >> > * @evpp Returns EVP_PKEY object, or NULL on failure >> > * Return: 0 if ok, -ve on error (in which case *evpp will be set to NULL) >> > */ >> > +#ifdef USE_PKCS11_ENGINE >> > static int rsa_engine_get_priv_key(const char *keydir, const char *name, >> > const char *keyfile, >> > ENGINE *engine, EVP_PKEY **evpp) >> > @@ -293,6 +370,7 @@ static int rsa_engine_get_priv_key(const char *keydir, const char *name, >> > >> > return 0; >> > } >> > +#endif >> > >> > /** >> > * rsa_get_priv_key() - read a private key >> > @@ -306,9 +384,11 @@ static int rsa_engine_get_priv_key(const char *keydir, const char *name, >> > static int rsa_get_priv_key(const char *keydir, const char *name, >> > const char *keyfile, ENGINE *engine, EVP_PKEY **evpp) >> > { >> > +#ifdef USE_PKCS11_ENGINE >> > if (engine) >> > return rsa_engine_get_priv_key(keydir, name, keyfile, engine, >> > evpp); >> > +#endif >> > return rsa_pem_get_priv_key(keydir, name, keyfile, evpp); >> >> Same remark as for rsa_engine_get_pub_key. Can't we drop this static >> function? It's only called once. >> >> Maybe do a cleanup patch first, that gets rid of the static functions >> and then do the provider support in a second patch of the same series? >> >> I think it will reduce the amount of #ifdefs, which seems a good >> argument. >> > > Again, I am trying to limit the scope of this proposal to preserve the > existing code. If we remove the helper function rsa_get_priv_key() then > the #ifdef guards also have to move inside the caller rsa_sign(). And > since we would no longer be able to check the return value of > rsa_get_priv_key() additional guards would be needed to recreate the > return value logic that's already in the helper function. No worries, I'm fine if you keep it this way for v4. > > > Eddie