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

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200121/0c9530f9/attachment.sig>