feat: add verification functions #986
Conversation
asraa
force-pushed
the
verification-funcs
branch
3 times, most recently
from
970fc7f
to
a8c575c
Compare
asraa
force-pushed
the
verification-funcs
branch
from
a8c575c
to
3ee0810
Compare
|
Hey @asraa is this ready for review? |
|
Reviewing now! sorry about that, was out for a few |
asraa
force-pushed
the
verification-funcs
branch
from
3ee0810
to
935cfc2
Compare
Signed-off-by: Asra Ali <asraa@google.com> lint and comment Signed-off-by: Asra Ali <asraa@google.com> update Signed-off-by: Asra Ali <asraa@google.com> remove more code Signed-off-by: Asra Ali <asraa@google.com> fix Signed-off-by: Asra Ali <asraa@google.com> comments Signed-off-by: Asra Ali <asraa@google.com> fix Signed-off-by: Asra Ali <asraa@google.com>
asraa
force-pushed
the
verification-funcs
branch
from
935cfc2
to
7fe8e5b
Compare
pkg/verify/verify.go
Outdated
| ) | ||
|
|
||
| // ProveConsistency verifies consistency between an initial, trusted STH | ||
| // and a second new STH. Assumes that the signature on the STHs' are verified. |
There was a problem hiding this comment.
I wonder if it'd be better to check the signatures on the STHs, this seems like something that'd be easy to skip as a client. Same with VerifyCurrentCheckpoint - It verifies the newest checkpoint, but not the previous.
There was a problem hiding this comment.
The previous/old one should already be trusted in VerifyCurrentCheckpoint, but I can add for rigor
There was a problem hiding this comment.
Note that for that, we would need to pass in the verifier. I think including this in the doc string is fine? ProveConsistency would mean prove consistency, not verify tree head? wdyt?
There was a problem hiding this comment.
Yea, I'm ok with that. An example somewhere of a few different ways of composing these functions might be useful
There was a problem hiding this comment.
Discussed, examples to be added elsewhere.
pkg/verify/verify.go
Outdated
| } | ||
|
|
||
| // VerifyInclusion verifies an entry's inclusion proof. Clients MUST also | ||
| // verify the root hash via VerifyRootHash. |
There was a problem hiding this comment.
nit: VerifyRootHash is the old function name
Nonetheless, a consistency proof is effectively an inclusion proof for each checkpoint, correct? So just checking consistency would be sufficient if you have a previous checkpoint
There was a problem hiding this comment.
I'm confused about the statement "consistency proof is effectively an inclusion proof for each checkpoint".
I'll update the comment
Clients MUST either verify
// the root hash against a new STH (via VerifyCurrentCheckpoint) or against a
// trusted, existing STH (via ProveConsistency).
There was a problem hiding this comment.
Chatted offline, I was incorrect cause I was missing the context on a comment on a doc.
The comment SG. Should we note that if you verify against the current checkpoint, the inclusion proof should have been one that's been persisted and is trusted?
There was a problem hiding this comment.
against the current checkpoint? then wouldn't you persist the new one?
i think that's usage / client storage is out of scope here
There was a problem hiding this comment.
I was asking when you'd use VerifyInclusion then VerifyCurrentCheckpoint. What is the use case? You're right that you'd just persist the new one, so I guess my point is that you'd probably only call ProveConsistency after VerifyInclusion
There was a problem hiding this comment.
Like the above comment, I think we should document the examples for various use cases to make it clear when these are to be used.
- A witness verifying from a previous checkpoint - VerifyCurrentCheckpoint
- Verifying an SET - VerifyLogEntry
- Verifying an inclusion proof - (I think) VerifyInclusion and ProveConsistency
- Verifying an inclusion proof offline - Just VerifyInclusion (and verifying STH signature, which isn't a function currently)
There was a problem hiding this comment.
I think this should be in a documentation file. Documentation in code would only be understood anyway from code examples.
I do have some local tests that I haven't committed yet: the trillian/merkle libs don't expose an easy way to generate proofs, so I'd have to hard-code some examples. |
Signed-off-by: Asra Ali <asraa@google.com>
|
@haydentherapper added tests, didn't test things that were trivial and already tested in other APIs. feel free to add more |
asraa
force-pushed
the
verification-funcs
branch
from
bd61999
to
28240a8
Compare
Signed-off-by: Asra Ali <asraa@google.com> fix Signed-off-by: Asra Ali <asraa@google.com>
asraa
force-pushed
the
verification-funcs
branch
from
2e363ef
to
8de1bf6
Compare
|
@priyawadhwa @bobcallaway sorry, could I get one more look on this? |
Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Asra Ali asraa@google.com
Summary
Release Note
Documentation
Fixes #891
Fixes #789
Creating follow-up issue for the TUF usage. Contingent on sigstore/sigstore#500 which I'll go back to today.