Hi Simon, On 08:29-20231012, Simon Glass wrote: > Hi Manorit, > > On Wed, 11 Oct 2023 at 22:46, Manorit Chawdhry <m-chawd...@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-chawd...@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-chawd...@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 <s...@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