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

[Alexei Starovoitov]

On Thu, Jun 9, 2022 at 2:00 AM Roberto Sassu <roberto.sassu@xxxxxxxxxx> wrote:
>
> > From: Alexei Starovoitov [mailto:alexei.starovoitov@xxxxxxxxx]
> > Sent: Thursday, June 9, 2022 2:13 AM
> > On Wed, Jun 8, 2022 at 4:15 AM Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > 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@xxxxxxxxxx>
> > > ---
> > >  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' ?

> 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