Fix clippy

I was noticing a lot of warnings while building that we typically don't allow. It turns out, clippy was broken. For some reason, the stage0/stage0_tdx targets do not cooperate with clippy. For now I have: * excluded those targets from clippy * updates the clippy script to properly signal errors. Change-Id: I2e8d5e298b1ec9919e9417d24b1ab2afc03e74e8
project-oak · Oct 11, 2024 · 94922a9 · 94922a9
1 parent 9e9591b
commit 94922a9
Show file tree

Hide file tree

Showing 37 changed files with 223 additions and 202 deletions.
diff --git a/.bazelrc b/.bazelrc
@@ -78,8 +78,8 @@ build:unsafe-fast-presubmit --remote_upload_local_results=true
 build:unsafe-fast-presubmit --google_default_credentials=true
 
 # https://github.com/bazelbuild/bazel/issues/9342
-# --experimental_check_desugar_deps (on by default) breaks Android builds with remote execution
-build:unsafe-fast-presubmit --noexperimental_check_desugar_deps
+# --experimental_check_desugar_deps (on by default) breaks Android builds
+build --noexperimental_check_desugar_deps
 
 # Set the rustc --sysroot flag to one generated by the toolchains. This is needed to support
 # rebuilding the standard libraries for stage 0 and the restricted kernel wrapper.

diff --git a/justfile b/justfile
@@ -335,6 +335,9 @@ list-bare-metal-crates:
     {{bare_metal_crates_query}}
 
 bazel-clippy:
+    bazel build --keep_going --config=clippy "$@" //...:all -- -third_party/...
+
+bazel-clippy-ci:
     scripts/clippy_clean
 
 bazel-repin:
@@ -346,7 +349,7 @@ bazel-fmt:
 bazel-rustfmt:
     bazel build --config=rustfmt --config=unsafe-fast-presubmit //...:all -- -third_party/...
 
-clippy-ci: bazel-clippy cargo-clippy
+clippy-ci: bazel-clippy-ci cargo-clippy
 
 cargo-clippy:
     #!/bin/sh

diff --git a/oak_attestation/src/dice.rs b/oak_attestation/src/dice.rs
@@ -49,9 +49,7 @@ impl MeasureDigest for &[u8] {
         let mut digest = sha2::Sha256::default();
         digest.update(self);
         let digest_bytes: [u8; 32] = digest.finalize().into();
-        let mut raw_digest = RawDigest::default();
-        raw_digest.sha2_256 = digest_bytes.to_vec();
-        raw_digest
+        RawDigest { sha2_256: digest_bytes.to_vec(), ..Default::default() }
     }
 }
 
@@ -323,7 +321,7 @@ impl TryFrom<DiceData> for DiceAttester {
             certificate_authority.eca_private_key.zeroize();
         }
 
-        Ok(DiceAttester { evidence: evidence.clone(), signing_key: signing_key })
+        Ok(DiceAttester { evidence: evidence.clone(), signing_key })
     }
 }
 
@@ -372,11 +370,11 @@ pub fn evidence_and_event_log_to_proto(
     let layers = vec![layer_evidence_to_proto(value.restricted_kernel_evidence)?];
     let application_keys = Some(application_keys_to_proto(value.application_keys)?);
     let event_log = encoded_event_log
-        .map(|data| EventLog::decode(data))
+        .map(EventLog::decode)
         .transpose()
         .map_err(anyhow::Error::msg)
         .context("couldn't decode event log")?;
-    Ok(Evidence { root_layer, layers, application_keys, event_log: event_log })
+    Ok(Evidence { root_layer, layers, application_keys, event_log })
 }
 
 fn root_layer_evidence_to_proto(

diff --git a/oak_attestation_integration_test_utils/src/lib.rs b/oak_attestation_integration_test_utils/src/lib.rs
@@ -17,14 +17,12 @@
 use oak_containers_sdk::{standalone::StandaloneOrchestrator, OrchestratorInterface};
 use oak_proto_rust::oak::{
     attestation::v1::{
-        binary_reference_value, extracted_evidence::EvidenceValues, kernel_binary_reference_value,
-        reference_values, root_layer_data::Report, text_reference_value, AmdSevReferenceValues,
-        ApplicationLayerReferenceValues, BinaryReferenceValue, ContainerLayerReferenceValues,
-        Digests, ExtractedEvidence, InsecureReferenceValues, KernelBinaryReferenceValue,
-        KernelDigests, KernelLayerData, KernelLayerReferenceValues, OakContainersReferenceValues,
-        OakRestrictedKernelReferenceValues, ReferenceValues, RootLayerData,
-        RootLayerReferenceValues, SkipVerification, Stage0Measurements, StringLiterals,
-        SystemLayerReferenceValues, TcbVersion, TextReferenceValue,
+        binary_reference_value, kernel_binary_reference_value, reference_values,
+        text_reference_value, BinaryReferenceValue, ContainerLayerReferenceValues, Digests,
+        InsecureReferenceValues, KernelBinaryReferenceValue, KernelDigests,
+        KernelLayerReferenceValues, OakContainersReferenceValues, ReferenceValues,
+        RootLayerReferenceValues, Stage0Measurements, StringLiterals, SystemLayerReferenceValues,
+        TextReferenceValue,
     },
     session::v1::EndorsedEvidence,
     RawDigest,

diff --git a/oak_attestation_integration_tests/BUILD b/oak_attestation_integration_tests/BUILD
@@ -41,7 +41,6 @@ rust_library(
     ],
 )
 
-# TODO: b/370463888 - Run this in CI.
 rust_binary(
     name = "update_testdata_assert_no_breaking_changes",
     srcs = ["src/main.rs"],

diff --git a/oak_attestation_integration_tests/src/lib.rs b/oak_attestation_integration_tests/src/lib.rs
@@ -34,7 +34,7 @@ impl SnapshotPath {
         Path::new(Self::TESTDATA_DIR).join(format!("{:05}", self.version))
     }
 
-    fn version(&self) -> u16 {
+    pub fn version(&self) -> u16 {
         self.version
     }
 
@@ -163,9 +163,9 @@ impl Snapshot {
         ];
 
         let (
-            mut self_endorsed_evidence_json,
+            self_endorsed_evidence_json,
             previous_endorsed_evidence_json,
-            mut self_reference_values_json,
+            self_reference_values_json,
             previous_reference_values_json,
         ) = tokio::try_join!(
             async {

diff --git a/oak_attestation_integration_tests/src/main.rs b/oak_attestation_integration_tests/src/main.rs
@@ -20,8 +20,6 @@
 //! that newer versions of the verification library continue to be able to
 //! verify older versions of these artifacts.
 
-// TODO: b/370445356 - Write tests that use the created testdata.
-
 use oak_attestation_integration_test_utils::create_oak_containers_standalone_endorsed_evidence_with_matching_reference_values;
 use oak_attestation_integration_tests::{Snapshot, SnapshotPath};
 

diff --git a/oak_attestation_integration_tests/tests/verifier_tests.rs b/oak_attestation_integration_tests/tests/verifier_tests.rs
@@ -17,25 +17,19 @@
 use oak_attestation::dice::evidence_and_event_log_to_proto;
 use oak_attestation_integration_tests::{Snapshot, SnapshotPath};
 use oak_attestation_verification::verifier::{to_attestation_results, verify, verify_dice_chain};
-use oak_containers_sdk::{standalone::StandaloneOrchestrator, OrchestratorInterface};
-use oak_proto_rust::oak::{
-    attestation::v1::{
-        attestation_results::Status, binary_reference_value, endorsements,
-        kernel_binary_reference_value, reference_values, text_reference_value,
-        ApplicationLayerReferenceValues, BinaryReferenceValue, ContainerLayerReferenceValues,
-        Digests, Endorsements, Event, EventLog, InsecureReferenceValues,
-        KernelBinaryReferenceValue, KernelDigests, KernelLayerReferenceValues,
-        OakContainersReferenceValues, OakRestrictedKernelEndorsements,
-        OakRestrictedKernelReferenceValues, ReferenceValues, RootLayerEndorsements,
-        RootLayerReferenceValues, SkipVerification, Stage0Measurements, StringLiterals,
-        SystemLayerReferenceValues, TextReferenceValue,
-    },
-    RawDigest,
+use oak_containers_sdk::OrchestratorInterface;
+use oak_proto_rust::oak::attestation::v1::{
+    attestation_results::Status, binary_reference_value, endorsements,
+    kernel_binary_reference_value, reference_values, text_reference_value,
+    ApplicationLayerReferenceValues, BinaryReferenceValue, ContainerLayerReferenceValues,
+    Endorsements, Event, InsecureReferenceValues, KernelBinaryReferenceValue,
+    KernelLayerReferenceValues, OakContainersReferenceValues, OakRestrictedKernelEndorsements,
+    OakRestrictedKernelReferenceValues, ReferenceValues, RootLayerEndorsements,
+    RootLayerReferenceValues, SkipVerification, Stage0Measurements, SystemLayerReferenceValues,
+    TextReferenceValue,
 };
 use oak_restricted_kernel_sdk::attestation::EvidenceProvider;
 use prost::Message;
-use sha2::Digest;
-use tokio;
 
 // Pretend the tests run at this time: 1 Nov 2023, 9:00 UTC
 const NOW_UTC_MILLIS: i64 = 1698829200000;

diff --git a/oak_attestation_verification/src/endorsement.rs b/oak_attestation_verification/src/endorsement.rs
@@ -19,7 +19,7 @@
 
 extern crate alloc;
 
-use alloc::{collections::BTreeMap, string::String, vec, vec::Vec};
+use alloc::{collections::BTreeMap, string::String, vec::Vec};
 
 use anyhow::Context;
 use base64::{prelude::BASE64_STANDARD, Engine as _};
@@ -184,7 +184,7 @@ pub fn verify_endorsement(
     // The signature verification is also part of log entry verification,
     // so in some cases this check will be dispensable. We verify the
     // signature nonetheless before parsing the endorsement.
-    verify_signature(&signature, &endorsement.serialized, &endorser_key_set)
+    verify_signature(signature, &endorsement.serialized, endorser_key_set)
         .context("verifying signature")?;
 
     let statement =
@@ -241,7 +241,7 @@ pub fn verify_binary_endorsement(
         .context("verifying signature")?;
 
     let statement = parse_statement(endorsement).context("parsing endorsement statement")?;
-    validate_statement(now_utc_millis, &vec![], &statement)
+    validate_statement(now_utc_millis, &[], &statement)
         .context("verifying endorsement statement")?;
 
     if !rekor_public_key.is_empty() {
@@ -267,10 +267,10 @@ pub fn verify_endorser_public_key(
         .iter()
         .find(|k| k.key_id == signature_key_id)
         .ok_or_else(|| anyhow::anyhow!("could not find key id in key set"))?;
-    return match key.r#type() {
+    match key.r#type() {
         KeyType::Undefined => anyhow::bail!("Undefined key type"),
         KeyType::EcdsaP256Sha256 => verify_endorser_public_key_ecdsa(log_entry, &key.raw),
-    };
+    }
 }
 
 /// Verifies that the endorser public key coincides with the one contained in
@@ -330,10 +330,10 @@ pub fn validate_statement(
 
     match &statement.predicate.validity {
         Some(validity) => {
-            if validity.not_before.unix_timestamp_millis() > now_utc_millis.into() {
+            if validity.not_before.unix_timestamp_millis() > now_utc_millis {
                 anyhow::bail!("the claim is not yet applicable")
             }
-            if validity.not_after.unix_timestamp_millis() < now_utc_millis.into() {
+            if validity.not_after.unix_timestamp_millis() < now_utc_millis {
                 anyhow::bail!("the claim is no longer applicable")
             }
         }

diff --git a/oak_attestation_verification/src/endorsement/tests.rs b/oak_attestation_verification/src/endorsement/tests.rs
@@ -16,7 +16,7 @@
 #[cfg(test)]
 extern crate std;
 
-use alloc::{vec, vec::Vec};
+use alloc::vec::Vec;
 use std::fs;
 
 use oak_file_utils::data_path;
@@ -98,7 +98,7 @@ fn test_validate_endorsement_statement_success() {
     let testdata = load_testdata();
     let statement =
         parse_statement(&testdata.endorsement).expect("could not parse endorsement statement");
-    let result = validate_statement(NOW_UTC_MILLIS, &vec![], &statement);
+    let result = validate_statement(NOW_UTC_MILLIS, &[], &statement);
     assert!(result.is_ok(), "{:?}", result);
 }
 
@@ -107,7 +107,7 @@ fn test_validate_endorsement_statement_fails_too_early() {
     let testdata = load_testdata();
     let statement =
         parse_statement(&testdata.endorsement).expect("could not parse endorsement statement");
-    let result = validate_statement(TOO_EARLY_UTC_MILLIS, &vec![], &statement);
+    let result = validate_statement(TOO_EARLY_UTC_MILLIS, &[], &statement);
     assert!(result.is_err(), "{:?}", result);
 }
 
@@ -116,7 +116,7 @@ fn test_validate_statement_fails_too_late() {
     let testdata = load_testdata();
     let statement =
         parse_statement(&testdata.endorsement).expect("could not parse endorsement statement");
-    let result = validate_statement(TOO_LATE_UTC_MILLIS, &vec![], &statement);
+    let result = validate_statement(TOO_LATE_UTC_MILLIS, &[], &statement);
     assert!(result.is_err(), "{:?}", result);
 }
 

diff --git a/oak_attestation_verification/src/extract.rs b/oak_attestation_verification/src/extract.rs
@@ -138,12 +138,12 @@ pub(crate) fn extract_evidence_values(evidence: &Evidence) -> anyhow::Result<Evi
                         anyhow::anyhow!("Failed to decode oak containers container layer data")
                     })?;
 
-                return Ok(EvidenceValues::OakContainers(OakContainersData {
+                Ok(EvidenceValues::OakContainers(OakContainersData {
                     root_layer,
                     kernel_layer: Some(kernel_layer),
                     system_layer: Some(system_layer),
                     container_layer: Some(container_layer),
-                }));
+                }))
             }
             EvidenceType::LegacyOakContainers => {
                 let kernel_layer = decoded_events[0]
@@ -172,12 +172,12 @@ pub(crate) fn extract_evidence_values(evidence: &Evidence) -> anyhow::Result<Evi
                             "Failed to decode legacy oak containers container layer data"
                         )
                     })?;
-                return Ok(EvidenceValues::OakContainers(OakContainersData {
+                Ok(EvidenceValues::OakContainers(OakContainersData {
                     root_layer,
                     kernel_layer: Some(kernel_layer),
                     system_layer: Some(system_layer),
                     container_layer: Some(container_layer),
-                }));
+                }))
             }
             EvidenceType::OakRestrictedKernel => {
                 let kernel_layer = decoded_events[0]
@@ -196,11 +196,11 @@ pub(crate) fn extract_evidence_values(evidence: &Evidence) -> anyhow::Result<Evi
                         anyhow::anyhow!("Failed to decode oak restricted application layer data")
                     })?;
 
-                return Ok(EvidenceValues::OakRestrictedKernel(OakRestrictedKernelData {
+                Ok(EvidenceValues::OakRestrictedKernel(OakRestrictedKernelData {
                     root_layer,
                     kernel_layer: Some(kernel_layer),
                     application_layer: Some(application_layer),
-                }));
+                }))
             }
             EvidenceType::CB => Ok(EvidenceValues::Cb(CbData {
                 root_layer,
@@ -574,9 +574,10 @@ fn value_to_raw_digest(value: &Value) -> anyhow::Result<RawDigest> {
                 anyhow::bail!("digest is not a byte array");
             }
         }
-        return Ok(result);
+        Ok(result)
+    } else {
+        Err(anyhow::anyhow!("value is not a map of digests"))
     }
-    Err(anyhow::anyhow!("value is not a map of digests"))
 }
 
 /// Translates [`Stage0Measurements`] to [`KernelLayerData`]. Both hold the same

diff --git a/oak_attestation_verification/src/policy/combined.rs b/oak_attestation_verification/src/policy/combined.rs
@@ -69,10 +69,10 @@ impl Policy for CombinedPolicy {
         );
         let event_attestation_results = verification_iterator
             .map(|(event_policy, event, event_endorsements)| {
-                event_policy.verify(event, event_endorsements).unwrap_or_else(|_| {
+                event_policy.verify(event, event_endorsements).unwrap_or(
                     // TODO: b/366186091 - Use Rust error types for failed attestation.
-                    EventAttestationResults {}
-                })
+                    EventAttestationResults {},
+                )
             })
             .collect::<Vec<EventAttestationResults>>();
 

diff --git a/oak_attestation_verification/src/test_util.rs b/oak_attestation_verification/src/test_util.rs
@@ -34,12 +34,12 @@ pub enum Usage {
     Kernel,
 }
 
-impl Usage {
-    fn to_string(&self) -> String {
+impl std::fmt::Display for Usage {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
         match self {
-            Self::None => "".to_string(),
-            Self::Firmware => "firmware".to_string(),
-            Self::Kernel => "kernel".to_string(),
+            Self::None => write!(f, ""),
+            Self::Firmware => write!(f, "firmware"),
+            Self::Kernel => write!(f, "kernel"),
         }
     }
 }