attestation: Update kernel module and proxy for AES-GCM, other small changes

[tylerfanelli]

This PR updates the attestation kernel module and proxy with two main modifications:

• Updating the kernel module and proxy to use AES-GCM as the standard encryption mechanism protecting secrets received into the SVSM kernel once successfully attested.
• Updating the proxy to use standard KBS 0.4.0 as used by the Trustee server.

The kbs-test server was also updated to emulate KBS 0.4.0 and thus can be used to test this series.

cc/ @stefano-garzarella @nicstange @berrange

Try for yourself

Start kbs-test

$ git clone https://github.com/tylerfanelli/kbs-test.git && cd kbs-test
$ git checkout main
$ cargo build
$ export SVSM=/path/to/svsm
$ MEASUREMENT="$(${SVSM}/bin/igvmmeasure --check-kvm ${SVSM}/bin/coconut-qemu.igvm measure -b)"
$ cargo run -- -m $MEASUREMENT

Start the attestation proxy

$ git clone https://github.com/tylerfanelli/svsm.git && cd svsm
$ git checkout attest-update
$ make aproxy
$ bin/aproxy --protocol kbs --url http://0.0.0.0:8080 --unix /tmp/svsm-proxy.sock --force

Run SVSM with attestation

$ FW_FILE=/path/to/OVMF.fd make FEATURES=attest 
$ ./scripts/launch_guest.sh --qemu /path/to/qemu-system-x86_64 --image /path/to/qcow2-file.qcow2 --aproxy /tmp/svsm-proxy.sock

[stefano-garzarella]

This PR updates the attestation kernel module and proxy with two main modifications:

* Updating the kernel module and proxy to use AES-GCM as the standard encryption mechanism protecting secrets received into the SVSM kernel once successfully attested.

* Updating the proxy to use standard KBS 0.4.0 as used by the Trustee server.

@tylerfanelli I took a quick look, and I also see some changes to the protocol used between SVSM and the proxy.
Can you explain better why they are necessary?
Are they generic enough to support any attestation protocol, or Trustee-specific?

We haven't made any stable releases yet, so it's fine, but we need to deal with this in the future.
(e.g. increasing the version in NegotiationRequest)

I think now is the time to add documentation/specifications for the protocol and finalize a version (if this is the final version).

[nicstange]

I think now is the time to add documentation/specifications for the protocol and finalize a version (if this is the final version).

As a disclaimer, I haven't looked into the details of this PR yet, but just to avoid unnecessary work I'm writing it now: one thing which we very likely need from the protocol and which wasn't implemented the time I last checked is some sort of authentication for the received key material. Otherwise an adversary would be able to impersonate the KBS and make the SVSM to protect its persisted state with a secret he controls. Using authenticated encryption for the key material is definitely a step in that direction, but not sufficient. I think a full solution would be outside the scope of this PR though, and would personally leave the protocol finalization for some later PR.

[tylerfanelli]

As a disclaimer, I haven't looked into the details of this PR yet, but just to avoid unnecessary work I'm writing it now: one thing which we very likely need from the protocol and which wasn't implemented the time I last checked is some sort of authentication for the received key material. Otherwise an adversary would be able to impersonate the KBS and make the SVSM to protect its persisted state with a secret he controls.

See the final commit (specifically this), which adds authentication to the received decryption key. This type of key is now required by SVSM.

Using authenticated encryption for the key material is definitely a step in that direction, but not sufficient. I think a full solution would be outside the scope of this PR though, and would personally leave the protocol finalization for some later PR.

With regards to this comment:

Otherwise an adversary would be able to impersonate the KBS and make the SVSM to protect its persisted state with a secret he controls.

The disk/filesystem should be encrypted before the VM is run, and should be done so on a trusted machine. That way, if there's any impersonation/tampering involved, it simply won't be able to unlock the disk without the correct key. Only the correct KBS would have access to the key.

A bad actor could impersonate the KBS and give the wrong key, but that is simply a DoS, not a breach of any data.

[tylerfanelli]

@tylerfanelli I took a quick look, and I also see some changes to the protocol used between SVSM and the proxy. Can you explain better why they are necessary?

The changes are as follows:

• Represent each byte field as a Vec<u8> instead of base64-encoded Strings. This relieves the kernel module from needing to worry about base64 {de}serialization which should instead be up to the proxy backend.
• Split the challenge nonce from the usual NegotiationParams. In KBS (and likely other protocols with nonce challenges), the respective nonce needs to be sent alongside the attestation evidence for validation. This allows the kernel module to identify and cache the nonce so it is able to send it alongside the evidence at a later point.
• Standardize around ECDH-ES+A256KW for secret decryption. We previously agreed that the attestation module would only support EC keys, and this is an established algorithm for fetching secrets that includes authentication.
• Add an option to write attestation artifacts (such as EAR tokens) back to SVSM upon successful attestation.

Are they generic enough to support any attestation protocol, or Trustee-specific?

They are generic.

I think now is the time to add documentation/specifications for the protocol and finalize a version (if this is the final version).

I think this is much closer to the final version, with only one change remaining. I can add some documentation to this PR.

[tylerfanelli]

@stefano-garzarella To test, you can use the branch found in coconut-svsm/kbs-test#6.

[stefano-garzarella]

@stefano-garzarella To test, you can use the branch found in coconut-svsm/kbs-test#6.

@tylerfanelli thanks! BTW, can you post a changelog when you update the PR? It will help reviewers to understand the difference between versions.

[luigix25]

I tested this PR running:

make aproxy
bin/aproxy --protocol kbs --url http://0.0.0.0:8080 --unix /tmp/svsm-proxy-luigi.sock --force

for kbs

cargo build
export SVSM=/path/to/svsm
MEASUREMENT="$(${SVSM}/bin/igvmmeasure --check-kvm ${SVSM}/bin/coconut-qemu.igvm measure -b)"
cargo run -- -m $MEASUREMENT

and svsm:

FW_FILE=/path/to/OVMF.fd make FEATURES=attest 
./scripts/launch_guest.sh --qemu /home/lleonard/qemu/build/qemu-system-x86_64 --image /home/lleonard/repo/snp-svsm-vtpm/images/fedora-luks.qcow2 --aproxy /tmp/svsm-proxy-luigi.sock

Everything run smoothly. Please check this is the correct way of running it, if so what about adding it in the PR description?

[luigix25]

aproxy/scr/main.rs:

/// Supported servers include:
/// kbs-test: https://github.com/tylerfanelli/kbs-test (for testing).
#[clap(long = "protocol")]
backend: backend::Protocol,

IIUC kbs-test has been replaced by kbs, also the repo url is now different.

[tylerfanelli]

aproxy/scr/main.rs:

/// Supported servers include:
/// kbs-test: https://github.com/tylerfanelli/kbs-test (for testing).
#[clap(long = "protocol")]
backend: backend::Protocol,

IIUC kbs-test has been replaced by kbs, also the repo url is now different.

Thanks. I've opted to just remove that comment altogether, as I feel an explanation of supported protocols/servers belongs in the documentation instead.

[tylerfanelli]

Everything run smoothly. Please check this is the correct way of running it, if so what about adding it in the PR description?

@luigix25 LGTM. I've added instructions to the PR description.

[stefano-garzarella]

@tylerfanelli great, LGTM, I just left a minor comment in the documentation that differs from the cover letter of this PR, indeed following it, I'm able to test this correctly.

[stefano-garzarella]

@tylerfanelli not for this PR, but I think will be nice to add kbs-test here as submodule or put in the documentation the commit to use (or a tag/branch) to avoid issues like #869 and maybe to support #773

Also, can we merge coconut-svsm/kbs-test#6 ?

[tylerfanelli]

@tylerfanelli not for this PR, but I think will be nice to add kbs-test here as submodule or put in the documentation the commit to use (or a tag/branch) to avoid issues like #869 and maybe to support #773

Sure, I can add another PR to do so.

Also, can we merge coconut-svsm/kbs-test#6 ?

Yes, that can be merged alongside this PR.

[tylerfanelli]

@stefano-garzarella there was a conflict in Cargo.lock. I rebased and resolved the conflict. Can you re-approve?