Hi Eddie,
Thanks for the follow-up.
This mail is only feedback for the "we need to fix the OpenSSL engine
support breaking builds on OpenSSL 4.x (and some distros with engines
disabled in OpenSSL 3.x) but keep OpenSSL engine support for other
distros that still support it" part. I'll get to the "support for
OpenSSL providers" part in other mails so we don't mix things up.
Patches 2 and 3 are not applicable to "unbreaking-build-on-openssl-4"
part of this series, please keep them separate for a v6.
On 10/29/26 9:13 PM, Eddie Kovsky wrote:
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 <[email protected]>
Date: Fri Sep 20 19:52:48 2024 +0300
sign-file,extract-cert: use pkcs11 provider for OPENSSL MAJOR >= 3
Tested-by: Enric Balletbo i Serra <[email protected]>
Tested-by: Mark Kettenis <[email protected]>
Signed-off-by: Eddie Kovsky <[email protected]>
---
lib/aes/aes-encrypt.c | 4 +-
lib/rsa/rsa-sign.c | 102 +++++++++++++++++++++++++++++++++++++++---
2 files changed, 100 insertions(+), 6 deletions(-)
diff --git a/lib/aes/aes-encrypt.c b/lib/aes/aes-encrypt.c
index def33a3b3248..0cb2221df800 100644
--- a/lib/aes/aes-encrypt.c
+++ b/lib/aes/aes-encrypt.c
@@ -16,7 +16,9 @@
#include <openssl/err.h>
#include <openssl/ssl.h>
#include <openssl/evp.h>
-#include <openssl/engine.h>
+#if !defined(OPENSSL_NO_ENGINE) && !defined(OPENSSL_NO_DEPRECATED_3_0)
+# include <openssl/engine.h>
+#endif
question: Considering there are no other changes in this file, is this
include actually needed? Maybe all we need to do is to unconditionally
remove the include (in a separate patch then, to highlight it really
isn't needed and not related to unbreaking the build).
#include <uboot_aes.h>
#if OPENSSL_VERSION_NUMBER >= 0x10000000L
diff --git a/lib/rsa/rsa-sign.c b/lib/rsa/rsa-sign.c
index 0e38c9e802fd..361ed0647bc6 100644
--- a/lib/rsa/rsa-sign.c
+++ b/lib/rsa/rsa-sign.c
@@ -19,7 +19,47 @@
#include <openssl/err.h>
#include <openssl/ssl.h>
#include <openssl/evp.h>
-#include <openssl/engine.h>
+#if OPENSSL_VERSION_MAJOR >= 3
+# define USE_PKCS11_PROVIDER
+# include <err.h>
+# include <openssl/provider.h>
+# include <openssl/store.h>
+#else
issue: remove for the "unbreaking-build-on-openssl-4" patch as that's
provider-specific.
+# if !defined(OPENSSL_NO_ENGINE) && !defined(OPENSSL_NO_DEPRECATED_3_0)
+# define USE_PKCS11_ENGINE
+# include <openssl/engine.h>
+# endif
+#endif
+
issue: The ifdefery logic is incorrect. This will break distros that are
running OpenSSL 3.x but still have engine support. Such is (was, when I
reported back in May) the case for Debian Trixie (13) and ArchLinux
(they still are both on OpenSSL 3.x). In any case, we still want to
support old(er) distros. I could build this code fine on Fedora 44 with
OpenSSL 3 and openssl-devel-engine installed, so it should still work
the same way now.
So, we know for sure that OpenSSL 4.x doesn't support engine so that's
an easy check. For earlier versions, it actually depends on whether
OpenSSL is built with engine support and we are allowed to use it. From
what I gathered looking at how Fedora handled the "built with engine
supports but doesn't want to let you build new software with engines
unless you have openssl-devel-engine package installed" part and how
Debian allows it, I figured the following (which almost matches your #else).
#if OPENSSL_VERSION_NUMBER < 0x40000000L && !defined(OPENSSL_NO_ENGINE)
&& !defined(OPENSSL_NO_DEPRECATED_3_0)
# define OSSL_ENGINE_SUPPORTED
# include <openssl/engine.h>
#endif
issue: please rename USE_PKCS11_ENGINE, we actually support other
engines so this is misleading and we technically could support both
engines AND providers on OpenSSL 3.x. I can suggest OSSL_ENGINE_SUPPORTED.
+#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 line:%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
issue: remove for the "unbreaking-build-on-openssl-4" patch as that's
provider-specific.
static int rsa_err(const char *msg)
{
@@ -94,10 +134,11 @@ err_cert:
*
* @keydir: Key prefix
* @name Name of key
- * @engine Engine to use
+ * @engine Engine to use or NULL when using pkcs11 provider
issue: unnecessary change for the "unbreaking-build-on-openssl-4" patch
as that's provider-specific.
* @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,21 +198,24 @@ static int rsa_engine_get_pub_key(const char *keydir,
const char *name,
return 0;
}
+#endif
/**
* rsa_get_pub_key() - read a public key
*
* @keydir: Directory containing the key (PEM file) or key prefix (engine)
* @name Name of key file (will have a .crt extension)
- * @engine Engine to use
+ * @engine Engine to use or NULL when using pkcs11 provider
issue: unnecessary change for the "unbreaking-build-on-openssl-4" patch
as that's provider-specific.
* @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)
*/
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
issue: we should fail hard if the user requests to use engines but
mkimage/rsa is built without engine support otherwise we may incorrectly
sign/verify things with keys the user didn't expect. For context, I've
actually managed to build an SPL without a pubkey and "verify" U-Boot
FIT. Fortunately enough, I caught this before shipping an update to a
secure-boot'ed device but the fewest surprises like those we have for
the user, the better.
mkimage will still happily accept the -N argument which sets the engine
parameter, even if engines aren't supported.
I think if you have a file key with the same name as in the engine,
you'll then sign with the wrong key.
Think
mkimage -N myengine -g key
and you have a key.key in the local directory. If you compile with
OpenSSL engines supported, you'll essentially use myengine:key, if not,
you'll use key.key, without mkimage even notifying you by using the same
command.
What I ended up doing is:
static int rsa_engine_get_pub_key(const char *keydir, const char *name,
ENGINE *engine, EVP_PKEY **evpp)
{
#ifdef OSSL_ENGINE_SUPPORTED
[same code as today]
#else
fprintf(stderr, "rsa-sign compiled without engine support\n");
return -ENOTSUP;
#endif
}
so we don't need to patch all callers of rsa_engine_get_pub_key() to
handle engines being supported or not. The same logic can be applied to
rsa_engine_get_priv_key(), rsa_engine_init(), rsa_engine_remove() and
then you have to modify much less of the file since you don't have to
patch every caller of the function (which you had to do because the
function doesn't exist anymore in your implementation).
return rsa_pem_get_pub_key(keydir, name, evpp);
}
@@ -207,13 +251,44 @@ 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
issue: remove for the "unbreaking-build-on-openssl-4" patch as that's
provider-specific.
if (!PEM_read_PrivateKey(f, evpp, NULL, path)) {
rsa_err("Failure reading private key");
fclose(f);
return -EPROTO;
}
fclose(f);
-
+#endif
return 0;
}
@@ -226,6 +301,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,22 +369,25 @@ static int rsa_engine_get_priv_key(const char *keydir,
const char *name,
return 0;
}
+#endif
/**
* rsa_get_priv_key() - read a private key
*
* @keydir: Directory containing the key (PEM file) or key prefix (engine)
* @name Name of key
- * @engine Engine to use for signing
+ * @engine Engine to use or NULL when using pkcs11 provider
issue: unnecessary change for the "unbreaking-build-on-openssl-4" patch
as that's provider-specific.
chore: (not necessarily you, not necessarily now) we need to fix binman
tests when engine support is disabled (e.g.
tools/binman/test/fit/dummy-rsa-engine.c won't compile, and we shouldn't
run engine-related tests in binman, and CST for NXP stuff doesn't
(didn't at least) compile either).
Cheers,
Quentin
