Implement simple remote attestation for the UEFI app #2742

jul-sh · 2022-04-21T13:08:50Z

rebased and ready for review. Implements attestation in the UEFI app side, adding the client side in a separate PR.

jul-sh · 2022-04-21T13:09:40Z

experimental/uefi/app/src/remote_attestation.rs

+//! A simplified version of the implementation from the `grpc_unary_attestation`
+//! crate. TODO(#2741): Refactor this to share more code between the two runtimes.


Will do some of that before opening this PR, to simplify review, ref #2741

done in #2743

Leaving this TODO in as some additional refactoring can probably be done with the AttestationHandler here, which mirrors the AttestationServer in the grpc attestation crate. — But redundancy is fairly small, so we can move ahead with this PR for now.

jul-sh · 2022-04-22T10:39:07Z

experimental/uefi/app/src/main.rs

+const MOCK_SESSION_ID: [u8; 8] = [0; 8];
+


Mock for now as we only have a single proof of concept client. Will swap this out once I amend the client code to support attestation

conradgrobler

Thanks

experimental/uefi/app/Cargo.toml

conradgrobler · 2022-04-22T10:34:40Z

experimental/uefi/app/src/main.rs

 //    * `index` - index of the serial port, zero-based.
 //
 //  Normally does not return, unless an error is raised.
 fn serial_echo(handle: Handle, bt: &BootServices, index: usize) -> Result<!, uefi::Error<()>> {


Looks like this will conflict with #2745. It is probably a good idea to wait for that to be merged and then rebase on top of it.

conradgrobler · 2022-04-22T10:38:18Z

experimental/uefi/app/src/remote_attestation.rs

+const MOCK_TEE_CERTIFICATE: [u8; 0] = [];
+const MOCK_ADDITIONAL_INFO: [u8; 0] = [];
+
+impl<F> AttestationHandler<F>


This feels like this is more generic functionality, and could be reused for different scenarios. It might be a good idea to move it somewhere more reusable (either remote_attestation_sessions (my preference) or the new runtime crate from #2745, or even yet another crate (although that is probably overkill)).

As you mentioned in #2742 (comment) this can be done later as part of #2471

conradgrobler · 2022-04-22T10:41:46Z

experimental/uefi/app/src/remote_attestation.rs

+        }
+    }
+
+    pub fn message(&mut self, session_id: SessionId, request: Vec<u8>) -> Vec<u8> {


I think this should return a Result<Vec<u8>> rather than panic. In the current Oak functions loader Tonic has a handler for request-related tasks that panic, but I think we should not panic anywhere in code for UEFI/bare-metal runtimes unless it is an unrecoverable fatal error that should kill the server.

Agreed. I left it like this for the initial remote attestation proof of concept only, but will swap it out for proper error handling by updating this PR.

Updated this PR with basic error handling inside the remote attestation module. However if an error occurs it'll still panic inside the main function of the UEFI app which unwraps any Result of the serial_echo function. Now that's "failed to read message from serial port", "failed remote attestation", "failed to write message to serial port".

We probably don't actually want to panic for "failed remote attestation", and possibly also not for "failed to read message from serial port". Instead we could send an error status code to the companion app, keeping the UEFI app running + serial port open.

I'll open a separate PR for implementing that bit, as it seems best to discuss that in isolation. Probs more related to the general UEFI app <-> loader comms protocol question than to remote attestation specifically.

cc @andrisaar

experimental/uefi/app/src/remote_attestation.rs

…2741

remove unused deps format oak-prefix crate name for consistency fix typo

andrisaar · 2022-04-22T14:35:42Z

experimental/uefi/loader/src/qemu.rs

+        // Needed to expose advanced CPU features to the UEFI app. Specifically
+        // RDRAND which is required for remote attestation.
+        cmd.arg("-enable-kvm");
+        cmd.args(&["-cpu", "Broadwell-IBRS"]);


Any particular reason for Broadwell-IBRS? Granted, I don't know what the default CPU that qemu emulates is, but if you want something with RDRAND I think you can explicitly ask for that with +rdrand, something like "-cpu host,+rdrand". I think explicitly listing that we require rdrand is better than hiding it in the CPU definition.

That being said, qemu docs are not the best, so please do check the syntax for the -cpu flag to see how it works.

Any particular reason for Broadwell-IBRS?

No strong reason for the particular model, other than it supports RDRAND while being old enough that it can hopefully be virtualized by most hosts. This particular line just matches what we went with in #2703.

In general we do want to specify a specific model. The reason is that in virtualization mode, qemu does not use a default CPU. Instead it's either host passthrough (unstable between hosts, not recommended by docs) or a named model. :)

See https://qemu-project.gitlab.io/qemu/system/qemu-cpu-models.html#two-ways-to-configure-cpu-models-with-qemu-kvm for more details

andrisaar · 2022-04-22T14:37:07Z

experimental/uefi/app/src/remote_attestation.rs

+where
+    F: Send + Sync + Clone + FnOnce(Vec<u8>) -> Vec<u8>,
+{
+    pub fn create(request_handler: F) -> Self {


isn't the normal Rust convention to call these constructor methods new?

Seems like there is a loose convention towards new indeed, https://doc.rust-lang.org/nomicon/constructors.html mentions it.

In our codebase we tend to use both create and new in different places , agreed that settling on a convention would be nice though probs out of scope for this. new does seem like a good choice. I choose create here to be consistent with the rest of the remote attestation related structs in the codebase.

jul-sh · 2022-04-22T16:06:10Z

@andrisaar Could you take a look at the updated version of this PR? (post merging #2745)

conradgrobler

Nice.

experimental/uefi/runtime/src/remote_attestation.rs

andrisaar

Nice, no changes to the uefi app itself!

jul-sh commented Apr 21, 2022

View reviewed changes

jul-sh force-pushed the handshake branch 2 times, most recently from 5cdc3d3 to 98e9f9d Compare April 21, 2022 17:03

jul-sh mentioned this pull request Apr 21, 2022

Move Remote Attestation Session Tracking logic into its own crate #2743

Merged

jul-sh force-pushed the handshake branch 4 times, most recently from b5538e0 to e36dbfd Compare April 22, 2022 10:07

jul-sh marked this pull request as ready for review April 22, 2022 10:16

jul-sh requested a review from a team as a code owner April 22, 2022 10:16

jul-sh requested review from andrisaar and conradgrobler and removed request for a team April 22, 2022 10:16

jul-sh commented Apr 22, 2022

View reviewed changes

conradgrobler reviewed Apr 22, 2022

View reviewed changes

jul-sh mentioned this pull request Apr 22, 2022

Refactoring: Improve naming of SessionsTracker method #2751

Merged

conradgrobler approved these changes Apr 22, 2022

View reviewed changes

Juliette Pretot added 6 commits April 22, 2022 14:20

Extract session logic for remote attestion, GH issue ref project-oak#…

f7894b0

…2741

Move sessions logic into its own crate

211d1ef

remove unused deps format oak-prefix crate name for consistency fix typo

Implement simple remote attestion for the UEFI app

3a39d44

remove unused dep

a314564

Update lockfile

03ca895

Shutdown with UEFI error code if a remote attestation error occurs

ef7cd63

jul-sh force-pushed the handshake branch from b4163d8 to ef7cd63 Compare April 22, 2022 13:20

Update method call to match project-oak#2751

cc8c239

jul-sh added the restricted kernel label Apr 22, 2022

andrisaar reviewed Apr 22, 2022

View reviewed changes

andrisaar approved these changes Apr 22, 2022

View reviewed changes

Merge branch 'main' into handshake

7d4778f

jul-sh requested a review from andrisaar April 22, 2022 16:05

conradgrobler approved these changes Apr 22, 2022

View reviewed changes

experimental/uefi/runtime/src/remote_attestation.rs Outdated Show resolved Hide resolved

Remove erroneous mention of gRPC

03c977a

andrisaar approved these changes Apr 22, 2022

View reviewed changes

tiziano88 changed the title ~~Implement simple remote attestion for the UEFI app~~ Implement simple remote attestation for the UEFI app Apr 22, 2022

jul-sh merged commit 16aad75 into project-oak:main Apr 22, 2022

jul-sh deleted the handshake branch April 22, 2022 16:45

jul-sh added this to the Bare-metal communication channel milestone Jul 7, 2022

jul-sh added the remote-attestation label Jul 8, 2022

		//! A simplified version of the implementation from the `grpc_unary_attestation`
		//! crate. TODO(#2741): Refactor this to share more code between the two runtimes.

Implement simple remote attestation for the UEFI app #2742

Implement simple remote attestation for the UEFI app #2742

Uh oh!

Conversation

jul-sh commented Apr 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jul-sh Apr 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

conradgrobler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jul-sh Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jul-sh Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jul-sh Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jul-sh commented Apr 22, 2022

Uh oh!

conradgrobler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andrisaar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jul-sh commented Apr 21, 2022 •

edited

Loading

jul-sh Apr 21, 2022 •

edited

Loading

jul-sh Apr 22, 2022 •

edited

Loading

jul-sh Apr 22, 2022 •

edited

Loading

jul-sh Apr 22, 2022 •

edited

Loading