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 5:38 PM
> On Thu, Jun 9, 2022 at 2:00 AM Roberto Sassu <roberto.sassu@huawei.com>
> wrote:
> >
> > > 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.
> 
> We shouldn't need to load into the secondary keyring.
> The helper needs to support an arbitrary key ring.
> The kernel shouldn't interfere with loading that test key into
> a test ring.
> 
> > But it would introduce unnecessary code, plus the keyring identifier
> 
> What kind of 'unnecessary code' ?

The code for handling eBPF-specific keyring identifiers.

But at the end, it is not that much.

Roberto

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

> > will be understood by eBPF only and not by verify_pkcs7_signature(),
> > as it happens for other keyring identifiers.
> 
> Maybe wrapping verify_pkcs7_signature as a helper is too high level?
> 
> > 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.
> 
> That's no go.
> 
> > 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