[PATCH v4 6/6] test: add rsa_verify() unit test

[AKASHI Takahiro]

On Tue, Jan 21, 2020 at 10:40:23AM -0500, Tom Rini wrote:
> On Tue, Jan 21, 2020 at 02:48:52PM +0900, AKASHI Takahiro wrote:
> > On Fri, Jan 17, 2020 at 06:26:34AM +0100, Heinrich Schuchardt wrote:
> > > On 1/17/20 2:53 AM, AKASHI Takahiro wrote:
> > > >On Tue, Jan 14, 2020 at 01:04:58PM +0100, Heinrich Schuchardt wrote:
> > > >>On 1/14/20 8:33 AM, AKASHI Takahiro wrote:
> > > >>>On Wed, Jan 08, 2020 at 06:43:51PM +0100, Heinrich Schuchardt wrote:
> > > >>>>
> > > >>>>
> > > >>>>On 11/21/19 1:11 AM, AKASHI Takahiro wrote:
> > > >>>>>In this patch, a very simple test is added to verify that rsa_verify()
> > > >>>>>using rsa_verify_with_pkey() work correctly.
> > > >>>>>
> > > >>>>>To keep the code simple, all the test data, either public key and
> > > >>>>>verified binary data, are embedded in the source.
> > > >>>>>
> > > >>>>>Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> > > >>>>>---
> > > >>>>>  test/Kconfig      |  12 +++
> > > >>>>>  test/lib/Makefile |   1 +
> > > >>>>>  test/lib/rsa.c    | 206 ++++++++++++++++++++++++++++++++++++++++++++++
> > > >>>>>  3 files changed, 219 insertions(+)
> > > >>>>>  create mode 100644 test/lib/rsa.c
> > > >>>>>
> > > >>>>>diff --git a/test/Kconfig b/test/Kconfig
> > > >>>>>index cb7954041eda..64d76c3b20a5 100644
> > > >>>>>--- a/test/Kconfig
> > > >>>>>+++ b/test/Kconfig
> > > >>>>>@@ -28,6 +28,18 @@ config UT_LIB_ASN1
> > > >>>>>  	  Enables a test which exercises asn1 compiler and decoder function
> > > >>>>>  	  via various parsers.
> > > >>>>>
> > > >>>>>+config UT_LIB_RSA
> > > >>>>>+	bool "Unit test for rsa_verify() function"
> > > >>>>>+	imply RSA
> > > >>>>>+	imply ASYMMETRIC_KEY_TYPE
> > > >>>>>+	imply ASYMMETRIC_PUBLIC_KEY_SUBTYPE
> > > >>>>>+	imply RSA_PUBLIC_KEY_PARSER
> > > >>>>>+	imply RSA_VERIFY_WITH_PKEY
> > > >>>>
> > > >>>>This test should depend on RSA_VERIFY_WITH_PKEY because is cannot be
> > > >>>>executed otherwise.
> > > >>>
> > > >>>See below.
> > > >>>
> > > >>>>Best regards
> > > >>>>
> > > >>>>Heinrich
> > > >>>>
> > > >>>>>+	default y
> > > >>>>>+	help
> > > >>>>>+	  Enables rsa_verify() test, currently rsa_verify_with_pkey only()
> > > >>>>>+	  only, at the 'ut lib' command.
> > > >>>>>+
> > > >>>>>  endif
> > > >>>>>
> > > >>>>>  config UT_TIME
> > > >>>>>diff --git a/test/lib/Makefile b/test/lib/Makefile
> > > >>>>>index 72d2ec74b5f4..2bf6ef3935bb 100644
> > > >>>>>--- a/test/lib/Makefile
> > > >>>>>+++ b/test/lib/Makefile
> > > >>>>>@@ -8,3 +8,4 @@ obj-y += lmb.o
> > > >>>>>  obj-y += string.o
> > > >>>>>  obj-$(CONFIG_ERRNO_STR) += test_errno_str.o
> > > >>>>>  obj-$(CONFIG_UT_LIB_ASN1) += asn1.o
> > > >>>>>+obj-$(CONFIG_UT_LIB_RSA) += rsa.o
> > > >>>>>diff --git a/test/lib/rsa.c b/test/lib/rsa.c
> > > >>>>>new file mode 100644
> > > >>>>>index 000000000000..44f8ade226f4
> > > >>>>>--- /dev/null
> > > >>>>>+++ b/test/lib/rsa.c
> > > >>>>>@@ -0,0 +1,206 @@
> > > >>>>>+// SPDX-License-Identifier: GPL-2.0+
> > > >>>>>+/*
> > > >>>>>+ * Copyright (c) 2019 Linaro Limited
> > > >>>>>+ * Author: AKASHI Takahiro
> > > >>>>>+ *
> > > >>>>>+ * Unit test for rsa_verify() function
> > > >>>>>+ */
> > > >>>>>+
> > > >>>>>+#include <common.h>
> > > >>>>>+#include <command.h>
> > > >>>>>+#include <image.h>
> > > >>>>>+#include <test/lib.h>
> > > >>>>>+#include <test/test.h>
> > > >>>>>+#include <test/ut.h>
> > > >>>>>+#include <u-boot/rsa.h>
> > > >>>>>+
> > > >>>>>+#ifdef CONFIG_RSA_VERIFY_WITH_PKEY
> > > >>>>
> > > >>>>Please, remove this ifdef. Simply do not build this module if
> > > >>>>CONFIG_RSA_VERIFY_WITH_PKEY is not defined.
> > > >>>
> > > >>>That is why I added this #ifdef here to avoid a build error.
> > > >>>I think it is a good idea to make test code as generic as possible
> > > >>>to easily add more test cases.
> > > >>
> > > >>It is preferable to move configuration dependencies to the Makefile.
> > > >>
> > > >>You can create a new C file if you have tests needing a different
> > > >>configuration.
> > > >>
> > > >>test/lib/rsa_verify.c might be a better name for the file.
> > > >>
> > > >>In test/Kconfig, please, replace
> > > >>
> > > >>config UT_LIB_RSA
> > > >>         bool "Unit test for rsa_verify() function"
> > > >>         imply RSA
> > > >>
> > > >>by
> > > >>
> > > >>config UT_LIB_RSA
> > > >>         bool "Unit test for rsa_verify() function"
> > > >>	default y
> > > >>         depends on RSA
> > > >
> > > >Initially in my development code, I did the exact same thing,
> > > >but I didn't adopt this approach.
> > > >
> > > >If we follow your proposal, UT_LIB_RSA won't be selected
> > > >unless RSA is enabled.
> > > >So UT_LIB_RSA won't be executed under most existing configurations
> > > >in the current CI.  This was not what I wanted.
> > > >
> > > >What I wanted here is that, if UT (and UT_LIB) is selected, all
> > > >the configurations needed for enabling all the test cases be
> > > >also selected *automatically*.
> > > 
> > > What I want is that if I run 'make menuconfig' and select CONFIG_UT
> > > manually I end up with a configuration that compiles and that does what
> > > I have configured.
> > 
> > Hmm, that is what I intend to do here, but the code doesn't work
> > as I expected. It works only if CONFIG_UT_LIB_RSA is on in *defconfig.
> > 
> > So I have no option other than changing reverse dependencies as follows,
> >     CONFIG_UT_LIB_RSA
> >       select IMAGE_SIGN_INFO
> >       select RSA
> >       imply RSA_VERIFY_WITH_PKEY
> > 
> > Using 'select' is still safe as CONFIG_RSA has no dependency.
> > (Please note we can still use 'imply' for VERIFY_WITH_PKEY here.)
> > 
> > Tom, do you agree to this change?
> 
> OK, my fault for not spotting it before.  Using imply (as we do today on
> UT_LIB_ASN1) is wrong.  All of the tests should "depends on" what they
> need in order to function.  Thanks!

I know that using "depends on" is an obvious solution here, but
in that case, we won't run those tests automatically in CI unless
the dependency targets, like CONFIG_RSA, are explicitly or
implicitly enabled by *defconfig.

Is that OK for you?

-Takahiro Akashi

> -- 
> Tom