Hi Eddie,
On 2026-10-29T20:13:40+11:00, Eddie Kovsky <[email protected]> wrote:
> Add support for OpenSSL Provider API
Please can you add a prefix, e.g. 'rsa: Add support for the OpenSSL
Provider API' here? Also, the commit message says the Engine API is
kept for older OpenSSL releases but doesn't say that engine support is
dropped completely on OpenSSL 3.x, even though 3.x still has engines
(deprecated, but present). This means 'mkimage -N' stops working
there, so please can you say so in the commit message and explain why?
> diff --git a/lib/aes/aes-encrypt.c b/lib/aes/aes-encrypt.c
> @@ -16,7 +16,9 @@
> -#include <openssl/engine.h>
> +#if !defined(OPENSSL_NO_ENGINE) && !defined(OPENSSL_NO_DEPRECATED_3_0)
> +# include <openssl/engine.h>
> +#endif
As far as I can see nothing in aes-encrypt.c uses the engine API, so
you could just drop this include.
> diff --git a/lib/rsa/rsa-sign.c b/lib/rsa/rsa-sign.c
> @@ -19,7 +19,47 @@
> +#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)
This comes from the Linux sign-file tool. That is a standalone
program, so errx() is fine there. Here we are in library code called
from mkimage, binman and so on, and the rest of the file reports
failures by returning an error code. Please can you use rsa_err() and
return an error instead? Then the ERR() macro, drain_openssl_errors()
(whose 'silent' argument is always 0) and the <err.h> include can all
go.
> diff --git a/lib/rsa/rsa-sign.c b/lib/rsa/rsa-sign.c
> @@ -207,13 +251,44 @@ static int rsa_pem_get_priv_key(const char *keydir,
> const char *name,
> +#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)");
Hmmm, this path handles ordinary PEM key files, but it now needs the
pkcs11 provider, and mkimage exits if that is missing. So anyone
signing with a plain .key file on OpenSSL 3 now needs pkcs11-provider,
which I suspect is why patch 2 adds it. As far as I know
PEM_read_PrivateKey() is not deprecated in 3.x, so is any change
needed here at all? I'd suggest keeping the existing PEM code for
files and only using OSSL_STORE with the pkcs11 provider for a
'pkcs11:' URI, as patch 3 already does. What do you think?
> + while (!OSSL_STORE_eof(store)) {
> + OSSL_STORE_INFO *info = OSSL_STORE_load(store);
> +
> + if (!info) {
> + drain_openssl_errors(__LINE__, 0);
> + continue;
> + }
If OSSL_STORE_load() keeps failing before EOF, this loop never ends.
Patch 3 checks OSSL_STORE_error() in its copy of this loop, and the
same check is needed here.
> + OSSL_STORE_close(store);
> +
> + *evpp = private_key;
> +#else
> + if (!PEM_read_PrivateKey(f, evpp, NULL, path)) {
> + rsa_err("Failure reading private key");
> + fclose(f);
> + return -EPROTO;
> + }
> + fclose(f);
> -
> +#endif
> return 0;
There are two problems here. First, the provider path opens 'f' above
but never closes it. Second, if no key is found the function returns 0
with *evpp set to NULL, so the caller goes on to sign with a NULL key.
Please can you return -EPROTO in that case, as the PEM path does?
> diff --git a/lib/rsa/rsa-sign.c b/lib/rsa/rsa-sign.c
> @@ -480,11 +561,13 @@ int rsa_sign(struct image_sign_info *info,
> +#ifdef USE_PKCS11_ENGINE
> if (info->engine_id) {
> ret = rsa_engine_init(info->engine_id, &e);
> if (ret)
> return ret;
> }
> +#endif
On OpenSSL 3, -N is now silently ignored. If an engine is passed,
mkimage should fail with a clear message rather than carry on, and
rsa_add_verify_data() needs the same check.
Also, binman's testFitSignEngine() uses the dummy-rsa-engine, and the
pkcs11 engine tests use softhsm2 with the engine. I suspect these fail
on a 3.x host after this change. Please can you run 'binman test' and
update the tests and the fit,engine documentation in
tools/binman/etype/fit.py to match?
>
> 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(-)
Regards,
Simon
