On Thu, Dec 04, 2025 at 12:52:17PM +0100, Quentin Schulz wrote:
> Hi Tom,
> 
> On 12/2/25 9:14 PM, Tom Rini wrote:
> > On Tue, Dec 02, 2025 at 08:06:02PM +0000, Simon Glass wrote:
> > > Hi Quentin,
> > > 
> > > On Wed, 26 Nov 2025 at 04:44, Quentin Schulz <[email protected]> 
> > > wrote:
> > > > 
> > > > Hi Simon,
> > > > 
> > > > On 11/25/25 11:15 PM, Simon Glass wrote:
> > > > > Hi Quentin,
> > > > > 
> > > > > On Fri, 21 Nov 2025 at 10:15, Quentin Schulz <[email protected]> 
> > > > > wrote:
> > > > > > 
> > > > > > From: Quentin Schulz <[email protected]>
> > > > > > 
> > > > > > This adds a test that signs a FIT and verifies the signature with
> > > > > > fit_check_sign.
> > > > > > 
> > > > > > OpenSSL engines are typically for signing with external HW so it's 
> > > > > > not
> > > > > > that straight-forward to simulate.
> > > > > > 
> > > > > > For a simple RSA OpenSSL engine, a dummy engine with a hardcoded RSA
> > > > > > 4096 private key is made available. It can be selected by setting 
> > > > > > the
> > > > > > OpenSSL engine argument to dummy-rsa-engine. This can only be done 
> > > > > > if
> > > > > > the engine is detected by OpenSSL, which works by setting the
> > > > > > OPENSSL_ENGINES environment variable. I have no clue if 
> > > > > > dummy-rsa-engine
> > > > > > is properly implementing what is expected from an RSA engine, but it
> > > > > > seems to be enough for testing.
> > > > > > 
> > > > > > For a simple PKCS11 engine, SoftHSMv2 is used, which allows to do 
> > > > > > PKCS11
> > > > > > without specific hardware. The keypairs and tokens are generated on 
> > > > > > the
> > > > > > fly. The "prod" token is generated with a different PIN (1234 
> > > > > > instead of
> > > > > > 1111) to also test MKIMAGE_SIGN_PIN env variable while we're at it.
> > > > > > 
> > > > > > Binman will not mess with the local SoftHSMv2 setup as it will only 
> > > > > > use
> > > > > > tokens from a per-test temporary directory enforced via the 
> > > > > > temporary
> > > > > > configuration file set via SOFTHSM2_CONF env variable in the tests. 
> > > > > > The
> > > > > > files created in the input dir should NOT be named the same as it is
> > > > > > shared between all tests in the same process (which is all tests 
> > > > > > when
> > > > > > running binman with -P 1 or with -T).
> > > > > > 
> > > > > > Once signed, it's checked with fit_check_sign with the associated
> > > > > > certificate.
> > > > > > 
> > > > > > Finally, a new softhsm2_util bintool is added so that we can 
> > > > > > initialize
> > > > > > the token and import keypairs. On Debian, the package also brings
> > > > > > libsofthsm2 which is required for OpenSSL to interact with 
> > > > > > SoftHSMv2. It
> > > > > > is not the only package required though, as it also needs p11-kit 
> > > > > > and
> > > > > > libengine-pkcs11-openssl (the latter bringing the former). We can 
> > > > > > detect
> > > > > > if it's properly installed by running openssl engine dynamic -c 
> > > > > > pkcs11.
> > > > > > If that fails, we simply skip the test.
> > > > > > The package is installed in the CI container by default.
> > > > > > 
> > > > > > Signed-off-by: Quentin Schulz <[email protected]>
> > > > > > ---
> > > > > >    tools/binman/btool/softhsm2_util.py                |  21 ++
> > > > > >    tools/binman/ftest.py                              | 223 
> > > > > > +++++++++++++++++++++
> > > > > >    tools/binman/test/340_dummy-rsa4096.crt            |  31 +++
> > > > > >    tools/binman/test/340_fit_signature_engine.dts     |  99 
> > > > > > +++++++++
> > > > > >    .../test/340_fit_signature_engine_encrypt.dts      | 100 
> > > > > > +++++++++
> > > > > >    .../test/340_fit_signature_engine_pkcs11.dts       |  99 
> > > > > > +++++++++
> > > > > >    .../340_fit_signature_engine_pkcs11_object.dts     | 100 
> > > > > > +++++++++
> > > > > >    tools/binman/test/340_openssl.conf                 |  10 +
> > > > > >    tools/binman/test/340_softhsm2.conf                |  16 ++
> > > > > >    tools/binman/test/Makefile                         |   6 +-
> > > > > >    tools/binman/test/dummy-rsa-engine.c               | 149 
> > > > > > ++++++++++++++
> > > > > >    11 files changed, 853 insertions(+), 1 deletion(-)
> > > > > 
> > > > > Not sure of the changes from last time, but I assume the test coverage
> > > > > is finished.
> > > > > 
> > > > 
> > > > They are listed in the cover letter in the Changes section.
> > > > 
> > > > $ b4 diff -v 2 3 --
> > > > https://lore.kernel.org/u-boot/[email protected]/T/\#t
> > > > 
> > > > will show you the git-range-diff between both versions for a given 
> > > > commit.
> > > 
> > > I normally review just in email (often on a Chromebook) so I don't
> > > have that. It is also an extra step and I don't know where your log
> > > argument comes from. It would be better to put the change log in the
> > > patch as well.
> > 
> > The cover letter is just an email. Perhaps a handy tips bit of
> > documentation (and external ref to the general b4 docs) would be
> > helpful, especially since b4 is a common and widely used tool these
> > days.
> 
> I can do that, what do you have in mind? What should we add to the docs?

Honestly, whatever you've found useful as a contributor and reviewer.
I'll follow-up with some handy things for custodians (mainly patchwork
integration and that cover letters are so important because it's an
automatic useful merge commit message). Thanks!

-- 
Tom

Attachment: signature.asc
Description: PGP signature

Reply via email to