Skip to content
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

Jump to bottom

Remove serde_jcs dependency #4641

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Remove serde_jcs dependency #4641

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 15 additions & 20 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.

2 changes: 1 addition & 1 deletion oak_attestation_verification/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ oak_dice = { workspace = true }
prost = { workspace = true }
p256 = { version = "*", features = ["ecdsa-core", "ecdsa", "pem"] }
serde = { version = "*", features = ["derive"] }
serde_jcs = "*"
serde_canonical_json = "*"
serde_json = "*"
sha2 = { version = "*", default-features = false }
time = { version = "0.3.28", features = ["serde", "parsing", "formatting"] }
Expand Down
179 changes: 118 additions & 61 deletions oak_attestation_verification/src/rekor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ use alloc::{collections::BTreeMap, string::String, vec::Vec};
use anyhow::Context;
use base64::{prelude::BASE64_STANDARD, Engine as _};
use serde::{Deserialize, Serialize};
use serde_json::Value;

Copy link
Collaborator

Choose a reason for hiding this comment

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

(nit) remove blank line

use serde_canonical_json::CanonicalFormatter;
use serde_json::Serializer;

use crate::util::{convert_pem_to_raw, hash_sha2_256, verify_signature_raw};

Expand All @@ -31,24 +35,22 @@ pub struct LogEntry {
/// We cannot directly use the type `Body` here, since body is Base64-encoded.
#[serde(rename = "body")]
pub body: String,

#[serde(rename = "integratedTime")]
pub integrated_time: usize,

/// This is the SHA256 hash of the DER-encoded public key for the log at the time the entry was
/// included in the log
/// Pattern: ^[0-9a-fA-F]{64}$
#[serde(rename = "logID")]
pub log_id: String,

/// Minimum: 0
#[serde(rename = "logIndex")]
pub log_index: u64,

/// Includes a signature over the body, integratedTime, logID, and logIndex.
#[serde(skip_serializing_if = "Option::is_none")]
#[serde(rename = "verification")]
pub verification: Option<LogEntryVerification>,
// #[serde(rename = "integratedTime")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is interesting, I guess these fields were just here for the canonicalization but we didn't actually need them for anything? In that case, do we think we may need any of them in the future (I cannot think of a reason, apart from perhaps logging the decoded log entry somewhere as part of the verification, which may actually be nice)

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree that a little more convenience is better, be it only for logging. The trouble with the canonicalisation shouldn't lead to removing this fields.

// pub integrated_time: usize,
// This is the SHA256 hash of the DER-encoded public key for the log at the time the entry was
// included in the log
// Pattern: ^[0-9a-fA-F]{64}$
// #[serde(rename = "logID")]
// pub log_id: String,

// Minimum: 0
// #[serde(rename = "logIndex")]
// pub log_index: u64,

// Includes a signature over the body, integratedTime, logID, and logIndex.
// #[serde(skip_serializing_if = "Option::is_none")]
// #[serde(rename = "verification")]
// pub verification: Option<LogEntryVerification>,
}

/// Struct representing the body in a Rekor LogEntry.
Expand Down Expand Up @@ -118,52 +120,53 @@ pub struct LogEntryVerification {
/// be obtained from the `/api/v1/log/publicKey` Rest API. For `sigstore.dev`, it is a PEM-encoded
/// x509/PKIX public key.
pub struct RekorSignatureBundle {
Copy link
Contributor

Choose a reason for hiding this comment

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

The type RekorSignatureBundle is dispensable, it is just a pair of two relatively unrelated things. Also not used outside this file. NB: This is just my personal taste, keep if you think differently.

// TODO: doc here
/// Canonicalized JSON representation, based on RFC 8785 rules, of a subset of a Rekor LogEntry
/// fields that are signed to generate `signedEntryTimestamp` (also a field in the Rekor
/// LogEntry). These fields include body, integratedTime, logID and logIndex.
pub canonicalized: Vec<u8>,
pub signed_data: Vec<u8>,

/// The signature over the canonicalized JSON document.
pub signature: Vec<u8>,
}

/// Converter for creating a RekorSignatureBundle from a Rekor LogEntry as described in
/// <https://github.com/sigstore/rekor/blob/4fcdcaa58fd5263560a82978d781eb64f5c5f93c/openapi.yaml#L433-L476>.
impl TryFrom<&LogEntry> for RekorSignatureBundle {
type Error = anyhow::Error;

fn try_from(log_entry: &LogEntry) -> anyhow::Result<Self> {
// Create a copy of the LogEntry, but skip the verification.
let entry_subset = LogEntry {
body: log_entry.body.clone(),
integrated_time: log_entry.integrated_time,
log_id: log_entry.log_id.clone(),
log_index: log_entry.log_index,
verification: None,
};

// Canonicalized JSON document that is signed. Canonicalization should follow the RFC 8785
// rules.
let canonicalized = serde_jcs::to_vec(&entry_subset)
.context("couldn't create canonicalized json string")?;

// Extract the signature from the LogEntry.
let sig_base64 = log_entry
.verification
.as_ref()
.ok_or_else(|| anyhow::anyhow!("no verification field in the log entry"))?
.signed_entry_timestamp
.clone();
let signature = BASE64_STANDARD
.decode(sig_base64)
.context("couldn't decode Base64 signature")?;

Ok(Self {
canonicalized,
signature,
})
}
}
// impl TryFrom<&LogEntry> for RekorSignatureBundle {
// type Error = anyhow::Error;

// fn try_from(log_entry: &LogEntry) -> anyhow::Result<Self> {
// // Create a copy of the LogEntry, but skip the verification.
// let entry_subset = LogEntry {
// body: log_entry.body.clone(),
// integrated_time: log_entry.integrated_time,
// log_id: log_entry.log_id.clone(),
// log_index: log_entry.log_index,
// verification: None,
// };

// // Canonicalized JSON document that is signed. Canonicalization should follow the RFC
// 8785 // rules.
// let signed_data = serde_jcs::to_vec(&entry_subset)
// .context("couldn't create canonicalized json string")?;

// // Extract the signature from the LogEntry.
// let sig_base64 = log_entry
// .verification
// .as_ref()
// .ok_or_else(|| anyhow::anyhow!("no verification field in the log entry"))?
// .signed_entry_timestamp
// .clone();
// let signature = BASE64_STANDARD
// .decode(sig_base64)
// .context("couldn't decode Base64 signature")?;

// Ok(Self {
// signed_data: signed_data,
// signature,
// })
// }
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove commented code (assuming it's just the original version kept around for reference).


/// Verifies a Rekor LogEntry. This includes verifying:
///
Expand Down Expand Up @@ -202,12 +205,14 @@ pub fn get_rekor_log_entry_body(log_entry: &[u8]) -> anyhow::Result<Body> {

/// Parses a blob into a Rekor log entry and verifies the signature in
/// `signedEntryTimestamp`` using Rekor's public key.
///
/// TODO: specify what log_entry needs to contain.
pub fn verify_rekor_signature(log_entry: &[u8], rekor_public_key: &[u8]) -> anyhow::Result<()> {
let signature_bundle = rekor_signature_bundle(log_entry)?;

verify_signature_raw(
&signature_bundle.signature,
&signature_bundle.canonicalized,
&signature_bundle.signed_data,
rekor_public_key,
)
.context("couldn't verify signedEntryTimestamp of the Rekor LogEntry")
Expand Down Expand Up @@ -255,10 +260,62 @@ pub fn verify_rekor_body(body: &Body, contents_bytes: &[u8]) -> anyhow::Result<(
.context("couldn't verify signature over the endorsement")
}

fn rekor_signature_bundle(log_entry: &[u8]) -> anyhow::Result<RekorSignatureBundle> {
let parsed: BTreeMap<String, LogEntry> =
serde_json::from_slice(log_entry).context("couldn't parse bytes into a LogEntry object")?;
let entry = parsed.values().next().context("no entry in the map")?;

RekorSignatureBundle::try_from(entry)
// TODO remove after tests. Keeping around for now to compare outputs from old and new.
// fn rekor_signature_bundle_old(log_entry_json_bytes: &[u8]) ->
// anyhow::Result<RekorSignatureBundle> { let parsed: BTreeMap<String, LogEntry> =
// serde_json::from_slice(log_entry_json_bytes) .context("couldn't parse bytes into a
// LogEntry object")?; let entry = parsed.values().next().context("no entry in the map")?;

// RekorSignatureBundle::try_from(entry)
// }

/// For a sample of expected JSON structure, see ../testdata/logentry.json .
fn rekor_signature_bundle(log_entry_json_bytes: &[u8]) -> anyhow::Result<RekorSignatureBundle> {
let mut log_entry_json = serde_json::from_slice::<Value>(log_entry_json_bytes)
.context("Couldn't parse bytes as JSON")?;

let log_entry_root_object = log_entry_json
.as_object_mut()
.context("JSON root expected to be a JSON object")?;

anyhow::ensure!(
log_entry_root_object.len() == 1,
"Expected exactly 1 entry in log entry root JSON object"
);

let log_entry_artifact_object = log_entry_root_object
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the method chain is quite long here, could you add a type annotation to help readers understand what type this is?

.values_mut()
.next()
.unwrap() // Already ensured one item must exist.
.as_object_mut()
.context("Artifact metadata expected to be a JSON object")?;

let verification: Value = log_entry_artifact_object
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you did the hard work of figuring out the actual logic upstream, it may be useful to document that more clearly here.

.remove("verification")
.context("'verification' key not found in artifact JSON")?;

// TODO does this always equal canonicalized?
// Note on preserving order: https://users.rust-lang.org/t/how-to-keep-order-after-using-serde-json-from-str-to-deserialize-a-string-to-struct/97727
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a good question -- if the fields are reordered, does this still work? Either way, it may be useful to add a few more test cases to show that (some valid , some not)


let mut serializer = Serializer::with_formatter(Vec::new(), CanonicalFormatter::new());
log_entry_artifact_object
.serialize(&mut serializer)
.expect("Failed to serialize Rekor signed payload to JSON");
Copy link
Contributor

Choose a reason for hiding this comment

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

expect panics and does not return an error. Here the message should be returned as Result.


let signed_json_bytes = serializer.into_inner();
Copy link
Contributor

Choose a reason for hiding this comment

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

Make a private function which encapsulates Serializer and CanonicalFormatter?


let signed_entry_timestamp_base64_encoded = verification
.as_object()
.context("Expected 'verification' entry to contain a JSON object")?["signedEntryTimestamp"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this will panic if it does not contain that field, which is probably undesirable

Copy link
Contributor

Choose a reason for hiding this comment

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

I stumbled over the naming signedEntryTimestamp just now. It is not a timestamp (as the name suggests), but a signature. Maybe at least reflect in the variable names/comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, SG! It's signed entry timestamp as in signed (entry timestamp), not (signed entry) timestamp 😂 . I'll add a comment.

.as_str()
.context("Expected 'signedEntryTimestamp' entry to contain a JSON string")?;

let signed_entry_timestamp_bytes = BASE64_STANDARD
.decode(signed_entry_timestamp_base64_encoded)
.context("Couldn't Base64 decode signedEntryTimestap")?;

Ok(RekorSignatureBundle {
signed_data: signed_json_bytes,
signature: signed_entry_timestamp_bytes,
})
}
36 changes: 35 additions & 1 deletion oak_attestation_verification/testdata/logentry.json
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to use the pretty printed JSON I think (so that's good). Saving a few bytes of whitespace is of no importance.

Original file line number Diff line number Diff line change
@@ -1 +1,35 @@
{"24296fb24b8ad77a51d549703a3a1c2dd2639ba49617fc563854031cb93e6d354e7b005065c334a8":{"body":"eyJhcGlWZXJzaW9uIjoiMC4wLjEiLCJraW5kIjoicmVrb3JkIiwic3BlYyI6eyJkYXRhIjp7Imhhc2giOnsiYWxnb3JpdGhtIjoic2hhMjU2IiwidmFsdWUiOiI4ZTA2Nzk1ODA5NjM3ZTI3MjNmNjQzODE5MTQ3NzU4NGRhOTI2MjQ2MTZmMTI2MDViODIwZjg1NjUzMDcyYzA5In19LCJzaWduYXR1cmUiOnsiY29udGVudCI6Ik1FVUNJUUQvaHJ3OWVjVlpHRE0zMUIycXE1dEdaNFZtSGNuRytDVml0NW93VURjK2RRSWdUQVI2V2FuY0ZaZUtuNzgwRmRTNkIxZ0cxakNlejFsTXZsTHFtdFBVc28wPSIsImZvcm1hdCI6Ing1MDkiLCJwdWJsaWNLZXkiOnsiY29udGVudCI6IkxTMHRMUzFDUlVkSlRpQlFWVUpNU1VNZ1MwVlpMUzB0TFMwS1RVWnJkMFYzV1VoTGIxcEplbW93UTBGUldVbExiMXBKZW1vd1JFRlJZMFJSWjBGRlVqUmlOUzlNWlZsNE9WZHpOMm93TVVZelNEUlJRVk5rYm1sVVF3cHhaakpJV1cxNEsyOVNLeklyVms1SmRtRllWRTVtVEU1WldHUTVTMVo0YW5OcllqRlVhMHMzU0VjeGVFVTVSMXA0ZW1waWQwWkRkWGxCUFQwS0xTMHRMUzFGVGtRZ1VGVkNURWxESUV0RldTMHRMUzB0Q2c9PSJ9fX19","integratedTime":1691754247,"logID":"c0d23d6ad406973f9559f3ba2d1ca01f84147d8ffc5b8445c224f98b9591801d","logIndex":30891523,"verification":{"inclusionProof":{"checkpoint":"rekor.sigstore.dev - 2605736670972794746\n26728094\nUMgdlhClBSzBbqefL5wyKzDrSb/Wf1yic2lWuqc490o=\nTimestamp: 1691754248454344594\n\n— rekor.sigstore.dev wNI9ajBFAiEAuhZGCMHAXSu1zARzPjKeUQF4i1jY+45EmKodcfQofE4CICxsQ/OxAP0r+9wLT+1PzDuF/kZ5LJ44k6f0oueozZBf\n","hashes":["030b9e9fb6a20219790c620da0677ae9a9d551300d5d53677d2e889b18f93408","665e92da8bb3ecfec55d7084c2e680da9627140ceebd5b90443194974478aaae","d493bd198b0273aaadd90b15daae59f8c437ad7669b1ef0f35ee3bdfcccb0c1c","1c4f5d27f667cf8fdfab11719cb2700c43b6ec0699c0e906582f81cc5bbe627f","6f77ba99b9061179f7cf7d94ad3fafe88137ec4939f7a2855caf7a25b6b4c3eb","f72f831d5e9f5c86157f56bc850d0c505d0baa8af389c91689b5c002b83e47c3","85bbefe750579844c4ef01ba7e50ee147867768adf376df59a7d46d9061a0529","e9f1cc1f52ef6fafa3c87d2c2031f14ef16da2ac47c267601a97c1671307c313","91e4eaeb84796946c5ad1570ea06f4fd07ee9261526b696c251119d888e641e2","e4a5b55c06b38419780a1a1b34b7e1ab1329b55948a105df191ec58325ae6220","1127441051032e9e2b9a7ff43ac6a8d4133438354f8b295c37b23f6292569ff5","fda678e668dd9896f1cdbf160943a690da123917de48afe85edd6c494d08e1b9","5cf299a407ce2c41b16dc87bd3bc7396ba9426d1b5e43ba70bbd979e417c45e6","ba60819f9a3f9ddabeb6ec73d1ba79d04fd2ad69ce8d95c777ab485a9fadb36b","8d152ae03f0ef85238ed66f0f7ab3bc870aee2acd6531a4855fc5011ea6b0e67","ad712c98424de0f1284d4f144b8a95b5d22c181d4c0a246518e7a9a220bdf643"],"logIndex":26728092,"rootHash":"50c81d9610a5052cc16ea79f2f9c322b30eb49bfd67f5ca2736956baa738f74a","treeSize":26728094},"signedEntryTimestamp":"MEYCIQCCN9ip/cW7QfS4EbLyigCs4OKz4wcWUQThuQY00i3PZAIhAKsTz7epe3Gh/9XGLzh4L1yPqcGUCETPPckPvMIZbL/7"}}}
{
"24296fb24b8ad77a51d549703a3a1c2dd2639ba49617fc563854031cb93e6d354e7b005065c334a8": {
"body": "eyJhcGlWZXJzaW9uIjoiMC4wLjEiLCJraW5kIjoicmVrb3JkIiwic3BlYyI6eyJkYXRhIjp7Imhhc2giOnsiYWxnb3JpdGhtIjoic2hhMjU2IiwidmFsdWUiOiI4ZTA2Nzk1ODA5NjM3ZTI3MjNmNjQzODE5MTQ3NzU4NGRhOTI2MjQ2MTZmMTI2MDViODIwZjg1NjUzMDcyYzA5In19LCJzaWduYXR1cmUiOnsiY29udGVudCI6Ik1FVUNJUUQvaHJ3OWVjVlpHRE0zMUIycXE1dEdaNFZtSGNuRytDVml0NW93VURjK2RRSWdUQVI2V2FuY0ZaZUtuNzgwRmRTNkIxZ0cxakNlejFsTXZsTHFtdFBVc28wPSIsImZvcm1hdCI6Ing1MDkiLCJwdWJsaWNLZXkiOnsiY29udGVudCI6IkxTMHRMUzFDUlVkSlRpQlFWVUpNU1VNZ1MwVlpMUzB0TFMwS1RVWnJkMFYzV1VoTGIxcEplbW93UTBGUldVbExiMXBKZW1vd1JFRlJZMFJSWjBGRlVqUmlOUzlNWlZsNE9WZHpOMm93TVVZelNEUlJRVk5rYm1sVVF3cHhaakpJV1cxNEsyOVNLeklyVms1SmRtRllWRTVtVEU1WldHUTVTMVo0YW5OcllqRlVhMHMzU0VjeGVFVTVSMXA0ZW1waWQwWkRkWGxCUFQwS0xTMHRMUzFGVGtRZ1VGVkNURWxESUV0RldTMHRMUzB0Q2c9PSJ9fX19",
"integratedTime": 1691754247,
"logID": "c0d23d6ad406973f9559f3ba2d1ca01f84147d8ffc5b8445c224f98b9591801d",
"logIndex": 30891523,
"verification": {
"inclusionProof": {
"checkpoint": "rekor.sigstore.dev - 2605736670972794746\n26728094\nUMgdlhClBSzBbqefL5wyKzDrSb/Wf1yic2lWuqc490o=\nTimestamp: 1691754248454344594\n\n— rekor.sigstore.dev wNI9ajBFAiEAuhZGCMHAXSu1zARzPjKeUQF4i1jY+45EmKodcfQofE4CICxsQ/OxAP0r+9wLT+1PzDuF/kZ5LJ44k6f0oueozZBf\n",
"hashes": [
"030b9e9fb6a20219790c620da0677ae9a9d551300d5d53677d2e889b18f93408",
"665e92da8bb3ecfec55d7084c2e680da9627140ceebd5b90443194974478aaae",
"d493bd198b0273aaadd90b15daae59f8c437ad7669b1ef0f35ee3bdfcccb0c1c",
"1c4f5d27f667cf8fdfab11719cb2700c43b6ec0699c0e906582f81cc5bbe627f",
"6f77ba99b9061179f7cf7d94ad3fafe88137ec4939f7a2855caf7a25b6b4c3eb",
"f72f831d5e9f5c86157f56bc850d0c505d0baa8af389c91689b5c002b83e47c3",
"85bbefe750579844c4ef01ba7e50ee147867768adf376df59a7d46d9061a0529",
"e9f1cc1f52ef6fafa3c87d2c2031f14ef16da2ac47c267601a97c1671307c313",
"91e4eaeb84796946c5ad1570ea06f4fd07ee9261526b696c251119d888e641e2",
"e4a5b55c06b38419780a1a1b34b7e1ab1329b55948a105df191ec58325ae6220",
"1127441051032e9e2b9a7ff43ac6a8d4133438354f8b295c37b23f6292569ff5",
"fda678e668dd9896f1cdbf160943a690da123917de48afe85edd6c494d08e1b9",
"5cf299a407ce2c41b16dc87bd3bc7396ba9426d1b5e43ba70bbd979e417c45e6",
"ba60819f9a3f9ddabeb6ec73d1ba79d04fd2ad69ce8d95c777ab485a9fadb36b",
"8d152ae03f0ef85238ed66f0f7ab3bc870aee2acd6531a4855fc5011ea6b0e67",
"ad712c98424de0f1284d4f144b8a95b5d22c181d4c0a246518e7a9a220bdf643"
],
"logIndex": 26728092,
"rootHash": "50c81d9610a5052cc16ea79f2f9c322b30eb49bfd67f5ca2736956baa738f74a",
"treeSize": 26728094
},
"signedEntryTimestamp": "MEYCIQCCN9ip/cW7QfS4EbLyigCs4OKz4wcWUQThuQY00i3PZAIhAKsTz7epe3Gh/9XGLzh4L1yPqcGUCETPPckPvMIZbL/7"
}
}
}