Hashes revocation #3

amanjeev · 2024-05-21T16:14:13Z

Adds the ability to check for revoked content which are served as signed hashes.

Ticket: https://ferroussystems.clickup.com/t/86947z6fp

crates/mock-download-server/Cargo.toml

crates/criticaltrust/src/manifests.rs

crates/criticaltrust/src/keys/public.rs

pietroalbini · 2024-05-22T10:07:44Z

crates/criticaltrust/src/signatures/payload.rs

@@ -106,7 +106,7 @@ fn verify_signature<T: Signable>(
            None => continue,
        };

-        match key.verify(T::SIGNED_BY_ROLE, &signed, &signature.signature) {
+        match key.verify_payload(T::SIGNED_BY_ROLE, &signed, &signature.signature) {


We should check the revocation in most cases when decoding a signed payload. The two only cases where we can't realistically do that is (a) when decoding the public keys (as at that point we haven't decoded the revocation key) and (b) when decoding the revocation payload.

To have a hard-to-misuse interface, I'd add something like this:

pub trait NoRevocationsCheck {} impl<T: Signable + NoRevocationsCheck> SignedPayload<T> { pub fn get_verified_without_checking_revocations(...) -> ... {} }

Then we can implement NoRevocationsCheck just on the payloads where we have to avoid checking revocations.

@pietroalbini I did add the trait but I could not achieve everything, including calling verify_signature standalone function. Given the deadline is cutting close, I created a ticket for 1.1 https://ferroussystems.clickup.com/t/8694mfy8z for myself.

Ticket: https://ferroussystems.clickup.com/t/86947z6fp

Reviewed-by: Ana Hobden <ana.hobden@ferrous-systems.com> Ticket: https://ferroussystems.clickup.com/t/86947z6fp

Ticket: https://ferroussystems.clickup.com/t/86947z6fp

Reviewed-by: Pietro Albini <pietro.albini@ferrous-systems.com> Ticket: https://ferroussystems.clickup.com/t/86947z6fp

…cific. Reviewed-by: Pietro Albini <pietro.albini@ferrous-systems.com> Ticket: https://ferroussystems.clickup.com/t/86947z6fp

Reviewed-by: Pietro Albini <pietro.albini@ferrous-systems.com> Ticket: https://ferroussystems.clickup.com/t/86947z6fp

amanjeev · 2024-05-22T16:41:27Z

crates/criticaltrust/src/keys/pair_aws_kms.rs

@@ -109,6 +109,7 @@ mod tests {
    // "localstack", a local replica of AWS services meant for testing.

    #[test]
+    #[ignore = "This test will be enabled in a later release."]
    fn test_roundtrip() {


@pietroalbini I have no idea why yet, but this test is super flaky.

It just started breaking???

Is this the failure you're referring to?

$ cargo test -p criticaltrust --features aws-kms roundtrip Finished `test` profile [unoptimized + debuginfo] target(s) in 0.08s Running unittests src/lib.rs (target/debug/deps/criticaltrust-6080a18de3f40575) running 1 test test keys::pair_aws_kms::tests::test_roundtrip ... FAILED failures: ---- keys::pair_aws_kms::tests::test_roundtrip stdout ---- running "docker" "pull" "localstack/localstack:2.2.0" finished running "docker" "pull" "localstack/localstack:2.2.0" running "docker" "create" "--name" "criticaltrust-localstack-3090418271525868829" "-p" "4566" "localstack/localstack:2.2.0" finished running "docker" "create" "--name" "criticaltrust-localstack-3090418271525868829" "-p" "4566" "localstack/localstack:2.2.0" running "docker" "start" "criticaltrust-localstack-3090418271525868829" finished running "docker" "start" "criticaltrust-localstack-3090418271525868829" running "docker" "port" "criticaltrust-localstack-3090418271525868829" "4566/tcp" finished running "docker" "port" "criticaltrust-localstack-3090418271525868829" "4566/tcp" thread 'keys::pair_aws_kms::tests::test_roundtrip' panicked at crates/criticaltrust/src/keys/pair_aws_kms.rs:214:18: failed to create kms key: DispatchFailure(DispatchFailure { source: ConnectorError { kind: Io, source: hyper::Error(Canceled, hyper::Error(IncompleteMessage)), connection: Unknown } }) note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace running "docker" "stop" "criticaltrust-localstack-3090418271525868829" "-t" "10" finished running "docker" "stop" "criticaltrust-localstack-3090418271525868829" "-t" "10" running "docker" "rm" "criticaltrust-localstack-3090418271525868829" finished running "docker" "rm" "criticaltrust-localstack-3090418271525868829" failures: keys::pair_aws_kms::tests::test_roundtrip test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 88 filtered out; finished in 4.19s error: test failed, to rerun pass `-p criticaltrust --lib`

This seems to break on main too. :(

…tory for EphemeralKeyPair Reviewed-by: Pietro Albini <pietro.albini@ferrous-systems.com> Ticket: https://ferroussystems.clickup.com/t/86947z6fp

Reviewed-by: Pietro Albini <pietro.albini@ferrous-systems.com> Ticket: https://ferroussystems.clickup.com/t/86947z6fp

crates/criticaltrust/src/keys/public.rs

Checking for payload's SHA256 is the requirement, not the base64 of the signature. Reviewed-by: Ana Hobden <ana.hobden@ferrous-systems.com> Ticket: https://ferroussystems.clickup.com/t/86947z6fp

pietroalbini · 2024-05-23T08:15:36Z

crates/criticaltrust/src/keys/public.rs

 use serde::{Deserialize, Serialize};
 use time::OffsetDateTime;

+/// Expiration of `RevocationInfo` should be at least this many number of days out from now.
+const MAX_REVOCATION_INFO_EXPIRATION_DURATION: i64 = 90;


Is this ok with the current expiration of the payload of 2024-10-01?

Days from today to Oct 1, 2024 are about 130. So, this seems fine to me.

pietroalbini · 2024-05-23T08:20:30Z

crates/criticaltrust/src/keys/public.rs

+/// Make sure verification of `PublicKey` type does no checks for revocations.
+///
+/// This is done because when decoding the public keys, we haven't yet decoded the revocation key.
+impl NoRevocationCheck for SignedPayload<PublicKey> {}


Suggested change

impl NoRevocationCheck for SignedPayload<PublicKey> {}

impl NoRevocationCheck for PublicKey {}

If we implement it for SignedPayload<PublicKey>:

The trait bound of impl<T: NoRevocationCheck> SignedPayload<T> will not work.

It won't be possible to implement NoRevocationCheck outside CriticalTrust due to the orphan rule (as both NoRevocationCheck and SignedPayload are defined in CriticalTrust).

Since, the get_verified method is on SignedPayload, you won't be able to restrict that method if you do impl NoRevocationCheck for PublicKey {}. IIUC, that was your idea, to make this API more stringent so harder to abuse. There is no get_verified on PublicKey, as an example.

I actually started with what you suggested and learned that, that does not work. as in one could still call .get_verified on PublicKey.

pietroalbini · 2024-05-23T08:21:39Z

crates/criticaltrust/src/manifests.rs

+/// Make sure verification of `RevocationInfo` type does no checks for revocations.
+///
+/// If we did, then this would be a circular logic. We say no to such logic.
+impl NoRevocationCheck for SignedPayload<RevocationInfo> {}


Same as the other NoRevocationCheck impl.

pietroalbini · 2024-05-23T08:22:36Z

crates/criticaltrust/src/signatures/payload.rs

@@ -106,7 +133,11 @@ fn verify_signature<T: Signable>(
            None => continue,
        };

-        match key.verify(T::SIGNED_BY_ROLE, &signed, &signature.signature) {
+        match key.verify_without_checking_revocations(


We still need to make sure this verifies revocations if called by get_verified or into_verified.

pietroalbini · 2024-05-23T08:23:20Z

crates/mock-download-server/src/lib.rs

+            revoked_signatures: SignedPayload::new(&RevocationInfo {
+                revoked_content_sha256: Vec::new(),
+                expires_at: OffsetDateTime::now_utc(),
+            })
+            .unwrap(),


This is not correct: it expires immediately, and it's not signed. It will cause CriticalUp tests to fail.

pietroalbini · 2024-05-23T08:24:08Z

crates/criticaltrust/src/keys/public.rs

+        let key = generate(KeyRole::Root, None);
+        let signature = key.sign(&SAMPLE_REVOKED_PAYLOAD).unwrap();
+
+        let payload_sha256 = String::from_utf8(SAMPLE_REVOKED_PAYLOAD.as_bytes().to_vec()).unwrap();


This is not the sha256?

pietroalbini · 2024-05-23T08:24:26Z

crates/criticaltrust/src/keys/public.rs

+        let payload_sha256 = String::from_utf8(SAMPLE_REVOKED_PAYLOAD.as_bytes().to_vec()).unwrap();
+        let signed_revocation_info = SignedPayload::new(&RevocationInfo {
+            revoked_content_sha256: vec![payload_sha256],
+            expires_at: days_diff(10000).unwrap(),


This test is failing due to the expires_at date, not due to the content being revoked.

pietroalbini · 2024-05-23T08:28:45Z

crates/criticaltrust/src/keys/public.rs

+        let verified_revoked_content = signed_revocation_info.get_verified(self)?;
+        let expiration_in_days =
+            (verified_revoked_content.expires_at - OffsetDateTime::now_utc()).whole_days();
+        if expiration_in_days < MAX_REVOCATION_INFO_EXPIRATION_DURATION {


I am a bit confused by this check. The check doesn't seem to actually check the revocation date, only that the period between the expiration and now is less than 90 days.

I would make two separate checks:

expires_at < now() + 90 days

expires_at > now()

amanjeev · 2024-05-27T14:41:12Z

A new PR will be opened at a later stage using reviews from this PR.

amanjeev requested review from Hoverbear and pietroalbini May 21, 2024 16:14

amanjeev self-assigned this May 21, 2024

amanjeev force-pushed the amanjeev/hashes-hashes-we-all-revoke-down branch 2 times, most recently from 912efed to 84db7b4 Compare May 21, 2024 19:13

Hoverbear reviewed May 21, 2024

View reviewed changes

crates/mock-download-server/Cargo.toml Outdated Show resolved Hide resolved

amanjeev marked this pull request as ready for review May 22, 2024 01:32

amanjeev force-pushed the amanjeev/hashes-hashes-we-all-revoke-down branch from 023caa2 to 5dd5500 Compare May 22, 2024 03:18

pietroalbini requested changes May 22, 2024

View reviewed changes

amanjeev force-pushed the amanjeev/hashes-hashes-we-all-revoke-down branch from ba103a7 to fbf9cde Compare May 22, 2024 14:04

amanjeev added 6 commits May 22, 2024 11:51

Hashes revocation: ability to add content revocation

b29982e

Ticket: https://ferroussystems.clickup.com/t/86947z6fp

Hashes revocation review: fmt newline

06e5797

Reviewed-by: Ana Hobden <ana.hobden@ferrous-systems.com> Ticket: https://ferroussystems.clickup.com/t/86947z6fp

Hashes revocation review: adds tests and refactors verify

f457a48

Ticket: https://ferroussystems.clickup.com/t/86947z6fp

Hashes revocation review: SIGNED_BY_ROLE should be Revocation

96478de

Reviewed-by: Pietro Albini <pietro.albini@ferrous-systems.com> Ticket: https://ferroussystems.clickup.com/t/86947z6fp

Hashes revocation review: rename method verify_payload to be more spe…

0065677

…cific. Reviewed-by: Pietro Albini <pietro.albini@ferrous-systems.com> Ticket: https://ferroussystems.clickup.com/t/86947z6fp

Hashes revocation review: base64-encode instead of serde roundtrip.

7733b97

Reviewed-by: Pietro Albini <pietro.albini@ferrous-systems.com> Ticket: https://ferroussystems.clickup.com/t/86947z6fp

amanjeev force-pushed the amanjeev/hashes-hashes-we-all-revoke-down branch from fbf9cde to 7733b97 Compare May 22, 2024 15:51

Hashes revocation review: RevocationInfo not to be optional

e186044

Reviewed-by: Pietro Albini <pietro.albini@ferrous-systems.com> Ticket: https://ferroussystems.clickup.com/t/86947z6fp

amanjeev commented May 22, 2024

View reviewed changes

amanjeev added 2 commits May 22, 2024 16:01

Hashes revocation review: Ease of use for tests. impl PublicKeyReposi…

c8d4329

…tory for EphemeralKeyPair Reviewed-by: Pietro Albini <pietro.albini@ferrous-systems.com> Ticket: https://ferroussystems.clickup.com/t/86947z6fp

Hashes revocation review: Introduce NoRevocationCheck to avoid leaky API

cc67add

Reviewed-by: Pietro Albini <pietro.albini@ferrous-systems.com> Ticket: https://ferroussystems.clickup.com/t/86947z6fp

Hoverbear reviewed May 22, 2024

View reviewed changes

crates/criticaltrust/src/keys/public.rs Outdated Show resolved Hide resolved

Hashes revocation review: bug in public.rs

43e57fa

Checking for payload's SHA256 is the requirement, not the base64 of the signature. Reviewed-by: Ana Hobden <ana.hobden@ferrous-systems.com> Ticket: https://ferroussystems.clickup.com/t/86947z6fp

amanjeev requested review from Hoverbear and pietroalbini May 22, 2024 21:17

pietroalbini requested changes May 23, 2024

View reviewed changes

amanjeev marked this pull request as draft May 23, 2024 16:08

amanjeev closed this May 27, 2024

amanjeev deleted the amanjeev/hashes-hashes-we-all-revoke-down branch August 30, 2024 01:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hashes revocation #3

Hashes revocation #3

amanjeev commented May 21, 2024 •

edited

Loading

pietroalbini May 22, 2024

amanjeev May 22, 2024

amanjeev May 22, 2024

Hoverbear May 22, 2024

Hoverbear May 22, 2024

pietroalbini May 23, 2024

amanjeev May 23, 2024

pietroalbini May 23, 2024

amanjeev May 23, 2024

pietroalbini May 23, 2024

pietroalbini May 23, 2024

pietroalbini May 23, 2024

pietroalbini May 23, 2024

pietroalbini May 23, 2024

pietroalbini May 23, 2024

amanjeev commented May 27, 2024

	impl NoRevocationCheck for SignedPayload<PublicKey> {}
	impl NoRevocationCheck for PublicKey {}

Hashes revocation #3

Hashes revocation #3

Conversation

amanjeev commented May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanjeev commented May 27, 2024

amanjeev commented May 21, 2024 •

edited

Loading