[U-Boot] [PATCH v1 00/11] import x509/pkcs7 parsers from linux

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Tom,

On Fri, Oct 18, 2019 at 08:35:23AM -0400, Tom Rini wrote:
> On Fri, Oct 18, 2019 at 05:36:28PM +0900, AKASHI Takahiro wrote:
> > Hi Tom,
> > 
> > On Thu, Oct 17, 2019 at 11:23:21AM -0400, Tom Rini wrote:
> > > On Tue, Oct 15, 2019 at 06:25:19PM +0900, AKASHI Takahiro wrote:
> > > > On Tue, Oct 15, 2019 at 07:33:18AM +0200, Heinrich Schuchardt wrote:
> > > > > On 10/15/19 5:18 AM, AKASHI Takahiro wrote:
> > > > > >On Sat, Oct 12, 2019 at 03:02:09PM +0200, Heinrich Schuchardt wrote:
> > > > > >>On 10/11/19 9:55 AM, AKASHI Takahiro wrote:
> > > > > >>>I hope this patch set will be reviewed promptly as I'm aiming to
> > > > > >>>push my "UEFI secure boot" patch for v2020.01.
> > > > > >>>
> > > > > >>
> > > > > >>How can I make all of these new files being built to check for build
> > > > > >>warnings?
> > > > > >
> > > > > >As always in my case of UEFI secure boot, they have gone through build and
> > > > > >run/tests as part of UEFI secure boot. This is also true for RSA
> > > > > >extension as UEFI secure boot is the only user of those features.
> > > > > 
> > > > > Did you run them through Travis?
> > > > 
> > > > As far as this patch set is concerned, no configuration enables
> > > > any of kconfig options listed below and running Travis doesn't make sense.
> > > 
> > > That's a problem that needs to be fixed.  I am not OK with the idea of
> > > adding a new feature that will not be put through our CI and so when
> > > things break it (which will happen, inadvertently) it won't be caught
> > > until much later.  Figuring out how to extend our CI to test this is a
> > > must.  Thanks!
> > 
> > I added a simple *unit* test under "test/lib."
> > As I said before, however, no existing configuration enables either
> >    CONFIG_X509_CERTIFICATE_PARSER, nor
> >    CONFIG_PKCS7_MESSAGE_PARSER
> > 
> > and the related code won't be built or exercised in any way.
> > So I made a small trick to Kconfig:
> > 
> > === 8< ===
> > config UT_LIB
> >         bool "Unit tests for library functions"
> >         depends on UNIT_TEST
> >         default y
> >         help
> >           Enables the 'ut lib' command which tests library functions like
> >           memcat(), memcyp(), memmove().
> > 
> > if UT_LIB
> > 
> > config UT_LIB_ASN1
> >         bool "Unit test for asn1 compiler and decoder function"
> >         default y
> >         imply ASYMMETRIC_KEY_TYPE
> >         imply ASYMMETRIC_PUBLIC_KEY_SUBTYPE
> >         imply X509_CERTIFICATE_PARSER
> >         imply PKCS7_MESSAGE_PARSER
> >         imply RSA_PUBLIC_KEY_PARSER
> >         help
> >           Enables a test which exercises asn1 compiler and decoder function
> >           via various parsers.
> > 
> > endif
> > === >8 ===
> > 
> > So as long as UT_LIB is enabled and run by one of Travis CI test cases,
> > a new test for ASN1 will also be exercised.
> > (I don't know which one will invoke "ut" command.)
> > 
> > Do you agree to this approach?

I'd like to confirm exactly what you suggested here:

> I think you're going to need to get a bit more familiar with some
> aspects of testing and building.  Today, CONFIG_UNIT_TEST is enabled for
> sandbox.  And we want as much as possible enabled on sandbox as that's
> also where coverity scan is performed.

Is this ("as much as possible") true? I wonder how it should be achieved.

As far as my RSA test approach above is concerned,
1) If UT_TEST is enabled, UT_LIB and then UT_LIB_ASN1 are also
   enabled *by default* and expected to be run automatically through
   Travis's sandbox build with test/py even if, say, X509_CERTIRFFICATE_PARSER
   is *not* enabled in any of sandbox_*_defconfig.
   Is this approach is OK for you?

Or,
2) Should we add ASYMMETRIC_*/X509_CERTIFICATE_PARSER/PKCS7_MESSAGE_PARSER
   to one (or all) of sandbox_*_defconfig (even though there is no explicit
   user of these features before my secure boot patch will be merged)?
Or,
3) Should we create a new sandbox_*_defconfig for any further tests?
Or,
4) Would we better set ASYMMETRIC_*/X509_CERTIFICATE_PARSER/PKCS7_MESSAGE_PARSER
   enabled by default on Sandbox? I mean,
=== 8< ===
menuconfig ASYMMETRIC_KEY_TYPE
	bool "Asymmetric (public-key cryptographic) key Support"
	default y if SANDBOX	<== added

if ASYMMETRIC_KEY_TYPE
...
config X509_CERTIFICATE_PARSER
        bool "RSA public key parser"
        depends on ASYMMETRIC_PUBLIC_KEY_SUBTYPE
	default y if SANDBOX	<== added
...
=== >8 ===

> So that will get us part of the
> way forward here longer term.  I think however you're going to also need
> to enable some tests on the QEMU platforms so that we can have all of
> this new secure boot code put through CI there.  Thanks!

Are you talking about this patch set(asn1 parsers) or is this your
general comment on my overall "UEFI secure boot" patch?

Please note that my secure boot patch is architecture agnostic and  will
perfectly work on Sandbox and all the related py tests will also be done
on Sandbox.

Thanks,
-Takahiro Akashi


> -- 
> Tom