[PATCH v3 6/6] test: add test for eficonfig secure boot key management

[Simon Glass]

Hi Heinrich,

On Wed, 19 Oct 2022 at 15:39, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 10/19/22 15:17, Simon Glass wrote:
> > iHi Heinrich,
> >
> > On Fri, 14 Oct 2022 at 22:43, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >>
> >> On 10/15/22 03:10, Simon Glass wrote:
> >>> Hi Ilias,
> >>>
> >>> On Fri, 14 Oct 2022 at 09:59, Ilias Apalodimas
> >>> <ilias.apalodimas at linaro.org> wrote:
> >>>>
> >>>> Hi Simon,
> >>>>
> >>>> On Fri, 14 Oct 2022 at 18:56, Simon Glass <sjg at chromium.org> wrote:
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> On Fri, 14 Oct 2022 at 00:58, Masahisa Kojima
> >>>>> <masahisa.kojima at linaro.org> wrote:
> >>>>>>
> >>>>>> Provide a unit test for the eficonfig secure boot key
> >>>>>> management menu.
> >>>>>>
> >>>>>> Signed-off-by: Masahisa Kojima <masahisa.kojima at linaro.org>
> >>>>>> ---
> >>>>>> No change since v2
> >>>>>>
> >>>>>> newly created in v2
> >>>>>>
> >>>>>>    test/py/tests/test_eficonfig/conftest.py      |  84 +++-
> >>>>>>    test/py/tests/test_eficonfig/defs.py          |  14 +
> >>>>>>    .../test_eficonfig/test_eficonfig_sbkey.py    | 472 ++++++++++++++++++
> >>>>>>    3 files changed, 568 insertions(+), 2 deletions(-)
> >>>>>>    create mode 100644 test/py/tests/test_eficonfig/defs.py
> >>>>>>    create mode 100644 test/py/tests/test_eficonfig/test_eficonfig_sbkey.py
> >>>>>
> >>>>> Please can this test be in C? Also, using down-arrow to select menus
> >>>>> is brittle. Add a function to select the one you want, e.g. by name.
> >>>>>
> >>>>
> >>>> Is there a very specific reason why we should do stuff like that in C?
> >>>
> >>> Yes, see here.
> >>>
> >>>>    Python is way easier to extend and test in our case.
> >>>
> >>> In what way? It seems a lot more complicated, plus the brittle nature
> >>> of this test suggests it will be a hassle to maintain.
> >>>
> >>> https://u-boot.readthedocs.io/en/latest/develop/tests_writing.html#python-or-c
> >>>
> >>> There is a pending update here too:
> >>>
> >>> https://patchwork.ozlabs.org/project/uboot/patch/20221013122927.636867-15-sjg@chromium.org/
> >>
> >> The discussion touches different aspects:
> >>
> >> ** What does it take to make a GUI easily testable? **
> >>
> >> Relying on cursor movement for testing is fragile. This is why GUIs
> >> often assign to each executable element the following:
> >>
> >> * Access key, e.g  <CTRL><S>
> >>     A key combination that when entered will have the the same
> >>     effect as selecting the GUI item
> >> * Access string, '@SAVE'
> >>     A string that when typed into the command field will have
> >>     the same effect as selecting the GUI item
> >>
> >> Let's look at U-Boot's menu entry definition:
> >>
> >> struct bootmenu_entry {
> >>       unsigned short int num;      // unique number 0 .. MAX_COUNT
> >>       char key[3];                 // key identifier of number
> >>       char *title;                 // title of entry
> >>       char *command;               // hush command of entry
> >>       enum boot_type type;         // boot type of entry
> >>       u16 bootorder;               // order for each boot type
> >>       struct bootmenu_data *menu;  // this bootmenu
> >>       struct bootmenu_entry *next; // next menu entry (num+1)
> >> }
> >>
> >> Our structure lacks an accessor element that can be used to select a
> >> menu item without using cursor actions.
> >>
> >> Compound keystrokes like <CTRL><F6> are send as multiple bytes on the
> >> console, e.g. 1b 5b 31 37 3b 35 7e.
> >>
> >> We may define a field shortcut of type char *. If the string is received
> >> by the menu loop, let it activate the matching menu entry. Let cursor
> >> actions (up, down, enter, space '+', '-') interrupt matching the
> >> shortcut string.
> >>
> >> Instead we could also use a convention for the title:
> >>
> >> If a letter in the title is preceded by '&', this is the shortcut key.
> >> This letter will be shown highlighted in the menu and the ampersand will
> >> not be shown.
> >>
> >> This is probably easier to implement.
> >>
> >> Adding the shortcut facility will allow both for easier testing and
> >> faster navigation.
> >>
> >> ** Choice of programming language **
> >>
> >> Several aspects control the choice of the programming language for tests:
> >>
> >> - Testing single library functions is only possible in C.
> >> - Checking contents of internal structures is only possible in C.
> >> - Testing the U-Boot's CLI is easily accessible in our Python framework.
> >> - Preparation of complex test data is easier to do in Python.
> >> - Mixed language tests should be avoided if not strictly necessary.
> >>     It is much easier to maintain a single code source for a test.
> >>
> >> ** What to do for secure boot key management? **
> >>
> >> Secure boot key management requires complex preparation which fits well
> >> into out Python testing framework.
> >>
> >> Once we provide shortcut keys to U-Boot menus the entries will be easily
> >> accessible from Python.
> >>
> >> As we want to avoid complexity due to mixed language tests we should
> >> stick to Python for testing key management at the user level.
> >
> > The test setup should be in Python (and needs to be) but the actual
> > test should be in C, in general.
> >
> >>From what I can tell these tests should be split up, one for each test case.
> >
> > Using keyboard shortcuts for menus would certainly help, but you still
> > have the dependency between the menu code in C and the Python code
> > that uses it. The complexity of this dependency is quite significant.
> >
> > Can we not design this all for easier testing? The functionality
> > should be broken down a little more so we can change the configs in a
> > test (e.g. using the eficonfig command), then check that booting does
> > what we expect.
> >
> > Overall I feel that this 'black box' testing is not a good approach
> > and we should try to test the components. It is OK to have a
> > happy-path test but here we are just writing hundreds of lines of
> > Python to get a very high-level signal about correctness.
>
>  From other projects I am used to differentiate between unit tests and
> integration tests.
>
> An integration test is what checks that all the parts work together
> while a unit test checks a single functionality works as designed.
>
> A description can be found in
> https://circleci.com/blog/unit-testing-vs-integration-testing/#c-consent-modal
>
> For an integration test this Python script is fine (except for the menu
> navigation.
>
> As I said in my last mail additional tests on a lower level may be
> added. But unit tests cannot replace an integration test.

We should do the C test first, then. It just has so much more
visibility into what is going on. I am OK with an integration test,
but it should never find a bug that is not found by the sandbox unit
test. The Python tests are much harder to debug!

So let's get this sorted out so we don't end up with a complete mess here.

Regards,
Simon