| Message ID | 20220223200731.1859670-1-yaelt@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] syscalls/keyctl09: test encrypted keys with provided decrypted data. | expand |
Hi!
The test looks good to me now.
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
Looking at the kernel counterpart it is still in the next tree. We
usually wait until the kernel functionality is part of a kernel release
in order to make sure that there are no changes done to the
kernel-userspace API.
I this case I guess that in this case the change is so minimal that we
can add this test into LTP once it reaches Linus tree.
On Wed, Mar 2, 2022 at 10:50 AM Cyril Hrubis <chrubis@suse.cz> wrote: > > Hi! > The test looks good to me now. > > Reviewed-by: Cyril Hrubis <chrubis@suse.cz> > > Looking at the kernel counterpart it is still in the next tree. We > usually wait until the kernel functionality is part of a kernel release > in order to make sure that there are no changes done to the > kernel-userspace API. > > I this case I guess that in this case the change is so minimal that we > can add this test into LTP once it reaches Linus tree. > > -- > Cyril Hrubis > chrubis@suse.cz Sounds good. Thank you!
Hi Cyril, Yael, > Hi! > The test looks good to me now. > Reviewed-by: Cyril Hrubis <chrubis@suse.cz> > Looking at the kernel counterpart it is still in the next tree. We > usually wait until the kernel functionality is part of a kernel release > in order to make sure that there are no changes done to the > kernel-userspace API. Good (last time I checked it wasn't there). For my record: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20220302&id=cd3bc044af483422cc81a93f23c78c20c978b17c > I this case I guess that in this case the change is so minimal that we > can add this test into LTP once it reaches Linus tree. Cyril, maybe we could finally merge our policy (waiting ack for you): https://patchwork.ozlabs.org/project/ltp/patch/20220203101803.10204-1-rpalethorpe@suse.com/ and put keyctl09 into runtest/staging now. Kind regards, Petr
Hi! > > I this case I guess that in this case the change is so minimal that we > > can add this test into LTP once it reaches Linus tree. > Cyril, maybe we could finally merge our policy (waiting ack for you): > https://patchwork.ozlabs.org/project/ltp/patch/20220203101803.10204-1-rpalethorpe@suse.com/ > and put keyctl09 into runtest/staging now. I guess that we still did not agree on exactly how this should be handled or did we?
Hi Cyril, > Hi! > > > I this case I guess that in this case the change is so minimal that we > > > can add this test into LTP once it reaches Linus tree. > > Cyril, maybe we could finally merge our policy (waiting ack for you): > > https://patchwork.ozlabs.org/project/ltp/patch/20220203101803.10204-1-rpalethorpe@suse.com/ > > and put keyctl09 into runtest/staging now. > I guess that we still did not agree on exactly how this should be > handled or did we? Isn't it enough "Once a feature is part of the stable kernel ABI the associated test must be moved out of staging." ? If not, we should raise this question on that patch thread to find a conclusion. Kind regards, Petr
Hi! > > > > I this case I guess that in this case the change is so minimal that we > > > > can add this test into LTP once it reaches Linus tree. > > > Cyril, maybe we could finally merge our policy (waiting ack for you): > > > https://patchwork.ozlabs.org/project/ltp/patch/20220203101803.10204-1-rpalethorpe@suse.com/ > > > and put keyctl09 into runtest/staging now. > > > I guess that we still did not agree on exactly how this should be > > handled or did we? > > Isn't it enough "Once a feature is part of the stable kernel ABI the associated > test must be moved out of staging." ? The main problem is that someone has to make sure that it happens and the process would be prone to errors. What I proposed instead was a flag that would set a kernel version in which the ABI is going to be merged and put the test right into the final runtest files. Then we can simply skip the test on older kernels or do anything else we see as a reasonable solution. At the same time we can easily add automatic checker that would look for these flags in metadata into the CI which would, for instance, send email to the ML once the flag is supposed to be removed. In this case it does not actually matter, since the test is guarded by a kernel config option that is introduced by the patchset and the change is fairly miniminal, so I do not think that there would be any changes to the ABI anyways.
Hi Cyril, [ Cc Richie, Li, Jan ] > Hi! > > > > > I this case I guess that in this case the change is so minimal that we > > > > > can add this test into LTP once it reaches Linus tree. > > > > Cyril, maybe we could finally merge our policy (waiting ack for you): > > > > https://patchwork.ozlabs.org/project/ltp/patch/20220203101803.10204-1-rpalethorpe@suse.com/ > > > > and put keyctl09 into runtest/staging now. > > > I guess that we still did not agree on exactly how this should be > > > handled or did we? > > Isn't it enough "Once a feature is part of the stable kernel ABI the associated > > test must be moved out of staging." ? > The main problem is that someone has to make sure that it happens and > the process would be prone to errors. What I proposed instead was a flag > that would set a kernel version in which the ABI is going to be merged > and put the test right into the final runtest files. Then we can simply > skip the test on older kernels or do anything else we see as a > reasonable solution. At the same time we can easily add automatic > checker that would look for these flags in metadata into the CI which > would, for instance, send email to the ML once the flag is supposed to > be removed. OK, you're missing that kernel version. OTOH things get sometimes backported, thus it's not error prone (if we forget to leave that flag after kernel being released). Also version is hard to say if you use maintainer tree (which applies patches on previous rc1 than what is being in Linus tree). Thus maintainer's tree would be left, also IMHO next tree has no specific version in uname, thus we'd only support rc from Linus' tree. But anyway, if all agree that this is better than both solutions Richie implemented I'd try to find time to implement it so that we have finally a solution. > In this case it does not actually matter, since the test is guarded by a > kernel config option that is introduced by the patchset and the change > is fairly miniminal, so I do not think that there would be any changes > to the ABI anyways. Correct. At this stage IMHO we can dare to merge it. Kind regards, Petr
On Thu, Mar 3, 2022 at 9:07 AM Petr Vorel <pvorel@suse.cz> wrote: > > Hi Cyril, > > [ Cc Richie, Li, Jan ] > > > Hi! > > > > > > I this case I guess that in this case the change is so minimal that we > > > > > > can add this test into LTP once it reaches Linus tree. > > > > > Cyril, maybe we could finally merge our policy (waiting ack for you): > > > > > https://patchwork.ozlabs.org/project/ltp/patch/20220203101803.10204-1-rpalethorpe@suse.com/ > > > > > and put keyctl09 into runtest/staging now. > > > > > I guess that we still did not agree on exactly how this should be > > > > handled or did we? > > > > Isn't it enough "Once a feature is part of the stable kernel ABI the associated > > > test must be moved out of staging." ? > > > The main problem is that someone has to make sure that it happens and > > the process would be prone to errors. What I proposed instead was a flag > > that would set a kernel version in which the ABI is going to be merged > > and put the test right into the final runtest files. Then we can simply > > skip the test on older kernels or do anything else we see as a > > reasonable solution. At the same time we can easily add automatic > > checker that would look for these flags in metadata into the CI which > > would, for instance, send email to the ML once the flag is supposed to > > be removed. > OK, you're missing that kernel version. OTOH things get sometimes backported, > thus it's not error prone (if we forget to leave that flag after kernel being > released). > > Also version is hard to say if you use maintainer tree (which applies patches on > previous rc1 than what is being in Linus tree). Thus maintainer's tree would be > left, also IMHO next tree has no specific version in uname, thus we'd only > support rc from Linus' tree. > > But anyway, if all agree that this is better than both solutions Richie > implemented I'd try to find time to implement it so that we have finally a > solution. > > > In this case it does not actually matter, since the test is guarded by a > > kernel config option that is introduced by the patchset and the change > > is fairly miniminal, so I do not think that there would be any changes > > to the ABI anyways. > Correct. At this stage IMHO we can dare to merge it. > > Kind regards, > Petr Hi Petr and Cyril, I wanted to check whether there is pending action left on my end? Thanks, Yael
> On Thu, Mar 3, 2022 at 9:07 AM Petr Vorel <pvorel@suse.cz> wrote: > > Hi Cyril, > > [ Cc Richie, Li, Jan ] > > > Hi! > > > > > > > I this case I guess that in this case the change is so minimal that we > > > > > > > can add this test into LTP once it reaches Linus tree. > > > > > > Cyril, maybe we could finally merge our policy (waiting ack for you): > > > > > > https://patchwork.ozlabs.org/project/ltp/patch/20220203101803.10204-1-rpalethorpe@suse.com/ > > > > > > and put keyctl09 into runtest/staging now. > > > > > I guess that we still did not agree on exactly how this should be > > > > > handled or did we? > > > > Isn't it enough "Once a feature is part of the stable kernel ABI the associated > > > > test must be moved out of staging." ? > > > The main problem is that someone has to make sure that it happens and > > > the process would be prone to errors. What I proposed instead was a flag > > > that would set a kernel version in which the ABI is going to be merged > > > and put the test right into the final runtest files. Then we can simply > > > skip the test on older kernels or do anything else we see as a > > > reasonable solution. At the same time we can easily add automatic > > > checker that would look for these flags in metadata into the CI which > > > would, for instance, send email to the ML once the flag is supposed to > > > be removed. > > OK, you're missing that kernel version. OTOH things get sometimes backported, > > thus it's not error prone (if we forget to leave that flag after kernel being > > released). > > Also version is hard to say if you use maintainer tree (which applies patches on > > previous rc1 than what is being in Linus tree). Thus maintainer's tree would be > > left, also IMHO next tree has no specific version in uname, thus we'd only > > support rc from Linus' tree. > > But anyway, if all agree that this is better than both solutions Richie > > implemented I'd try to find time to implement it so that we have finally a > > solution. > > > In this case it does not actually matter, since the test is guarded by a > > > kernel config option that is introduced by the patchset and the change > > > is fairly miniminal, so I do not think that there would be any changes > > > to the ABI anyways. > > Correct. At this stage IMHO we can dare to merge it. > > Kind regards, > > Petr > Hi Petr and Cyril, > I wanted to check whether there is pending action left on my end? @Yael nothing needed from you. @Cyril cd3bc044af48 ("KEYS: encrypted: Instantiate key with user-provided decrypted data") is in Mimi Zohar's git tree and in next tree, going to be merged in next merge window. Can we merge it now as is? Kind regards, Petr > Thanks, > Yael
Hi all, > > Hi Petr and Cyril, > > I wanted to check whether there is pending action left on my end? > @Yael nothing needed from you. > @Cyril cd3bc044af48 ("KEYS: encrypted: Instantiate key with user-provided > decrypted data") is in Mimi Zohar's git tree and in next tree, going to be > merged in next merge window. Can we merge it now as is? @Cyril cd3bc044af48 is in the next tree. Are we going to wait till it reaches Linus' tree (in 5.18-rc1) or can I merge it now? https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20220323&id=cd3bc044af483422cc81a93f23c78c20c978b17c Kind regards, Petr > Kind regards, > Petr
Hi! Applied, thanks.
diff --git a/runtest/syscalls b/runtest/syscalls index bcf3d56c9..ccea1ddbd 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -643,6 +643,7 @@ keyctl05 keyctl05 keyctl06 keyctl06 keyctl07 keyctl07 keyctl08 keyctl08 +keyctl09 keyctl09 kcmp01 kcmp01 kcmp02 kcmp02 diff --git a/testcases/kernel/syscalls/keyctl/.gitignore b/testcases/kernel/syscalls/keyctl/.gitignore index 3544ac79c..f9948c176 100644 --- a/testcases/kernel/syscalls/keyctl/.gitignore +++ b/testcases/kernel/syscalls/keyctl/.gitignore @@ -6,3 +6,4 @@ /keyctl06 /keyctl07 /keyctl08 +/keyctl09 diff --git a/testcases/kernel/syscalls/keyctl/keyctl09.c b/testcases/kernel/syscalls/keyctl/keyctl09.c new file mode 100644 index 000000000..71fc2f2a9 --- /dev/null +++ b/testcases/kernel/syscalls/keyctl/keyctl09.c @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2022 Google, Inc. + */ + +/*\ + * [Description] + * Test that encrypted keys can be instantiated using user-provided decrypted + * data that is hex-ascii encoded. + */ + +#include "tst_test.h" +#include "lapi/keyctl.h" + +#define ENCRYPTED_KEY_VALID_PAYLOAD "new enc32 user:masterkey 32 abcdefABCDEF1234567890aaaaaaaaaa" +#define ENCRYPTED_KEY_INVALID_PAYLOAD "new enc32 user:masterkey 32 plaintext123@123!123@123!123@123" + +static void do_test(void) +{ + char buffer[128]; + + TST_EXP_POSITIVE(add_key("user", "user:masterkey", "foo", 3, + KEY_SPEC_PROCESS_KEYRING)); + + if (!TST_PASS) + return; + + TST_EXP_POSITIVE(add_key("encrypted", "ltptestkey1", + ENCRYPTED_KEY_VALID_PAYLOAD, + 60, KEY_SPEC_PROCESS_KEYRING)); + + if (!TST_PASS) + return; + + TST_EXP_POSITIVE(keyctl(KEYCTL_READ, TST_RET, buffer, sizeof(buffer))); + + if (!TST_PASS) + return; + + TST_EXP_FAIL2(add_key("encrypted", "ltptestkey2", + ENCRYPTED_KEY_INVALID_PAYLOAD, 60, + KEY_SPEC_PROCESS_KEYRING), EINVAL); + + keyctl(KEYCTL_CLEAR, KEY_SPEC_PROCESS_KEYRING); +} + +static struct tst_test test = { + .test_all = do_test, + .needs_kconfigs = (const char *[]) { + "CONFIG_USER_DECRYPTED_DATA=y", + NULL + } +};
Test that encrypted keys can be instantiated using hex-ascii encoded user-provided decrypted data. (https://lore.kernel.org/lkml/20220215141953.1557009-1-yaelt@google.com/). Signed-off-by: Yael Tzur <yaelt@google.com> --- Notes: v -> v2: added key revocation and made styling changes. v2 -> v3: updated per latest kernel patch version. v3 -> v4: made styling changes. runtest/syscalls | 1 + testcases/kernel/syscalls/keyctl/.gitignore | 1 + testcases/kernel/syscalls/keyctl/keyctl09.c | 53 +++++++++++++++++++++ 3 files changed, 55 insertions(+) create mode 100644 testcases/kernel/syscalls/keyctl/keyctl09.c