Make remote_attestation crate no_std compatible #2556

conradgrobler · 2022-02-22T16:46:42Z

This PR reduces dependencies of the remote_attestation crate and makes it usable in no_std environments.

Reducing the dependencies should simplify future code audits. An important part of the change is to move away from the bincode crate to manual serialization of the handshake messages. The bincode crate requires std, and manual serialization will also provide more control over the structure of these handshake messages.

Fixes #2294

ipetr0v · 2022-02-23T17:46:38Z

Do we have a CI command that compiles it with no_std?

conradgrobler · 2022-02-23T17:51:55Z

By default it is built as no_std, so the CI tests it in no_std mode. It only uses std if the feature std is specified. Nothing specifies the feature at the moment, but I manually used it with cargo test while debugging.

ipetr0v · 2022-02-23T17:49:12Z

remote_attestation/rust/Cargo.toml

 log = "*"
-prost = "*"
+prost = { version = "*", default-features = false, features = ["prost-derive"] }


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

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

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

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.

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.

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.

ipetr0v · 2022-02-23T17:51:04Z

remote_attestation/rust/src/crypto.rs

    hkdf::{Salt, HKDF_SHA256},
    rand::{SecureRandom, SystemRandom},
    signature::{EcdsaKeyPair, EcdsaSigningAlgorithm, EcdsaVerificationAlgorithm, KeyPair},
 };
-use sha2::{digest::Digest, Sha256};


Did sha2 require std?

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

tiziano88 · 2022-02-23T18:39:28Z

remote_attestation/rust/Cargo.toml

+[features]
+default = ["alloc"]
+std = ["anyhow/std", "prost/std"]
+alloc = []


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

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

tiziano88 · 2022-02-23T18:40:00Z

remote_attestation/rust/Cargo.toml

 log = "*"
-prost = "*"
+prost = { version = "*", default-features = false, features = ["prost-derive"] }


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

tiziano88 · 2022-02-23T18:42:57Z

remote_attestation/rust/src/crypto.rs

@@ -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!(


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)

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.

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.

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?

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

Created #2564

conradgrobler

Thanks.

conradgrobler · 2022-02-24T09:12:48Z

remote_attestation/rust/Cargo.toml

+[features]
+default = ["alloc"]
+std = ["anyhow/std", "prost/std"]
+alloc = []


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

conradgrobler · 2022-02-24T09:14:24Z

remote_attestation/rust/Cargo.toml

 log = "*"
-prost = "*"
+prost = { version = "*", default-features = false, features = ["prost-derive"] }


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.

conradgrobler · 2022-02-24T09:19:27Z

remote_attestation/rust/src/crypto.rs

@@ -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!(


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.

github-actions · 2022-02-24T13:59:28Z

Reproducibility Index:

d16e9c355afdf34390d3991d52504b574665f6b829136b83c1de9b57aa230a24  ./target/x86_64-unknown-linux-musl/release/oak_functions_loader_base
537dda6bc3e1eb4854d9ff76abb5b019b743b0e0ae8e83f02331629079a51e38  ./target/x86_64-unknown-linux-musl/release/oak_functions_loader_unsafe

Reproducibility Index diff:

diff --git a/reproducibility_index b/reproducibility_index
index c47b5ab..5cb7e3b 100644
--- a/reproducibility_index
+++ b/reproducibility_index
@@ -1,2 +1,2 @@
-e81b77bd5e6aa364b806b14ab64215ef811892a6492f7d4b2cfdf3066b60d029  ./target/x86_64-unknown-linux-musl/release/oak_functions_loader_base
-ed45262f841ab276d9411219c7f5eee7a3aaeefbc7dbe32740089f07e1939407  ./target/x86_64-unknown-linux-musl/release/oak_functions_loader_unsafe
+d16e9c355afdf34390d3991d52504b574665f6b829136b83c1de9b57aa230a24  ./target/x86_64-unknown-linux-musl/release/oak_functions_loader_base
+537dda6bc3e1eb4854d9ff76abb5b019b743b0e0ae8e83f02331629079a51e38  ./target/x86_64-unknown-linux-musl/release/oak_functions_loader_unsafe

conradgrobler added WIP Work in progress dependencies Pull requests that update a dependency file labels Feb 22, 2022

conradgrobler force-pushed the no_std_attestation branch 7 times, most recently from df1e7c8 to 297ab62 Compare February 23, 2022 16:36

Make remote_attestation crate no_std compatible

917e0be

conradgrobler force-pushed the no_std_attestation branch from 297ab62 to 917e0be Compare February 23, 2022 17:10

conradgrobler removed the WIP Work in progress label Feb 23, 2022

conradgrobler marked this pull request as ready for review February 23, 2022 17:27

conradgrobler requested review from tiziano88, ipetr0v, mariaschett and andrisaar February 23, 2022 17:27

ipetr0v approved these changes Feb 23, 2022

View reviewed changes

tiziano88 approved these changes Feb 23, 2022

View reviewed changes

Address review comments

97977e1

conradgrobler force-pushed the no_std_attestation branch from 0191a18 to 97977e1 Compare February 24, 2022 09:24

conradgrobler commented Feb 24, 2022

View reviewed changes

conradgrobler mentioned this pull request Feb 24, 2022

Create a prelude for common types, traits etc. to use in no_std crates. #2564

Closed

conradgrobler merged commit 1cf9668 into project-oak:main Feb 24, 2022

conradgrobler deleted the no_std_attestation branch February 24, 2022 13:46

andrisaar approved these changes Feb 24, 2022

View reviewed changes

jul-sh mentioned this pull request Mar 15, 2022

Stop cloning additional_info into each ServerHandshaker #2574

Merged

Make remote_attestation crate no_std compatible #2556

Make remote_attestation crate no_std compatible #2556

Uh oh!

Conversation

conradgrobler commented Feb 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ipetr0v commented Feb 23, 2022

Uh oh!

conradgrobler commented Feb 23, 2022

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!

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 Feb 24, 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!

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!

conradgrobler Feb 24, 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!

github-actions bot commented Feb 24, 2022

Uh oh!

Uh oh!

conradgrobler commented Feb 22, 2022 •

edited

Loading

conradgrobler Feb 24, 2022 •

edited

Loading

conradgrobler Feb 24, 2022 •

edited

Loading