WIP: Signing TI x509 certificates using binman

[Simon Glass]

Hi Neha,

On Mon, 3 Apr 2023 at 02:19, Neha Malcom Francis <n-francis at ti.com> wrote:
>
> Hi Simon
>
> On 02/04/23 18:00, Neha Malcom Francis wrote:
> > Hi Simon
> >
> > On 01/04/23 12:02, Simon Glass wrote:
> >> Hi Neha,
> >>
> >> On Sat, 1 Apr 2023 at 00:14, Neha Malcom Francis <n-francis at ti.com>
> >> wrote:
> >>>
> >>> Hi Simon
> >>>
> >>> On 31/03/23 02:01, Simon Glass wrote:
> >>>> Hi Neha,
> >>>>
> >>>> On Fri, 24 Mar 2023 at 22:28, Neha Malcom Francis <n-francis at ti.com>
> >>>> wrote:
> >>>>>
> >>>>> Hi Simon,
> >>>>>
> >>>>> Before I roll out the entire series that works for packaging K3
> >>>>> bootloader images, wanted to get some reviews and comments
> >>>>> regarding the
> >>>>> implementation of the signing etype [1] . I believe I've taken into
> >>>>> consideration what we've discussed earlier [2].
> >>>>>
> >>>>> Let me know your thoughts. The tree is also WIP, and was mainly
> >>>>> designed
> >>>>> for testing the signing etype on two of the devices. I will add the
> >>>>> remaining and refine the series based on the next comments.
> >>>>
> >>>> Yes this looks reasonable to me. For the openssl method, can you
> >>>> create a new 'real' method and put the cert stuff in there instead of
> >>>> using a 'custom' one? It seems to have a lot in common with what is
> >>>> there. We should really have an internal cert-generator method in that
> >>>> class, with other functions in that class doing appropriate things. I
> >>>> am looking for code reuse here, as well as clear indication of what
> >>>> the cert is for or does.
> >>>
> >>> So you're suggesting to include the config creation within the openssl
> >>> btool, am I right? For example methods to generate a config, add section
> >>> to a config, add extension to a section of a config... etc? I can take a
> >>> look at implementing this if this is what you have suggested.
> >>
> >> I am more thinking that your use case could be a new method in that
> >> file, with arguments that suit your case, but with common code to
> >> create a cert of the correct type. That means pulling out the existing
> >> cert creation into a new (internal) function in that btool that does
> >> both types of cert. For the CN args I think a dict would be suitable
> >> to pass in the settings.
> >>
> >> I'm trying to avoid people passing in a cert every time, since that is
> >> going to create a lot of code duplication and it will be hard to take
> >> advantage of common algos.
> >>
> >
> > Okay I think I get it, let me respin with this in mind.
>
> I am still a little doubtful about putting in *this* cert generation in
> the openssl btool. These sections and extensions are TI specific and I
> don't think there would be code duplication in future. The least code
> duplication along with functionality would be acheived by allowing
> creating configs, sections, extensions etc. in openssl btool or x509
> etype. and then creating the final cert binary in openssl. What do you
> think?

It is tricky because we only have one example in the etype at present
so it is always hard to know which way to go. I think it will be
clearer down the track when we have a bit more code here. But if we go
with having the bintool just be a shell, then we definitely won't get
any reuse for the next customer.

The bit I would like to see in the bintool is the creation of the text
file containing the cert. The etype should ideally provide properties
(which can be set up the ti_secure etype in the __init__() method)
that cover the required openssl params. The simplest thing would be
just 'this is TI signing' but I would like to pick this apart a bit so
that the properties are more generic, if possible.

So the btool takes arguments that tell it about the text file to
create. It creates the text file and runs openssl.

The x509 etype calls the openssl bintool's z509 method to actually
create the cert, presumably passing quite a few args and maybe a dict.

So:

- expand openssl.x509_cert() with more args
- expand x509 etype with more properties (using the defaults)
- update your ti-secure etype to set those properties in certain ways

Since the bintool code that is there is just an example cert, you can
switch it to do whatever you want for this case, i.e. change the
defaults. That might simplify things for you.

Anyway, for an initial series I think that will provide a good basis
for this work. If we need to refactor later, we have our test coverage
to help us.

Regards,
Simon


>
> >>>
> >>> If most of the cert is the same, you could
> >>>> pass a dict for the CN stuff, perhaps?
> >>>>
> >>>> What is the taml for? It is hard to tell, from the example provided.
> >>> Did you mean YAML? If so in the patch I linked, I don't think I had a
> >>> example for that. Could you point out what exactly you're asking about?
> >>
> >> Well I am wondering why it is in the code...is there a yaml file that
> >> you need to ingest and process? What is it for?
> >>
> >> ...actually I see the yaml files in your tree mentioned below. Does
> >> this come out of some other tool?
> >>
> >
> > Right, so currently we have certain board specific configurations in
> > YAML. This is input to our board configuration binary entry type [6],
> > which converts YAML into binary blobs prepended with certain headers
> > needed.
> >
> >>>
> >>>>
> >>>> Do you have a .dts which shows the full image for a board? I think the
> >>>> cert stuff looks right, but it's a bit hard to tell.
> >>>
> >>> Yes, in the same github link I have the whole tree [3] that contains
> >>> DTBs for couple of the devices [4] and [5], please have a look and let
> >>> me know (I might force push a few changes in the next couple of days, so
> >>> better to look from the tree)
> >>
> >> OK I see. Yes it looks pretty good to me.
> >>
> >>>
> >>>>
> >>>> When sending the patches please do do follow the function-commenting
> >>>> style and make sure that it is clear what each arg means. E.g. I saw a
> >>>> hash integer which I assume is used to pass 256 or 384 or 512 for sha
> >>>> hashing. It should indicate the possible values / meaning in the arg.
> >>>> In fact, in that case, it might be better to pass a string like
> >>>> 'sha256'.
> >>>
> >>> Yes, you're right. Will address that when sending the patch series
> >>> completely.
> >>
> >> OK
> >>
> >>>
> >>>>
> >>>> Anyway, apart from my questions it seems good.
> >>>>
> >>>> Regards,
> >>>> Simon
> >>>>
> >>>>>
> >>>>> [1]
> >>>>> https://github.com/nehamalcom/u-boot/commit/ea7413ed5864340bd6f01e704e8bdcc073a7896b#diff-efb03d61a324724c4f86bf42b45c4e4e614cab18e1b3184f63721d62280a11b5
> >>>>>
> >>>>> [2]
> >>>>> https://patchwork.ozlabs.org/project/uboot/patch/20230224120340.587786-1-n-francis@ti.com/
> >>>>>
> >>>>> --
> >>>>> Thanking You
> >>>>> Neha Malcom Francis
> >>>
> >>> [3] https://github.com/nehamalcom/u-boot/commits/ti-secure-functionality
> >>> [4]
> >>> https://github.com/nehamalcom/u-boot/commit/dda1f9caf436df59c576466f35262df90aa1c0af
> >>>
> >>
> >> Regards,
> >> Simon
> >
> > [6]
> > https://github.com/nehamalcom/u-boot/commit/3ca258562c374e553a7005a5b6d6441358801e78
> >
>
> --
> Thanking You
> Neha Malcom Francis