-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Attestation driver and proxy (with KBS attestation) #528
base: main
Are you sure you want to change the base?
Conversation
kernel/src/attest.rs
Outdated
if !response.success { | ||
panic!("attestation failed"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we return an error and have the panic in the main function (e.g. when calling driver.attest()
). Feel free to introduce an AttestationError
and a new variant for SvsmError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. What about the rest of the unwrap()
s and panic()
s in the file? Should they be replaced with returns of AttestationError
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say so, yes. The distinction between what to return and what to panic on immediately is subtle, but in general I'd favor panicking as high on the stack as possible. Unless it's a hard bug condition, I'd just bubble it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 8f5b677, can you please re-review?
Do you have any insight on why this is so slow, or is it remaining for a future debugging effort ? |
Have you tried using RdRand instead of RdSeed? |
As a general point, I wonder whether these should really all live in the 'svsm' git repo. Consider that the execution context of the proxy is the host OS, and the context of SVSM's driver is in guest VM. With this we're defining an ABI for a protocol between the host OS and guest VM, that potentially needs to remain stable & backwards compatible more or less indefinitely (forever). ie it is unlikely to be viable to mandate that the deployed host side proxy is upgraded in lock-step with SVSM releases, and thus we need to expect mis-matched releases of the two and ensure things continue to work. Splitting the proxy off as a separate repository, and defining a formal document specifying the interaction between SVSM & the proxy, will re-inforce the message that these two sides are independent and need to retain backwards compatibility as their respective impls evolve over time. |
/// Based on JSON Web Key | ||
/// See for examples: https://www.rfc-editor.org/rfc/rfc7517#appendix-A.1 | ||
#[derive(Serialize, Deserialize, Debug)] | ||
pub enum AttestationKey { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would much prefer this to be named after the serialization target, given that COSE_Key is also a potential format that I'm considering. JWE doesn't have the content encryption key agreement algorithm support I want: ECDH+HKDF-SHA-256 to derive A256KW. Haven't bothered writing the registration request to https://www.iana.org/assignments/jose/jose.xhtml for COSE parity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just so I understand correctly, you mean something like:
pub enum AttestationKey {
Jwk(JwkKey),
Cose(CoseKey)
}
pub enum JwkKey {
RSA { ... }
EC { ... }
}
pub enum CoseKey {
...
}
Which essentially allows for different serialization formats for the TEE key. Do I understand this right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's what I meant, though the more I look at this, the less I'm convinced the organization will solve the problem I see myself having with this. So, disregard the encoding comment, since that's just a matter of how you've implemented the KbsProtocol. However there is still a category error calling this an attestation key. It's a key used for key exchange, not attesting anything. Your AttestationRequest has an implicit binding method associated with the the key variant for key exchange. The attestation key signed the evidence.
So, AttestationRequest is more AttestedClientKeyExchange, and the evidence detached from the key seems wrong to me, since the binding method would be driven by the key exchange method. All you have support for is a KbsKeyExchange mechanism.
Crypto wonk if interested
I don't think that the KBS architecture is particularly scalable for a key management service to store an encryption key per VM, so it makes more sense to me for the KBS to store a KEK that's sealed by an HSM key, so the KEK can be used by the same user for any number of a stamped template of VMs. I don't think this JWE use is appropriate for communicating keys, either. It's much better suited for secret management than key management, since your content encryption key is standard AES-GCM instead of a key wrapping algorithm like AES-256-KW (A256KW).
When you restrict to keys, you can persist a wrapped key with the rest of an instance's metadata and not have to burden the key management service with VM data. The KBS becomes responsible for instead authorizing an HSM to rewrap a given blob in a COSE_Encrypt0, where the key agreement layers are
Layer 0: The NVDATA encryption key KEK encrypted by A256KW
Layer 1: The A256KW key encrypted by AES-256 seeded by a shared secret and HKDF salt
Layer 2: The ECDH public key from SVSM and an ephemeral ECDH public key from the HSM that combine with ECDH-SS, that mix with HKDF-SHA256 and given salt parameter.
We can leave the ECDH HSM private key generation to the attestation verification service, but the public key recipient information would be in the result. Given that the only state the HSM would have is its root secret for unwrapping objects, we would need to send along the wrapped KEK and the HSM wrap key used to wrap the KEK. It's a lot of stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's what I meant, though the more I look at this, the less I'm convinced the organization will solve the problem I see myself having with this. So, disregard the encoding comment, since that's just a matter of how you've implemented the KbsProtocol. However there is still a category error calling this an attestation key. It's a key used for key exchange, not attesting anything.
Right, perhaps another name is warranted. It's known as the "TEE public key" in KBS documentation, but I don't like that very much as I think it's easy to confuse that with the public key that's used to verify evidence signatures (for instance, the VCEK in SEV-SNP).
Perhaps "Secret Integrity Key"? It's main purpose is to encrypt secrets sent back to the client after all.
Your AttestationRequest has an implicit binding method associated with the the key variant for key exchange. The attestation key signed the evidence.
Bit confused here, the key generated by SVSM (known as AttestationKey
here) doesn't sign the evidence, but is hashed into the REPORT_DATA (that is, if NegotiationParam::TeeKeyPublicComponents
is found in negotiation params).
So, AttestationRequest is more AttestedClientKeyExchange, and the evidence detached from the key seems wrong to me, since the binding method would be driven by the key exchange method. All you have support for is a KbsKeyExchange mechanism.
Are you suggesting we de-couple the key exchange from the sending of attestation evidence to support other protocols? I'm not opposed to that if you feel it would be more flexible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit confused here, the key generated by SVSM (known as AttestationKey here) doesn't sign the evidence, but is hashed into the REPORT_DATA (that is, if NegotiationParam::TeeKeyPublicComponents is found in negotiation params).
Ah I see what I said was ambiguous. I just meant "attestation key" is a role of a key that attests to something, like signing evidence. That's why I think this naming is a category error. It's a key meant for key exchange, and yes that's bound to the attestation with REPORT_DATA.
Are you suggesting we de-couple the key exchange from the sending of attestation evidence to support other protocols? I'm not opposed to that if you feel it would be more flexible.
Not in terms of adding a third method to Protocol, but just that the {evidence string + enum type} where the enum choice changes the interpretation of evidence just is aesthetically displeasing to me, but since it's factoring out an expected component from all key exchange methods, I suppose it's fine. The enum carries with it two design choices: a binding method with key representation, and the key exchange method itself. The former is more about getting authorization from the attestation verifier, and the latter is more about how to get key information from a key management service.
kernel/src/attest.rs
Outdated
n: BASE64_STANDARD.encode(public.n().to_bytes_be()), | ||
e: BASE64_STANDARD.encode(public.e().to_bytes_be()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JWK uses base64url encoding. Why change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a mistake, and I've changed to use base64url. However, the evidence itself is still serialized with base64 standard. Would it make more sense to encode everything with base64url for uniformity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would, though I would still say that the variant name should indicate which Base64 encoding it means.
impl HttpClient { | ||
pub fn new(url: String, protocol: Protocol) -> anyhow::Result<Self> { | ||
let cli = Client::builder() | ||
.cookie_provider(Arc::new(Jar::default())) | ||
.build() | ||
.context("unable to build HTTP client to interact with attestation server")?; | ||
|
||
Ok(Self { cli, url, protocol }) | ||
} | ||
|
||
pub fn negotiation(&mut self, req: NegotiationRequest) -> anyhow::Result<NegotiationResponse> { | ||
// Depending on the underlying protocol of the attestation server, gather negotiation | ||
// parameters accordingly. | ||
match self.protocol { | ||
Protocol::Kbs(mut kbs) => kbs.negotiation(self, req), | ||
} | ||
} | ||
|
||
pub fn attestation(&self, req: AttestationRequest) -> anyhow::Result<AttestationResponse> { | ||
// Depending on the underlying protocol of the attestation server, attest TEE evidence | ||
// accoridngly. | ||
match self.protocol { | ||
Protocol::Kbs(kbs) => kbs.attestation(self, req), | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a registration pattern could help decouple protocol providers from the main logic of running the generic protocol, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate on "registration pattern"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have a registrar class with a static map and static Register method. You use static initialization in the implementation of a register macro to create a class and top level instance of the class whose static initialization calls the registrar's Register method with the instance of the registrant.
This decouples your support for a protocol from a set of enums that disperse their variant handling amongst dispatching methods.
@berrange I'm not sure yet, I haven't looked in much detail besides seeing some other issues in the rust RSA crate discussing it.
@tlendacky I have, and my tests suggest RdRand is about 100% slower than RdSeed (~90 seconds for creation+decryption on a 3072-bit RSA key). |
faf75aa
to
39727d1
Compare
They're saying RustCrypto is x3 slower than native, but that still doesn't feel like it is a root cause here. That bug reports says generating 100 keys takes 18 seconds. If we need 1 key, that's on the order of 0.2 secs to generate. The actual encryption/decryption operations shouldn't be even measurable given the small size of the data we're processing. So I'm surprised anything crypto related can add up to as much as a 1 minute - that's orders of magnitude slower than it ought to be.
That's surprising, as I wouldn't have thought either of those two should be "hot path" calls. There's something altogether strange going on here to make this so slow. |
For a universal interface when reading or writing multiple bytes over a transport channel, introduce generic Read/Write traits. Implement these traits over the serial::SerialPort for use in the attestation driver. This is an attempt to mimic the std::io::{Read, Write} traits. Co-developed-by: Stefano Garzarella <[email protected]> Signed-off-by: Tyler Fanelli <[email protected]>
Signed-off-by: Tyler Fanelli <[email protected]>
The attestation module provides services early attestation and measurement of the SVSM environment. This commits initializes the attestation driver and sends an initial negotiation request to the attestation proxy. Co-developed-by: Stefano Garzarella <[email protected]> Signed-off-by: Tyler Fanelli <[email protected]>
To validate the freshness of attestation evidence, KBS servers return a challenge upon client authentication. This challenge contains a ephemeral nonce in the form of base64-encoded bytes. This nonce *must* be hashed into the attestation report along with the components of the TEE public key. This commit reads the negotiation request from SVSM, and depending on the backend, fetches the negotiation parameters accordingly. As KBS is the backend used in the initial implementation, this translates into a call to the KBS /auth handler to fetch the nonce, and then a specification of both the nonce and TEE public components to be included in the attestation evidence (in the form of negotiation parameters). Co-developed-by: Stefano Garzarella <[email protected]> Signed-off-by: Tyler Fanelli <[email protected]>
Given the negotiation parameters from the proxy, SVSM can now hash the parameters and embed them in the attestation evidence. It can also read the negotiation key specified and create key accordingly. This commit required increasing the amount of stack pages by 4 (from 8 pages to 12 pages) as creating a new RSA private key (via rsa::RsaPrivateKey::new) caused a stack overflow. As SEV-SNP is the only supported TEE architecture at the moment, there's also a few changes required to serialize a SEV-SNP attestation report into bytes. Co-developed-by: Stefano Garzarella <[email protected]> Signed-off-by: Tyler Fanelli <[email protected]>
With serialized TEE evidence and key, the proxy can now transfer these to the attestation server for evaluation. KBS will first transfer the evidence to the /attest endpoint for evaluation. Upon successful attestation, the client can then retrieve resources from the server. The proxy attempts to retrieve a resource identified as "svsm_secret". If all is successful, an attestation response indicating success and containing the encrypted secret is written back to SVSM for decryption. NOTE: The proxy hard-codes the fetchable resource value as "svsm_secret". KBS servers will likely need to configure their RVPS to name this resource accordingly. Co-developed-by: Stefano Garzarella <[email protected]> Signed-off-by: Tyler Fanelli <[email protected]>
Use the TEE key to decrypt the secret received from the proxy. Co-developed-by: Stefano Garzarella <[email protected]> Signed-off-by: Tyler Fanelli <[email protected]>
Introduce error handling in the attestation module. Instead of unwrapping and panicking, wrap errors in their own enum. The only public attestation method, AttestationDriver::attest(), wraps the AttestationError into a SvsmError::Attestation to follow with other error implementations. Signed-off-by: Tyler Fanelli <[email protected]>
The `attest` feature allows for SVSM to use the attestation driver to verify its launch evidence. Signed-off-by: Tyler Fanelli <[email protected]>
At present, there are performance limitations when using the Rust RSA crate with no_std. There is also a desire for EC key encryption/decryption. As such, replace the usage of RSA keys with EC keys. The initial implementation uses ECDH384+SHA256+AES128 symmetric encryption. Signed-off-by: Tyler Fanelli <[email protected]>
From advice during the weekly call, I've dropped support for RSA keys (at least for now) and replaced with EC keys. See 5dc6dac for more details and review. The initial implementation uses ECDH384+SHA256+AES128 (no salt) encryption. The previous performance issues seen from RSA keys have been resolved by this. To support this, changes were made to the testing attestation server ( Furthermore, I listed one of the goals before this can be supported upstream as: Ensure that aproxy's KBS backend works with production KBS servers such as Trustee. Although in future plans, none of the servers support EC keys yet. Therefore, this goal will be put on TODO and the proxy can be marked experimental until this support is available. I've also added an There still exists 56ccb1b which simply prints a secret for the demo. Once this is approved, I can remove that commit. |
This series introduces the attestation module in SVSM as well as the attestation proxy to facilitate communication between SVSM and a remote server for TEE evidence evaluation.
Abstract
To unlock early persistent state in SVSM (for example, persistent vTPM data), an attestation of the launch environment must take place to ensure that SVSM is running on trusted hardware and launched correctly. As SVSM does not have a network stack available, we have opted to introduce a proxy that would run on the host (for now, although this may change as other options are explored) and facilitate attestation between SVSM and a remote verification server such as KBS.
Much of what is implemented here is described in the Early Attestation and Measurement Architecture in COCONUT SVSM document.
Changes
There are three main modules introduced into SVSM with this series:
kernel/attest.rs
: Attestation driver that communicates with the proxy through the COM3 serial port. This will eventually communicate over another channel (virtio-vsock, etc..) when available.aproxy
: Attestation proxy that runs on a host and facilitates communication with a remote attestation server.libaproxy
: Shared types betweenkernel/attest.rs
andaproxy
for (de)serialization of data.Attestation
There exists two different phases of attestation:
As there exists multiple protocols for TEE attestation, the proxy was built to be configurable to different protocols. As such, SVSM can be completely agnostic of the attestation protocol used. This is done by splitting the proxy up into two main components:
Front-end: To keep SVSM agnostic to the underlying attestation protocol, we define a standard front-end interface between SVSM and the proxy, the types of this interface for both negotiation and attestation are defined in the
libaproxy
crate. In the negotiation phase, SVSM will write aNegotiationRequest
and receive aNegotiationResponse
from the proxy. In the attestation phase, SVSM will write aAttestationRequest
and receive aAttestationResponse
from the proxy. This allows SVSM to not be tied to a specific attestation protocol, leaving the possibility for new protocols to be enabled in the future (for example, aTLS).Back-end: This implements the specific attestation protocol that the communicating server implements. It is configurable with the
--backend
argument when launching the proxy. The initial backend option allowed is KBS, but the proxy can be configured to implement something like aTLS support if desired.Try for yourself
To try for yourself, I've set up a quick demo with a "dummy" KBS server that requires no configuration and simply returns a secret:
hello, SVSM!
that SVSM can then print. This requires a SEV-SNP machine with a SVSM-enabled kernel.This will run a "KBS server" (at least suitable for testing) at http://0.0.0.0:8080.
This runs the proxy with the following specified in the arguments:
--url http://0.0.0.0:8080
: The attestation server is running at http://0.0.0.0:8080.--protocol kbs
: The attestation server communicates via the KBS protocol, configure the backend to use the KBS protocol.--unix /tmp/svsm-proxy.sock
: Listen for messages from SVSM on a socket created in file/tmp/svsm-proxy-sock
.--force
: Remove the/tmp/svsm-proxy.sock
file (if it already exists) before creating the socket.Initially, SVSM communicates over the COM3 serial port. The attestation proxy socket will need to be available in the correct
-serial
port argument position to ensure it communicates with the right socket.NOTE
Upon running, you may think that there is a hang in the proxy, as SVSM seems to freeze:
This is not a hang, but rather, the creation of a 3072-bit RSA (as well as decryption with this key) is very slow at present (sometimes taking around ~1 minute for creation+decryption in my tests). Eventually, SVSM should begin running again and show the following message:
[SVSM] Decrypted secret from attestation server: hello, SVSM!
And thus we have decrypted secrets from a KBS attestation server (via the proxy).
TODO
The only purpose of b192792 is for others to see a secret decrypted from the server available and printed by SVSM. This must be removed once others have tried for themselves.
Other TODOs before this can be considered for merging:
kernel/attest
module behind a feature flag in thekernel
module. Perhaps a feature calledattest
?Makefile
to build thekernel
module with theattest
feature.Future work:
Co-developed-by: Stefano Garzarella [email protected]
Signed-off-by: Tyler Fanelli [email protected]
cc/ @stefano-garzarella @joergroedel @deeglaze @berrange @fitzthum @stringlytyped @IT302 @Isaac-Matthews