feat: add test for secure boot key enrollment in setup mode#704
Open
adsuna wants to merge 1 commit intovirt-s1:masterfrom
Open
feat: add test for secure boot key enrollment in setup mode#704adsuna wants to merge 1 commit intovirt-s1:masterfrom
adsuna wants to merge 1 commit intovirt-s1:masterfrom
Conversation
Signed-off-by: Aditya Nair <adnair@redhat.com>
Reviewer's GuideAdds a new lifecycle test that enables Secure Boot on UEFI systems running in setup mode using virt-fw-vars/efivar, and introduces a targeted teardown path to fully recreate the VM after this test runs. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new tearDown branch for
test_uefi_enable_securebootduplicates the existingvm_deleteVM-recreation logic; consider consolidating this into a shared helper or a single conditional path to avoid repetition and reduce chances of future divergence. - Within
test_uefi_enable_secureboot, there are multiplecd {work_dir} && ...command invocations; wrapping those operations in a small helper or building absolute paths (e.g., usingos.path.join) would make the test easier to read and less error-prone if the working directory changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new tearDown branch for `test_uefi_enable_secureboot` duplicates the existing `vm_delete` VM-recreation logic; consider consolidating this into a shared helper or a single conditional path to avoid repetition and reduce chances of future divergence.
- Within `test_uefi_enable_secureboot`, there are multiple `cd {work_dir} && ...` command invocations; wrapping those operations in a small helper or building absolute paths (e.g., using `os.path.join`) would make the test easier to read and less error-prone if the working directory changes.
## Individual Comments
### Comment 1
<location path="os_tests/tests/test_lifecycle.py" line_range="1014-1015" />
<code_context>
+ self.assertIn('SecureBoot enabled', sb_state_after,
+ msg='Secure Boot should be enabled after reboot, got: {}'.format(sb_state_after))
+
+ utils_lib.run_cmd(self, 'sudo keyctl list %:.platform', expect_ret=0,
+ expect_kw='Signature Database key',
+ msg='Verify custom cert in kernel keyring')
+
</code_context>
<issue_to_address>
**suggestion (testing):** The test only validates the custom db cert, not that the expected Microsoft / Red Hat CAs are present in db as described in the test plan.
The `expect_result` docstring says Secure Boot should be enabled with both the custom cert and Microsoft CAs in db, but the test only checks for `Signature Database key` (the custom cert) in the keyring. Please extend the assertions to also confirm the expected Microsoft/Red Hat certs are present (e.g., by grepping for known subjects or key IDs) so the test validates the full enrollment pipeline, including vendor keys.
Suggested implementation:
```python
self.assertIn('SecureBoot enabled', sb_state_after,
msg='Secure Boot should be enabled after reboot, got: {}'.format(sb_state_after))
# Verify custom and vendor certificates (Microsoft / Red Hat) are present in the platform keyring
keyring_output = utils_lib.run_cmd(
self,
'sudo keyctl list %:.platform',
expect_ret=0,
msg='List platform keyring to verify enrolled certificates'
)
# Custom DB certificate from the test
self.assertIn(
'Signature Database key',
keyring_output,
msg='Custom DB certificate (Signature Database key) should be present in the platform keyring, got: {}'.format(
keyring_output
),
)
# Vendor certificates – validate that both Microsoft and Red Hat keys are present to
# cover the full enrollment pipeline as described in the test plan.
expected_vendor_markers = [
# Common Microsoft DB cert subjects / labels
'Microsoft Windows Production PCA',
'Microsoft Corporation UEFI CA',
# Common Red Hat Secure Boot DB cert subjects / labels
'Red Hat Secure Boot CA',
'Red Hat Secure Boot (CA key 1)',
]
for marker in expected_vendor_markers:
if marker in keyring_output:
continue
self.assertTrue(
any(marker in keyring_output for marker in expected_vendor_markers if 'Microsoft' in marker),
msg='Expected at least one Microsoft Secure Boot DB certificate in platform keyring, got: {}'.format(
keyring_output
),
)
self.assertTrue(
any(marker in keyring_output for marker in expected_vendor_markers if 'Red Hat' in marker),
msg='Expected at least one Red Hat Secure Boot DB certificate in platform keyring, got: {}'.format(
keyring_output
),
)
```
1. If your environment uses different subject strings or labels for the Microsoft / Red Hat DB certificates, update the `expected_vendor_markers` list to match the actual key names as they appear in `keyctl list %:.platform`.
2. The small `for marker in expected_vendor_markers` loop is currently only used to short-circuit on matches; you can remove it if you prefer and rely solely on the two `assertTrue(any(...))` checks.
3. If `utils_lib.run_cmd` already supports a list for `expect_kw`, you could alternatively pass `expect_kw=[...]` instead of doing manual `assertIn` checks, but the current implementation is explicit and independent of that behavior.
</issue_to_address>
### Comment 2
<location path="os_tests/tests/test_lifecycle.py" line_range="963-972" />
<code_context>
+ work_dir = '/tmp/secureboot_setup'
</code_context>
<issue_to_address>
**suggestion (testing):** Test leaves artifacts and changed state if it fails before the final cleanup; consider using a `try/finally` or `addCleanup`.
Because the temp directory and certs are only removed on success, any failure after `work_dir` is created will leave them behind and can affect later test runs. Using a `try/finally` or `self.addCleanup` to register cleanup for the directory and other temp resources will keep the test isolated and avoid state leakage, which is important given it modifies UEFI vars and keys.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+1014
to
+1015
| utils_lib.run_cmd(self, 'sudo keyctl list %:.platform', expect_ret=0, | ||
| expect_kw='Signature Database key', |
There was a problem hiding this comment.
suggestion (testing): The test only validates the custom db cert, not that the expected Microsoft / Red Hat CAs are present in db as described in the test plan.
The expect_result docstring says Secure Boot should be enabled with both the custom cert and Microsoft CAs in db, but the test only checks for Signature Database key (the custom cert) in the keyring. Please extend the assertions to also confirm the expected Microsoft/Red Hat certs are present (e.g., by grepping for known subjects or key IDs) so the test validates the full enrollment pipeline, including vendor keys.
Suggested implementation:
self.assertIn('SecureBoot enabled', sb_state_after,
msg='Secure Boot should be enabled after reboot, got: {}'.format(sb_state_after))
# Verify custom and vendor certificates (Microsoft / Red Hat) are present in the platform keyring
keyring_output = utils_lib.run_cmd(
self,
'sudo keyctl list %:.platform',
expect_ret=0,
msg='List platform keyring to verify enrolled certificates'
)
# Custom DB certificate from the test
self.assertIn(
'Signature Database key',
keyring_output,
msg='Custom DB certificate (Signature Database key) should be present in the platform keyring, got: {}'.format(
keyring_output
),
)
# Vendor certificates – validate that both Microsoft and Red Hat keys are present to
# cover the full enrollment pipeline as described in the test plan.
expected_vendor_markers = [
# Common Microsoft DB cert subjects / labels
'Microsoft Windows Production PCA',
'Microsoft Corporation UEFI CA',
# Common Red Hat Secure Boot DB cert subjects / labels
'Red Hat Secure Boot CA',
'Red Hat Secure Boot (CA key 1)',
]
for marker in expected_vendor_markers:
if marker in keyring_output:
continue
self.assertTrue(
any(marker in keyring_output for marker in expected_vendor_markers if 'Microsoft' in marker),
msg='Expected at least one Microsoft Secure Boot DB certificate in platform keyring, got: {}'.format(
keyring_output
),
)
self.assertTrue(
any(marker in keyring_output for marker in expected_vendor_markers if 'Red Hat' in marker),
msg='Expected at least one Red Hat Secure Boot DB certificate in platform keyring, got: {}'.format(
keyring_output
),
)- If your environment uses different subject strings or labels for the Microsoft / Red Hat DB certificates, update the
expected_vendor_markerslist to match the actual key names as they appear inkeyctl list %:.platform. - The small
for marker in expected_vendor_markersloop is currently only used to short-circuit on matches; you can remove it if you prefer and rely solely on the twoassertTrue(any(...))checks. - If
utils_lib.run_cmdalready supports a list forexpect_kw, you could alternatively passexpect_kw=[...]instead of doing manualassertInchecks, but the current implementation is explicit and independent of that behavior.
Comment on lines
+963
to
+972
| work_dir = '/tmp/secureboot_setup' | ||
| utils_lib.run_cmd(self, 'mkdir -p {}'.format(work_dir), expect_ret=0) | ||
|
|
||
| ovmf_dir = '/usr/share/edk2/ovmf' | ||
| dbx = utils_lib.run_cmd(self, | ||
| 'ls -t {}/DBXUpdate*.bin 2>/dev/null | head -1'.format(ovmf_dir), | ||
| msg='Find DBXUpdate binary').strip() | ||
| if not dbx: | ||
| self.skipTest('DBXUpdate binary not found in {}'.format(ovmf_dir)) | ||
|
|
There was a problem hiding this comment.
suggestion (testing): Test leaves artifacts and changed state if it fails before the final cleanup; consider using a try/finally or addCleanup.
Because the temp directory and certs are only removed on success, any failure after work_dir is created will leave them behind and can affect later test runs. Using a try/finally or self.addCleanup to register cleanup for the directory and other temp resources will keep the test isolated and avoid state leakage, which is important given it modifies UEFI vars and keys.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add
test_uefi_enable_secureboottotest_lifecycle.pyto cover enabling Secure Boot on UEFI systems in setup mode.Test steps:
openssl,efivar,python3-virt-firmware,edk2-ovmf)virt-fw-vars --enroll-redhatto build a UEFI variable store with RH PK/KEK, Microsoft (2011+2023) db certs, the custom db cert, and dbx list.Summary by Sourcery
Add a lifecycle test that enables Secure Boot on UEFI systems in setup mode and adjusts teardown to fully recreate the VM after this test.
New Features:
Tests: