Skip to content
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

do-not-merge: runtime: agent: verify the agent policy hash #8469

Closed
Closed
Show file tree
Hide file tree
Changes from all 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
112 changes: 112 additions & 0 deletions src/agent/Cargo.lock

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

7 changes: 6 additions & 1 deletion src/agent/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,11 @@ reqwest = { version = "0.11.14", optional = true }
# The "vendored" feature for openssl is required for musl build
openssl = { version = "0.10.54", features = ["vendored"], optional = true }

# Policy validation
sha2 = { version = "0.10.6", optional = true }
sev = { version = "2.0.2", default-features = false, features = ["snp"], optional = true }
vmm-sys-util = { version = "0.11.0", optional = true }

[dev-dependencies]
tempfile = "3.1.0"
test-utils = { path = "../libs/test-utils" }
Expand All @@ -89,7 +94,7 @@ lto = true
[features]
seccomp = ["rustjail/seccomp"]
standard-oci-runtime = ["rustjail/standard-oci-runtime"]
agent-policy = ["http", "openssl", "reqwest"]
agent-policy = ["http", "openssl", "reqwest", "sev", "sha2", "vmm-sys-util"]

[[bin]]
name = "kata-agent"
Expand Down
4 changes: 4 additions & 0 deletions src/agent/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@ mod tracer;

#[cfg(feature = "agent-policy")]
mod policy;
#[cfg(feature = "agent-policy")]
mod sev;
#[cfg(feature = "agent-policy")]
mod tdx;

cfg_if! {
if #[cfg(target_arch = "s390x")] {
Expand Down
67 changes: 67 additions & 0 deletions src/agent/src/policy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,14 @@
use anyhow::{bail, Result};
use protobuf::MessageDyn;
use serde::{Deserialize, Serialize};
use sha2::{Digest, Sha256, Sha384};
use slog::Drain;
use tokio::io::AsyncWriteExt;
use tokio::time::{sleep, Duration};

use crate::rpc::ttrpc_error;
use crate::sev::get_snp_host_data;
use crate::tdx::get_tdx_mrconfigid;
use crate::AGENT_POLICY;

static EMPTY_JSON_INPUT: &str = "{\"input\":{}}";
Expand Down Expand Up @@ -176,6 +179,8 @@ impl AgentPolicy {

/// Replace the Policy in OPA.
pub async fn set_policy(&mut self, policy: &str) -> Result<()> {
verify_policy_digest(policy)?;

if let Some(opa_client) = &mut self.opa_client {
// Delete the old rules.
opa_client.delete(&self.policy_path).send().await?;
Expand Down Expand Up @@ -297,3 +302,65 @@ fn start_opa(opa_addr: &str) -> Result<()> {
}
bail!("OPA binary not found in {:?}", &bin_dirs);
}

fn verify_policy_digest(policy: &str) -> Result<()> {
// TODO: check if the user configured this Guest VM as using either SNP or TDX,
// and if that's the case return an error if the TEE attestation report didn't
// provide a policy digest value.

if let Ok(expected_digest) = get_tdx_mrconfigid() {
info!(sl!(), "policy: TDX expected digest ({:?})", expected_digest);
verify_sha_384(policy, expected_digest.as_slice())
} else if let Ok(expected_digest) = get_snp_host_data() {
info!(sl!(), "policy: SNP expected digest ({:?})", expected_digest);
// TODO: fix the SNP CI machines and then don't ignore zeroes anymore.
let ignore_zero_digest = true;
verify_sha_256(policy, expected_digest.as_slice(), ignore_zero_digest)
} else {
warn!(sl!(), "policy: integrity has not been verified!");
Ok(())
}
}

fn verify_sha_384(policy: &str, expected_digest: &[u8]) -> Result<()> {
let mut hasher = Sha384::new();
hasher.update(policy.as_bytes());
let digest = hasher.finalize();
info!(sl!(), "policy: calculated digest ({:?})", digest);

if expected_digest != digest.as_slice() {
bail!(
"policy: rejecting unexpected digest ({:?}), expected ({:?})",
digest.as_slice(),
expected_digest
);
}

Ok(())
}

pub fn verify_sha_256(
policy: &str,
expected_digest: &[u8],
ignore_zero_digest: bool,
) -> Result<()> {
let mut hasher = Sha256::new();
hasher.update(policy.as_bytes());
let digest = hasher.finalize();
info!(sl!(), "policy: calculated digest ({:?})", digest);

if expected_digest != digest.as_slice() {
if ignore_zero_digest && !expected_digest.iter().any(|&x| x != 0) {
error!(sl!(), "Temporarily ignoring incorrect SNP Host Data field");
return Ok(());
}

bail!(
"policy: rejecting unexpected digest ({:?}), expected ({:?})",
digest,
expected_digest
);
}

Ok(())
}
19 changes: 19 additions & 0 deletions src/agent/src/sev.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Copyright (c) 2023 Microsoft Corporation
//
// SPDX-License-Identifier: Apache-2.0
//

use anyhow::Result;

pub fn get_snp_host_data() -> Result<Vec<u8>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Get hostdata field in kata-agent seems arch-specific. I'd suggest to do this in AA side. As it already has the ability to get hardware evidence/quote. TDX is the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we should force users to use the CoCo AA. Users should be able to perform remote attestation on their own if they prefer to - based on the arch specific docs.

However, sharing a rust crate between the Kata Agent and AA would sound reasonable to me - not sure if this is what you meant.

If you meant adding some protocol for Policy validation between the Kata Agent and AA, please describe the details when we'll meet. For example, we'd need that kind of protocol to succeed before executing many other Kata Agent API calls - otherwise a not-yet-validated Policy might allow dangerous actions.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we should try to make kata-agent arch-independent. Some reasons:

  1. CoCoAA has already had the ability of remote attestation related things, so we should try to leverage them rather than re-invention. As you said, users should be able to perform remote attestation on their own if they prefer to. We are refactoring AA to support only have attestation related functionalities like GetAttestationToken and GetEvidence. This means that not only KBS/CoCo-AS can be accessed, but also MAA/Intel TA and other ASes/ways could implement the specific plugins. If we insist to implement the logic inside kata-agent, there will be two copies of the code.
  2. If we want to check the hostdata field, it hints that we already enable the confidential computing feature. Furthermore confidential computing feature means that CoCo-AA will be built-in.

match sev::firmware::guest::Firmware::open() {
Ok(mut firmware) => {
let report_data: [u8; 64] = [0; 64];
match firmware.get_report(None, Some(report_data), Some(0)) {
Ok(report) => Ok(report.host_data.to_vec()),
Err(e) => Err(e.into()),
}
}
Err(e) => Err(e.into()),
}
}
Loading
Loading