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

verifier: Use the VirTEE CA Chain #446

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

larrydewey
Copy link
Contributor

This introduces changes into the code to utilize the cert chain provided by the VirTEE sev crate, pushing resposibility for upkeep and maintainership back to the library itself. This also reduces duplication of code and expands functionality for future changes.

@larrydewey
Copy link
Contributor Author

/cc @fitzthum @DGonzalezVillal

@larrydewey
Copy link
Contributor Author

Waiting on virtee/sev#206

@larrydewey larrydewey force-pushed the cleanup branch 2 times, most recently from 0e781eb to cba1195 Compare July 29, 2024 15:41
Copy link
Member

@ryansavino ryansavino left a comment

Choose a reason for hiding this comment

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

LGTM

@larrydewey larrydewey changed the title WIP: verifier: Use the VirTEE CA Chain verifier: Use the VirTEE CA Chain Aug 8, 2024
@larrydewey larrydewey marked this pull request as ready for review August 8, 2024 17:00
Copy link
Member

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

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

CI not looking great.

There are some formatting changes here. I'm not sure they are necessary, but if they are consider moving them to another commit.

cc: @mkulke

let certs = X509::stack_from_pem(include_bytes!("milan_ask_ark_asvk.pem"))?;
if certs.len() != 3 {
bail!("Malformed Milan ASK/ARK/ASVK");
fn read_certs_from_file() -> Vec<X509> {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure of the value of splitting this into two functions. This new one should still refer to milan since it is only loading the milan certs. Also this isn't exactly reading them from a file.

Copy link
Contributor Author

@larrydewey larrydewey Aug 12, 2024

Choose a reason for hiding this comment

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

I am splitting this into two functions because the former behavior of utilizing a OnceLock around a Result leaves room for unexpected behavior, as it returns a reference to a Result. I suppose I could re-implement this using a lazy_static! to ensure it is only allocated once, and then use a Mutex to make sure access is serial.

Ultimately, I was just trying to get rid of the UB.

Copy link
Contributor

Choose a reason for hiding this comment

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

UB often means "undefined behaviour", which should not happen in safe rust. You are referring to "unexpected behaviour". Can you elaborate what unexpected behaviour you are concerned about?

Afaiu OnceLock/OnceCell/lazy_static! should do mostly the same, with the Once* family being the more modern alternative to the lazy_static macro.

ark: Certificate::from(certs[1].clone()),
ask: Certificate::from(certs[0].clone()),
},
vek: Certificate::from(certs[2].clone()),
Copy link
Member

Choose a reason for hiding this comment

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

I thought this was the asvk. Has the term changed? If so, please remove references to asvk. I thought vek already referred to the guest encryption key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, the ASK and the VEK are generic terms. The AMD Signing Key (ASK) covers both the traditional ASK and the ASVK. The Versioned Endorsement Key (VEK) covers VCEK or VLEK.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't certs[2] going to be the ASVK? That shouldn't go into the VEK field. The current VendorCertificates struct holds both the ASK and ASVK. It doesn't like the CaChain struct does that so you might need to tweak the logic a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are absolutely correct. I misunderstood your comment. I will have to think about this, as a certificate chain usually should contain one or the other, not both.

Cargo.toml Outdated Show resolved Hide resolved
deps/verifier/Cargo.toml Outdated Show resolved Hide resolved
base64 = "0.21"
bincode = "1.3.3"
byteorder = "1"
cfg-if = "1.0.0"
codicon = { version = "3.0", optional = true }
# TODO: change it to "0.1", once released.
csv-rs = { git = "https://github.com/openanolis/csv-rs", rev = "b74aa8c", optional = true }
csv-rs = { version = "=0.1.0", git = "https://github.com/openanolis/csv-rs", rev = "b74aa8c", optional = true }
Copy link
Contributor

Choose a reason for hiding this comment

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

is this intentionally both version and ref?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure if it is a bug in Cargo, or something else, but if I didn't enforce the specific version of the crate, it was being mangled on re-build.

@@ -46,7 +50,7 @@ sha2 = "0.10"
shadow-rs = "0.19.0"
strum = { version = "0.25", features = ["derive"] }
thiserror = "1.0"
tokio = { version = "1", features = ["full"] }
tokio = { version = "1", features = ["full"], default-features = false }
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we want to turn on the tokio default-features?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have any clear insight into why the project chose to have full tokio features enabled. The PR moves the default-features = false into the workspace toml. If we have it inside of the verifier Cargo.toml, it is completely ignored, and it throws a warning.

sha2.workspace = true
tokio = { workspace = true, optional = true, default-features = false }
tokio = { workspace = true, optional = true }
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we want to turn on the tokio default-features?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, I am not sure. in this case, I am just removing the ignored default-features = false flag from the Cargo.toml and moving into the workspace level Cargo.toml

}
Result::Err(err) => panic!("Error reading certificates file: {err}"),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think initially there was an ask to avoid panicking if the parsing of the bundled X509 failed. I'm fine with this, since this is very const-y and unlikely to fail.

However, we're not reading a file here, the bytes are embedded in the binary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed the file to more accurately describe what is happening.

@@ -124,6 +124,7 @@ impl CcEventLog {

/// Defined in TCG PC Client Platform Firmware Profile Specification section
/// 'UEFI_PLATFORM_FIRMWARE_BLOB Structure Definition'
#[allow(dead_code)]
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is unrelated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was to remove a warning when compiling, as the TDX library is unused.

@larrydewey larrydewey force-pushed the cleanup branch 4 times, most recently from 11840dc to 183b9d6 Compare August 12, 2024 20:47
This introduces changes into the code to utilize the cert
chain provided by the VirTEE sev crate, pushing resposibility
for upkeep and maintainership back to the library itself. This
also reduces duplication of code and expands functionality for
future changes.

Review Changes:

- Adding whitespace back to Makefile

- Updating sev to 4.0.0

- Removed Cargo.toml reformatting

- Renamed function per mkulke's comment

Signed-off-by: Larry Dewey <[email protected]>
Copy link
Contributor

@mkulke mkulke left a comment

Choose a reason for hiding this comment

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

@larrydewey maybe the compiler/cargo errors you are seeing are due to some specifics in your local build environment? (e.g. cargo, rust toolchain, etc.) In this case you can maybe take a look at

- name: Install Rust toolchain (${{ env.RUSTC_VERSION }})
run: |
rustup update --no-self-update ${{ env.RUSTC_VERSION }}
rustup component add --toolchain ${{ env.RUSTC_VERSION }} rustfmt rustc clippy
rustup target add x86_64-unknown-linux-gnu
rustup default ${{ env.RUSTC_VERSION }}
to find a working configuration. The CI should define the canonical build configuration that we want to support.

@larrydewey
Copy link
Contributor Author

@larrydewey maybe the compiler/cargo errors you are seeing are due to some specifics in your local build environment? (e.g. cargo, rust toolchain, etc.) In this case you can maybe take a look at

- name: Install Rust toolchain (${{ env.RUSTC_VERSION }})
run: |
rustup update --no-self-update ${{ env.RUSTC_VERSION }}
rustup component add --toolchain ${{ env.RUSTC_VERSION }} rustfmt rustc clippy
rustup target add x86_64-unknown-linux-gnu
rustup default ${{ env.RUSTC_VERSION }}

to find a working configuration. The CI should define the canonical build configuration that we want to support.

This is my default configuration, so I don't believe it is my workflow or configuration.

@larrydewey
Copy link
Contributor Author

larrydewey commented Aug 13, 2024

A large number of errors I believe are coming from the sev update from 3.2.0 to 4.0.0. Particularly due to the dependencies not using the latest version

#15 28.38 warning: output filename collision.
#15 28.38 The lib target `sev` in package `sev v4.0.0` has the same output filename as the lib target `sev` in package `sev v3.2.0`.
#15 28.38 Colliding filename is: /usr/src/attestation-service/target/release/libsev.so
#15 28.38 The targets should have unique names.
#15 28.38 Consider changing their names to be unique or compiling them separately.
#15 28.38 This may become a hard error in the future; see <https://github.com/rust-lang/cargo/issues/6313>.
#15 28.38 warning: output filename collision.
#15 28.38 The lib target `sev` in package `sev v4.0.0` has the same output filename as the lib target `sev` in package `sev v3.2.0`.
#15 28.38 Colliding filename is: /usr/src/attestation-service/target/release/libsev.so.dwp
#15 28.38 The targets should have unique names.
#15 28.38 Consider changing their names to be unique or compiling them separately.
#15 28.38 This may become a hard error in the future; see <https://github.com/rust-lang/cargo/issues/6313>.

See: kinvolk/azure-cvm-tooling#56

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants