Add a derived keys test - #242
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an SNP guest “derived keys” test module to the guest image and wires its results into the host-side reporting so derived-key behavior is exercised and summarized alongside existing attestation tests.
Changes:
- Introduce a new guest test module (
key-derivation) implemented as a Python script invoked by a systemd oneshot service. - Integrate the new service into the guest test stage ordering and into host-side log display / certificate summary generation.
- Ensure the guest image includes Python 3 to run the new test.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/test/guest/test-done/mkosi.extra/usr/local/lib/systemd/system/test-done.service | Adds key-derivation.service as a dependency of the test barrier. |
| modules/test/guest/mkosi.conf | Includes the new key-derivation guest test module in the guest test build. |
| modules/test/guest/key-derivation/README.md | Documents the new derived-keys test module and how to run/interpret it. |
| modules/test/guest/key-derivation/mkosi.extra/usr/local/lib/systemd/system/key-derivation.service | Adds a systemd oneshot service to run the derived-keys test script. |
| modules/test/guest/key-derivation/mkosi.extra/usr/local/lib/scripts/snpguest_key_derivation.py | Implements key derivation tests (determinism, VMPL isolation, root key selection, parameter sensitivity). |
| modules/report/host/sev-certificate-generator/.../sev_certificate_version_3_0_0_0.py | Extends guest summary generation to include step-by-step key-derivation results and inferred status fallback. |
| modules/report/host/display-guest-logs/.../display-guest-logs.sh | Includes key-derivation.service in the host’s guest-log display output. |
| modules/build/guest/mkosi.conf | Adds python3 to the guest image package set to support the new test script. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not derive_key(key_vmpl1_file, root_key="vcek", vmpl=1): | ||
| print(" Note: VMPL1 derivation failed (expected if not running at VMPL0)") | ||
| print("✓ PASS: N/A") | ||
| return True | ||
|
|
| def test_root_key_difference() -> bool: | ||
| """Test that different root keys (VCEK vs VMRK) produce different keys.""" | ||
| key_vck_file = KEY_DERIVATION_DIR / "vck_key.bin" |
There was a problem hiding this comment.
@markg-github I know that you meant vcek ignore this comment.
There was a problem hiding this comment.
I believe I saw "VCK" somewhere and it may me wonder about something that I was already curious about: in the context of derived keys, VCEK, as an asymmetric key (pair), shouldn't be relevant. What's relevant is the derivation chain from fuses (PUFs?) to the derived keys and I thought VCK might be the name of something in this chain.
| for step, status_code in key_derivation_data.items(): | ||
| key_derivation_data[step] = "passed" if int(status_code) == 0 else "failed" | ||
|
|
|
@markg-github I had added some certificate versioning logic in #241 (it still needs to get reviewed), but I think it might be good to discuss what we want to keep and how we'll merge both. For that one I was intending the next few tests to be under v3.0.0-1. Do you have some passing certificate examples you can link here? It should have a milestone assigned if it reaches some certificate level. I looked at your recent ones in your repository and they appear to have issues. One of the issues looked familiar, I had a fix for snpguest ok running too early sometimes. But not sure if the others were expected. Note that you'll need to create the milestone in your fork for it to be assigned if you haven't yet (c3.0.0-0, and later when we enable it, c3.0.0-1) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (2)
modules/report/host/sev-certificate-generator/mkosi.extra/usr/local/lib/scripts/generate_sev_certificate/sev_certificate/sev_certificate_version_3_0_0_0.py:171
key-derivation.serviceis appended tosnpguest_services_listeven when it has no journal entries. The subsequentextract_service_status()call usessubprocess.run(..., check=True)(inService.get_service_message) and will raiseCalledProcessErrorif the unit is absent, which can crash certificate generation.
# key-derivation.service may finish after other services and miss the journal
# upload window, so ensure it is always included even if discovery missed it.
if "key-derivation.service" not in snpguest_services_list:
snpguest_services_list.append("key-derivation.service")
# Map SNP Guest test service name with its status
snpguest_services_status ={}
for service in snpguest_services_list:
service_status = self.sev_service.extract_service_status(service, "guest")
snpguest_services_status[service] = service_status
modules/build/guest/mkosi.conf:14
- The new key-derivation test module executes
/usr/bin/python3, but most guest image templates define their ownPackages=list and currently do not includepython3. If mkosi does not mergePackagesacross includes, the key-derivation service will fail at runtime. Please ensure python3 is included in the final guest images (e.g., add it to each guest imagePackages=list, or confirm/document thatmodules/build/guest/mkosi.confreliably augments packages).
[Content]
KernelCommandLine=console=ttyS0
Packages=
python3
| if not derive_key(key_vmpl1_file, root_key="vcek", vmpl=1): | ||
| print(" Note: VMPL1 derivation failed (expected if not running at VMPL0)") | ||
| print("✓ PASS: N/A") | ||
| return True | ||
|
|
| # the firmware; treat all loop failures as expected so they don't flood | ||
| # the log with ERROR output. | ||
| if not derive_key(key_file, root_key="vcek", vmpl=0, guest_svn=svn, | ||
| guest_field_select=1 << 4, expected_failure=True): | ||
| failed_svns.append(svn) |
| key_file = KEY_DERIVATION_DIR / f"tcb_{tcb_u64:016x}_key.bin" | ||
| if not derive_key(key_file, root_key="vcek", vmpl=0, tcb_version=tcb_u64, | ||
| guest_field_select=1 << 5, expected_failure=True): | ||
| failed_tcbs.append(tcb_u64) |
| After=system.target | ||
|
|
||
| # Putting the key derivation test, which should never fail, first | ||
| # avoids the effect of other guest test services that fail. Doing |
| This script tests the snpguest key derivation functionality, verifying: | ||
| 1. Deterministic key generation (same params -> same key) | ||
| 2. VMPL-based key isolation (different VMPL -> different keys) | ||
| 3. Root key differences (VCK vs VMRK -> different keys) | ||
| 4. Parameter sensitivity (different params -> different keys) |
| python3-pip | ||
| python3-emoji | ||
| jq | ||
| apt |
| openssl | ||
| ca-certificates | ||
| jq | ||
| xxd | ||
|
|
DGonzalezVillal
left a comment
DGonzalezVillal
left a comment
There was a problem hiding this comment.
You went above and beyond! I think this really covers all of the derived key functionality testing, covered pretty much most test cases.
I added some comment on you tcb derivation logic, tcb is derived from chip and not from report version.
Also just an idea on how to maybe parse the contents of the report. I know historically we've done it by printing the report and the parsing the contents, but I wonder if for us trying to handle bytes could be cleaner.
I also think that the way you have it set up could lend itself well to migrate to the new test harness.
For some of the tests you're generating a large amount of keys, that's the one thing that worries me since the new harness doesn't really have a way of running functions on guest, so we might need to brainstorm how to migrate the size of your test into the harness.
| """ | ||
| AMD SEV-SNP TCB_VERSION packed as a u64: | ||
| bits 7:0 - Boot Loader SVN | ||
| bits 15:8 - TEE SVN | ||
| bits 47:16 - Reserved (zero) | ||
| bits 55:48 - SNP firmware SVN | ||
| bits 63:56 - Microcode SVN | ||
| """ | ||
| boot_loader: int = 0 | ||
| tee: int = 0 | ||
| snp: int = 0 | ||
| microcode: int = 0 |
There was a problem hiding this comment.
This will be different according to generation:
This matches the Milan and Genoa TCB spec. We need to extend to the Turin definition. Venice will be a completely different look as well.
| f"snp=0x{self.snp:02x} mc=0x{self.microcode:02x}") | ||
|
|
||
|
|
||
| EXPECTED_REPORT_VERSION = 2 # ATTESTATION_REPORT schema version this test was written for |
There was a problem hiding this comment.
Is there a particular reason you need to be aware of the report version?
There was a problem hiding this comment.
You mean since the code doesn't parse the report directly, per your earlier comment?
In general, I would say any type of parsing should condition on a (schema/struct) version whenever possible.
If you're asking why I need to read the report in the 1st place, it's to learn what the derived keys valid (and invalid) inputs are.
| info = ReportInfo() | ||
|
|
||
| m = re.search(r'^\s*Version\s*[:\s]+(0x[0-9a-fA-F]+|[0-9]+)', | ||
| display_output, re.IGNORECASE | re.MULTILINE) | ||
| if m: | ||
| info.version = int(m.group(1), 0) | ||
|
|
||
| m = re.search(r'Guest\s+SVN\s*[:\s]+(0x[0-9a-fA-F]+|[0-9]+)', | ||
| display_output, re.IGNORECASE) | ||
| if m: | ||
| info.guest_svn = int(m.group(1), 0) | ||
|
|
||
| m = re.search(r'Family\s+ID\s*[:\s]+([0-9a-fA-F]+)', display_output, re.IGNORECASE) | ||
| if m: | ||
| info.family_id = m.group(1).lower() | ||
|
|
||
| m = re.search(r'Image\s+ID\s*[:\s]+([0-9a-fA-F]+)', display_output, re.IGNORECASE) | ||
| if m: | ||
| info.image_id = m.group(1).lower() | ||
|
|
||
| boundary = r'(?:Current|Committed|Reported|Launch)\s+TCB' | ||
| for section_name, attr in [ | ||
| ('Current TCB', 'current_tcb'), | ||
| ('Committed TCB', 'committed_tcb'), | ||
| ('Reported TCB', 'reported_tcb'), | ||
| ('Launch TCB', 'launch_tcb'), | ||
| ]: | ||
| pattern = rf'{re.escape(section_name)}\s*:?(.*?)(?={boundary}|\Z)' | ||
| m = re.search(pattern, display_output, re.DOTALL | re.IGNORECASE) | ||
| if m: | ||
| setattr(info, attr, parse_tcb_section(m.group(1))) | ||
|
|
||
| return info | ||
| except Exception as e: | ||
| print(f"WARNING: Failed to parse report info: {e}", file=sys.stderr) | ||
| return None |
There was a problem hiding this comment.
I'm wondering, I have not implemented a parser in python, but maybe if it would be easier to find the info you're looking for by looking at the bit ranges rather than parsing the display format of snpguest?
like read bytes in and parse the specific locations.
Quick AI gave me this:
import struct
def extract_tcb_values(report_filepath):
# Dictionary mapping the TCB names to their byte offset in the specification
# Each TCB version field is 64 bits (8 bytes) long
tcb_offsets = {
"CURRENT_TCB": 0x38,
"REPORTED_TCB": 0x180,
"COMMITTED_TCB": 0x1E0,
"LAUNCH_TCB": 0x1F0
}
tcb_results = {}
with open(report_filepath, 'rb') as f:
for name, offset in tcb_offsets.items():
# Seek directly to the byte location
f.seek(offset)
# Read exactly 8 bytes (64 bits)
byte_data = f.read(8)
if len(byte_data) < 8:
tcb_results[name] = "Error: Out of bounds / File too short"
continue
# Unpack as an unsigned 64-bit integer (<Q for Little-Endian, >Q for Big-Endian)
# x86 platform hardware structures typically use Little-Endian format
tcb_value = struct.unpack('<Q', byte_data)[0]
tcb_results[name] = tcb_value
return tcb_results
def decode_tcb_components(tcb_int):
# Convert the 64-bit integer back to 8 raw bytes
tcb_bytes = struct.pack('<Q', tcb_int)
# AMD SEV-SNP maps these to individual security version numbers (SVNs)
return {
"Microcode SVN": tcb_bytes[0],
"SNP SVN": tcb_bytes[1],
"TEE SVN": tcb_bytes[2],
"Bootloader SVN":tcb_bytes[3],
# Bytes 4-7 are typically reserved or specific platform SVNs
"Raw Bytes": list(tcb_bytes)
}Not mandatory but I feel this type of data works easier with bytes, but also python is not created to work with bytes. So both approaches are fine with me.
There was a problem hiding this comment.
I remembered this comment, but thought it was on my ID block PR. No problem, they should both use the same approach. I'll either split out the binary parser (from ID block PR) into a new PR or if ID block merges, rebase and use the binary parser. I do NOT want to "stack" any PRs. :)
| if report_info.version is not None: | ||
| version_note = ( | ||
| "" if report_info.version == EXPECTED_REPORT_VERSION | ||
| else f" *** UNEXPECTED (expected {EXPECTED_REPORT_VERSION}) —" | ||
| f" TCB layout assumptions may not apply ***" |
There was a problem hiding this comment.
Oh I see your logic is that the TCB changes according to your report version.
The TCB really changes according to your CPU as I noted before. The only way of being able to tell what CPU you're running on (from a guest perspective) is through the attestation report family/model/stepping fields, and those are only present on reports v3+. I think we can fix up this TCB logic up a bit on the new harness, it should be easier to extract some of this information.
|
|
||
|
|
||
| def test_root_key_difference() -> bool: | ||
| """Test that different root keys (VCEK vs VMRK) produce different keys.""" |
There was a problem hiding this comment.
This is just out of my own curiosity, but did you find out if there is a way of generating, or finding the vmrk? I know we can request vcek from kds, but I have no idea where the vmrk lives, or if there is ever a situation where you want to generate it.
There was a problem hiding this comment.
Not sure. It's migration-related, right? If I had to guess, I'd say a more or less dedicated FW API for it...?
| def test_root_key_difference() -> bool: | ||
| """Test that different root keys (VCEK vs VMRK) produce different keys.""" | ||
| key_vck_file = KEY_DERIVATION_DIR / "vck_key.bin" |
There was a problem hiding this comment.
@markg-github I know that you meant vcek ignore this comment.
| if len(svn_values) < 2: | ||
| print(" Only one valid SVN value (0); sensitivity cannot be tested.") | ||
| print(" (Expected when guest was launched without an ID block.)") | ||
| print("✓ PASS: N/A (single valid value)") | ||
| return True |
There was a problem hiding this comment.
If guest svn is 0, could you at least test that 1 fails? I don't know if that's how it works. But maybe if svn is 0, then try to derive with 1 and expect a failure. And then only test ranges if svn is > 0.
There was a problem hiding this comment.
I tried to generalize your idea here, @DGonzalezVillal , in updated version(s) of the PR.
420b5f2 to
179ae24
Compare
| # ── Level 3.0.0-2 ─────────────────────────────────────────── | ||
|
|
||
| [[tests]] | ||
| name = "key-derivation" | ||
| description = "Launch SEV-SNP guest and run key derivation tests" |
| for svn in svn_values: | ||
| key_file = KEY_DERIVATION_DIR / f"svn{svn}_key.bin" | ||
| if not derive_key(key_file, root_key="vcek", vmpl=0, guest_svn=svn, | ||
| guest_field_select=1 << 4, expected_failure=True): | ||
| failed_svns.append(svn) |
| for tcb_u64 in candidates: | ||
| key_file = KEY_DERIVATION_DIR / f"tcb_{tcb_u64:016x}_key.bin" | ||
| if not derive_key(key_file, root_key="vcek", vmpl=0, tcb_version=tcb_u64, | ||
| guest_field_select=1 << 5, expected_failure=True): | ||
| failed_tcbs.append(tcb_u64) |
| key_derivation_service = "key-derivation.service" | ||
| key_derivation_cmd = f"journalctl -D {self.guest_logs_path} -u {key_derivation_service} -o cat" | ||
| result = subprocess.run(key_derivation_cmd, shell=True, text=True, capture_output=True) | ||
|
|
| Step.for_host( | ||
| name="snphost ok", | ||
| type="required", | ||
| type="info", | ||
| command="snphost ok", | ||
| ), |
There was a problem hiding this comment.
Not sure I agree with changing this to info... or at least, a fail here shouldn't grant a milestone/pass even if everything else passes.
You added this b/c your machine was failing the snphost alias check right? I briefly asked @DGonzalezVillal and @amd-kkin if we should move the alias check into a --mitigations flag or something (along with other cve checks in the future). Maybe that would help here and more reason to keep it required.
There was a problem hiding this comment.
I asked mark to move it back to required, but do you think info is fine for snphost ok check? I think we need to introduce the fix to snphost ok, but keep this particular step as required.
There was a problem hiding this comment.
Yeah I would prefer to keep it as required for now
There was a problem hiding this comment.
I'm fine with changing it back to required and I did so. (I also updated my system's TCB enough to avoid the failure.)
fwiw, it seems that the alias check issue/mitigation at play here is different, if not entirely unique, in that the mitigation isn't effective if loaded at runtime. That said, it's not intuitive to me that missing "reboot required" mitigations like this should be considered failures while missing "runtime okay" mitigations aren't.
| [Content] | ||
| KernelCommandLine=console=ttyS0 | ||
| Packages= | ||
| python3 |
There was a problem hiding this comment.
Don't think it overwrites like copilot says, but at this point not needed anymore since we added python3 for the sev_verify harness already (@DGonzalezVillal's vsock change), so guests already have python3 (unless you're trying to deduplicate, but then I think that can be its own PR?)
There was a problem hiding this comment.
I think if it's named differently it might cause issues.
There was a problem hiding this comment.
I'll remove and update the PR again after testing.
| if not derive_key(key_file, root_key="vcek", vmpl=0, guest_svn=svn, | ||
| guest_field_select=1 << 4, expected_failure=True): | ||
| failed_svns.append(svn) |
| if not derive_key(key_file, root_key="vcek", vmpl=0, tcb_version=tcb_u64, | ||
| guest_field_select=1 << 5, expected_failure=True): | ||
| failed_tcbs.append(tcb_u64) |
| # key-derivation.service may finish after other services and miss the journal | ||
| # upload window, so ensure it is always included even if discovery missed it. | ||
| if "key-derivation.service" not in snpguest_services_list: | ||
| snpguest_services_list.append("key-derivation.service") | ||
|
|
amd-aliem
left a comment
amd-aliem
left a comment
There was a problem hiding this comment.
Reviewed only for structure, it currently has a lot of extra files from the legacy systemd modules - if you could please rebase against main and clean that up would make it a lot shorter.
Will be doing another review on the test script and content tomorrow!
| [Content] | ||
| KernelCommandLine=console=ttyS0 | ||
| Packages= | ||
| python3 |
There was a problem hiding this comment.
Don't think it overwrites like copilot says, but at this point not needed anymore since we added python3 for the sev_verify harness already (@DGonzalezVillal's vsock change), so guests already have python3 (unless you're trying to deduplicate, but then I think that can be its own PR?)
| @@ -0,0 +1,871 @@ | |||
| #!/usr/bin/env python3 | |||
| """ | |||
There was a problem hiding this comment.
imo this script should be co-located with the module in sev_verify
There was a problem hiding this comment.
thinking about this more, it may be harder than I thought since we don't have a 'file push' for vsock yet. I'm ok with keeping it here for now - but really would like to discuss the convention for future tests and potentially move this script to sev_verify in the future.
Reasons I would prefer to NOT add it to the guest image:
- harder to test changes (whole image build for any change to this script)
- image will get larger unnecessarily, esp if we allow multiple VM launches. Bring up a VM to test 4.0 will have extra data from all previous tests.
@DGonzalezVillal maybe a new step type for "run this script on the VM" that bundles together pushing a script and then executing it?
There was a problem hiding this comment.
I addressed by adding the following to the main test file, key_derivation_test.py:
from sev_verify.guest_vsock import GuestCommandError, fetch_guest_file_bytes, run_guest_command
and using GuestCommandError, fetch_guest_file_bytes and run_guest_command directly.
To the point raised by @amd-aliem and @DGonzalezVillal , I agree that it would be better if sev_verify itself supported "dynamic tests" like this better.
| @@ -0,0 +1,18 @@ | |||
| [Unit] | |||
| Description=Run SNP Key Derivation Tests after boot | |||
There was a problem hiding this comment.
why is this a service? I see you're already using the .for_guest() step type, which is what I think would launch it. I think this file should be removed unless I'm missing something?
| description = "Launch SEV-SNP guest and run key derivation tests" | ||
| module = "cert_tests.c3_0.c3_0_0_2.key_derivation_test" | ||
| scope = "mixed" | ||
| level = "3.0.0-2" |
There was a problem hiding this comment.
I believe we were having this under 3.0.0-1 (released together with snp config commit?), not one test per -X level
There was a problem hiding this comment.
No problem. I'll change.
| Step.for_host( | ||
| name="snphost ok", | ||
| type="required", | ||
| type="info", | ||
| command="snphost ok", | ||
| ), |
There was a problem hiding this comment.
Not sure I agree with changing this to info... or at least, a fail here shouldn't grant a milestone/pass even if everything else passes.
You added this b/c your machine was failing the snphost alias check right? I briefly asked @DGonzalezVillal and @amd-kkin if we should move the alias check into a --mitigations flag or something (along with other cve checks in the future). Maybe that would help here and more reason to keep it required.
c1d7d7d to
16f9e2c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (5)
sev_verify/cert_tests/common/snp_ok.py:52
snp_okis invoked fromcert_tests/common/prereqs.toml(prereqs are intended to gate the certification run), but changingsnphost oktotype="info"means the CLI will not mark the overall test/certification as failed when this step fails (onlysetup/requiredaffectoverall; seesev_verify/cli.py:467-476). This can allow later tests to run on a host where SNP isn't functional.
Step.for_host(
name="snphost ok",
type="info",
command="snphost ok",
),
sev_verify/cert_tests/c3_0/manifest.toml:27
- This manifest adds a new level
3.0.0-2, butmax_certification_levelis still set to3.0.0-0. The runner clamps the reported highest certified level to that cap even if higher levels pass (sev_verify/cli.py:_highest_certified_level, lines 557-584), so passingkey-derivationwill never result in a certified level above3.0.0-0.
# ── Level 3.0.0-2 ───────────────────────────────────────────
[[tests]]
name = "key-derivation"
description = "Launch SEV-SNP guest and run key derivation tests"
sev_verify/cert_tests/c3_0/c3_0_0_2/key_derivation_test.py:80
ReportInfo.committed_tcbis annotated asTcbVersionbut initialized withNoneand then patched in__post_init__. This defeats type checking and makes it easy to introduceNonebugs when refactoring. Make the annotation explicitly optional (or use a default factory).
@dataclass
class ReportInfo:
version: Optional[int] = None
guest_svn: int = 0
committed_tcb: TcbVersion = None
def __post_init__(self):
if self.committed_tcb is None:
self.committed_tcb = TcbVersion()
sev_verify/cert_tests/c3_0/c3_0_0_2/key_derivation_test.py:206
test_determinismcan incorrectly pass if both key reads fail:_read_keyreturnsNoneon error, andNone == Noneevaluates true. Explicitly fail when either derived key file can't be read.
k1 = _read_key(ctx.artifact_dir / "det_key1.bin")
k2 = _read_key(ctx.artifact_dir / "det_key2.bin")
if k1 == k2:
return StepHandlerResult(exit_code=0, stdout="Keys match (deterministic)")
return StepHandlerResult(exit_code=1, stderr="Keys differ — derivation is not deterministic")
modules/build/guest/mkosi.conf:14
- Adding
python3to the sharedmodules/build/guestmodule is redundant with the per-image guest configs already listingpython3(e.g.images/guest-ubuntu-26.04/mkosi.conf:11-25). Keeping it here makes it easy to accidentally diverge package sets across images (or later forget wherepython3is coming from).
[Content]
KernelCommandLine=console=ttyS0
Packages=
python3
|
I changed to use the generic guest image. The message from the one new commit: feat: move key derivation test logic to host, use vsock for guest commands |
DGonzalezVillal
left a comment
DGonzalezVillal
left a comment
There was a problem hiding this comment.
Need to extend the TCB definition for Turin vs Genoa and consider Venice one will look different as well. If need be we can chat on ways to do this.
| [Content] | ||
| KernelCommandLine=console=ttyS0 | ||
| Packages= | ||
| python3 |
There was a problem hiding this comment.
I think if it's named differently it might cause issues.
| class TcbVersion: | ||
| boot_loader: int = 0 | ||
| tee: int = 0 | ||
| snp: int = 0 | ||
| microcode: int = 0 | ||
|
|
||
| def to_u64(self) -> int: | ||
| return ( | ||
| (self.boot_loader & 0xFF) | | ||
| ((self.tee & 0xFF) << 8) | | ||
| ((self.snp & 0xFF) << 48) | | ||
| ((self.microcode & 0xFF) << 56) | ||
| ) |
There was a problem hiding this comment.
Your TCB generation still needs updating for Turin version (I'll allow no Venice for now)
| Step.for_host( | ||
| name="snphost ok", | ||
| type="required", | ||
| type="info", |
There was a problem hiding this comment.
| type="info", | |
| type="required", |
| Step.for_host( | ||
| name="snphost ok", | ||
| type="required", | ||
| type="info", | ||
| command="snphost ok", | ||
| ), |
There was a problem hiding this comment.
I asked mark to move it back to required, but do you think info is fine for snphost ok check? I think we need to introduce the fix to snphost ok, but keep this particular step as required.
|
|
||
|
|
||
| def test_vmpl_isolation(ctx: StepContext) -> StepHandlerResult: | ||
| ok0, err0 = _derive_key(ctx, "vmpl0_key.bin", vmpl=0) |
There was a problem hiding this comment.
For forwards compatibility should we NOT hard-code vmpl=0? In the fuure possibly distros may have svsm enabled by default so VMs would be on vmpl 1 right? Could check current level first and then increment by 1 perhaps. Also ok with leaving this for whenever we run into it.
There was a problem hiding this comment.
Good point.
Co-Authored-By: Claude <noreply@anthropic.com>
Always verify that values above the active bound are rejected, even when the bound is 0 (no ID block). Tests 3 values above the bound for both guest SVN and each TCB component. Co-Authored-By: Claude <noreply@anthropic.com>
16f9e2c to
2a5d649
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (9)
sev_verify/cert_tests/c3_0/manifest.toml:26
- This manifest still caps the certification at max_certification_level="3.0.0-0" (line 3), so even if level 3.0.0-2 runs and passes, the computed highest certified level will be clamped to 3.0.0-0. If this test is intended to be part of the 3.0 track, the cap should be bumped to include 3.0.0-2.
# ── Level 3.0.0-2 ───────────────────────────────────────────
[[tests]]
name = "key-derivation"
sev_verify/cert_tests/common/snp_ok.py:50
- Changing this step to type="info" means a failing
snphost okno longer fails or aborts the test run (only "setup"/"required" failures affect the overall result in sev_verify/cli.py). That can allow certification to proceed on a host that cannot run SNP guests.
type="info",
sev_verify/cert_tests/c3_0/c3_0_0_2/key_derivation_test.py:220
- A read failure returning None can cause a false pass here (e.g., None != evaluates true). Treat unreadable key files as a test failure before comparing values.
k0 = _read_key(ctx.artifact_dir / "vmpl0_key.bin")
k1 = _read_key(ctx.artifact_dir / "vmpl1_key.bin")
if k0 != k1:
sev_verify/cert_tests/c3_0/c3_0_0_2/key_derivation_test.py:232
- A read failure returning None can cause a false pass here (e.g., None != evaluates true). Fail the step if either key file can't be read.
kv = _read_key(ctx.artifact_dir / "vcek_key.bin")
km = _read_key(ctx.artifact_dir / "vmrk_key.bin")
if kv != km:
sev_verify/cert_tests/c3_0/c3_0_0_2/key_derivation_test.py:353
- A read failure returning None can cause a false pass here (e.g., None != evaluates true). Fail the step if either derived key can't be read before comparing.
k1 = _read_key(ctx.artifact_dir / "gfs1_key.bin")
k2 = _read_key(ctx.artifact_dir / "gfs2_key.bin")
if k1 != k2:
sev_verify/cert_tests/c3_0/c3_0_0_2/key_derivation_test.py:363
- If reading the baseline key fails (baseline=None), comparisons can produce misleading passes/failures. Treat an unreadable baseline derived key as a hard failure.
ok, err = _derive_key(ctx, "gfs_baseline.bin", gfs=0)
if not ok:
return StepHandlerResult(exit_code=1, stderr=f"Baseline derivation failed: {err}")
baseline = _read_key(ctx.artifact_dir / "gfs_baseline.bin")
sev_verify/cert_tests/c3_0/c3_0_0_2/key_derivation_test.py:384
- If a per-bit derived key can't be read (k=None), the current
k != baselinecheck will treat it as “differs from baseline” and incorrectly pass that bit. Handle unreadable key files explicitly as failures.
k = _read_key(ctx.artifact_dir / f"gfs_bit{bit}.bin")
if k != baseline:
lines.append(f"GFS bit {bit} ({label}): differs from baseline")
else:
failed.append(f"GFS bit {bit} ({label}): same as baseline")
modules/build/guest/mkosi.conf:14
- All existing guest image templates already include python3 in their own
images/guest-*/mkosi.confpackage lists. Adding it again here in the shared module is redundant and can be confusing about where guest package requirements are defined (docs/how-to-add-new-os-images.md shows packages living in the per-image templates).
[Content]
KernelCommandLine=console=ttyS0
Packages=
python3
sev_verify/cert_tests/c3_0/c3_0_0_2/key_derivation_test.py:203
- If
_read_key()fails and returns None for both reads, this comparison will incorrectly pass as “deterministic”. Make the test fail when either derived key cannot be read.
This issue also appears in the following locations of the same file:
- line 218
- line 230
- line 351
- line 360
- line 380
k1 = _read_key(ctx.artifact_dir / "det_key1.bin")
k2 = _read_key(ctx.artifact_dir / "det_key2.bin")
if k1 == k2:
|
Add so-called cross-CVM (sub) test: launch a CVM - save a derived key - stop the CVM - launch a CVM - verify ability to generate the same key. This version of the test was in a 2nd, draft PR (#285) --> decided no need to split up the test across two PRs and to close the 2nd PR. |
Import calculate_measurement from attestation_test instead of duplicating it. Conditionally add generate_id_block step when sev_verify.id_block is available. Co-Authored-By: Claude <noreply@anthropic.com>
…mands Replace the guest-side Python script approach with individual snpguest key commands over vsock, pulling derived keys to the host for comparison. All test logic (determinism, VMPL isolation, root key difference, SVN/TCB bound enforcement, GFS sensitivity and field mixing) now lives in the sev_verify test module as callable steps. Use callable steps that loop over snpguest key via run_guest_command/fetch_guest_file_bytes while the VM is running. SVN and TCB sweeps are fully dynamic based on the runtime attestation report. TCB above-bound tests use committed+1..committed+3 per component from the runtime report, capped at 0xFF to avoid 8-bit overflow. Remove key-derivation guest module. The guest-side Python script and systemd service are no longer needed and snpguest is available in the generic guest image. Co-Authored-By: Claude <noreply@anthropic.com>
Derives a key at the end of CVM 1, stops it, launches CVM 2, derives the same key with identical parameters, and compares. Proves the key is bound to platform identity rather than transient VM state. Co-Authored-By: Claude <noreply@anthropic.com>
2a5d649 to
343c959
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
sev_verify/cert_tests/c3_0/c3_0_0_2/key_derivation_test.py:303
- The TCB above-bound test builds
tcb_u64with only the tested component set and all other components zero. If the firmware’s committed-bound check is lexicographic / holistic, this can create a TCB value that is not actually above the committed bound (because other components are lowered), producing false results. Construct the test TCB from the committed TCB and bump only the component under test.
for val in above_vals:
tcb_u64 = TcbVersion(**{comp: val}).to_u64()
ok, _ = _derive_key(ctx, f"tcb_above_{comp}_{val}.bin",
tcb=tcb_u64, gfs=1 << 5)
if ok:
sev_verify/cert_tests/c3_0/c3_0_0_2/key_derivation_test.py:327
- The TCB sensitivity loop currently computes
tcb_u64with only the varied component set and all other components zero. This doesn’t isolate per-component sensitivity relative to the committed TCB and can also change the overall TCB value in unintended ways. Use the committed TCB as the baseline and vary one component at a time.
tcb_u64 = TcbVersion(**{comp: val}).to_u64()
if tcb_u64 in keys:
continue # already derived this exact TCB value
fname = f"tcb_{comp}_{val}.bin"
ok, err = _derive_key(ctx, fname, tcb=tcb_u64, gfs=1 << 5)
| [Content] | ||
| KernelCommandLine=console=ttyS0 | ||
| Packages= | ||
| python3 |
| k1 = _read_key(ctx.artifact_dir / "det_key1.bin") | ||
| k2 = _read_key(ctx.artifact_dir / "det_key2.bin") | ||
| if k1 == k2: | ||
| return StepHandlerResult(exit_code=0, stdout="Keys match (deterministic)") | ||
| return StepHandlerResult(exit_code=1, stderr="Keys differ — derivation is not deterministic") |
Signed-off-by: Mark Gentry <markg.inspire@gmail.com>
… level 3.0.0-1 Signed-off-by: Mark Gentry <markg.inspire@gmail.com>
| [[tests]] | ||
| name = "key-derivation" | ||
| description = "Launch SEV-SNP guest and run key derivation tests" | ||
| module = "cert_tests.c3_0.c3_0_0_2.key_derivation_test" | ||
| scope = "mixed" | ||
| level = "3.0.0-1" |
| def _read_key(path: Path) -> Optional[bytes]: | ||
| try: | ||
| return path.read_bytes() | ||
| except Exception: | ||
| return None |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (6)
Previously missed (4) — in code that hasn't changed since the last review.
sev_verify/cert_tests/c3_0/c3_0_0_2/key_derivation_test.py:80
ReportInfo.committed_tcbis annotated asTcbVersionbut defaulted toNone. Even though__post_init__fills it, this is inconsistent and can trip type checkers / readers into thinkingNoneis a valid runtime value. Annotate it as optional (or use a default factory) to match the actual initialization pattern.
class ReportInfo:
version: Optional[int] = None
guest_svn: int = 0
vmpl: int = 0
committed_tcb: TcbVersion = None
def __post_init__(self):
if self.committed_tcb is None:
self.committed_tcb = TcbVersion()
sev_verify/cert_tests/c3_0/c3_0_0_2/key_derivation_test.py:157
subprocess.run()inparse_report()has no timeout. Ifsnpguest display reporthangs (e.g., corrupted report file or tool bug), the whole harness can stall indefinitely. Add a reasonable timeout consistent with other host-side subprocess calls.
This issue also appears on line 520 of the same file.
result = subprocess.run(
["snpguest", "display", "report", str(report_file)],
capture_output=True, text=True, check=False,
)
sev_verify/cert_tests/c3_0/c3_0_0_2/key_derivation_test.py:213
test_determinism()treatsNonekeys as comparable values. If both reads fail and returnNone, the test will incorrectly pass (None == None). Fail the step when either derived key cannot be read.
This issue also appears on line 376 of the same file.
k1 = _read_key(ctx.artifact_dir / "det_key1.bin")
k2 = _read_key(ctx.artifact_dir / "det_key2.bin")
if k1 == k2:
return StepHandlerResult(exit_code=0, stdout="Keys match (deterministic)")
return StepHandlerResult(exit_code=1, stderr="Keys differ — derivation is not deterministic")
sev_verify/cert_tests/c3_0/c3_0_0_2/key_derivation_test.py:353
test_tcb()returns success when fewer than 2 keys were derived, even if that happened due to unexpected rejections or other failures. Only treat this as N/A when the committed TCB is actually all zeros; otherwise this should fail the test to surface a real regression.
if len(keys) < 2:
lines.append("TCB sensitivity N/A — all committed components are zero")
return StepHandlerResult(exit_code=0, stdout="\n".join(lines))
sev_verify/cert_tests/c3_0/c3_0_0_2/key_derivation_test.py:380
test_gfs_field_mixing()doesn't validate that the baseline key was actually read. If_read_key()returnsNone, every per-bit key will appear to "differ" and the test can pass incorrectly. Fail early if the baseline key cannot be read.
ok, err = _derive_key(ctx, "gfs_baseline.bin", gfs=0)
if not ok:
return StepHandlerResult(exit_code=1, stderr=f"Baseline derivation failed: {err}")
baseline = _read_key(ctx.artifact_dir / "gfs_baseline.bin")
sev_verify/cert_tests/c3_0/c3_0_0_2/key_derivation_test.py:524
test_tcbcan perform up to 1021snpguest keyinvocations in the worst case (4 components * 256 values minus dedup). With a step timeout of 120s, it’s easy for this to time out on slower hosts. Consider increasing the step timeout to match the possible workload (or sampling the value range).
Step.for_callable(
name="Test TCB bound enforcement and sensitivity",
type="required",
handler="test_tcb",
timeout=120,
The test invokes snpguest repeatedly, especially the key subcommand, to test the SEV-SNP derived keys feature. The test mostly verifies that the derived keys are different when they're supposed to be.
More complete testing could be achieved by including an ID block. Including an ID block is completely independent and I plan a PR for it soon.
I think AI did a decent job at maintaining "certificate consistency", but I'm not sure about the versioning.
Closes #105 and https://amd.atlassian.net/browse/DEAEOCC-304 (Jira).