Add host validation case for secure launch features#6813
Open
liang-cong-red-hat wants to merge 1 commit intoautotest:masterfrom
Open
Add host validation case for secure launch features#6813liang-cong-red-hat wants to merge 1 commit intoautotest:masterfrom
liang-cong-red-hat wants to merge 1 commit intoautotest:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new test configuration file libvirt/tests/cfg/cpu/secure_host_validate.cfg defining a secure_host_validate profile with variants for amd_sev, amd_sev_es, amd_sev_snp, intel_sgx, and intel_tdx (vendor, cpu_flags, kernel options, kvm_probe, required commands, kernel message checks, domcapability XPath and success patterns). Adds a test module libvirt/tests/src/cpu/secure_host_validate.py implementing run(test, params, env) that validates prerequisites (vendor, CPU flags, kernel options), installs required commands, runs kernel/message and utility checks against configured patterns, and verifies domcapability XML and libvirt feature support. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
libvirt/tests/cfg/cpu/secure_host_validate.cfg (1)
11-14: Reduce duplicated AMD config blocks with shared${...}variables.
req_kernel_optsandreq_cmdsare duplicated across all AMD variants. Centralizing them will reduce drift risk when these requirements change.♻️ Suggested cfg refactor
- cpu.secure_launch.secure_host_validate: type = secure_host_validate start_vm = "no" only x86_64 kernel_msg_cmd = "journalctl -b 0 -k" + amd_req_kernel_opts = ["mem_encrypt=on", "amd_iommu=on"] + amd_req_cmds = {"sevctl": {"package": "sevctl", "cmd_params": "ok"}, "virt-host-validate": {"package": "libvirt", "cmd_params": ""}} variants: - amd_sev: cpu_vendor = "amd" secure_feature = "sev" cpu_flags = ["sev"] - req_kernel_opts = ["mem_encrypt=on", "amd_iommu=on"] + req_kernel_opts = ${amd_req_kernel_opts} kvm_probe_module_force_load = yes kvm_probe_module_parameters = "sev=1" - req_cmds = {"sevctl": {"package": "sevctl", "cmd_params": "ok"}, "virt-host-validate": {"package": "libvirt", "cmd_params": ""}} + req_cmds = ${amd_req_cmds} sevctl_patterns = ['\[.*PASS.*\]\s+-\s+Secure\sEncrypted\sVirtualization\s+\(SEV\)'] virt-host-validate_patterns = ['QEMU: Checking for secure guest support\s+:\s+PASS\n\s*\(SEV'] - amd_sev_es: cpu_vendor = "amd" secure_feature = "sev-es" cpu_flags = ["sev", "sev_es"] - req_kernel_opts = ["mem_encrypt=on", "amd_iommu=on"] + req_kernel_opts = ${amd_req_kernel_opts} kvm_probe_module_force_load = yes kvm_probe_module_parameters = "sev=1 sev-es=1" - req_cmds = {"sevctl": {"package": "sevctl", "cmd_params": "ok"}, "virt-host-validate": {"package": "libvirt", "cmd_params": ""}} + req_cmds = ${amd_req_cmds} sevctl_patterns = ['\[.*PASS.*\]\s+-\s+Secure\sEncrypted\sVirtualization\s+\(SEV\)', '\[.*PASS.*\]\s+-\s+Encrypted\sState\s+\(SEV-ES\)'] virt-host-validate_patterns = ['QEMU: Checking for secure guest support\s+:\s+PASS\n\s*\(SEV'] - amd_sev_snp: cpu_vendor = "amd" secure_feature = "sev-snp" cpu_flags = ["sev", "sev_es", "sev_snp"] - req_kernel_opts = ["mem_encrypt=on", "amd_iommu=on"] + req_kernel_opts = ${amd_req_kernel_opts} kvm_probe_module_force_load = yes kvm_probe_module_parameters = "sev=1 sev-es=1 sev-snp=1" - req_cmds = {"sevctl": {"package": "sevctl", "cmd_params": "ok"}, "virt-host-validate": {"package": "libvirt", "cmd_params": ""}} + req_cmds = ${amd_req_cmds} sevctl_patterns = ['\[.*PASS.*\]\s+-\s+Secure\sEncrypted\sVirtualization\s+\(SEV\)', '\[.*PASS.*\]\s+-\s+Encrypted\sState\s+\(SEV-ES\)', '\[.*PASS.*\]\s+-\s+Secure Nested Paging\s+\(SEV-SNP\)'] virt-host-validate_patterns = ['QEMU: Checking for secure guest support\s+:\s+PASS\n\s*\(SEV-SNP\)']Based on learnings: In tp-libvirt test config files (
*.cfgunderlibvirt/tests/cfg), the cartesian config parser supports variable substitution using the${variable_name}syntax.Also applies to: 21-24, 31-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/cfg/cpu/secure_host_validate.cfg` around lines 11 - 14, Centralize the duplicated AMD-specific entries by extracting req_kernel_opts and req_cmds into shared variables and referencing them in each AMD variant via the cartesian parser substitution syntax (e.g. set req_kernel_opts = ["mem_encrypt=on","amd_iommu=on"] and req_cmds = {...} once, then use "${req_kernel_opts}" and "${req_cmds}" in the individual AMD configs); ensure kvm_probe_module_force_load and kvm_probe_module_parameters remain applied where needed and update each block that currently repeats those arrays (references: req_kernel_opts, req_cmds, kvm_probe_module_force_load, kvm_probe_module_parameters) so only the variant-specific values stay in place and common values are pulled from the shared variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libvirt/tests/src/cpu/secure_host_validate.py`:
- Line 76: The tests call process.run(...) with shell=True for parameter-driven
commands (see kernel_msg_cmd and cmd_info); replace shell=True with shell=False
and pass an argv list instead of a single string: for kernel_msg_cmd
build/ensure a list of arguments (or use shlex.split(kernel_msg_cmd)) and for
cmd_info convert the string to a list via shlex.split(cmd_info) before calling
process.run, keeping other call sites and error handling unchanged.
- Line 42: Replace all unsafe eval() calls used to parse config params (e.g. the
cpu_flags parsing at cpu_flags = eval(params.get("cpu_flags", "[]")) and the
similar sites mentioned) with ast.literal_eval to safely parse Python literals;
add an import for the ast module at the top of the file and change each
occurrence (lines referenced: the eval calls at cpu_flags and the other eval
uses) to use ast.literal_eval(params.get(..., default)) and keep the same
default string values.
---
Nitpick comments:
In `@libvirt/tests/cfg/cpu/secure_host_validate.cfg`:
- Around line 11-14: Centralize the duplicated AMD-specific entries by
extracting req_kernel_opts and req_cmds into shared variables and referencing
them in each AMD variant via the cartesian parser substitution syntax (e.g. set
req_kernel_opts = ["mem_encrypt=on","amd_iommu=on"] and req_cmds = {...} once,
then use "${req_kernel_opts}" and "${req_cmds}" in the individual AMD configs);
ensure kvm_probe_module_force_load and kvm_probe_module_parameters remain
applied where needed and update each block that currently repeats those arrays
(references: req_kernel_opts, req_cmds, kvm_probe_module_force_load,
kvm_probe_module_parameters) so only the variant-specific values stay in place
and common values are pulled from the shared variables.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/cpu/secure_host_validate.cfglibvirt/tests/src/cpu/secure_host_validate.py
|
|
||
| def _validate_cpu_flag(): | ||
| host_cpu_flags = set(cpu.get_cpu_flags()) | ||
| cpu_flags = eval(params.get("cpu_flags", "[]")) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n libvirt/tests/src/cpu/secure_host_validate.pyRepository: autotest/tp-libvirt
Length of output: 4622
Replace eval() with safe literal parsing for cfg params.
These eval() calls parse config-provided strings as executable Python, enabling arbitrary code execution. Use ast.literal_eval() instead, which safely parses Python literals.
Suggested fix
+import ast
from avocado.utils import cpu as cpu_utils
from avocado.utils import process
@@
def run(test, params, env):
@@
+ def _parse_literal_param(name, default):
+ raw = params.get(name, default)
+ try:
+ return ast.literal_eval(raw)
+ except (ValueError, SyntaxError) as err:
+ test.error("Invalid literal for '%s': %s (raw=%r)" % (name, err, raw))
+
def validate_prerequisites():
@@
- cpu_flags = eval(params.get("cpu_flags", "[]"))
+ cpu_flags = _parse_literal_param("cpu_flags", "[]")
@@
- req_kernel_opts = eval(params.get("req_kernel_opts", "[]"))
+ req_kernel_opts = _parse_literal_param("req_kernel_opts", "[]")
@@
- kernel_msg_patterns = eval(params.get("kernel_msg_patterns", "[]"))
+ kernel_msg_patterns = _parse_literal_param("kernel_msg_patterns", "[]")
@@
- pkg_patterns = eval(params.get("%s_patterns" % cmd, "[]"))
+ pkg_patterns = _parse_literal_param("%s_patterns" % cmd, "[]")
@@
- domcap_xpath = eval(params.get("domcap_xpath", "[]"))
+ domcap_xpath = _parse_literal_param("domcap_xpath", "[]")
@@
- req_cmds = eval(params.get("req_cmds", "{}"))
+ req_cmds = _parse_literal_param("req_cmds", "{}")Also applies to lines: 47, 75, 82, 90, 97
🧰 Tools
🪛 Ruff (0.15.2)
[error] 42-42: Use of possibly insecure function; consider using ast.literal_eval
(S307)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libvirt/tests/src/cpu/secure_host_validate.py` at line 42, Replace all unsafe
eval() calls used to parse config params (e.g. the cpu_flags parsing at
cpu_flags = eval(params.get("cpu_flags", "[]")) and the similar sites mentioned)
with ast.literal_eval to safely parse Python literals; add an import for the ast
module at the top of the file and change each occurrence (lines referenced: the
eval calls at cpu_flags and the other eval uses) to use
ast.literal_eval(params.get(..., default)) and keep the same default string
values.
| test.log.info("TEST_STEP1: Check expected kernel msg") | ||
| kernel_msg_cmd = params.get("kernel_msg_cmd") | ||
| kernel_msg_patterns = eval(params.get("kernel_msg_patterns", "[]")) | ||
| result = process.run(kernel_msg_cmd, shell=True) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check if the file exists and examine the relevant lines
if [ -f libvirt/tests/src/cpu/secure_host_validate.py ]; then
echo "=== File found. Checking lines around 76 and 85 ==="
sed -n '70,90p' libvirt/tests/src/cpu/secure_host_validate.py | cat -n
else
echo "File not found at libvirt/tests/src/cpu/secure_host_validate.py"
# Try to find similar files
find . -name "*secure_host_validate*" -type f 2>/dev/null | head -10
fiRepository: autotest/tp-libvirt
Length of output: 1322
🏁 Script executed:
#!/bin/bash
# Search for shell=True usage in the file to verify the claim
echo "=== Checking for shell=True in the file ==="
rg -n 'shell=True' libvirt/tests/src/cpu/secure_host_validate.py 2>/dev/null || echo "Pattern not found or file not accessible"Repository: autotest/tp-libvirt
Length of output: 231
🏁 Script executed:
#!/bin/bash
# Get file size and full context around the flagged lines
echo "=== Full file structure ==="
wc -l libvirt/tests/src/cpu/secure_host_validate.py 2>/dev/null || echo "Cannot determine file size"
# Look at broader context to understand command construction
echo "=== Context around command execution ==="
sed -n '1,100p' libvirt/tests/src/cpu/secure_host_validate.py | cat -n 2>/dev/null || echo "Cannot read file"Repository: autotest/tp-libvirt
Length of output: 4719
Avoid shell=True for parameter-driven command execution.
Lines 76 and 85 use shell=True with configuration-driven commands (from params and cmd_info), unnecessarily expanding the injection surface and triggering Ruff's S604 security check.
Both calls can safely run without a shell. Replace shell=True with shell=False, and convert the string command at line 85 to a list using shlex.split():
Suggested fix
+import shlex
from avocado.utils import cpu as cpu_utils
from avocado.utils import process
@@
- result = process.run(kernel_msg_cmd, shell=True)
+ result = process.run(kernel_msg_cmd, shell=False)
@@
- pkg_cmd = "%s %s" % (cmd, cmd_info["cmd_params"])
- result = process.run(pkg_cmd, shell=True)
+ pkg_cmd = [cmd] + shlex.split(cmd_info.get("cmd_params", ""))
+ result = process.run(pkg_cmd, shell=False)🧰 Tools
🪛 Ruff (0.15.2)
[error] 76-76: Function call with shell=True parameter identified, security issue
(S604)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libvirt/tests/src/cpu/secure_host_validate.py` at line 76, The tests call
process.run(...) with shell=True for parameter-driven commands (see
kernel_msg_cmd and cmd_info); replace shell=True with shell=False and pass an
argv list instead of a single string: for kernel_msg_cmd build/ensure a list of
arguments (or use shlex.split(kernel_msg_cmd)) and for cmd_info convert the
string to a list via shlex.split(cmd_info) before calling process.run, keeping
other call sites and error handling unchanged.
liang-cong-red-hat
force-pushed
the
new_case_secure_host_validate
branch
from
17abb5a to
7a0754b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libvirt/tests/src/cpu/secure_host_validate.py`:
- Line 104: The call to libvirt_version.is_libvirt_feature_supported(params) in
secure_host_validate.py currently ignores its boolean result; update the test to
capture the return value and skip the test when it returns False (e.g., use the
test framework's skip/skipTest/pytest.skip mechanism) so the test does not run
on unsupported libvirt versions; ensure the check occurs before any
feature-dependent logic and include a clear skip message mentioning the
unsupported feature and params.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/cpu/secure_host_validate.cfglibvirt/tests/src/cpu/secure_host_validate.py
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/cpu/secure_host_validate.cfg
| domcap_xml = domcapability_xml.DomCapabilityXML() | ||
| libvirt_vmxml.check_guest_xml_by_xpaths(domcap_xml, domcap_xpath) | ||
|
|
||
| libvirt_version.is_libvirt_feature_supported(params) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the file to understand the context
find . -name "secure_host_validate.py" -type fRepository: autotest/tp-libvirt
Length of output: 110
🏁 Script executed:
#!/bin/bash
# Search for is_libvirt_feature_supported function definition in the codebase
rg -n 'def is_libvirt_feature_supported' --type=pyRepository: autotest/tp-libvirt
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Search for how is_libvirt_feature_supported is used - looking for its behavior
rg -n -B 2 -A 2 'is_libvirt_feature_supported\(' --type=py | head -100Repository: autotest/tp-libvirt
Length of output: 8209
🏁 Script executed:
#!/bin/bash
# Look at the context around line 104 in the file
find . -name "secure_host_validate.py" -type f -exec sed -n '95,115p' {} +Repository: autotest/tp-libvirt
Length of output: 712
🏁 Script executed:
#!/bin/bash
# Find where libvirt_version module is defined
find . -name "libvirt_version.py" -type fRepository: autotest/tp-libvirt
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Search for the function definition and examine its implementation
rg -n -A 10 'def is_libvirt_feature_supported' --type=pyRepository: autotest/tp-libvirt
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Examine the libvirt_version.py file to understand its structure and imports
head -100 ./provider/libvirt_version.pyRepository: autotest/tp-libvirt
Length of output: 1815
🏁 Script executed:
#!/bin/bash
# Search for is_libvirt_feature_supported in the libvirt_version.py file
rg -n 'is_libvirt_feature_supported' ./provider/libvirt_version.pyRepository: autotest/tp-libvirt
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Check the imports in secure_host_validate.py to see where is_libvirt_feature_supported comes from
head -30 ./libvirt/tests/src/cpu/secure_host_validate.pyRepository: autotest/tp-libvirt
Length of output: 875
🏁 Script executed:
#!/bin/bash
# Search for imports of is_libvirt_feature_supported or where it's coming from
rg -n 'import.*is_libvirt_feature_supported|from.*import.*is_libvirt_feature_supported' --type=pyRepository: autotest/tp-libvirt
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Check if libvirt_version is imported and how
rg -n 'import.*libvirt_version|from.*libvirt_version' ./libvirt/tests/src/cpu/secure_host_validate.pyRepository: autotest/tp-libvirt
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Check if TestSkip or similar exceptions are used in the test file
rg -n 'TestSkip|TestCancel|test.skip|skip_test' ./libvirt/tests/src/cpu/secure_host_validate.pyRepository: autotest/tp-libvirt
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Check how exceptions are typically handled in test files by looking at error handling patterns
rg -n 'except.*:' ./libvirt/tests/src/cpu/secure_host_validate.py | head -20Repository: autotest/tp-libvirt
Length of output: 45
🌐 Web query:
virttest is_libvirt_feature_supported avocado-vt behavior
💡 Result:
In Avocado-VT (virttest), helpers named like is_libvirt_feature_supported(...) are generally meant to be feature gates: tests call them to decide whether to run a libvirt-dependent code path or SKIP/CANCEL early when the host/libvirt stack can’t support it (rather than failing mid-test). Avocado-VT’s libvirt backend is built around libvirt APIs plus virsh-driven capability probing. [1]
What “supported” usually means in this context is:
- Libvirt advertises the capability via its XML capability queries (for overall driver capabilities,
virConnectGetCapabilities()/virsh capabilities). [2] - Or (for domain/guest features) via Domain Capabilities (
virsh domcapabilities), whose<features>entries are explicitly markedsupported='yes'/'no'(and may also enumerate supported values). [3] - And/or a minimum libvirt version / hypervisor version and/or API presence check is satisfied (a common pattern in libvirt tooling is: try a direct API/function/flag probe first, then fall back to daemon+HV version checks; if the minimum isn’t met, return
False). [4]
So, behavior you should expect at call sites is:
- returns
Trueonly when the feature is actually available on the current connection/driver according to capability reporting and/or version/API checks; - returns
Falsewhen the feature is missing, not advertised, or can’t be validated on that platform/connection—so tests can skip instead of failing.
Sources: Avocado-VT architecture overview (libvirt backend usage) [1]; libvirt capabilities XML (virConnectGetCapabilities) [2]; libvirt domain capabilities <features supported=...> [3]; example of the common “function probe then version gating” support-check pattern in libvirt-related tooling (virtinst) [4].
[1] (deepwiki.com)
[2] (libvirt.org)
[3] (libvirt.org)
[4] (sources.debian.org)
Citations:
- 1: https://deepwiki.com/avocado-framework/avocado-vt?utm_source=openai
- 2: https://libvirt.org/formatcaps.html?utm_source=openai
- 3: https://libvirt.org/formatdomaincaps.html?utm_source=openai
- 4: https://sources.debian.org/src/virt-manager/1%3A3.2.0-3/virtinst/support.py?utm_source=openai
Check the return value of is_libvirt_feature_supported(params) to skip the test when the feature is unsupported.
The is_libvirt_feature_supported() function returns a boolean indicating whether the libvirt feature is available on the host. The return value must be checked—if it returns False, the test should skip rather than proceed on unsupported libvirt versions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libvirt/tests/src/cpu/secure_host_validate.py` at line 104, The call to
libvirt_version.is_libvirt_feature_supported(params) in secure_host_validate.py
currently ignores its boolean result; update the test to capture the return
value and skip the test when it returns False (e.g., use the test framework's
skip/skipTest/pytest.skip mechanism) so the test does not run on unsupported
libvirt versions; ensure the check occurs before any feature-dependent logic and
include a clear skip message mentioning the unsupported feature and params.
7a0754b to
326c879
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
libvirt/tests/src/cpu/secure_host_validate.py (2)
48-48:⚠️ Potential issue | 🟠 MajorReplace
eval()with safe literal parsing.
eval()on parameter values (Line 48, Line 56, Line 84, Line 91, Line 99, Line 106) allows arbitrary code execution from config/test params. Useast.literal_eval()with centralized error handling.Suggested fix
+import ast from avocado.utils import cpu as cpu_utils from avocado.utils import process @@ def run(test, params, env): @@ + def _parse_literal_param(name, default): + raw = params.get(name, default) + try: + return ast.literal_eval(raw) + except (ValueError, SyntaxError) as err: + test.error("Invalid literal for '%s': %s (raw=%r)" % (name, err, raw)) + def validate_prerequisites(): @@ - cpu_flags = eval(params.get("cpu_flags", "[]")) + cpu_flags = _parse_literal_param("cpu_flags", "[]") @@ - req_kernel_opts = eval(params.get("req_kernel_opts", "[]")) + req_kernel_opts = _parse_literal_param("req_kernel_opts", "[]") @@ - kernel_msg_patterns = eval(params.get("kernel_msg_patterns", "[]")) + kernel_msg_patterns = _parse_literal_param("kernel_msg_patterns", "[]") @@ - pkg_patterns = eval(params.get("%s_patterns" % cmd, "[]")) + pkg_patterns = _parse_literal_param("%s_patterns" % cmd, "[]") @@ - domcap_xpath = eval(params.get("domcap_xpath", "[]")) + domcap_xpath = _parse_literal_param("domcap_xpath", "[]") @@ - req_cmds = eval(params.get("req_cmds", "{}")) + req_cmds = _parse_literal_param("req_cmds", "{}")Also applies to: 56-56, 84-84, 91-91, 99-99, 106-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/cpu/secure_host_validate.py` at line 48, Replace unsafe eval() calls that parse test params (e.g., cpu_flags = eval(params.get("cpu_flags", "[]")) and the similar usages at the other locations) with ast.literal_eval and centralized error handling: add "import ast" and implement a small helper (e.g., parse_literal_param(param_name, default)) that calls ast.literal_eval on params.get(param_name, default) and catches ValueError/SyntaxError to log a clear error and re-raise or return the default; then replace each eval(...) occurrence with parse_literal_param("cpu_flags", "[]") (and the corresponding param names at lines 56, 84, 91, 99, 106) so no arbitrary code execution can occur.
85-85:⚠️ Potential issue | 🟠 MajorAvoid
shell=Truefor parameter-driven command execution.Line 85 and Line 94 execute parameter-built command strings with
shell=True, which unnecessarily increases injection risk. Build argv lists and run withshell=False.Suggested fix
+import shlex @@ - result = process.run(kernel_msg_cmd, shell=True) + result = process.run(shlex.split(kernel_msg_cmd), shell=False) @@ - pkg_cmd = "%s %s" % (cmd, cmd_info["cmd_params"]) - result = process.run(pkg_cmd, shell=True) + pkg_cmd = [cmd] + shlex.split(cmd_info.get("cmd_params", "")) + result = process.run(pkg_cmd, shell=False)Also applies to: 94-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/cpu/secure_host_validate.py` at line 85, The calls using process.run with shell=True (notably the call using kernel_msg_cmd and the other parameter-built command at line 94) should be converted to safe argv-style invocations: stop passing a single command string, build a list of arguments (either by constructing the list directly or using shlex.split on the known-safe command string), call process.run(<argv_list>, shell=False) and preserve existing flags like capture_output, check, or timeout as appropriate; update both the kernel_msg_cmd invocation and the other process.run(...) occurrence to use argv lists instead of shell=True to eliminate shell injection risk.
🧹 Nitpick comments (1)
libvirt/tests/cfg/cpu/secure_host_validate.cfg (1)
11-17: Reduce AMD variant duplication with cfg variable substitution.
req_kernel_opts,req_cmds, and common patterns are repeated across AMD variants. Consider defining shared vars once and reusing them to reduce drift and simplify future updates.Based on learnings: In tp-libvirt test cfg files, the cartesian config parser supports
${variable_name}substitution for values before Python receives params.Also applies to: 22-28, 33-39
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/cfg/cpu/secure_host_validate.cfg` around lines 11 - 17, Define shared AMD-specific configuration variables (e.g., a single shared req_kernel_opts, shared req_cmds, shared sevctl_patterns and virt-host-validate_patterns) at the top of the cfg and replace duplicated literal lists in each AMD variant with ${variable_name} substitutions; update the AMD variant blocks to reference ${req_kernel_opts}, ${req_cmds}, ${sevctl_patterns}, and ${virt-host-validate_patterns} so the cartesian parser performs variable substitution and removes duplicated values across variants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@libvirt/tests/src/cpu/secure_host_validate.py`:
- Line 48: Replace unsafe eval() calls that parse test params (e.g., cpu_flags =
eval(params.get("cpu_flags", "[]")) and the similar usages at the other
locations) with ast.literal_eval and centralized error handling: add "import
ast" and implement a small helper (e.g., parse_literal_param(param_name,
default)) that calls ast.literal_eval on params.get(param_name, default) and
catches ValueError/SyntaxError to log a clear error and re-raise or return the
default; then replace each eval(...) occurrence with
parse_literal_param("cpu_flags", "[]") (and the corresponding param names at
lines 56, 84, 91, 99, 106) so no arbitrary code execution can occur.
- Line 85: The calls using process.run with shell=True (notably the call using
kernel_msg_cmd and the other parameter-built command at line 94) should be
converted to safe argv-style invocations: stop passing a single command string,
build a list of arguments (either by constructing the list directly or using
shlex.split on the known-safe command string), call process.run(<argv_list>,
shell=False) and preserve existing flags like capture_output, check, or timeout
as appropriate; update both the kernel_msg_cmd invocation and the other
process.run(...) occurrence to use argv lists instead of shell=True to eliminate
shell injection risk.
---
Nitpick comments:
In `@libvirt/tests/cfg/cpu/secure_host_validate.cfg`:
- Around line 11-17: Define shared AMD-specific configuration variables (e.g., a
single shared req_kernel_opts, shared req_cmds, shared sevctl_patterns and
virt-host-validate_patterns) at the top of the cfg and replace duplicated
literal lists in each AMD variant with ${variable_name} substitutions; update
the AMD variant blocks to reference ${req_kernel_opts}, ${req_cmds},
${sevctl_patterns}, and ${virt-host-validate_patterns} so the cartesian parser
performs variable substitution and removes duplicated values across variants.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/cpu/secure_host_validate.cfglibvirt/tests/src/cpu/secure_host_validate.py
liang-cong-red-hat
force-pushed
the
new_case_secure_host_validate
branch
from
326c879 to
b56fe89
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
libvirt/tests/cfg/cpu/secure_host_validate.cfg (1)
35-35: Inconsistent whitespace handling in regex pattern.The SNP pattern uses a literal space before
\(SEV-SNP\):'\[.*PASS.*\]\s+-\s+Secure Nested Paging\s+\(SEV-SNP\)'However, there's a literal space between "Secure" and "Nested Paging" rather than
\s+. If the tool output has multiple spaces or tabs, this pattern may fail to match. Consider using\s+consistently for all whitespace.Proposed fix for consistent regex
- sevctl_patterns = ['\[.*PASS.*\]\s+-\s+Secure\sEncrypted\sVirtualization\s+\(SEV\)', '\[.*PASS.*\]\s+-\s+Encrypted\sState\s+\(SEV-ES\)', '\[.*PASS.*\]\s+-\s+Secure Nested Paging\s+\(SEV-SNP\)'] + sevctl_patterns = ['\[.*PASS.*\]\s+-\s+Secure\sEncrypted\sVirtualization\s+\(SEV\)', '\[.*PASS.*\]\s+-\s+Encrypted\sState\s+\(SEV-ES\)', '\[.*PASS.*\]\s+-\s+Secure\s+Nested\s+Paging\s+\(SEV-SNP\)']🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/cfg/cpu/secure_host_validate.cfg` at line 35, The regex in sevctl_patterns contains a literal space between "Secure" and "Nested Paging" which makes whitespace handling inconsistent versus the other patterns; update the third pattern in sevctl_patterns to use \s+ between "Secure" and "Nested" (i.e., change the literal space to \s+) so all whitespace in the three patterns is matched consistently and will handle multiple spaces or tabs.libvirt/tests/src/cpu/secure_host_validate.py (1)
82-86: Consider skipping kernel message check when no patterns are defined.The kernel message command runs unconditionally, but for variants like AMD SEV where
kernel_msg_patternsis empty, this executesjournalctlunnecessarily. Consider guarding the execution:Proposed optimization
test.log.info("TEST_STEP1: Check expected kernel msg") kernel_msg_cmd = params.get("kernel_msg_cmd") kernel_msg_patterns = eval(params.get("kernel_msg_patterns", "[]")) - result = process.run(kernel_msg_cmd, shell=True) - libvirt.check_result(result, expected_match=kernel_msg_patterns) + if kernel_msg_patterns: + result = process.run(kernel_msg_cmd, shell=True) + libvirt.check_result(result, expected_match=kernel_msg_patterns)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/cpu/secure_host_validate.py` around lines 82 - 86, The kernel message command is executed unconditionally even when kernel_msg_patterns is empty; modify the logic around kernel_msg_cmd/kernel_msg_patterns so you only call process.run(kernel_msg_cmd, shell=True) and libvirt.check_result(...) when kernel_msg_patterns is non-empty (e.g., truthy list), otherwise log or skip the check; update the block that currently references kernel_msg_cmd, kernel_msg_patterns, process.run and libvirt.check_result to guard execution based on kernel_msg_patterns being defined and non-empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libvirt/tests/src/cpu/secure_host_validate.py`:
- Line 67: Change the typo "Setup the enviroment:" to "Setup the environment:"
(or "Set up the environment:" if meant as a verb) wherever the string "Setup the
enviroment:" appears (e.g., in the test docstring/comment) so the word
"environment" is spelled correctly.
---
Nitpick comments:
In `@libvirt/tests/cfg/cpu/secure_host_validate.cfg`:
- Line 35: The regex in sevctl_patterns contains a literal space between
"Secure" and "Nested Paging" which makes whitespace handling inconsistent versus
the other patterns; update the third pattern in sevctl_patterns to use \s+
between "Secure" and "Nested" (i.e., change the literal space to \s+) so all
whitespace in the three patterns is matched consistently and will handle
multiple spaces or tabs.
In `@libvirt/tests/src/cpu/secure_host_validate.py`:
- Around line 82-86: The kernel message command is executed unconditionally even
when kernel_msg_patterns is empty; modify the logic around
kernel_msg_cmd/kernel_msg_patterns so you only call process.run(kernel_msg_cmd,
shell=True) and libvirt.check_result(...) when kernel_msg_patterns is non-empty
(e.g., truthy list), otherwise log or skip the check; update the block that
currently references kernel_msg_cmd, kernel_msg_patterns, process.run and
libvirt.check_result to guard execution based on kernel_msg_patterns being
defined and non-empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd37da4c-2ca5-49af-a135-95dcf51100c1
📒 Files selected for processing (2)
libvirt/tests/cfg/cpu/secure_host_validate.cfglibvirt/tests/src/cpu/secure_host_validate.py
liang-cong-red-hat
force-pushed
the
new_case_secure_host_validate
branch
from
b56fe89 to
987dd26
Compare
Introduce a new test case to verify host environment supports required secure launch technologies. The validation checks for the availability of the following features: - AMD: SEV, SEV-ES, and SEV-SNP - Intel: SGX and TDX Signed-off-by: Liang Cong <lcong@redhat.com>
liang-cong-red-hat
force-pushed
the
new_case_secure_host_validate
branch
from
de26421 to
db1719d
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (4)
libvirt/tests/src/cpu/secure_host_validate.py (4)
104-105:⚠️ Potential issue | 🟠 MajorHandle unsupported libvirt feature-gate result explicitly.
The return value of
libvirt_version.is_libvirt_feature_supported(params)is currently ignored. If it returnsFalse, cancel early to avoid downstream false-failures on unsupported hosts.Suggested fix
- libvirt_version.is_libvirt_feature_supported(params) secure_feature = params.get("secure_feature") + feature_supported = libvirt_version.is_libvirt_feature_supported(params) + if feature_supported is False: + test.cancel("libvirt does not support secure feature '%s' on this host." % secure_feature)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/cpu/secure_host_validate.py` around lines 104 - 105, The call to libvirt_version.is_libvirt_feature_supported(params) is ignored, so if it returns False the test can produce false failures; update the code around libvirt_version.is_libvirt_feature_supported(params) and secure_feature = params.get("secure_feature") to capture the boolean result, and if it is False cancel/return early (mark the test as skipped or no-op) with a clear message about the unsupported feature so downstream logic does not run on unsupported hosts.
48-48:⚠️ Potential issue | 🟠 MajorReplace
eval()parameter parsing with safe literal parsing.These values are config-driven; executing them via
eval()is unsafe and unnecessary. Parse withast.literal_eval()through one helper and fail early on malformed literals.Suggested fix
+import ast from avocado.utils import cpu as cpu_utils from avocado.utils import process @@ def run(test, params, env): @@ + def _parse_literal_param(name, default): + raw = params.get(name, default) + try: + return ast.literal_eval(raw) + except (ValueError, SyntaxError) as err: + test.error("Invalid literal for '%s': %s (raw=%r)" % (name, err, raw)) + def validate_prerequisites(): @@ - cpu_flags = eval(params.get("cpu_flags", "[]")) + cpu_flags = _parse_literal_param("cpu_flags", "[]") @@ - req_kernel_opts = eval(params.get("req_kernel_opts", "[]")) + req_kernel_opts = _parse_literal_param("req_kernel_opts", "[]") @@ - kernel_msg_patterns = eval(params.get("kernel_msg_patterns", "[]")) + kernel_msg_patterns = _parse_literal_param("kernel_msg_patterns", "[]") @@ - pkg_patterns = eval(params.get("%s_patterns" % cmd, "[]")) + pkg_patterns = _parse_literal_param("%s_patterns" % cmd, "[]") @@ - domcap_xpath = eval(params.get("domcap_xpath", "[]")) + domcap_xpath = _parse_literal_param("domcap_xpath", "[]") @@ - req_cmds = eval(params.get("req_cmds", "{}")) + req_cmds = _parse_literal_param("req_cmds", "{}")Also applies to: 56-56, 84-84, 91-91, 99-99, 106-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/cpu/secure_host_validate.py` at line 48, Replace unsafe eval calls that parse config strings (e.g., cpu_flags = eval(params.get("cpu_flags", "[]"))) with a small helper like safe_literal_eval(value, key) that uses ast.literal_eval to parse params.get(...) and raises a clear exception on malformed input; update all occurrences (the cpu_flags assignment and the similar eval usages at the other noted sites) to call this helper so parsing is safe and fails early with a descriptive error referencing the param key.
85-85:⚠️ Potential issue | 🟠 MajorAvoid
shell=Truefor config/parameter-driven commands.Both invocations execute values assembled from params/cfg through a shell. Prefer argv +
shell=Falseto reduce injection surface.Suggested fix
+import shlex from avocado.utils import cpu as cpu_utils from avocado.utils import process @@ - result = process.run(kernel_msg_cmd, shell=True) + result = process.run(shlex.split(kernel_msg_cmd), shell=False) @@ - pkg_cmd = "%s %s" % (cmd, cmd_info["cmd_params"]) - result = process.run(pkg_cmd, shell=True) + pkg_cmd = [cmd] + shlex.split(cmd_info.get("cmd_params", "")) + result = process.run(pkg_cmd, shell=False)Also applies to: 94-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/cpu/secure_host_validate.py` at line 85, The calls using process.run(kernel_msg_cmd, shell=True) open an injection surface; change them to invoke process.run with shell=False and pass the command as an argv list (e.g., split the constructed kernel_msg_cmd into a list via shlex.split or build the list when composing the command) and preserve stdout/stderr handling; update both occurrences of process.run(kernel_msg_cmd, shell=True) in this file so kernel_msg_cmd is a sequence (or replaced by an explicit list) and shell is removed/set False.
67-69:⚠️ Potential issue | 🟡 MinorFix setup docstring typos.
Please change “enviroment” → “environment” and “require packages” → “required packages”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/cpu/secure_host_validate.py` around lines 67 - 69, Fix the typos in the setup docstring in secure_host_validate.py: replace “enviroment” with “environment” and “require packages” with “required packages” in the docstring block that begins with "Setup the enviroment:" so the text reads "Setup the environment:" and "1. install required packages".
🧹 Nitpick comments (1)
libvirt/tests/cfg/cpu/secure_host_validate.cfg (1)
7-36: Consider deduplicating repeated AMD block values via cfg variable substitution.
cpu_vendor,req_kernel_opts, andreq_cmdsare repeated across AMD variants. Hoisting shared values into variables and referencing${...}will reduce drift and simplify future updates.Example direction
+ amd_vendor = "amd" + amd_req_kernel_opts = ["mem_encrypt=on", "amd_iommu=on"] + amd_req_cmds = {"sevctl": {"package": "sevctl", "cmd_params": "ok"}, "virt-host-validate": {"package": "libvirt", "cmd_params": ""}} @@ - amd_sev: - cpu_vendor = "amd" + cpu_vendor = ${amd_vendor} @@ - req_kernel_opts = ["mem_encrypt=on", "amd_iommu=on"] + req_kernel_opts = ${amd_req_kernel_opts} @@ - req_cmds = {"sevctl": {"package": "sevctl", "cmd_params": "ok"}, "virt-host-validate": {"package": "libvirt", "cmd_params": ""}} + req_cmds = ${amd_req_cmds}Based on learnings, tp-libvirt cfg files support
${variable_name}substitution before parameters reach Python.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/cfg/cpu/secure_host_validate.cfg` around lines 7 - 36, The three AMD entries (amd_sev, amd_sev_es, amd_sev_snp) repeat identical cpu_vendor, req_kernel_opts, and req_cmds values; hoist those shared values into cfg variables and reference them via substitution (e.g., define amd_cpu_vendor, amd_req_kernel_opts, amd_req_cmds at top) then replace the repeated fields in amd_sev, amd_sev_es, and amd_sev_snp with ${amd_cpu_vendor}, ${amd_req_kernel_opts}, and ${amd_req_cmds} respectively so updates apply to all variants; ensure you keep unique fields (secure_feature, cpu_flags, kvm_probe_module_parameters, sevctl_patterns, virt-host-validate_patterns) inline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@libvirt/tests/src/cpu/secure_host_validate.py`:
- Around line 104-105: The call to
libvirt_version.is_libvirt_feature_supported(params) is ignored, so if it
returns False the test can produce false failures; update the code around
libvirt_version.is_libvirt_feature_supported(params) and secure_feature =
params.get("secure_feature") to capture the boolean result, and if it is False
cancel/return early (mark the test as skipped or no-op) with a clear message
about the unsupported feature so downstream logic does not run on unsupported
hosts.
- Line 48: Replace unsafe eval calls that parse config strings (e.g., cpu_flags
= eval(params.get("cpu_flags", "[]"))) with a small helper like
safe_literal_eval(value, key) that uses ast.literal_eval to parse
params.get(...) and raises a clear exception on malformed input; update all
occurrences (the cpu_flags assignment and the similar eval usages at the other
noted sites) to call this helper so parsing is safe and fails early with a
descriptive error referencing the param key.
- Line 85: The calls using process.run(kernel_msg_cmd, shell=True) open an
injection surface; change them to invoke process.run with shell=False and pass
the command as an argv list (e.g., split the constructed kernel_msg_cmd into a
list via shlex.split or build the list when composing the command) and preserve
stdout/stderr handling; update both occurrences of process.run(kernel_msg_cmd,
shell=True) in this file so kernel_msg_cmd is a sequence (or replaced by an
explicit list) and shell is removed/set False.
- Around line 67-69: Fix the typos in the setup docstring in
secure_host_validate.py: replace “enviroment” with “environment” and “require
packages” with “required packages” in the docstring block that begins with
"Setup the enviroment:" so the text reads "Setup the environment:" and "1.
install required packages".
---
Nitpick comments:
In `@libvirt/tests/cfg/cpu/secure_host_validate.cfg`:
- Around line 7-36: The three AMD entries (amd_sev, amd_sev_es, amd_sev_snp)
repeat identical cpu_vendor, req_kernel_opts, and req_cmds values; hoist those
shared values into cfg variables and reference them via substitution (e.g.,
define amd_cpu_vendor, amd_req_kernel_opts, amd_req_cmds at top) then replace
the repeated fields in amd_sev, amd_sev_es, and amd_sev_snp with
${amd_cpu_vendor}, ${amd_req_kernel_opts}, and ${amd_req_cmds} respectively so
updates apply to all variants; ensure you keep unique fields (secure_feature,
cpu_flags, kvm_probe_module_parameters, sevctl_patterns,
virt-host-validate_patterns) inline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9d6b48e-821b-49c6-8cc6-14119d16d961
📒 Files selected for processing (2)
libvirt/tests/cfg/cpu/secure_host_validate.cfglibvirt/tests/src/cpu/secure_host_validate.py
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.
Introduce a new test case to verify host environment supports required secure launch technologies.
The validation checks for the availability of the following features:
depends on: avocado-framework/avocado-vt#4328
Test results:
(1/2) type_specific.io-github-autotest-libvirt.cpu.secure_launch.secure_host_validate.intel_sgx: STARTED
(1/2) type_specific.io-github-autotest-libvirt.cpu.secure_launch.secure_host_validate.intel_sgx: PASS (22.77 s)
(2/2) type_specific.io-github-autotest-libvirt.cpu.secure_launch.secure_host_validate.intel_tdx: STARTED
(2/2) type_specific.io-github-autotest-libvirt.cpu.secure_launch.secure_host_validate.intel_tdx: PASS (23.48 s)
(1/3) type_specific.io-github-autotest-libvirt.cpu.secure_launch.secure_host_validate.amd_sev: STARTED
(1/3) type_specific.io-github-autotest-libvirt.cpu.secure_launch.secure_host_validate.amd_sev: PASS (67.54 s)
(2/3) type_specific.io-github-autotest-libvirt.cpu.secure_launch.secure_host_validate.amd_sev_es: STARTED
(2/3) type_specific.io-github-autotest-libvirt.cpu.secure_launch.secure_host_validate.amd_sev_es: PASS (67.69 s)
(3/3) type_specific.io-github-autotest-libvirt.cpu.secure_launch.secure_host_validate.amd_sev_snp: STARTED
(3/3) type_specific.io-github-autotest-libvirt.cpu.secure_launch.secure_host_validate.amd_sev_snp: PASS (67.84 s)
Summary by CodeRabbit