attest: add attestation testing with kbs-test integration#1002
attest: add attestation testing with kbs-test integration#1002nihalxkumar wants to merge 3 commits into
Conversation
nihalxkumar
requested review from
joergroedel,
msft-jlange,
peterfang and
stefano-garzarella
as code owners
nihalxkumar
force-pushed
the
improve-attestation-tests-kbs
branch
from
69c30ad to
ef1af17
Compare
|
Thanks for 67351f9! That comes in really handy. |
|
This commit "scripts: fix bash array expansion and update license" can be squashed, right? Please do that. |
|
sorry, but I'm a little bit confused. How do you run attestation testing in the CI/no-cc mode without snp hardware? |
nihalxkumar
force-pushed
the
improve-attestation-tests-kbs
branch
from
ef1af17 to
69a3f87
Compare
luigix25
left a comment
luigix25
left a comment
There was a problem hiding this comment.
Real attestation cannot be performed on a normal x86 machines, it requires an AMD SEV SNP capable machine.
This means that the nocc option does not make any sense. As a consequence the CI will fail, as it's running on a x86 machine.
That said, adding a fake attestation report, for testing purposes, could be a good idea.
I see that you added a lot of tests, most of them being extremely trivial, are they useful?
Also please avoid all unnecessary changes, like new blank-lines.
Sorry to say, but to me this PR looks like AI generated.
|
@luigix25 Thanks for the feedback I did take AI's help. I apologize if some of the tests ended up being too trivial. The extra blank lines in the Makefile definitely slipped past my review, sorry for the noise there. |
There is some value in this PR, having a script that sets up all the attestation pieces IMO is useful. CI part can be dropped entirely. It would be useful is we could do some sort of "emulated" attestation, but we don't have it yet as the SVSM kernel would require some changes, but it's not a small task. Having unit tests for attestation would be great, but please review them carefully and leave only those that make non-trivial checks. It's important to explain the reason behind any test. I think there is also some overlap in the documentation. So, before submitting a new version, please review your PR carefully. Make sure that all the changes are intended and documented. Commit message should explain why you are doing something, it's (often) easy to understand what you are doing. Hope this can help you: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes |
|
The kernel docs were very helpful. Please check and suggest any changes. |
Add a dedicated attestation launcher that starts kbs-test and aproxy and runs the test IGVM with attestation enabled. Reuse the existing test-in-svsm target for this flow by selecting the launcher and extra dependencies through make variables, and document the manual invocation in TESTING.md. The attestation runner rejects --nocc Signed-off-by: Nihal <nihalxkumar@protonmail.com>
nihalxkumar
force-pushed
the
improve-attestation-tests-kbs
branch
from
a4e6458 to
0d99da3
Compare
|
Changes as asked by @luigix25 |
|
@nihalxkumar please resolve all the conversations that you addressed. Only the PR author and maintainer can do that, so it's a good practice for the PR author to close them (if addressed of course). |
Where is this? and also why?
Ditto, I'm really really confused. Please update the PR description, but don't do a list of things, you should describe the goal of this PR, what is adding/fixing, and the reason of some choice.
I don't think this PR is related to that. I can't see any test stressing the KBS protocol, aproxy, etc. Maybe that issue was not clear, but in SVSM we have:
That issue is more for 1, since we are adding more tests for the attestation protocol. I see something also for pre-boot attestation, but again I don't understand how aproxy, kbs-test are used by those tests, can you explain that, even in the commit description? And please split the tests at least in 2 commits: one for pre-boot attestation (kernel/src/attest.rs) and one for attestation protocol (kernel/src/protocols/attest.rs). |
Add unit tests for the hash() function used during pre-boot attestation negotiation. hash() feeds NegotiationParams into SHA-512 in the order dictated by the server's NegotiationResponse. Because the server controls which params contribute and in what sequence, ordering sensitivity is a security property: different orderings must produce different digests. Tests cover: - Challenge-first and EcPublicKeyBytes-first orderings, verified against a manually computed SHA-512 reference. - That swapping the param order produces a distinct digest. Signed-off-by: Nihal <nihalxkumar@protonmail.com>
Add unit tests for the attestation protocol request handler (kernel/src/protocols/attest.rs). The SVSM attestation protocol exposes a fixed wire ABI defined by Table 11 and Table 13 of "Secure VM Service Module for SEV-SNP Guests, Rev 1.00". Any accidental field reordering or padding change would silently break the guest interface, so the layout of AttestServicesOp and AttestSingleServiceOp is now checked with compile-time const assertions (following the pattern in greq/msg.rs). Runtime tests cover: - Reserved-field validation: every reserved region in both structs must be rejected when non-zero, guarding forward-compatibility with future spec revisions. - Certificate region boundary: size==0 is valid (no extended report); size<=MAX_CERTIFICATE_SIZE is accepted; size>MAX_CERTIFICATE_SIZE is rejected. - GUID serialization: single-entry and multi-entry GuidTable wire format, including offset arithmetic between header and payload. - Protocol routing: unknown request codes must return UNSUPPORTED_PROTOCOL. Signed-off-by: Nihal <nihalxkumar@protonmail.com>
nihalxkumar
force-pushed
the
improve-attestation-tests-kbs
branch
from
0d99da3 to
198e38a
Compare
| Attestation can be tested in the same infrastructure by running the | ||
| attest-enabled test image together with `kbs-test` and `aproxy`. |
There was a problem hiding this comment.
Sorry, I'm still confused, why you need to run kbs-test and aproxy for the tests you added.
Can you tell me which of the tests we're adding require communication with KBS?
Maybe I missed something.
There was a problem hiding this comment.
None of the unit tests in commits 2 and 3 require kbs-test or aproxy.
commit 1 (test-in-svsm-attest.sh) needs kbs-test and aproxy as it is an end-to-end integration test
that boots the SVSM test image on real SNP hardware with the attest feature enabled, connects it to a local kbs-test server via aproxy, and checks that pre-boot attestation completes successfully ("attestation successful" in output).
This is the part that addresses the kbs-test integration mentioned in the original issue.
The TESTING.md is documenting the integration script.
There was a problem hiding this comment.
So commit 1 and commits 2/3 are completely unrelated, right?
Why putting them in a single PR ?
| distclean: clean | ||
|
|
||
| .PHONY: test miri clean clippy bin/stage2.bin bin/svsm-kernel.elf bin/test-kernel.elf stage1_elf_trampoline distclean $(APROXYBIN) $(IGVM_FILES) $(IGVM_TEST_FILES) | ||
| .PHONY: test miri clean clippy bin/stage2.bin bin/svsm-kernel.elf bin/test-kernel.elf stage1_elf_full stage1_elf_trampoline stage1_elf_test distclean $(APROXYBIN) $(IGVM_FILES) $(IGVM_TEST_FILES) test-in-svsm |
There was a problem hiding this comment.
Why adding stage1_elf_full and stage1_elf_test ?
| test-in-svsm: bin/coconut-test-qemu.igvm $(IGVMMEASUREBIN) | ||
| ./scripts/test-in-svsm.sh $(TEST_ARGS) | ||
| test-in-svsm: $(IGVMMEASUREBIN) $(TEST_IN_SVSM_DEPS) | ||
| cargo xbuild $(XBUILD_ARGS_TEST) ./configs/test/qemu-test-target.json |
There was a problem hiding this comment.
Why removing the dep with bin/coconut-test-qemu.igvm and adding the step here to build it?
| make FEATURES_TEST=vtpm,virtio-drivers,block,attest \ | ||
| TEST_IN_SVSM_SCRIPT=./scripts/test-in-svsm-attest.sh \ | ||
| TEST_IN_SVSM_DEPS=aproxy \ | ||
| test-in-svsm |
There was a problem hiding this comment.
Please don't reuse test-in-svsm target that is used to run rust test in the SVSM context.
This new test is more like scripts/test-qemu-nocc-svsm.sh, so if you want to add a target in the makefile, just define a new one, or just document how to run the script directly.
| Attestation can be tested in the same infrastructure by running the | ||
| attest-enabled test image together with `kbs-test` and `aproxy`. |
There was a problem hiding this comment.
So commit 1 and commits 2/3 are completely unrelated, right?
Why putting them in a single PR ?
| set -e | ||
|
|
||
| SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd ) | ||
| : "${TEST_IGVM:=$SCRIPT_DIR/../bin/coconut-test-qemu.igvm}" |
There was a problem hiding this comment.
How this change is related?
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
The SPDX and SignedOff workflows override the checkout ref to `github.event.pull_request.head.sha`. This breaks PRs whose branches were created before the scripts used by the pipelines were added to main. For example the CI of PR coconut-svsm#1002 is failing to execute the SPDX pipeline [1] with the following: scripts/check-spdx.sh: No such file or directory Process completed with exit code 127. For `pull_request` events, GitHub runs workflows from a merge commit of the PR head into the target branch [2]: "GITHUB_SHA for this event is the last merge commit on the pull request merge branch." So the workflow file from main is picked up and executed, but the checkout then switches to the PR head, where the scripts do not exist. Drop the `ref:` override to let actions/checkout use its default, which checks out the merge commit. Both the scripts (from main) and the PR changes are available there. The SPDX script reads files from the working tree and the SignedOff script only inspects git objects, so both work correctly with the merge commit. [1] https://github.com/coconut-svsm/svsm/actions/runs/23894438075/job/69675546900 [2] https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#pull_request Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
stefano-garzarella
added
the
in-review
The SPDX and SignedOff workflows before merging PR coconut-svsm#1032, and the new PR Compliance Checks after the merge, override the checkout ref to `github.event.pull_request.head.sha`. This breaks PRs whose branches were created before the scripts used by the pipelines were added to main. For example the CI of PR coconut-svsm#1002 is failing to execute the SPDX pipeline [1] with the following: scripts/check-spdx.sh: No such file or directory Process completed with exit code 127. For `pull_request` events, GitHub runs workflows from a merge commit of the PR head into the target branch [2]: "GITHUB_SHA for this event is the last merge commit on the pull request merge branch." So the workflow file from main is picked up and executed, but the checkout then switches to the PR head, where the scripts do not exist. Drop the `ref:` override to let actions/checkout use its default, which checks out the merge commit. Both the scripts (from main) and the PR changes are available there. The SPDX script reads files from the working tree and the SignedOff script only inspects git objects, so both work correctly with the merge commit. [1] https://github.com/coconut-svsm/svsm/actions/runs/23894438075/job/69675546900 [2] https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#pull_request Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
stefano-garzarella
added
the
wait-for-update
|
@nihalxkumar are you still working on this, otherwise I'll fixup the remaining issues and try to merge it. |
|
Apologies for the delay, Stefano. I’ve been occupied with other things.
It would be great if you could take this up.
…On 30/06/26 20:09, Stefano Garzarella wrote:
*stefano-garzarella* left a comment (coconut-svsm/svsm#1002)
<#1002 (comment)>
@nihalxkumar <https://github.com/nihalxkumar> are you still working on
this, otherwise I'll fixup the remaining issues and try to merge it.
—
Reply to this email directly, view it on GitHub
<#1002?email_source=notifications&email_token=A45QUBNYCKKZJXACGHJCUPL5CPGJLA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOBUGQ3DOMJRGI22M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-4844671125>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A45QUBJGZSNUWTCF3FRELJT5CPGJLAVCNFSNUABFKJSXA33TNF2G64TZHM3DCMZSG43TIOJSHNEXG43VMU5TIMBXHAYDEOBYGY32C5QC>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
7 participants
This PR adds tests for both attestation surfaces.
SVSM exposes two distinct attestation surfaces: the pre-boot attestation feature (the attest Rust feature flag), which runs before the guest OS starts and uses a KBS to retrieve secrets, and the in-guest attestation
protocol, through which a VMPL0 guest can request a hardware attestation
report via SVSM.
For the in-guest attestation protocol the wire format of AttestServicesOp and AttestSingleServiceOp is fixed by the SVSM spec (Tables 11 and 13 of "Secure VM Service Module for SEV-SNP Guests, Rev 1.00"). A silent field reorder or padding change would break the binary interface with the guest, so the layout is now enforced with compile-time
constassertions, following the pattern established ingreq/msg.rs. Runtime tests cover reserved-field rejection, certificate region boundary enforcement, GUID table serialization, and that unknown request codes returnUNSUPPORTED_PROTOCOL.For pre-boot attestation, the
hash()function feedsNegotiationParamsinto SHA-512 in the order dictated by theserver's
NegotiationResponse. Because the server controls which params contribute and in what sequence, ordering sensitivity is a security property.Tests verify that
Challenge → EcPublicKeyBytesandEcPublicKeyBytes → Challengeproduce the expected individual digests and that swapping the order produces a distinct output.The third commit adds a test script that exercises the pre-boot KBS flow end-to-end: it starts
kbs-testandaproxy, launches the SVSM test image built with the attest feature, and checks that "attestation successful" appears in the output. This requires a real SNP-capable machine and is not wired into the standard QEMU CIworkflow for that reason.
Related: #773