Add a derived keys test#242
Conversation
See the last paragraph for more on ordering the new keys test. See README.md in modules/test/guest/key-derivation for more about the derived keys test, in general. Old TCB components (eg, PSP FW, microcode) can cause some guest tests to fail. In general, I don't think this should be the case, but if the TCB components are very old, then maybe it makes sense. By putting the derived keys test first, this test's contributions to the certificates should be present regardless of how old the TCB components are. Note that preserving the derived keys test contributions could also be achieved by using Wants= instead of Requires= for the services corresponding to the tests that fail due to the old TCB components.
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.
| 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) |
Signed-off-by: Harika Nittala <lnittala@amd.com>
- Add network device (e1000) with user-mode networking to enable network connectivity for the guest VM. Signed-off-by: Harika Nittala <lnittala@amd.com>
Signed-off-by: Harika Nittala <lnittala@amd.com>
Signed-off-by: Harika Nittala <lnittala@amd.com>
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?
| 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.
| 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.
| 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.
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