Skip to content

Conversation

@vembacher
Copy link
Contributor

@vembacher vembacher commented Jul 19, 2023

Summary

Related to: #283

This adds implementations for:

  • consistency proofs
  • inclusion proofs
  • checkpoint/STH related functionality

The Merkle proofs are essentially ports of the transparency-dev implementations, including the test suite.
The checkpoint related code is based on the implementation in the rekor Go package.

I also changed the Rekor models to use the new SignedCheckpoint type, which implements the serde traits. For now I think this is the only major breaking change, the rest are mostly new or private APIs.

I have not implemented the logic to verify that Checkpoints and the corresponding consistency/inclusion proof are sound together. I want to discuss how to handle this here.

Release Note

  • added functions to relevant Rekor models to support the verification of Merkle inclusion and consistency proofs
  • added addition variants to SigstoreError enum
  • added Checkpoint type to handle verification of checkpoints/STHs
  • changed type of signed_tree_head fiekd from String to Checkpoint
  • added field checkpoint: Option<Checkpoint> to InclusionProof struct

Documentation

  • relevant new features are documented via RustDoc or examples
  • breaking changes primarily affect LogInfo and LogEntry due to changes to struct fields, this should only require minor changes

Todos:

  • add examples
  • add tests for Rekor models verification functions
  • write change-log
  • discuss how to verify that checkpoint + proof are valid together
  • update Summary, Release Notes, Documentation once code is finalized.

@lukehinds
Copy link
Member

One for the TODO would be something for examples/

Nice work btw!

@vembacher
Copy link
Contributor Author

I added examples for consistency and inclusion proofs.

I will also add some tests in the future to the methods I added to the Rekor models. What test data should I use for this? As these require log entries + keys.

Also I'm unsure whether my checks to ensure whether proofs and checkpoints are valid together are correct. What is the correct way to do this?

@flavio
Copy link
Member

flavio commented Sep 7, 2023

Sorry, this has been sitting on my review list for a long time. I need some time to get familiar with the feature being implemented 😓

@vembacher
Copy link
Contributor Author

Sorry, this has been sitting on my review list for a long time. I need some time to get familiar with the feature being implemented 😓

No worries! I'm not in a rush to complete this.

@gaetanww
Copy link

I received an error with the current implementation when I tested for the entry with index 25579.

Problem

I received the following error although the verification should have succeeded.

InclusionProofError(MismatchedRoot { expected: "db0822d0508b54bfb770ba5d3394d331bd9a22c91d9a8c0b906e7ea2208903be", got: "4d006aa46efcb607dd51d900b1213754c50cc9251c3405c6c2561d9d6a2f3239" })

Underlying problem

The underlying error is, I believe, related to the assumption that the body is formatted as canonical JSON when it is appended to the log. Indeed, to verify that the body is included in the merkle tree, the implementation currently reserializes the body using a canonical formatter. However, I think the original entry wasn't formatted using a canonical formatter (see files below).

To fix

We can't make the assumption that the body was formatted canonically, so I think the solution should be to keep the "original" base64-decoded body as bytes in the LogEntry struct and use this to verify the inclusion proof instead of reserializing it.

If you agree with the fix, I'm happy to work on it.

To reproduce

The following code:

let rekor_config = Configuration::default();
let rt = tokio::runtime::Runtime::new().unwrap();
let log_entry = rt.block_on(get_log_entry_by_index(&rekor_config, 25579)).unwrap();
let mut encoded = vec![];
let mut ser = serde_json::Serializer::with_formatter(
    &mut encoded,
    olpc_cjson::CanonicalFormatter::new(),
);
log_entry.body.serialize(&mut ser).unwrap();
println!("{}", String::from_utf8(encoded).unwrap());

returns:

{"apiVersion":"0.0.1","kind":"rekord","spec":{"data":{"hash":{"algorithm":"sha256","value":"ce9a7c82f32194995888758cf107ef0cc52e0b8cdce73b4240658ee9e73783cb"}},"signature":{"content":"MGUCMD3oKzgsGnPAkJEXegDIsdlh4BFCQbM6jng4Sy3axY/+2tlK97oe/CkxabT1ZXUqCAIxAJDq+zLfRZZEJD5DvaKhFEu+Jm+jD4UXc3CaZp2MSajiralmtalA6fSGCXjwGfUzOw==","format":"x509","publicKey":{"content":"LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUNrakNDQWhpZ0F3SUJBZ0lVQU0rK0dYRFN5bUNPSW82YmxMMG5EZngxb21nd0NnWUlLb1pJemowRUF3TXcKS2pFVk1CTUdBMVVFQ2hNTWMybG5jM1J2Y21VdVpHVjJNUkV3RHdZRFZRUURFd2h6YVdkemRHOXlaVEFlRncweQpNVEEzTWpnd09ETTNOREphRncweU1UQTNNamd3T0RVM05ERmFNQUF3ZGpBUUJnY3Foa2pPUFFJQkJnVXJnUVFBCklnTmlBQVJjMDMrUU4vTHBrOGpqUFQwTmV5a01ucm9mMnpZUkJxNm05ei9TMXhRSzduZnZhU3grRjUrTEtwN3gKR2ExbHY2SWNvRXdwUHA2MUdsYnd5S0VQVWJLdzJrYnJyRVpPMnhKV3kxb0VEUHBYMlJqcTBYS0RZcEF5Zi9mQwoyZzJjSnVtamdnRW5NSUlCSXpBT0JnTlZIUThCQWY4RUJBTUNCNEF3RXdZRFZSMGxCQXd3Q2dZSUt3WUJCUVVICkF3TXdEQVlEVlIwVEFRSC9CQUl3QURBZEJnTlZIUTRFRmdRVTRBUDhtTkI4ejhSZFJyTVVLZ1A2Mm0xUFErd3cKSHdZRFZSMGpCQmd3Rm9BVXlNVWRBRUdhSkNreVVTVHJEYTVLN1VvRzArd3dnWTBHQ0NzR0FRVUZCd0VCQklHQQpNSDR3ZkFZSUt3WUJCUVVITUFLR2NHaDBkSEE2THk5d2NtbDJZWFJsWTJFdFkyOXVkR1Z1ZEMwMk1ETm1aVGRsCk55MHdNREF3TFRJeU1qY3RZbVkzTlMxbU5HWTFaVGd3WkRJNU5UUXVjM1J2Y21GblpTNW5iMjluYkdWaGNHbHoKTG1OdmJTOWpZVE0yWVRGbE9UWXlOREppT1daallqRTBOaTlqWVM1amNuUXdIZ1lEVlIwUkFRSC9CQlF3RW9FUQpZM1JoWkdWMVFHZHRZV2xzTG1OdmJUQUtCZ2dxaGtqT1BRUURBd05vQURCbEFqRUE3TTJwSzhRUFRrSGs1bzZ0CmdnampZdjBLV1BUajRKUTAwU3RjR0xqa1g3SU1iNC9HdXpYRkQ4czZDOEd3NmpwMEFqQW1Xa2JROTVsMzlnUGQKR2pjUjBSQURaT0dYb0NPQURwOE5lSzhBL2dKdWdnR0ZINHZYZ2l1ODJsQm5MOEZSc09jPQotLS0tLUVORCBDRVJUSUZJQ0FURS0tLS0tCg=="}}}}

when a direct call to the api:

curl 'https://rekor.sigstore.dev/api/v1/log/entries?logIndex=25579' | jq -r '.[ (keys_unsorted)[0] ].body' | base64 -d

returns:

{"apiVersion":"0.0.1","spec":{"data":{"hash":{"algorithm":"sha256","value":"ce9a7c82f32194995888758cf107ef0cc52e0b8cdce73b4240658ee9e73783cb"}},"signature":{"content":"MGUCMD3oKzgsGnPAkJEXegDIsdlh4BFCQbM6jng4Sy3axY/+2tlK97oe/CkxabT1ZXUqCAIxAJDq+zLfRZZEJD5DvaKhFEu+Jm+jD4UXc3CaZp2MSajiralmtalA6fSGCXjwGfUzOw==","format":"x509","publicKey":{"content":"LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUNrakNDQWhpZ0F3SUJBZ0lVQU0rK0dYRFN5bUNPSW82YmxMMG5EZngxb21nd0NnWUlLb1pJemowRUF3TXcKS2pFVk1CTUdBMVVFQ2hNTWMybG5jM1J2Y21VdVpHVjJNUkV3RHdZRFZRUURFd2h6YVdkemRHOXlaVEFlRncweQpNVEEzTWpnd09ETTNOREphRncweU1UQTNNamd3T0RVM05ERmFNQUF3ZGpBUUJnY3Foa2pPUFFJQkJnVXJnUVFBCklnTmlBQVJjMDMrUU4vTHBrOGpqUFQwTmV5a01ucm9mMnpZUkJxNm05ei9TMXhRSzduZnZhU3grRjUrTEtwN3gKR2ExbHY2SWNvRXdwUHA2MUdsYnd5S0VQVWJLdzJrYnJyRVpPMnhKV3kxb0VEUHBYMlJqcTBYS0RZcEF5Zi9mQwoyZzJjSnVtamdnRW5NSUlCSXpBT0JnTlZIUThCQWY4RUJBTUNCNEF3RXdZRFZSMGxCQXd3Q2dZSUt3WUJCUVVICkF3TXdEQVlEVlIwVEFRSC9CQUl3QURBZEJnTlZIUTRFRmdRVTRBUDhtTkI4ejhSZFJyTVVLZ1A2Mm0xUFErd3cKSHdZRFZSMGpCQmd3Rm9BVXlNVWRBRUdhSkNreVVTVHJEYTVLN1VvRzArd3dnWTBHQ0NzR0FRVUZCd0VCQklHQQpNSDR3ZkFZSUt3WUJCUVVITUFLR2NHaDBkSEE2THk5d2NtbDJZWFJsWTJFdFkyOXVkR1Z1ZEMwMk1ETm1aVGRsCk55MHdNREF3TFRJeU1qY3RZbVkzTlMxbU5HWTFaVGd3WkRJNU5UUXVjM1J2Y21GblpTNW5iMjluYkdWaGNHbHoKTG1OdmJTOWpZVE0yWVRGbE9UWXlOREppT1daallqRTBOaTlqWVM1amNuUXdIZ1lEVlIwUkFRSC9CQlF3RW9FUQpZM1JoWkdWMVFHZHRZV2xzTG1OdmJUQUtCZ2dxaGtqT1BRUURBd05vQURCbEFqRUE3TTJwSzhRUFRrSGs1bzZ0CmdnampZdjBLV1BUajRKUTAwU3RjR0xqa1g3SU1iNC9HdXpYRkQ4czZDOEd3NmpwMEFqQW1Xa2JROTVsMzlnUGQKR2pjUjBSQURaT0dYb0NPQURwOE5lSzhBL2dKdWdnR0ZINHZYZ2l1ODJsQm5MOEZSc09jPQotLS0tLUVORCBDRVJUSUZJQ0FURS0tLS0tCg=="}}},"kind":"rekord"}

Note that the the "kind" field is at the end of the json in the API and in second place in the canonicalized format.

@tnytown
Copy link
Contributor

tnytown commented Feb 16, 2024

Thanks for the thorough triage @gaetanww!

I won't speak too much on this as I haven't tried this out myself, but I suspect that the issue you're seeing regarding canonicalization may be related to the canonicalizer being used. This implementation currently uses olpc_cjson, while Rekor uses RFC 8785 canonicalization. (OLPC canonical JSON is regrettably distinct from RFC 8785 canonical JSON.) In the sign module, we've used json-syntax to canonicalize the LogEntry body in accordance with RFC 8785 rules.

@gaetanww
Copy link

gaetanww commented Feb 16, 2024

np! Thanks for the pointer, yes that could be it. I think the following code should work (using the json_syntax crate with features ["canonicalize", "serde_json"]). I won't have time to try it out today though.

pub fn verify_inclusion(&self, rekor_key: &CosignVerificationKey) -> Result<(), SigstoreError> {
        self.verification
            .inclusion_proof
            .as_ref()
            .ok_or(UnexpectedError("missing inclusion proof".to_string()))
            .and_then(|proof| {
                // encode as canonical JSON
                let mut json_value = json_syntax::Value::from_serde_json(serde_json::to_value(&self.body)?);
                json_value.canonicalize();
                let encoded_entry = json_value.compact_print().to_string().into_bytes();
                proof.verify(&encoded_entry, rekor_key)
            })
    }

@vembacher
Copy link
Contributor Author

I hopefully should be able to take a closer look at this some time next week, and apply the fix/update the PR.

@gaetanww
Copy link

gaetanww commented Feb 16, 2024

One more data point on issue above. It works perfectly without modification with index: 71844338, however, my hand-rolled merkle tree verification now fails. My hand-rolled implementation is just using the b64 decoded body in the json, so the underlying issue might not be canonicalization.

@vembacher
Copy link
Contributor Author

I haven't looked at it in a few months, but if I remember correctly there are two types of indices. With one being the index in the log and one being the index in the tree. So you might be using the incorrect index.

@vembacher
Copy link
Contributor Author

@gaetanww is your issue resolved?

@gaetanww
Copy link

Sorry for the slow response. I made it work for the latest rekor records (e.g., index 71844338), the early one (25579) still fails AFAICT.

@vembacher
Copy link
Contributor Author

vembacher commented Mar 18, 2024

I did some investigating, I think @gaetanww your original analysis was correct and the formatting of the log entry is the issue, as both serializers produce identical outputs for me.

The Go client also uses the Base64 encoded entry provided by the log, is this the correct way to handle this? I feel like this might conflict with the description on how SETs are supposed to be verified.

@gaetanww
Copy link

Yes, the documentation does conflict with the actual definition. There should at least be a comment in the go code to explain why it's implemented that way.
I think we should raise the point with the maintainers of the cosign project. I'll open an issue.

Relates to: sigstore#283

Signed-off-by: Victor Embacher <victor@embacher.xyz>
…added some functionality.

Relates to: sigstore#283

Signed-off-by: Victor Embacher <victor@embacher.xyz>
Signed-off-by: Victor Embacher <victor@embacher.xyz>
…tions.

Signed-off-by: Victor Embacher <victor@embacher.xyz>
Signed-off-by: Victor Embacher <victor@embacher.xyz>
Signed-off-by: Victor Embacher <victor@embacher.xyz>
@exFalso
Copy link

exFalso commented Jun 6, 2024

This is failing on 32bit targets because of the use of usize for bitwise arithmetic. Changing to u64 fixes it.

@tannaurus
Copy link
Contributor

Howdy 👋 is there someone in the community equipped to adequately review this MR? We at 1Password would be happy to review elements of this work but the core team that has directed energy towards this project doesn't have previous experience with merkle tree proof implementations.

@loosebazooka
Copy link
Member

@tannaurus I'm going to look at the merkle tree proof as I previously did a lot of work on a different client there. However I would need someone else to do the rust review.

@vembacher
Copy link
Contributor Author

vembacher commented Aug 19, 2024

This is failing on 32bit targets because of the use of usize for bitwise arithmetic. Changing to u64 fixes it.

@exFalso

I will update it to u64, it seems like RFC6962 also prescribes a uint64.

Signed-off-by: Victor Embacher <victor@embacher.xyz>
@loosebazooka
Copy link
Member

Sorry my ability to read rust is bad.

I'm a little confused by the edits in rekor/models -- those seem like generated files? But the added code doesn't seem generated? Maybe I'm just reading rust wrong.

The inclusion proof looks like its following the spec in 6962, there's a slightly different implementation in 9162 -- I don't know if it matters but it's not recursive.

It might be valuable to add some tests on the rekor inclusion proof itself (rather than the sub-functions):

one random example (from the java client):

      "inclusionProof": {
        "hashes": [
          "810320ec3029914695826d60133c67021f66ee0cfb09a6f79eb267ed9f55de2c",
          "67e9d9f66f0ad388f7e1a20991e9a2ae3efad5cbf281e8b3d2aaf1ef99a4618c",
          "16a106400c53465f6e18c2475df6ba889ca30f5667bacf32b1a5661f14a5080c",
          "b4439e8d71edbc96271723cb7a969dd725e23e73d139361864a62ed76ce8dc11",
          "49b3e90806c7b63b5a86f5748e3ecb7d264ea0828eb74a45bc1a2cd7962408e8",
          "5059ad9b48fa50bd9adcbff0dd81c5a0dcb60f37e0716e723a33805a464f72f8",
          "6c2ce64219799e61d72996884eee9e19fb906e4d7fa04b71625fde4108f21762",
          "784f79c817abb78db3ae99b6c1ede640470bf4bb678673a05bf3a6b50aaaddd6",
          "c6d92ebf4e10cdba500ca410166cd0a8d8b312154d2f45bc4292d63dea6112f6",
          "1768732027401f6718b0df7769e2803127cfc099eb130a8ed7d913218f6a65f6",
          "0da021f68571b65e49e926e4c69024de3ac248a1319d254bc51a85a657b93c33",
          "bc8cf0c8497d5c24841de0c9bef598ec99bbd59d9538d58568340646fe289e9a",
          "be328fa737b8fa9461850b8034250f237ff5b0b590b9468e6223968df294872b",
          "6f06f4025d0346f04830352b23f65c8cd9e3ce4b8cb899877c35282521ddaf85"
        ],
        "logIndex": 1227,
        "rootHash": "effa4fa4575f72829016a64e584441203de533212f9470d63a56d1992e73465d",
        "treeSize": 14358,
        "checkpoint": "rekor.sigstage.dev - 108574341321668964\n14358\n7/pPpFdfcoKQFqZOWERBID3lMyEvlHDWOlbRmS5zRl0=\n\n— rekor.sigstage.dev 0y8wozBFAiB8OkuzdwlL6/rDEu2CsIfqmesaH/KLfmIMvlH3YTdIYgIhAPFZeXK6+b0vbWy4GSU/YZxiTpFrrzjsVOShN4LlPdZb\n"
      },

and then another failure test where you mangle this data a bit.

viccuad and others added 2 commits November 18, 2025 17:06
Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
Signed-off-by: Víctor Cuadrado Juan <2196685+viccuad@users.noreply.github.com>
viccuad
viccuad previously approved these changes Nov 18, 2025
Copy link
Collaborator

@viccuad viccuad left a comment

Choose a reason for hiding this comment

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

Hi everybody, many thanks to @vembacher and all the reviewers of this PR, it's been quite the journey! I'm infinitely glad for the effort you put in this PR.

I had a long ramp-up time on the cryptography and Merkle trees, and understanding the RFCs 6962 and 9162 this last week. After this, I can say I can understand the feature good enough to review it. The PR looks great to me in its core implementation. I have added some minor comments and pushed some changes to make it compile and pass linters, I hope it's ok. I found this the best way to try to move it forward.

There's some hurdles as it stands:

  • The format of consistency proofs as hex instead of base64 that @rozbb raised here. Non-blocker for me.
  • Reusing the upstream testsuite. Non-block for me, we can tackle that in a follow-up PR.
  • Rebasing onto main and correct DCO sign-off.

The PR has taken a long time, and I think it would be more productive to merge without further bigger changes, so we can build upon it. Hence, I'm approving.

We still need it to be mergeable though.

@vembacher: would you be ok with trying a rebase and correct DCO of this PR?
If not, I will take care of it by just fixing the conflicts, but potentially by opening a new PR with your changes already rebased.

Conflicts:
  Cargo.toml
  src/cosign/bundle.rs
  src/cosign/signature_layers.rs
  src/lib.rs
  src/registry/oci_caching_client.rs
  src/trust/mod.rs

Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
Needs unimplemented DSSE model.

Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
@exFalso
Copy link

exFalso commented Nov 21, 2025

A clarifying question: does/will this work with Rekor V2 tiling?

@viccuad
Copy link
Collaborator

viccuad commented Nov 24, 2025

A clarifying question: does/will this work with Rekor V2 tiling?

No, this is an implementation of Rekor Merkle tree approach, as it stands right now. Rekor v2 with tile trees will need to come later on.

BASE64_STANDARD.encode([self.key_fingerprint.as_slice(), self.raw.as_slice()].concat());
format!("— {} {sig_b64}\n", self.name)
}
fn decode(s: &str) -> Result<Self, ParseCheckpointError> {
Copy link
Member

Choose a reason for hiding this comment

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

nit: I would add an empty line

Not a nit: add some documentation explaining what is the expected format. We could copy paste https://github.com/sigstore/rekor/blob/4b1fa6661cc6dfbc844b4c6ed9b1f44e7c5ae1c0/pkg/util/signed_note.go#L129-L140 and add a reference to it (the actual URL I just pasted).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I found https://github.com/transparency-dev/formats/blob/main/log/README.md#signed-envelope, which documents the signed_note format and the signature format, will add it to the functions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed in bc19a18.

Comment on lines 207 to 209
let [_, name, sig_b64] = s.split(' ').collect::<Vec<_>>()[..] else {
return Err(DecodeError(format!("unexpected signature format {s:?}")));
};
Copy link
Member

Choose a reason for hiding this comment

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

We are not making sure the line begins with \u2014, see here

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed in 5f29463.

.map_err(|_| DecodeError("failed to decode signature".to_string()))?;

// first four bytes of signature are fingerprint of key
let (key_fingerprint, sig) = sig.split_at(4);
Copy link
Member

Choose a reason for hiding this comment

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

Do not use split_at since it panics if sig length is < 4.

Copy link
Member

Choose a reason for hiding this comment

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

Note split_at is also used elsewhere, please replace it with split_at_checked

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed in 61a2158. Didn't find any other similar occurrence.

@viccuad viccuad mentioned this pull request Nov 26, 2025
Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

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

Initial round of reviews, I still have to go through some files of the PR

return Err(DecodeError("unexpected checkpoint format".to_string()));
};

let signature = CheckpointSignature::decode(signature)?;
Copy link
Member

Choose a reason for hiding this comment

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

A checkpoint can have multiple signatures:

Copy link
Collaborator

@viccuad viccuad Nov 28, 2025

Choose a reason for hiding this comment

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

Fixed in f68128d.

pub(crate) fn encode(&self) -> String {
let note = self.note.marshal();
let signature = self.signature.encode();
format!("{note}\n{signature}")
Copy link
Member

Choose a reason for hiding this comment

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

I think this encoding is wrong, it should be:

{note}\n\n{signature}

note.marshal produces a string that is not terminated by \n, hence there would be just 1 \n between the note and the signature, instead of 2.

Also this code needs to be updated because there can be multiple signatures.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Totally agree, a line with just \n separates the note and signatures. Plus Rekor also does it that way.
Plus multiple signatures.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed in abfefdd.


/// The metadata that is contained in a checkpoint.
#[derive(Debug, PartialEq, Clone, Eq)]
pub struct CheckpointNote {
Copy link
Member

Choose a reason for hiding this comment

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

This is what the Go implementation calls Checkpoint, isn't it?

If that's the case, I would prefer to keep the same naming convention as the Go code. That makes easier to look at the Go codebase to understand what needs to be done inside of our Rust implementation

/// The `note` field stores this data,
/// and its authenticity can be verified with the data in `signature`.
#[derive(Debug, PartialEq, Clone, Eq)]
pub struct Checkpoint {
Copy link
Member

Choose a reason for hiding this comment

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

This is what the Go implementation calls SignedCheckpoint.

This is evident when looking at the code that unmarshalls the go SignedNote (which is embedded into the Go SignedCheckpoint); that's the same logic used by the Rust Checkpoint.decode.

I find this discrepancy between the Go types and the Rust types super confusing. It makes comparing the Go implementation with ours harder.

I would stick with the Go naming

fn encode(&self) -> String {
let sig_b64 =
BASE64_STANDARD.encode([self.key_fingerprint.as_slice(), self.raw.as_slice()].concat());
format!("— {} {sig_b64}\n", self.name)
Copy link
Member

Choose a reason for hiding this comment

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

replace the unicode char with its escape code \u2014

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done in 6ab967d.

self.origin, self.size
)
}
fn unmarshal(s: &str) -> Result<Self, ParseCheckpointError> {
Copy link
Member

Choose a reason for hiding this comment

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

I would copy-paste this documentation and add that link too.

Copy link
Collaborator

@viccuad viccuad Nov 27, 2025

Choose a reason for hiding this comment

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

Found the source, https://github.com/transparency-dev/formats/blob/main/log/README.md, will use that throughout in addtion to this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done as part of bc19a18.

)
}
fn unmarshal(s: &str) -> Result<Self, ParseCheckpointError> {
// refer to: https://github.com/sigstore/rekor/blob/d702f84e6b8b127662c5e717ee550de1242a6aec/pkg/util/checkpoint.go
Copy link
Member

Choose a reason for hiding this comment

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

nit, this link is too generic, you still have to go spelunking into the file to find what we're talking about. I would remove it (see previous comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done in c915227.

// refer to: https://github.com/sigstore/rekor/blob/d702f84e6b8b127662c5e717ee550de1242a6aec/pkg/util/checkpoint.go
// note is separated by new lines
let split_note = s.split('\n').collect::<Vec<_>>();
let [origin, size, hash_b64, other_content @ ..] = split_note.as_slice() else {
Copy link
Member

Choose a reason for hiding this comment

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

We need to raise an error if origin is empty: see the go implementation

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done in b6af9ca.

.map_err(|_| DecodeError("failed to decode signature".to_string()))?;

// first four bytes of signature are fingerprint of key
let (key_fingerprint, sig) = sig.split_at(4);
Copy link
Member

Choose a reason for hiding this comment

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

Note split_at is also used elsewhere, please replace it with split_at_checked

let proof_hashes = self
.hashes
.iter()
.map(hex_to_hash_output)
Copy link
Member

Choose a reason for hiding this comment

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

Why are consistency proofs formatted as newline-separated hex strings? The tlog API seems to prefer newline-separated base64

Let's use base64

Also a nit: I think this low-level struct should carry bytestrings, not Strings. The conversion should happen somewhere higher up

True, I think there are other places where we should do this optimization. It would be nice to have this done as part of this PR, but I don't consider that as a blocker.

I think we should also investigate the usage of the bytes crate a bit

Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
@viccuad viccuad force-pushed the feature/merkle-proofs branch from 771872c to 450189d Compare November 28, 2025 15:17
Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
@viccuad viccuad force-pushed the feature/merkle-proofs branch from 0db8581 to c915227 Compare November 28, 2025 16:31
@vembacher
Copy link
Contributor Author

Hello everyone, thanks for keeping this PR alive!

@vembacher: would you be ok with trying a rebase and correct DCO of this PR?
If not, I will take care of it by just fixing the conflicts, but potentially by opening a new PR with your changes already rebased.

Can I still help with this? I tried to run the rebase+signoff command but it made go through a lot of conflicts. I haven't kept up with the development of the crate so I did not feel confident to merge these correctly and from my understanding I would have to merge them manually. Is this correct?

I tried to get an overview of the current state of the PR, but I'm not sure if I missed something. But let me know if there is anything else I can assist you with in this PR or in the corresponding PR for the fork.

I will try my best to be assistive, but I unfortunately only have limited time to spend on this. But I will do my best to be responsive.

Ping: @viccuad @flavio @wolfv

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.