Remove serde_jcs dependency #4641

ernoc · 2024-01-10T15:33:25Z

And make Rekor signature verification more robust.

WIP WIP WIP - feedback wanted:

For the purpose of verifying the Rekor LogEntry signedEntryTimestap field, we don't really care about the contests of the rest of the JSON. That is, we extract root[0].verification.signedEntryTimestamp, but the rest of root[0] (having removed verification, what we really want is its verbatim text.

At the moment we parse and re-render the entire thing: parse JSON bytes into Rust structs --> process --> render to JSON bytes. Parsing will break if Rekor changes the structure in any way, including changes to parts we don't care about .

We render RFC 8785 compliant text as suggested by Rekor here. If there were any differences (e.g. Rekor's payload didn't really comform to RFC 8785, or our output doesn't, or somehow there's a yet undiscovered ambiguity in the standard such that 2 different byte arrays are compliant), then this part breaks. The only solution tough would be doing complicated string manipulation which I'm not suggesting.

I'm proposing:

Don't fully parse the structure as we don't need most of it. Thus our code becomes robust to changes in the bits we don't care about.
Try rendering the rest of JSON using generic JSON structures and serde_json::to_string. Given the structure only contains integers and strings, the only bit that would be likely to cause problems is field ordering, which may be solvable with serde_json. So I'd write a bunch of tests and try to disprove this. If it fails, then I'd add serde_canonical_json as a dependency.

Thoughts?

ernoc · 2024-01-10T15:34:03Z

oak_attestation_verification/testdata/logentry.json

@@ -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"}}}


Just pretty printing so it's easier to refer to this while reading test code.

ernoc · 2024-01-10T15:45:39Z

On second thoughts, even if with the current structure, serde_json::to_vec produces an output that is canonical, if Rekor adds/changes fields in the future (e.g. to include a date), then our verifier will stop working. So I'll try using serde_canonical_json now.

thmsbinder · 2024-01-10T15:42:46Z

oak_attestation_verification/src/rekor.rs

@@ -159,7 +161,7 @@ impl TryFrom<&LogEntry> for RekorSignatureBundle {
            .context("couldn't decode Base64 signature")?;

        Ok(Self {
-            canonicalized,
+            signed_data: signed_data,


Just signed_data should be enough?

…e robust.

ernoc · 2024-01-10T17:23:27Z

Ok so I tried with serde_canonical_json and it seems to work. Also ryu-js dep is gone.

2 things I'd like your opinion on are:

I believe it's good to keep structures like LogEntry for the cases where we use specific fields from it. But we only use body and verification. I'd remove all the others to avoid being fragile to changes in fields we don't care about, but leave a note that there exist more fields and a link to Rekor docs.
How much testing around rekor.rs do you think we need, and how much variation of samples of log entry JSON payloads can we get? It seems to me that the one example we have is quite representative, although I'd add a couple of "bad" examples to test that we are resilient to them / get the expected error messages.

tiziano88

thanks, this seems much more resilient to future changes

tiziano88 · 2024-01-10T17:44:08Z

oak_attestation_verification/src/rekor.rs

@@ -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;
+


(nit) remove blank line

tiziano88 · 2024-01-10T17:45:49Z

oak_attestation_verification/src/rekor.rs

-    #[serde(skip_serializing_if = "Option::is_none")]
-    #[serde(rename = "verification")]
-    pub verification: Option<LogEntryVerification>,
+    // #[serde(rename = "integratedTime")]


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)

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.

tiziano88 · 2024-01-10T17:47:55Z

oak_attestation_verification/src/rekor.rs

+        .as_object_mut()
+        .context("Artifact metadata expected to be a JSON object")?;
+
+    let verification: Value = log_entry_artifact_object


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

tiziano88 · 2024-01-10T17:49:26Z

oak_attestation_verification/src/rekor.rs

+        "Expected exactly 1 entry in log entry root JSON object"
+    );
+
+    let log_entry_artifact_object = log_entry_root_object


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

tiziano88 · 2024-01-10T17:59:38Z

oak_attestation_verification/src/rekor.rs

+        .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


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)

tiziano88 · 2024-01-10T18:01:03Z

oak_attestation_verification/src/rekor.rs

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


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

thmsbinder · 2024-01-11T09:18:12Z

oak_attestation_verification/src/rekor.rs

+//             signature,
+//         })
+//     }
+// }


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

thmsbinder · 2024-01-11T09:53:55Z

oak_attestation_verification/src/rekor.rs

@@ -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 {


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.

thmsbinder · 2024-01-11T10:33:13Z

oak_attestation_verification/src/rekor.rs

-    #[serde(skip_serializing_if = "Option::is_none")]
-    #[serde(rename = "verification")]
-    pub verification: Option<LogEntryVerification>,
+    // #[serde(rename = "integratedTime")]


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.

thmsbinder · 2024-01-11T10:40:00Z

oak_attestation_verification/testdata/logentry.json

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

thmsbinder · 2024-01-11T10:41:39Z

oak_attestation_verification/src/rekor.rs

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


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?

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

thmsbinder · 2024-01-11T10:46:57Z

oak_attestation_verification/src/rekor.rs

+        .serialize(&mut serializer)
+        .expect("Failed to serialize Rekor signed payload to JSON");
+
+    let signed_json_bytes = serializer.into_inner();


Make a private function which encapsulates Serializer and CanonicalFormatter?

thmsbinder · 2024-01-11T10:49:56Z

oak_attestation_verification/src/rekor.rs

+    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");


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

ernoc requested a review from thmsbinder January 10, 2024 15:33

ernoc commented Jan 10, 2024

View reviewed changes

ernoc force-pushed the oak-rekor branch from bcb5555 to 99dd502 Compare January 10, 2024 15:35

ernoc requested a review from tiziano88 January 10, 2024 15:36

thmsbinder reviewed Jan 10, 2024

View reviewed changes

Remove serde_jcs dependency and make Rekor signature verification mor…

dd1e4fe

…e robust.

ernoc force-pushed the oak-rekor branch from 99dd502 to dd1e4fe Compare January 10, 2024 17:18

ernoc marked this pull request as draft January 10, 2024 17:19

tiziano88 reviewed Jan 10, 2024

View reviewed changes

thmsbinder reviewed Jan 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove serde_jcs dependency #4641

Remove serde_jcs dependency #4641

ernoc commented Jan 10, 2024

ernoc Jan 10, 2024

ernoc commented Jan 10, 2024

thmsbinder Jan 10, 2024

ernoc commented Jan 10, 2024

tiziano88 left a comment

tiziano88 Jan 10, 2024

tiziano88 Jan 10, 2024

thmsbinder Jan 11, 2024

tiziano88 Jan 10, 2024

tiziano88 Jan 10, 2024

tiziano88 Jan 10, 2024

tiziano88 Jan 10, 2024

thmsbinder Jan 11, 2024

thmsbinder Jan 11, 2024

thmsbinder Jan 11, 2024

thmsbinder Jan 11, 2024

thmsbinder Jan 11, 2024

ernoc Jan 11, 2024

thmsbinder Jan 11, 2024

thmsbinder Jan 11, 2024

		@@ -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"}}}

Remove serde_jcs dependency #4641

Are you sure you want to change the base?

Remove serde_jcs dependency #4641

Conversation

ernoc commented Jan 10, 2024

Choose a reason for hiding this comment

ernoc commented Jan 10, 2024

Choose a reason for hiding this comment

ernoc commented Jan 10, 2024

tiziano88 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment