add azure linux-specific fips tests #3688
Conversation
lisa/tools/fips.py
Outdated
| return True | ||
|
|
||
| @abstractmethod | ||
| def set_fips_mode(self, fips_mode: bool) -> None: |
There was a problem hiding this comment.
This method should be in the fips class.
There was a problem hiding this comment.
Fips has the methods enable_fips and disable_fips. These handle the overall, multi-part process of enabling/disabling fips. One of those steps is to set or clear the fips kernel flag. That process is different depending on how we configure grub, which is encapsulated in the GrubConfig.set_fips_mode class and its subclasses (Note: I just changed it from Grub to GrubConfig).
Perhaps changing this method's name would help clarify that? Maybe set_fips_kernel_flag?
There was a problem hiding this comment.
My point is the "fips" related methods shouldn't be in the GrubConfig class. The GrubConfig tool provides its own functionality, and not provide high level functionality. So, the set_fips_mode should be moved to fips level.
There was a problem hiding this comment.
But the exact way to set the fips flag is fundamentally different between AZL2 and AZL3, at the grub level, not at the higher-level Fips level.
At the Fips level, you want to either turn that specific kernel flag on or off.
At the grub level, there are different ways to do that depending on how use/configure grub on the specific platform.
Moving this function into Fips means those classes need to the internals of grub our grub configuration, which I don't want.
There was a problem hiding this comment.
It seems like azl2/3 have different way on removing args in grub config. It's a different behavior from grub config, not for fips only. The remove_kernel_cmdline_arg is well wrapped the behavior.
Another way to simplify it to accept "fips=0" in azl3, is to add same_as_root for remove_kernel_cmdline_arg. So it can be used in azl3 to decide set "key=" or remove "key".
| @@ -32,61 +137,84 @@ class Fips(TestSuite): | |||
| 4. Verify that FIPS was enabled properly | |||
| """, | |||
| priority=3, | |||
| requirement=simple_requirement( | |||
| unsupported_os=[CBLMariner], | |||
There was a problem hiding this comment.
if it's by design, I prefer to have the same verify_fips_enable for all distros. Azl can go with a separated path.
| supported_os=[CBLMariner], | ||
| ), | ||
| ) | ||
| def verify_azl_fips_is_enabled( |
There was a problem hiding this comment.
I think this test case can be merged with verify_azl_fips_is_disabled. When only one of test cases runs, it doesn't need to have two cases.
The logic can check consistent between variable/metadata with the actual fips mode. Let me know, if there are other situations.
|
|
||
| # Re-disable FIPS to make sure the test can be run multiple times. | ||
| azl_fips.disable_fips() | ||
| node.reboot() |
There was a problem hiding this comment.
Recover the fips status, because we don't know the order of test cases. If the test case run before verifying initial state case, the initial state case will be failed. If it's hard to guarantee reset, you can mark the environment is dirty at end of this test case. So this env won't be reused.
| supported_os=[CBLMariner], | ||
| ), | ||
| ) | ||
| def verify_azl_fips_enable( |
There was a problem hiding this comment.
It's similar with another pair of test cases. This case should be able to merge with verify_azl_fips_disable. We don't prefer a fine granularity of test cases, especially it makes more skipped test cases.
There was a problem hiding this comment.
Wouldn't it be better, when looking at test results, to immediately see from the test name that enabling or disabling fips specifically didn't work? Rather than something generic like verify_azl_fips_enable_disable failed and needing to dig into the specific assert to know which part failed? As a consumer of the test output, I much prefer that.
There was a problem hiding this comment.
The name could be like verify_azl_fips_enablement. Some partners will look at each skipped test cases, so small number of skipped tests will reduce the overhead a little bit. LISA test results always bring the error message, it's the right place to understand failure reasons. A shorter test case name is also better for documentation and display.
One more reason, it's important, but I haven't thought about. The test cases run on Azl only, but the distro version checks happen runtime. So, one test case can reduce one deployment.
| @@ -9,6 +9,7 @@ | |||
| from lisa.executable import Tool | |||
| from lisa.operating_system import CBLMariner | |||
| from lisa.tools import Blkid, Cat | |||
| from .grub_config import GrubConfig | |||
There was a problem hiding this comment.
Please follow dev env setup in vscode, the isort can fix the order automatically when saving a file.
| returns True for "true", False for "false", and None for any other value. | ||
| Allows for casing and leading/trailing whitespace. | ||
| """ | ||
| str_to_bool_map = { |
There was a problem hiding this comment.
@squirrelsc -- I made this only support true and false (allowing for casing and leading/trailing whitespace). Happy to expand that to things like 0/1, yes/no or other things you think would be good.
Also, I didn't go looking for other places that could use this -- kind of a needle in a haystack.
There was a problem hiding this comment.
It would be great, if it follows the original implementation in Python lib, which supports 0/1, yes/no. And it should also support bool. For example, str_to_bool(True) should return True. In case it's used for safe conversion.
Add Azure Linux-specific tests around FIPS compliance. Specifically, this change adds:
verify_fips_is_enabled_azl: Verifies that an AZL system is in FIPS mode.verify_fips_is_disabled_azl: Verifies that an AZL system is not in FIPS mode.verify_fips_enable_azl: Verifies that an AZL system can be put into FIPS mode.verify_fips_disable_azl: Verifies that an AZL system can be taken out of FIPS mode.It also removes the AZL code from
verify_fips_enable.Due to the nature of these tests, they only make sense to run if we believe a machine is already in the correct mode. For example,
verify_fips_is_enabled_azlwould make sense on the marketplace imageMicrosoftCBLMariner:azure-linux-3:azure-linux-3-fips:latestbut not onMicrosoftCBLMariner:azure-linux-3:azure-linux-3:latest.To accomplish this, we look at two things (see function
ensure_fips_expectations).First, we see if a variable
testing_fips_imagewas provided to the test case. If so, and if it isyesorno, we respect that.Second, we try to hit the azure metadata endpoint to get the image sku name. If that is a FIPS sku, we consider it to be a FIPS machine.
If neither exist, we skip the test.