Skip to content

Make remote_attestation crate no_std compatible #2556

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.

Sign up for GitHub

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

Merged
conradgrobler merged 2 commits into from
Feb 24, 2022
Merged

Make remote_attestation crate no_std compatible #2556

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
917e0be
Make remote_attestation crate no_std compatible
conradgrobler Feb 22, 2022
97977e1
Address review comments
conradgrobler Feb 24, 2022
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
16 changes: 2 additions & 14 deletions Cargo.lock
View file
Open in desktop

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

1 change: 1 addition & 0 deletions grpc_attestation/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ oak_remote_attestation = { path = "../remote_attestation/rust/" }
oak_functions_abi = { path = "../oak_functions/abi/" }
prost = "*"
prost-types = "*"
serde = { version = "*", features = ["derive"] }
tokio = { version = "*", features = [
"fs",
"macros",
Expand Down
80 changes: 2 additions & 78 deletions oak_functions/loader/fuzz/Cargo.lock
View file
Open in desktop

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

14 changes: 8 additions & 6 deletions remote_attestation/rust/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,17 @@ authors = ["Ivan Petrov <[email protected]>"]
edition = "2021"
license = "Apache-2.0"

[features]
default = ["alloc"]
std = ["anyhow/std", "prost/std"]
alloc = []
Copy link
Collaborator

@tiziano88 tiziano88 Feb 23, 2022

Choose a reason for hiding this comment

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

Is this actually needed for anything, or is it more for documentation purposes?

Copy link
Collaborator Author

@conradgrobler conradgrobler Feb 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It was something I experimented with and forgot to delete. Deleted now.


[dependencies]
anyhow = "*"
bincode = "*"
anyhow = { version = "*", default-features = false }
bytes = { version = "*", default-features = false }
log = "*"
prost = "*"
prost = { version = "*", default-features = false, features = ["prost-derive"] }
Copy link
Contributor

@ipetr0v ipetr0v Feb 23, 2022

Choose a reason for hiding this comment

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

Since we are not restricting versions, is it possible that the future versions of our dependencies will become std instead of no_std?

Copy link
Collaborator Author

@conradgrobler conradgrobler Feb 23, 2022

Choose a reason for hiding this comment

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

Yes, it is possible. I am not sure whether there is a good way to detect that automatically.

Copy link
Collaborator

@tiziano88 tiziano88 Feb 23, 2022

Choose a reason for hiding this comment

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

Wouldn't this crate fail to compile in that case (at least with the defaul features, i.e. alloc instead of std)?

Copy link
Collaborator Author

@conradgrobler conradgrobler Feb 24, 2022

Choose a reason for hiding this comment

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

If prost removes no_std compatibility and removes the std feature so that it always pulls in std I think it will still compile. std reexports the core and alloc types, so the types will still be compatible.

Copy link
Contributor

@daviddrysdale daviddrysdale Mar 1, 2022

Choose a reason for hiding this comment

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

Drive-by: the best way I've found to check no_std compatibility is to have a CI step that cross-compiles for a target with no std available.

Copy link
Collaborator Author

@conradgrobler conradgrobler Mar 1, 2022

Choose a reason for hiding this comment

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

Thanks. At the moment the build requires a target OS because that is required by ring. We should be able to build it for a UEFI target once briansmith/ring#1406 has been merged.

ring = "*"
serde = { version = "*", features = ["derive"] }
serde-big-array = { version = "*", features = ["const-generics"] }
sha2 = "*"

[build-dependencies]
prost-build = "*"
Expand Down
3 changes: 1 addition & 2 deletions remote_attestation/rust/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,10 @@
// limitations under the License.
//

fn main() -> Result<(), Box<dyn std::error::Error>> {
fn main() {
prost_build::compile_protos(
&["remote_attestation/proto/remote_attestation.proto"],
&["../.."],
)
.expect("Proto compilation failed");
Ok(())
}
23 changes: 11 additions & 12 deletions remote_attestation/rust/src/crypto.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,17 @@
// protocol.

use crate::message::EncryptedData;
use alloc::vec::Vec;
use anyhow::{anyhow, Context};
use core::convert::TryInto;
use ring::{
aead::{self, BoundKey},
agreement,
digest::{digest, SHA256},
hkdf::{Salt, HKDF_SHA256},
rand::{SecureRandom, SystemRandom},
signature::{EcdsaKeyPair, EcdsaSigningAlgorithm, EcdsaVerificationAlgorithm, KeyPair},
};
use sha2::{digest::Digest, Sha256};
Copy link
Contributor

@ipetr0v ipetr0v Feb 23, 2022

Choose a reason for hiding this comment

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

Did sha2 require std?

Copy link
Collaborator Author

@conradgrobler conradgrobler Feb 23, 2022

Choose a reason for hiding this comment

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

No, but ring already provides the ability to generate sha256 hashes, so I removed sha2 to reduce the dependencies.

use std::convert::TryInto;

/// Length of the encryption nonce.
/// `ring::aead` uses 96-bit (12-byte) nonces.
Expand Down Expand Up @@ -193,7 +194,7 @@ impl KeyNegotiator {
.map_err(|error| anyhow!("Couldn't get public key: {:?}", error))?
.as_ref()
.to_vec();
public_key.as_slice().try_into().context(format!(
public_key.as_slice().try_into().context(alloc::format!(
"Incorrect public key length, expected {}, found {}",
KEY_AGREEMENT_ALGORITHM_KEY_LENGTH,
public_key.len()
Expand Down Expand Up @@ -234,7 +235,7 @@ impl KeyNegotiator {
&agreement::UnparsedPublicKey::new(KEY_AGREEMENT_ALGORITHM, peer_public_key),
anyhow!("Couldn't derive session keys"),
|key_material| {
let key_material = key_material.try_into().context(format!(
let key_material = key_material.try_into().context(alloc::format!(
Copy link
Collaborator

@tiziano88 tiziano88 Feb 23, 2022

Choose a reason for hiding this comment

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

I would suggest just re-use all these things from alloc, instead of qualifying them everywhere. In fact I'm surprised they are not already in some prelude. Maybe we should build our own prelude? (see https://doc.rust-lang.org/core/prelude/v1/index.html)

Copy link
Collaborator Author

@conradgrobler conradgrobler Feb 24, 2022

Choose a reason for hiding this comment

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

It seems that there used to be an experimental alloc prelude, but it no longer exists. Importing custom preludes does not seem to be encouraged (https://doc.rust-lang.org/beta/unstable-book/language-features/prelude-import.html), so I added use statements as needed.

Copy link
Collaborator

@tiziano88 tiziano88 Feb 24, 2022

Choose a reason for hiding this comment

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

I don't see anything that discourages custom preludes, a lot of libraries do that. The feature to bake a prelude in the compiler is another story, I don't recommend we use that, just use oak_uefi::prelude::* basically.

Copy link
Collaborator Author

@conradgrobler conradgrobler Feb 24, 2022

Choose a reason for hiding this comment

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

Do you mean that we have to add the use statement to all modules that use it? Or is there another way (apart from prelude-import) to use it for the entire crate?

Copy link
Collaborator

@tiziano88 tiziano88 Feb 24, 2022

Choose a reason for hiding this comment

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

Yes we would need to import it. e.g. https://docs.rs/futures/latest/futures/prelude/index.html

Copy link
Collaborator Author

@conradgrobler conradgrobler Feb 24, 2022

Choose a reason for hiding this comment

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

Created #2564

"Incorrect key material length, expected {}, found {}",
KEY_AGREEMENT_ALGORITHM_KEY_LENGTH,
key_material.len()
Expand Down Expand Up @@ -298,7 +299,7 @@ impl KeyNegotiator {
client_public_key: &[u8; KEY_AGREEMENT_ALGORITHM_KEY_LENGTH],
) -> anyhow::Result<[u8; AEAD_ALGORITHM_KEY_LENGTH]> {
// Session key is derived from a purpose string and two public keys.
let info = vec![key_purpose.as_bytes(), server_public_key, client_public_key];
let info = alloc::vec![key_purpose.as_bytes(), server_public_key, client_public_key];

// Initialize key derivation function.
let salt = Salt::new(HKDF_SHA256, KEY_DERIVATION_SALT.as_bytes());
Expand Down Expand Up @@ -339,6 +340,7 @@ pub struct Signer {

impl Signer {
pub fn create() -> anyhow::Result<Self> {
// TODO(#2557): Ensure SystemRandom work when building for x86_64 UEFI targets.
let rng = ring::rand::SystemRandom::new();
let key_pair_pkcs8 = EcdsaKeyPair::generate_pkcs8(SIGNING_ALGORITHM, &rng)
.map_err(|error| anyhow!("Couldn't generate PKCS#8 key pair: {:?}", error))?;
Expand All @@ -350,7 +352,7 @@ impl Signer {

pub fn public_key(&self) -> anyhow::Result<[u8; SIGNING_ALGORITHM_KEY_LENGTH]> {
let public_key = self.key_pair.public_key().as_ref().to_vec();
public_key.as_slice().try_into().context(format!(
public_key.as_slice().try_into().context(alloc::format!(
"Incorrect public key length, expected {}, found {}",
SIGNING_ALGORITHM_KEY_LENGTH,
public_key.len()
Expand All @@ -365,7 +367,7 @@ impl Signer {
.map_err(|error| anyhow!("Couldn't sign input: {:?}", error))?
.as_ref()
.to_vec();
signature.as_slice().try_into().context(format!(
signature.as_slice().try_into().context(alloc::format!(
"Incorrect signature length, expected {}, found {}",
SIGNATURE_LENGTH,
signature.len()
Expand Down Expand Up @@ -397,11 +399,8 @@ impl SignatureVerifier {

/// Computes a SHA-256 digest of `input` and returns it in a form of raw bytes.
pub fn get_sha256(input: &[u8]) -> [u8; SHA256_HASH_LENGTH] {
let mut hasher = Sha256::new();
hasher.update(&input);
hasher
.finalize()
.as_slice()
digest(&SHA256, input)
.as_ref()
.try_into()
.expect("Incorrect SHA-256 hash length")
}
Expand Down
Loading