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.

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

Verification: Return an STH in a Verification response #988

Closed
Tracked by #1005
asraa opened this issue Aug 18, 2022 · 9 comments
Closed
Tracked by #1005

Verification: Return an STH in a Verification response #988

asraa opened this issue Aug 18, 2022 · 9 comments
Assignees
Labels
bug Something isn't working ga_candidate Proposed blocking issue for GA release

Comments

@asraa
Copy link
Contributor

asraa commented Aug 18, 2022

Description

I propose adding an STH in a rekor entry Verification.

Right now, a full verification on an entry would do the following:

  1. Verify the inclusion proof using the entry Body as the leaf hash
  2. Verify the root hash against an STH
  3. Verify the SET

See impl here:

if err := VerifyRootHash(ctx, rClient, verifier,

Note that most of this can be done with the returned info in an entry LogEntry EXCEPT 2. That requires a Rekor client to fetch LogInfo to retrieved an STH and then also a consistency proof from the root hash of the inclusion proof to the STH. Instead, we should return the STH for the root hash of the inclusion proof. Overall, the struct would look something like

Entry
  Verification:
    - InclusionProof
    - SignedTreeHead (correspnding to root hash of inclusion proof)
    - SignedEntryTimestamp

alternatively we can also place the SignedTreeHead checkpoint inside the inclusion proof, and remove the roothash/treesize fields and consume them from the sth.

Related #986

cc @haydentherapper @bobcallaway

Version

@asraa asraa added the bug Something isn't working label Aug 18, 2022
@haydentherapper
Copy link
Contributor

haydentherapper commented Aug 19, 2022

Wrote up a doc with more details on offline and online verification - https://docs.google.com/document/d/1_Ykbb1n_rXQjZIairCBKj6ZTpvw5s19fjpwGTksGCbI/edit# (sigstore-dev has access)

@haydentherapper haydentherapper added the ga_candidate Proposed blocking issue for GA release label Aug 25, 2022
@priyawadhwa priyawadhwa mentioned this issue Aug 26, 2022
7 tasks
@haydentherapper
Copy link
Contributor

Task: Update verification functions once the STH is returned

@haydentherapper
Copy link
Contributor

The plan for changes:

  • Sign a tree head on successful inclusion
  • Return the tree head in the response - I'd propose that a checkpoint field be added to InclusionProof instead of alongside InclusionProof. Then verification is either an SET or inclusion proof (A checkpoint doesn't need to be included with an SET)
  • Persist the checkpoint in a database (including the current tree, size, etc)
  • Change GetLogInfo to fetch the latest checkpoint from the db

The last step is necessary for gossiping. We can't sign a new tree head - Otherwise the tree head signatures would differ between an inclusion proof's checkpoint and GetLogInfo's checkpoint.

@asraa @bobcallaway @priyawadhwa Please review

@asraa
Copy link
Contributor Author

asraa commented Aug 29, 2022

Then verification is either an SET or inclusion proof (A checkpoint doesn't need to be included with an SET)

That makes logical sense to me. Note that I think some languages JSON decoders may have more strict validation than Go, and that it would be a breaking change and we should publish a release for it.

Persist the checkpoint in a database (including the current tree, size, etc)

Like Rekor server should do that? Can we table that for later to make this more incremental?

Change GetLogInfo to fetch the latest checkpoint from the db

Also, can we table that for later? Let's just make this issue on changing return response

@haydentherapper
Copy link
Contributor

Yes, we can fix these incrementally, but I think to fully close this issue, we will need to persist the latest checkpoint. Otherwise, there will be no way for a client to get quorum with other witnesses, given the STH from an inclusion proof.

@haydentherapper
Copy link
Contributor

haydentherapper commented Aug 29, 2022

Persistance can be punted. A client with a checkpoint can't ask a witness if it's seen the checkpoint with the current model for Rekor anyways, since the checkpoints update so frequently.

Just some thoughts for if we do this, a way to avoid writing to a database: We only need to persist the latest, so we can just store it on the global api object, with appropriate locking and checks that we aren't writing an old version. We'd need to figure out how to handle startup though.

haydentherapper added a commit to haydentherapper/rekor that referenced this issue Aug 30, 2022
This associates a root hash in an inclusion proof with a signed
commitment from the log. Previously, without this included, there was no
connection between an inclusion proof and the log. An inclusion proof
and checkpoint can be an alternative proof of inclusion instead of a
SET.

Ref sigstore#988

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
haydentherapper added a commit to haydentherapper/rekor that referenced this issue Aug 30, 2022
This associates a root hash in an inclusion proof with a signed
commitment from the log. Previously, without this included, there was no
connection between an inclusion proof and the log. An inclusion proof
and checkpoint can be an alternative proof of inclusion instead of a
SET.

Ref sigstore#988

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
haydentherapper added a commit to haydentherapper/rekor that referenced this issue Sep 1, 2022
This associates a root hash in an inclusion proof with a signed
commitment from the log. Previously, without this included, there was no
connection between an inclusion proof and the log. An inclusion proof
and checkpoint can be an alternative proof of inclusion instead of a
SET.

Ref sigstore#988

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
priyawadhwa pushed a commit that referenced this issue Sep 1, 2022
* Include checkpoint (STH) in entry upload and retrieve responses

This associates a root hash in an inclusion proof with a signed
commitment from the log. Previously, without this included, there was no
connection between an inclusion proof and the log. An inclusion proof
and checkpoint can be an alternative proof of inclusion instead of a
SET.

Ref #988

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

* Refactor with common implementation for creating signed checkpoint

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

* Update client to verify checkpoint signature

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

* Address linter

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

* Add checkpoint verification to VerifyLogEntry

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
@priyawadhwa
Copy link
Contributor

@asraa I believe this can be closed now?

@asraa
Copy link
Contributor Author

asraa commented Sep 2, 2022

Agree!

@asraa asraa closed this as completed Sep 2, 2022
@haydentherapper
Copy link
Contributor

Thanks! Going to open up another non-blocking issue for figuring out the story around returning a consistent checkpoint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ga_candidate Proposed blocking issue for GA release
Projects
None yet
Development

No branches or pull requests

3 participants