Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions experimental/uefi/app/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions experimental/uefi/app/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ log = { version = "*" }
ciborium = { version = "*", default-features = false }
ciborium-io = "*"
oak_remote_attestation = { path = "../../../remote_attestation/rust" }
oak_remote_attestation_sessions = { path = "../../../remote_attestation_sessions" }
anyhow = { version = "*", default-features = false }

[dev-dependencies]
anyhow = { version = "*", default-features = false }
Expand Down
13 changes: 12 additions & 1 deletion experimental/uefi/app/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,11 @@
extern crate log;
extern crate alloc;

use crate::remote_attestation::AttestationHandler;
use ciborium::{de, ser};
use uefi::{prelude::*, table::runtime::ResetType};

mod remote_attestation;
mod serial;

// The main entry point of the UEFI application.
Expand Down Expand Up @@ -71,6 +73,8 @@ fn main(handle: Handle, system_table: &mut SystemTable<Boot>) -> Status {
serial_echo(handle, system_table.boot_services(), ECHO_SERIAL_PORT_INDEX).unwrap();
}

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Opens the index-th serial port on the system and echoes back all frames sent over the serial
// port.
//
Expand All @@ -85,6 +89,7 @@ fn main(handle: Handle, system_table: &mut SystemTable<Boot>) -> Status {
// Normally does not return, unless an error is raised.
fn serial_echo(handle: Handle, bt: &BootServices, index: usize) -> Result<!, uefi::Error<()>> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sgtm

let mut serial = serial::Serial::get(handle, bt, index)?;
let attestation_handler = &mut AttestationHandler::create(|v| v);
loop {
let msg: alloc::vec::Vec<u8> = de::from_reader(&mut serial).map_err(|err| match err {
de::Error::Io(err) => err,
Expand All @@ -98,7 +103,13 @@ fn serial_echo(handle: Handle, bt: &BootServices, index: usize) -> Result<!, uef
}
de::Error::RecursionLimitExceeded => uefi::Error::from(Status::ABORTED),
})?;
ser::into_writer(&msg, &mut serial).map_err(|err| match err {
let response = attestation_handler
.message(MOCK_SESSION_ID, msg)
.map_err(|err| {
error!("Error handling remote attestation: {:?}", err);
uefi::Error::from(Status::PROTOCOL_ERROR)
})?;
ser::into_writer(&response, &mut serial).map_err(|err| match err {
ser::Error::Io(err) => err,
ser::Error::Value(msg) => {
error!("Error serializing value: {}", msg);
Expand Down
92 changes: 92 additions & 0 deletions experimental/uefi/app/src/remote_attestation.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
//
// Copyright 2022 The Project Oak Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//

//! Server-side implementation of the bidirectional gRPC remote attestation handshake
//! protocol.
//!
//! A simplified version of the implementation from the `grpc_unary_attestation`
//! crate. TODO(#2741): Refactor this to share more code between the two runtimes.
Copy link
Author

@jul-sh jul-sh Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in #2743

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

use alloc::vec::Vec;
use anyhow::Context;
use oak_remote_attestation_sessions::{SessionId, SessionState, SessionTracker};

/// Number of sessions that will be kept in memory.
const SESSIONS_CACHE_SIZE: usize = 10000;

pub struct AttestationHandler<F> {
session_tracker: SessionTracker,
request_handler: F,
}

const MOCK_TEE_CERTIFICATE: [u8; 0] = [];
const MOCK_ADDITIONAL_INFO: [u8; 0] = [];

impl<F> AttestationHandler<F>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@jul-sh jul-sh Apr 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let session_tracker = SessionTracker::create(
SESSIONS_CACHE_SIZE,
MOCK_TEE_CERTIFICATE.to_vec(),
MOCK_ADDITIONAL_INFO.to_vec(),
);

Self {
session_tracker,
request_handler,
}
}

pub fn message(&mut self, session_id: SessionId, request: Vec<u8>) -> anyhow::Result<Vec<u8>> {
let mut session_state = {
self.session_tracker
.pop_or_create_session_state(session_id)
.expect("Couldn't pop session state")
};
let response_body = match session_state {
SessionState::HandshakeInProgress(ref mut handshaker) => {
handshaker
.next_step(&request)
.context("Couldn't process handshake message")?
// After receiving a valid `ClientIdentity` message
// (the last step of the key exchange)
// ServerHandshaker.next_step returns `None`. For unary
// request we do want to send an explicit confirmation in
// the form of a status message. Hence in case of `None`
// fallback to a default (empty) response.
.unwrap_or_default()
}
SessionState::EncryptedMessageExchange(ref mut encryptor) => {
let decrypted_request = encryptor
.decrypt(&request)
.context("Couldn't decrypt response")?;

let response = (self.request_handler.clone())(decrypted_request);

encryptor
.encrypt(&response)
.context("Couldn't encrypt response")?
}
};

self.session_tracker
.put_session_state(session_id, session_state);

Ok(response_body)
}
}
4 changes: 4 additions & 0 deletions experimental/uefi/loader/src/qemu.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@ impl Qemu {
// Construct the command-line arguments for `qemu`. See
// https://www.qemu.org/docs/master/system/invocation.html for all available options.

// 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"]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@jul-sh jul-sh Apr 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// We're going to run qemu as a noninteractive embedded program, so disable any
// graphical outputs.
cmd.arg("-nographic");
Expand Down