Skip to content

Use vendored version of ring in remote attestation #2661

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

Merged
merged 5 commits into from
Apr 8, 2022
Merged
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
2 changes: 1 addition & 1 deletion .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ jobs:
uses: actions/cache@v2
env:
# Increment this value to invalidate previous cache entries.
CACHE_VERSION: 2
CACHE_VERSION: 3
with:
path: |
./cargo-cache/bin
Expand Down
38 changes: 28 additions & 10 deletions Cargo.lock

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

1 change: 0 additions & 1 deletion deny.toml
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ copyleft = "deny"

[[licenses.clarify]]
name = "ring"
version = "*"
Copy link
Author

Choose a reason for hiding this comment

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

The reason this needed removing is somewhat counterintuitive:

In cargo, version = "*" does not in fact mean "any version", but instead "any version that is published on crates.io". We need to remove this (optional) key to truly check any version (in our case our vendored version).

expression = "MIT AND ISC AND OpenSSL"
license-files = [{ path = "LICENSE", hash = 3171872035 }]

Expand Down
40 changes: 32 additions & 8 deletions oak_functions/loader/fuzz/Cargo.lock

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

2 changes: 1 addition & 1 deletion remote_attestation/rust/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ anyhow = { version = "*", default-features = false }
bytes = { version = "*", default-features = false }
log = "*"
prost = { version = "*", default-features = false, features = ["prost-derive"] }
ring = "*"
ring = { path = "../../third_party/ring" }

[build-dependencies]
prost-build = "*"
Expand Down
70 changes: 40 additions & 30 deletions remote_attestation/rust/src/crypto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -234,11 +234,10 @@ impl KeyNegotiator {
) -> anyhow::Result<(EncryptionKey, DecryptionKey)> {
let type_ = self.type_.clone();
let self_public_key = self.public_key().context("Couldn't get self public key")?;
let (encryption_key, decryption_key) = agreement::agree_ephemeral(
agreement::agree_ephemeral(
self.private_key,
&agreement::UnparsedPublicKey::new(KEY_AGREEMENT_ALGORITHM, peer_public_key),
anyhow!("Couldn't derive session keys"),
|key_material| {
|key_material| -> anyhow::Result<(EncryptionKey, DecryptionKey)> {
Comment on lines +237 to +240
Copy link
Author

Choose a reason for hiding this comment

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

The updated ring version no longer takes an error as a parameter, but instead returns an additional result

let key_material = key_material
.try_into()
.map_err(anyhow::Error::msg)
Expand All @@ -251,44 +250,54 @@ impl KeyNegotiator {
match type_ {
// On the server side `self_public_key` is the server key.
KeyNegotiatorType::Server => {
let encryption_key = Self::key_derivation_function(
key_material,
SERVER_KEY_PURPOSE,
&self_public_key,
&peer_public_key,
let encryption_key = EncryptionKey(
Self::key_derivation_function(
key_material,
SERVER_KEY_PURPOSE,
&self_public_key,
&peer_public_key,
)
.context("Couldn't derive decryption key")?,
);
let decryption_key = Self::key_derivation_function(
key_material,
CLIENT_KEY_PURPOSE,
&self_public_key,
&peer_public_key,
let decryption_key = DecryptionKey(
Self::key_derivation_function(
key_material,
CLIENT_KEY_PURPOSE,
&self_public_key,
&peer_public_key,
)
.context("Couldn't derive encryption key")?,
);
Ok((encryption_key, decryption_key))
}
// On the client side `peer_public_key` is the server key.
KeyNegotiatorType::Client => {
let encryption_key = Self::key_derivation_function(
key_material,
CLIENT_KEY_PURPOSE,
&peer_public_key,
&self_public_key,
let encryption_key = EncryptionKey(
Self::key_derivation_function(
key_material,
CLIENT_KEY_PURPOSE,
&peer_public_key,
&self_public_key,
)
.context("Couldn't derive decryption key")?,
);
let decryption_key = Self::key_derivation_function(
key_material,
SERVER_KEY_PURPOSE,
&peer_public_key,
&self_public_key,
let decryption_key = DecryptionKey(
Self::key_derivation_function(
key_material,
SERVER_KEY_PURPOSE,
&peer_public_key,
&self_public_key,
)
.context("Couldn't derive encryption key")?,
);
Ok((encryption_key, decryption_key))
}
}
},
)
.context("Couldn't agree on session keys")?;
Ok((
EncryptionKey(encryption_key.context("Couldn't derive encryption key")?),
DecryptionKey(decryption_key.context("Couldn't derive decryption key")?),
))
.map_err(anyhow::Error::msg)
.context("Couldn't derive session keys")?
.context("Couldn't agree on session keys")
}

/// Derives a session key from `key_material` using HKDF.
Expand Down Expand Up @@ -351,8 +360,9 @@ impl Signer {
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))?;
let key_pair = EcdsaKeyPair::from_pkcs8(SIGNING_ALGORITHM, key_pair_pkcs8.as_ref())
.map_err(|error| anyhow!("Couldn't parse generated key pair: {:?}", error))?;
let key_pair =
EcdsaKeyPair::from_pkcs8(SIGNING_ALGORITHM, key_pair_pkcs8.as_ref(), &rng)
.map_err(|error| anyhow!("Couldn't parse generated key pair: {:?}", error))?;

Ok(Self { key_pair })
}
Expand Down
12 changes: 11 additions & 1 deletion third_party/ring/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,17 @@ fn ring_build_rs_main() {
// If `.git` doesn't exist then assume that this is a packaged build where
// we want to optimize for minimizing the build tools required: No Perl,
// no nasm, etc.
let use_pregenerated = !is_git;
//
// Oak Note:
// ring includes pre-generated objects in cargo releases. It uses logic in
// its build scripts to determine whether the build is from a version
// published on cargo, or from source code. This logic is somewhat naively
// implemented by checking for the absence of a `.git` directory. This
// check falesly leads ring to believe it is building from a version
// published on crates.io, and fails when it tries to include the
// pregenerated files not present in the source code. To build from source
// code we must patch this flag.
let use_pregenerated = false;

// During local development, force warnings in non-Rust code to be treated
// as errors. Since warnings are highly compiler-dependent and compilers
Expand Down
5 changes: 4 additions & 1 deletion third_party/ring/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,10 @@
missing_debug_implementations,
non_camel_case_types,
non_snake_case,
unsafe_code
unsafe_code,
// Oak Note: Do not warn when building deprecated code. Ring includes
// code that is, and our CI fails on warnings
deprecated
)]
// `#[derive(...)]` uses `trivial_numeric_casts` and `unused_qualifications`
// internally.
Expand Down