| Message ID | 20220608111221.373833-3-roberto.sassu@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Add bpf_verify_pkcs7_signature() helper | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-PR | fail | PR summary |
| bpf/vmtest-bpf-next-VM_Test-1 | fail | Logs for Kernel LATEST on ubuntu-latest with gcc |
| bpf/vmtest-bpf-next-VM_Test-2 | fail | Logs for Kernel LATEST on ubuntu-latest with llvm-15 |
| bpf/vmtest-bpf-next-VM_Test-3 | fail | Logs for Kernel LATEST on z15 with gcc |
| netdev/tree_selection | success | Not a local patch, async |
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.
> 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
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' ? > 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
> 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
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; case ARGP_KEY_ARG: argp_usage(state); break; diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h index 5fe1365c2bb1..9a860a59f06a 100644 --- a/tools/testing/selftests/bpf/test_progs.h +++ b/tools/testing/selftests/bpf/test_progs.h @@ -123,6 +123,9 @@ struct test_env { pid_t *worker_pids; /* array of worker pids */ int *worker_socks; /* array of worker socks */ int *worker_current_test; /* array of current running test for each worker */ + + const char *sign_file_path; /* sign-file path */ + const char *kernel_priv_cert_path; /* kernel priv key and cert path */ }; #define MAX_LOG_TRUNK_SIZE 8192
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(+)