RE: [PATCH v2 2/3] selftests/bpf: Add test_progs opts for sign-file and kernel priv key + cert

[Roberto Sassu <roberto.sassu@huawei.com>]

> From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com]
> Sent: Thursday, June 9, 2022 2:13 AM
> On Wed, Jun 8, 2022 at 4:15 AM Roberto Sassu <roberto.sassu@huawei.com>
> wrote:
> >
> > According to the logs of the eBPF CI, built kernel and tests are copied to
> > a virtual machine to run there.
> >
> > Since a test for a new helper to verify PKCS#7 signatures requires to sign
> > data to be verified, extend test_progs to store in the test_env data
> > structure (accessible by individual tests) the path of sign-file and of the
> > kernel private key and cert.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  tools/testing/selftests/bpf/test_progs.c | 12 ++++++++++++
> >  tools/testing/selftests/bpf/test_progs.h |  3 +++
> >  2 files changed, 15 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/test_progs.c
> b/tools/testing/selftests/bpf/test_progs.c
> > index c639f2e56fc5..90ce2c06a15e 100644
> > --- a/tools/testing/selftests/bpf/test_progs.c
> > +++ b/tools/testing/selftests/bpf/test_progs.c
> > @@ -707,6 +707,8 @@ enum ARG_KEYS {
> >         ARG_TEST_NAME_GLOB_DENYLIST = 'd',
> >         ARG_NUM_WORKERS = 'j',
> >         ARG_DEBUG = -1,
> > +       ARG_SIGN_FILE = 'S',
> > +       ARG_KERNEL_PRIV_CERT = 'C',
> >  };
> >
> >  static const struct argp_option opts[] = {
> > @@ -732,6 +734,10 @@ static const struct argp_option opts[] = {
> >           "Number of workers to run in parallel, default to number of cpus." },
> >         { "debug", ARG_DEBUG, NULL, 0,
> >           "print extra debug information for test_progs." },
> > +       { "sign-file", ARG_SIGN_FILE, "PATH", 0,
> > +         "sign-file path " },
> > +       { "kernel-priv-cert", ARG_KERNEL_PRIV_CERT, "PATH", 0,
> > +         "kernel private key and cert path " },
> >         {},
> >  };
> >
> > @@ -862,6 +868,12 @@ static error_t parse_arg(int key, char *arg, struct
> argp_state *state)
> >         case ARG_DEBUG:
> >                 env->debug = true;
> >                 break;
> > +       case ARG_SIGN_FILE:
> > +               env->sign_file_path = arg;
> > +               break;
> > +       case ARG_KERNEL_PRIV_CERT:
> > +               env->kernel_priv_cert_path = arg;
> > +               break;
> 
> That's cumbersome approach to use to force CI and
> users to pass these args on command line.
> The test has to be self contained.
> test_progs should execute it without any additional input.
> For example by having test-only private/public key
> that is used to sign and verify the signature.

I thought a bit about this. Just generating a test key does not work,
as it must be signed by the kernel signing key (otherwise, loading
in the secondary keyring will be rejected). Having the test key around
is as dangerous as having the kernel signing key around copied
somewhere.

Allowing users to specify a test keyring in the helper is possible.
But it would introduce unnecessary code, plus the keyring identifier
will be understood by eBPF only and not by verify_pkcs7_signature(),
as it happens for other keyring identifiers.

We may have environment variables directly in the eBPF test, to
specify the location of the signing key, but there is a risk of
duplication, as other tests wanting the same information might
not be aware of them.

I would not introduce any code that handles the kernel signing
key (in the Makefile, or in a separate script). This information is
so sensible, that it must be responsibility of an external party
to do the work of making that key available and tell where it is.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Yang Xi, Li He