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

[Manorit Chawdhry]

Hi Simon,

On 08:29-20231012, Simon Glass wrote:
> 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.

I was working on this so I realised that Firewall class that I have is
not an etype so I have hacked a bit around to get that self.Raise in but
hoping that shouldn't be much of a problem..

Does this error look okay now?

[..]
  COPY    spl/u-boot-spl.bin
  BINMAN  .binman_stamp
binman: Node '/binman/ti-spl/fit/images/atf/ti-secure': Subnode 'firewall' is missing properties: id,region

> 
> Any possible user error should ideally be checked, to avoid later confusion.
> 
> See Entry.ensure_props()

I was thinking of using this as well but I noticed that this gets called
in the beginning itself and since the Firewall class that I have is not
an etype then I won't be getting the information that all this is
failing in firewalling related node and might be confused with debugging
in ti-secure hence thought of updating the dataclass only with the
function only as you suggested below. Thanks for the suggestions!

Regards,
Manorit

> 
> 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