| Message ID | 20230823202010.652027-1-magali.lemes@canonical.com |
|---|---|
| Headers | show |
| Series | Fix failing net selftests | expand |
Acked-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
On 23.08.23 22:20, Magali Lemes wrote: > BugLink: https://bugs.launchpad.net/bugs/2019868 > BugLink: https://bugs.launchpad.net/bugs/2019880 > > [Impact] > Due to the introduction of net tests that rely on cryptographic > functions to work, some test cases from net/tls and net/vrf-xfrm-tests > that use non-compliant FIPS algorithms fail when fips=1. > > [Fix] > To fix these failures in FIPS mode, we can, on a case-by-case basis, > 1) skip the tests that require non-compliant FIPS algorithms or > 2) change the algorithms to FIPS-compliant ones. > For net/tls, we skip the test variants that use the ChaCha20-Poly1305 > algorithm. > For net/net:vrf-xfrm-tests, we can simply replace the algorithms that > are not FIPS-compliant with compliant ones. > > [Test Plan] > With a fips kernel installed, pass fips=1 as a kernel parameter, run the > net/tls and net/vrf-xfrm-tests tests with these patches applied, and > check that they are all passing. > > [Where problems could occur] > Regression risk is very low and would hardly affect any user, since the > changes only touch the selftests. > > [Other Info] > I'm sending this to be applied on the generic kernel, as Jammy FIPS > derivative kernels will easily inherit these changes. I do not feel quite happy there. The first patch, sure no problem. That might even allow tweaking things we know are failing in general. For patch #2: has that ever been run on a non-fips kernel? What happens if the file in the new constructor does not exist? Which would be any non-fips kernel, right? With patch #3, that would change the algorithms used for any kernel. Apart from the worry that this might cause problems on some hw you do not see with fips, this makes results at least not comparable across series. Also having two bug reports mingled in one submission is at least a source of confusion (or likely missed in some process). So I don't want to refuse straight away but there is a bad feeling about this in all kernels... -Stefan > > Changes in v2: > - Target generic kernel. > - fcnal-test.sh: dropped as it will be picked from upstream stable. > - tls.c: skip tests right at setup if in FIPS mode, this requires commit > 372b304c ("selftests/harness: allow tests to be skipped during setup"). > > Magali Lemes (3): > selftests/harness: allow tests to be skipped during setup > selftests: net: tls: check if FIPS mode is enabled > selftests: net: vrf-xfrm-tests: change authentication and encryption > algos > > tools/testing/selftests/kselftest_harness.h | 6 ++-- > tools/testing/selftests/net/tls.c | 21 ++++++++++++ > tools/testing/selftests/net/vrf-xfrm-tests.sh | 32 +++++++++---------- > 3 files changed, 40 insertions(+), 19 deletions(-) >
On Thu, Aug 24, 2023 at 6:07 AM Stefan Bader <stefan.bader@canonical.com> wrote: > > On 23.08.23 22:20, Magali Lemes wrote: > > BugLink: https://bugs.launchpad.net/bugs/2019868 > > BugLink: https://bugs.launchpad.net/bugs/2019880 > > > > [Impact] > > Due to the introduction of net tests that rely on cryptographic > > functions to work, some test cases from net/tls and net/vrf-xfrm-tests > > that use non-compliant FIPS algorithms fail when fips=1. > > > > [Fix] > > To fix these failures in FIPS mode, we can, on a case-by-case basis, > > 1) skip the tests that require non-compliant FIPS algorithms or > > 2) change the algorithms to FIPS-compliant ones. > > For net/tls, we skip the test variants that use the ChaCha20-Poly1305 > > algorithm. > > For net/net:vrf-xfrm-tests, we can simply replace the algorithms that > > are not FIPS-compliant with compliant ones. > > > > [Test Plan] > > With a fips kernel installed, pass fips=1 as a kernel parameter, run the > > net/tls and net/vrf-xfrm-tests tests with these patches applied, and > > check that they are all passing. > > > > [Where problems could occur] > > Regression risk is very low and would hardly affect any user, since the > > changes only touch the selftests. > > > > [Other Info] > > I'm sending this to be applied on the generic kernel, as Jammy FIPS > > derivative kernels will easily inherit these changes. > > I do not feel quite happy there. The first patch, sure no problem. That > might even allow tweaking things we know are failing in general. > For patch #2: has that ever been run on a non-fips kernel? What happens > if the file in the new constructor does not exist? Which would be any > non-fips kernel, right? Yes, I locally ran this test on 5.15.0-79-generic and checked that the output remains unchanged. Right, `/proc/sys/crypto/fips_enabled` only exists in FIPS kernels. For all the other kernels, `fopen("/proc/sys/crypto/fips_enabled", "r");` should return NULL and `fips_enabled` is kept as 0. So I don't think this would be a cause of issue to the other kernels. > With patch #3, that would change the algorithms used for any kernel. > Apart from the worry that this might cause problems on some hw you do > not see with fips, this makes results at least not comparable across series. The new algos should be common enough so as not to cause such problems. Plus, the tests are no longer comparable in series with kernel version v6.4 and greater, since these patches landed in upstream v6.4. > > Also having two bug reports mingled in one submission is at least a > source of confusion (or likely missed in some process). > > So I don't want to refuse straight away but there is a bad feeling about > this in all kernels... I understand the concern, yet I feel like the regression risk is very low and would make it a lot easier to keep things synced across FIPS (and other) kernels. Kind regards, Magali. > > -Stefan > > > > > Changes in v2: > > - Target generic kernel. > > - fcnal-test.sh: dropped as it will be picked from upstream stable. > > - tls.c: skip tests right at setup if in FIPS mode, this requires commit > > 372b304c ("selftests/harness: allow tests to be skipped during setup"). > > > > Magali Lemes (3): > > selftests/harness: allow tests to be skipped during setup > > selftests: net: tls: check if FIPS mode is enabled > > selftests: net: vrf-xfrm-tests: change authentication and encryption > > algos > > > > tools/testing/selftests/kselftest_harness.h | 6 ++-- > > tools/testing/selftests/net/tls.c | 21 ++++++++++++ > > tools/testing/selftests/net/vrf-xfrm-tests.sh | 32 +++++++++---------- > > 3 files changed, 40 insertions(+), 19 deletions(-) > > >
On 23/08/2023 22:20, Magali Lemes wrote: > BugLink: https://bugs.launchpad.net/bugs/2019868 > BugLink: https://bugs.launchpad.net/bugs/2019880 > > [Impact] > Due to the introduction of net tests that rely on cryptographic > functions to work, some test cases from net/tls and net/vrf-xfrm-tests > that use non-compliant FIPS algorithms fail when fips=1. > > [Fix] > To fix these failures in FIPS mode, we can, on a case-by-case basis, > 1) skip the tests that require non-compliant FIPS algorithms or > 2) change the algorithms to FIPS-compliant ones. > For net/tls, we skip the test variants that use the ChaCha20-Poly1305 > algorithm. > For net/net:vrf-xfrm-tests, we can simply replace the algorithms that > are not FIPS-compliant with compliant ones. > > [Test Plan] > With a fips kernel installed, pass fips=1 as a kernel parameter, run the > net/tls and net/vrf-xfrm-tests tests with these patches applied, and > check that they are all passing. > > [Where problems could occur] > Regression risk is very low and would hardly affect any user, since the > changes only touch the selftests. > > [Other Info] > I'm sending this to be applied on the generic kernel, as Jammy FIPS > derivative kernels will easily inherit these changes. > > Changes in v2: > - Target generic kernel. > - fcnal-test.sh: dropped as it will be picked from upstream stable. > - tls.c: skip tests right at setup if in FIPS mode, this requires commit > 372b304c ("selftests/harness: allow tests to be skipped during setup"). > > Magali Lemes (3): > selftests/harness: allow tests to be skipped during setup > selftests: net: tls: check if FIPS mode is enabled > selftests: net: vrf-xfrm-tests: change authentication and encryption > algos > > tools/testing/selftests/kselftest_harness.h | 6 ++-- > tools/testing/selftests/net/tls.c | 21 ++++++++++++ > tools/testing/selftests/net/vrf-xfrm-tests.sh | 32 +++++++++---------- > 3 files changed, 40 insertions(+), 19 deletions(-) > Let's keep an eye on net:vrf-xfrm-tests.sh results next cycle since the algorithm has changed in the last patch. I personally find it strange to link 2 buglinks in the cover letter, but I also understand why these were submitted together. Maybe a "parent" bug would make sense in this case. Acked-by: Roxana Nicolescu <roxana.nicolescu@canonical.com>
On Fri, Aug 25, 2023 at 08:36:13AM +0200, Roxana Nicolescu wrote: [...] > Let's keep an eye on net:vrf-xfrm-tests.sh results next cycle since the > algorithm has changed in the last patch. > I personally find it strange to link 2 buglinks in the cover letter, but I > also understand why these were submitted together. > Maybe a "parent" bug would make sense in this case. > > Acked-by: Roxana Nicolescu <roxana.nicolescu@canonical.com> Though I think it is important to understand the end goal here and be able to track its progress, I am not sure launchpad bugs have been used for that much. In this particular case, it is about "Adjust tests that use crypto to work in FIPS mode", and that task has been broken in subtasks, like "adjust TLS socket tests", "adjust xfrm tests", etc. So Magali created those two bugs for each of the subtasks and submitted them as a single cover letter. That didn't get in the way of me reviewing them and understand why they are for. The cover letter suits it fine. Our tooling even manages commits having two BugLinks or a BugLink plus a CVE when producing changelogs. My points are: 1) let's not introduce further paperwork to get patches reviewed and applied, Magali did a stelar job in her submission here, not only in writing the cover letter, but all work that I believe was behind it (specially testing). 2) If any tracking of tasks is necessary, we have different tools to deal with that. Let's use them, but not let them meddle with how we submit and review patches. Cascardo.
On 25.08.23 12:57, Thadeu Lima de Souza Cascardo wrote: > On Fri, Aug 25, 2023 at 08:36:13AM +0200, Roxana Nicolescu wrote: > [...] >> Let's keep an eye on net:vrf-xfrm-tests.sh results next cycle since the >> algorithm has changed in the last patch. >> I personally find it strange to link 2 buglinks in the cover letter, but I >> also understand why these were submitted together. >> Maybe a "parent" bug would make sense in this case. >> >> Acked-by: Roxana Nicolescu <roxana.nicolescu@canonical.com> > > Though I think it is important to understand the end goal here and be able to > track its progress, I am not sure launchpad bugs have been used for that much. That is _exactly_ for what launchpad bugs are used. > > In this particular case, it is about "Adjust tests that use crypto to work in > FIPS mode", and that task has been broken in subtasks, like "adjust TLS socket > tests", "adjust xfrm tests", etc. > > So Magali created those two bugs for each of the subtasks and submitted them as > a single cover letter. That didn't get in the way of me reviewing them and > understand why they are for. The cover letter suits it fine. Our tooling even > manages commits having two BugLinks or a BugLink plus a CVE when producing > changelogs. However when patches are applied, we set them manually applied in the tracking bug. And when a kernels releases launchpad automatically set fixed release. The problematic part might be if something comes up and requires undoing. Then the complete submission might be reverted even if only one part caused a problem. > > My points are: 1) let's not introduce further paperwork to get patches reviewed > and applied, Magali did a stelar job in her submission here, not only in > writing the cover letter, but all work that I believe was behind it (specially > testing). This is through and will get applied in time. And not to say Magali did not a great job. Just that it could also have used less paperwork by having one bug to say "fix selftests under fips" or so. > > 2) If any tracking of tasks is necessary, we have different tools to deal with > that. Let's use them, but not let them meddle with how we submit and review > patches. This is SRU policy. And it is there to make things manageable. This is a simple case but next time there will be a submission fixing 15 different graphic bugs under one submission and likely only 10 bug reports. And this is why we try to keep people to one submission for one goal and linked to one bug report. > > Cascardo. >
On Fri, Aug 25, 2023 at 02:33:05PM +0200, Stefan Bader wrote: > On 25.08.23 12:57, Thadeu Lima de Souza Cascardo wrote: > > On Fri, Aug 25, 2023 at 08:36:13AM +0200, Roxana Nicolescu wrote: > > [...] > > > Let's keep an eye on net:vrf-xfrm-tests.sh results next cycle since the > > > algorithm has changed in the last patch. > > > I personally find it strange to link 2 buglinks in the cover letter, but I > > > also understand why these were submitted together. > > > Maybe a "parent" bug would make sense in this case. > > > > > > Acked-by: Roxana Nicolescu <roxana.nicolescu@canonical.com> > > > > Though I think it is important to understand the end goal here and be able to > > track its progress, I am not sure launchpad bugs have been used for that much. > > That is _exactly_ for what launchpad bugs are used. > > > > In this particular case, it is about "Adjust tests that use crypto to work in > > FIPS mode", and that task has been broken in subtasks, like "adjust TLS socket > > tests", "adjust xfrm tests", etc. > > > > So Magali created those two bugs for each of the subtasks and submitted them as > > a single cover letter. That didn't get in the way of me reviewing them and > > understand why they are for. The cover letter suits it fine. Our tooling even > > manages commits having two BugLinks or a BugLink plus a CVE when producing > > changelogs. > > However when patches are applied, we set them manually applied in the > tracking bug. And when a kernels releases launchpad automatically set fixed > release. > The problematic part might be if something comes up and requires undoing. > Then the complete submission might be reverted even if only one part caused > a problem. > > > > My points are: 1) let's not introduce further paperwork to get patches reviewed > > and applied, Magali did a stelar job in her submission here, not only in > > writing the cover letter, but all work that I believe was behind it (specially > > testing). > > This is through and will get applied in time. And not to say Magali did not > a great job. Just that it could also have used less paperwork by having one > bug to say "fix selftests under fips" or so. > > > > > 2) If any tracking of tasks is necessary, we have different tools to deal with > > that. Let's use them, but not let them meddle with how we submit and review > > patches. > > This is SRU policy. And it is there to make things manageable. This is a > simple case but next time there will be a submission fixing 15 different > graphic bugs under one submission and likely only 10 bug reports. And this > is why we try to keep people to one submission for one goal and linked to > one bug report. > > > > Cascardo. > > > > -- > - Stefan I think we are in agreement here about not having extra paperwork. I just don't think it would be reasonable to have 2 bugs and one extra parent bug, where this would all be even more complicated. I would agree with either a single bug or multiple submissions, but I still fail to see the particular problem with this one. Each patch had a single BugLink, the cover letter had the two that were part of it. Sometimes, things do get complicated, like for recent submissions where one patchset may depend on another one. We can choose to either send them separate, making review easier, but having to pay attention on the order when applying them, or submit them together, which would hit the exact same case here: each patch with its own BugLink or CVE, cover letter having multiple of them. These are not the common case, and I just think that having stricter policy is not going to help anyone: not the submitter, not the reviewer. Cascardo.
On 23/08/2023 22:20, Magali Lemes wrote: > BugLink: https://bugs.launchpad.net/bugs/2019868 > BugLink: https://bugs.launchpad.net/bugs/2019880 > > [Impact] > Due to the introduction of net tests that rely on cryptographic > functions to work, some test cases from net/tls and net/vrf-xfrm-tests > that use non-compliant FIPS algorithms fail when fips=1. > > [Fix] > To fix these failures in FIPS mode, we can, on a case-by-case basis, > 1) skip the tests that require non-compliant FIPS algorithms or > 2) change the algorithms to FIPS-compliant ones. > For net/tls, we skip the test variants that use the ChaCha20-Poly1305 > algorithm. > For net/net:vrf-xfrm-tests, we can simply replace the algorithms that > are not FIPS-compliant with compliant ones. > > [Test Plan] > With a fips kernel installed, pass fips=1 as a kernel parameter, run the > net/tls and net/vrf-xfrm-tests tests with these patches applied, and > check that they are all passing. > > [Where problems could occur] > Regression risk is very low and would hardly affect any user, since the > changes only touch the selftests. > > [Other Info] > I'm sending this to be applied on the generic kernel, as Jammy FIPS > derivative kernels will easily inherit these changes. > > Changes in v2: > - Target generic kernel. > - fcnal-test.sh: dropped as it will be picked from upstream stable. > - tls.c: skip tests right at setup if in FIPS mode, this requires commit > 372b304c ("selftests/harness: allow tests to be skipped during setup"). > > Magali Lemes (3): > selftests/harness: allow tests to be skipped during setup > selftests: net: tls: check if FIPS mode is enabled > selftests: net: vrf-xfrm-tests: change authentication and encryption > algos > > tools/testing/selftests/kselftest_harness.h | 6 ++-- > tools/testing/selftests/net/tls.c | 21 ++++++++++++ > tools/testing/selftests/net/vrf-xfrm-tests.sh | 32 +++++++++---------- > 3 files changed, 40 insertions(+), 19 deletions(-) > > Applied to jammy:master-next. Thanks! Roxana
BugLink: https://bugs.launchpad.net/bugs/2019868 BugLink: https://bugs.launchpad.net/bugs/2019880 [Impact] Due to the introduction of net tests that rely on cryptographic functions to work, some test cases from net/tls and net/vrf-xfrm-tests that use non-compliant FIPS algorithms fail when fips=1. [Fix] To fix these failures in FIPS mode, we can, on a case-by-case basis, 1) skip the tests that require non-compliant FIPS algorithms or 2) change the algorithms to FIPS-compliant ones. For net/tls, we skip the test variants that use the ChaCha20-Poly1305 algorithm. For net/net:vrf-xfrm-tests, we can simply replace the algorithms that are not FIPS-compliant with compliant ones. [Test Plan] With a fips kernel installed, pass fips=1 as a kernel parameter, run the net/tls and net/vrf-xfrm-tests tests with these patches applied, and check that they are all passing. [Where problems could occur] Regression risk is very low and would hardly affect any user, since the changes only touch the selftests. [Other Info] I'm sending this to be applied on the generic kernel, as Jammy FIPS derivative kernels will easily inherit these changes. Changes in v2: - Target generic kernel. - fcnal-test.sh: dropped as it will be picked from upstream stable. - tls.c: skip tests right at setup if in FIPS mode, this requires commit 372b304c ("selftests/harness: allow tests to be skipped during setup"). Magali Lemes (3): selftests/harness: allow tests to be skipped during setup selftests: net: tls: check if FIPS mode is enabled selftests: net: vrf-xfrm-tests: change authentication and encryption algos tools/testing/selftests/kselftest_harness.h | 6 ++-- tools/testing/selftests/net/tls.c | 21 ++++++++++++ tools/testing/selftests/net/vrf-xfrm-tests.sh | 32 +++++++++---------- 3 files changed, 40 insertions(+), 19 deletions(-)