Repository navigation
test(images): validate kernel FIPS boot options - #19023
Tobias Brick (tobiasb-ms) merged 2 commits into
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Adds a static image test to validate kernel FIPS boot options by parsing kernel arguments from each BLS boot entry and enforcing fips=1 only for -fips machine-bootable images.
Changes:
- Introduces a session-scoped fixture that parses kernel options from BLS boot entry
.conffiles. - Adds a static test asserting
fips=1appears exactly once on-fipsimages and never on non-FIPS images. - Documents the new fixture in the image tests README.
| File | Description |
|---|---|
| base/images/tests/conftest.py | Adds boot_entry_kernel_options fixture to parse kernel cmdline options from BLS entries. |
| base/images/tests/cases/static/test_fips.py | New static test enforcing FIPS kernel option expectations by image variant. |
| base/images/tests/README.md | Documents the new fixture in the fixture table. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
a30aa9f to
8ce0860
Compare
|
/azp run |
|
Azure Pipelines: 2 pipeline(s) were filtered out due to trigger conditions. |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 1
Open (2)
Resolved since last review (1)
8ce0860 to
66333c1
Compare
66333c1 to
1971803
Compare
|
/azp run |
|
Azure Pipelines: 2 pipeline(s) were filtered out due to trigger conditions. |
1971803 to
f1253fe
Compare
|
/azp run |
|
Azure Pipelines: 2 pipeline(s) were filtered out due to trigger conditions. |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 2
f1253fe to
4618bfa
Compare
|
/azp run |
|
Azure Pipelines: 2 pipeline(s) were filtered out due to trigger conditions. |
4618bfa to
5241307
Compare
|
/azp run |
|
Azure Pipelines: 2 pipeline(s) were filtered out due to trigger conditions. |
|
|
||
|
|
||
| @pytest.fixture(scope="session") | ||
| def boot_entry_kernel_options(rootfs: Path) -> dict[Path, list[str]]: |
There was a problem hiding this comment.
issue(blocking): Since this method is a test fixture it should not do any validation other than required to make sure data population or fixture setup works as expected or fails gracefully. That means it ideally only parses the conf file and returns the values as is. Interpretation or validation is done by the consumers of the fixture returned values.
A direct impact is a fixture failure instead of a test failure when something is wrong with the configuration.
There was a problem hiding this comment.
Good callout. I've refactored so the fixture just returns raw line and then there's a parsing helper that tests can use.
| @pytest.fixture(scope="session") | ||
| def boot_entry_kernel_options(rootfs: Path) -> dict[Path, list[str]]: | ||
| """Kernel command-line options keyed by BLS boot entry path.""" | ||
| entries_dirs = ( |
There was a problem hiding this comment.
suggestion(non-blocking): I am not very comfortable with the implicit selection of whichever boot configs are found in the image. Much like image capability driving what to expect in the image can't we drive this fixture the same way?
There was a problem hiding this comment.
It feels like we should check everything and then caller can decide what to do. In our case, it feels very unlikely that any images will ever actually have multiple entrieson a freshly-baked image so we should be safe. But either way, in principle, I'd prefer to cover them all.
Thoughts?
There was a problem hiding this comment.
Your argument is valid and that anticipation is why the comment is non-blocking. To illustrate what I am trying to convey if we look at the rootfs fixture, it checks the image type and does the mounting in different ways. Similarly, since we know what kind of image we are inspecting, we can look at the deterministic loader configuration path for that image. This definitely does not break the test and it is your choice whether to make a change.
Parse BLS kernel options once through a shared session fixture and reuse the parsed arguments for swap validation. Cover both standard and EFI BLS entry locations while retaining separate handling for grub.cfg. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Require every machine-bootable image to carry kernel FIPS options matching its declared image capabilities. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
5241307 to
524e2da
Compare
|
/azp run |
|
Azure Pipelines: 2 pipeline(s) were filtered out due to trigger conditions. |
Summary
fips=1exactly once for-fipsmachine-bootable imagesTesting
pre-commit run --all-files: Ruff, Pyright, editorconfig-checker, and lychee passedazurelinux-repos-4.0-23.azl4from the current 4.0 sourcestatic-image-checksagainst all 12 configured image artifacts: 315 passed, 177 capability-based skipsiso-validationsuite is LISA metadata-only and cannot run locally without a pinned LISA source