Add case for checking default secure db#705
Conversation
Reviewer's GuideAdd a new secure boot validation test that runs mokutil- and rpm-based checks when Secure Boot is enabled, alongside a minor whitespace fix in an existing test. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new test case, test_check_secureboot, to verify Secure Boot status and its associated certificates (DB, PK, KEK) along with the shim package. A review comment identifies an issue where a shell pipeline in the PK check could mask command failures, suggesting a more robust approach using expect_kw to verify output content directly.
|
|
||
| utils_lib.run_cmd(self, "sudo rpm -qa | grep shim", expect_ret=0, msg="Check installed shim package") | ||
| utils_lib.run_cmd(self, "sudo mokutil --db --short", expect_ret=0, msg="Check default certs in DB") | ||
| utils_lib.run_cmd(self, "sudo mokutil --pk | grep -E '(Subject:|Not After)' | head -2", expect_ret=0, msg="Check default PK value") |
There was a problem hiding this comment.
The shell pipeline sudo mokutil --pk | grep -E '(Subject:|Not After)' | head -2 masks potential failures from the mokutil command. In a standard shell execution, the exit status of a pipeline is the exit status of the last command (head -2), which will almost always return 0 even if mokutil fails or grep finds no matches. Consequently, expect_ret=0 does not effectively verify that the PK value was successfully retrieved. It is more robust to run the command directly and use expect_kw to verify that the expected fields are present in the output.
| utils_lib.run_cmd(self, "sudo mokutil --pk | grep -E '(Subject:|Not After)' | head -2", expect_ret=0, msg="Check default PK value") | |
| utils_lib.run_cmd(self, "sudo mokutil --pk", expect_ret=0, expect_kw="Subject,Not After", msg="Check default PK value") |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
test_check_secureboot, consider addingexpect_ret=0to the initialmokutil --sb-statecall (and handling failure) so the test clearly fails or skips whenmokutilis missing or the command errors instead of silently proceeding with an unexpected output string. - The check for the shim package using
rpm -qa | grep shimcould be made more robust by querying the specific package name viarpm -q shim*or similar, avoiding potential false positives from unrelated packages with 'shim' in their name.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_check_secureboot`, consider adding `expect_ret=0` to the initial `mokutil --sb-state` call (and handling failure) so the test clearly fails or skips when `mokutil` is missing or the command errors instead of silently proceeding with an unexpected output string.
- The check for the shim package using `rpm -qa | grep shim` could be made more robust by querying the specific package name via `rpm -q shim*` or similar, avoiding potential false positives from unrelated packages with 'shim' in their name.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Summary by Sourcery
Add a new general check to validate Secure Boot configuration and certificates, and fix minor formatting in an existing product key test.
Tests: