[PATCH v4 1/8] binman: ti-secure: Add support for firewalling entities

[Simon Glass]

Hi Manorit,

On Wed, 11 Oct 2023 at 22:46, Manorit Chawdhry <m-chawdhry at ti.com> wrote:
>
> Hi Simon,
>
> On 20:41-20231011, Simon Glass wrote:
> > Hi Manorit,
> >
> > On Tue, 10 Oct 2023 at 23:25, Manorit Chawdhry <m-chawdhry at ti.com> wrote:
> > >
> > > We can now firewall entities while loading them through our secure
> > > entity TIFS, the required information should be present in the
> > > certificate that is being parsed by TIFS.
> > >
> > > The following commit adds the support to enable the certificates to be
> > > generated if the firewall configurations are present in the binman dtsi
> > > nodes.
> > >
> > > Signed-off-by: Manorit Chawdhry <m-chawdhry at ti.com>
> > > ---
> > >  tools/binman/btool/openssl.py   | 16 +++++++-
> > >  tools/binman/etype/ti_secure.py | 90 +++++++++++++++++++++++++++++++++++++++++
> > >  tools/binman/etype/x509_cert.py |  3 +-
> > >  3 files changed, 106 insertions(+), 3 deletions(-)
> > >
> >
> > Reviewed-by: Simon Glass <sjg at chromium.org>
> >
> > I'm still a little worried about the error reporting if the user
> > leaves out a property. Does it do the right thing?
> >
>
> I did make a test also along the lines that would check all the
> firewalling properties and just auth-in-place isn't checked at this
> stage and people could end up adding the firewalling node without
> auth-in-place. Do you want to cover that test case as well? Regarding
> the other firewalling properties ( immitating the test by removing
> "id"), We get this:
>
> [..]
>   AR      spl/common/spl/built-in.o
>   LD      spl/u-boot-spl
>   OBJCOPY spl/u-boot-spl-nodtb.bin
>   SYM     spl/u-boot-spl.sym
>   CAT     spl/u-boot-spl-dtb.bin
>   COPY    spl/u-boot-spl.bin
>   BINMAN  .binman_stamp
> binman: id can't be None in firewall node

That's a bit vague...at least it should show the full path to the node
that failure, e.g. use self.Raise(). Also, I assume the property is
missing and the None refers to the Python value for the var. Better to
provide a nice error.

Any possible user error should ideally be checked, to avoid later confusion.

See Entry.ensure_props()

You could add an optional argument - the list of props to check. If
present it could use that instead of self.required_props

Hmm better might be to have a new function which tells the user that
since the firewall property is present, it needs these others ones
too.

Regards,
Simon