-
Notifications
You must be signed in to change notification settings - Fork 75
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
refactor(core): alpha.7 rewrite #574
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 Great work 🚀
Overall looking very good! Most of the complexity is encapsulated and organized well into modules. Apart from the remaining todo
s and clippy comments I think the only thing needed is more unit tests.
I have e general question while still re-viewing this PR. Some time ago we started using the "TLS peer" terminology since we allow for the TLS Server to act as a TLSN Prover. |
👨🍳 |
For now I think those semantics will only contribute to confusion. In the future can always add a field for client certificate commitment. |
bc75ed9
to
5118309
Compare
* refactor(core): alpha.7 rewrite * allow empty idx * fix empty assumption * further encapsulate rangeset * added presentation, finishing touches * remove unwrap * refactor(core): integrate rewrite changes * remove obsolete tests * add secp256r1 support * update index naming * add secp256r1 support * add attestation to presentation output, and serde derives * handle k256 in KeyAlgId Display * unnecessary newline * fix variable name
Let's get this merged! Only examples are failing AFAICT, will fix in follow up PRs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
self.state | ||
.mux_fut | ||
.poll_with(async { | ||
// Send the partial transcript to the verifier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sinui0 , what is the motivation here for sending a zeroed out transcript to the verifier? We could introduce a new type which doesn't store the 0s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This crossed my mind, I just moved it out of scope of the PR. A good solution is to write a custom serializer/deserializer which removes the 0s instead of adding a new type
ACK, including our rs-merkle fork, (will open a PR with some fixes) |
This PR completely rewrites
tlsn-core
for our alpha.7 release. Closes #438There are still a couple of loose ends that I'm tidying up but the vast majority of the deliverable is there. Prior to accepting this PR we will need to review our new fork of
rs_merkle
which was required to support hash algorithm customization.A follow up PR will be provided which completes the integration of these changes into the other crates. I have most of this integration work complete on a separate branch already.
Highlights
Request
pattern which will allow a Prover to configure aspects of the attestation depending on what the Notary supports.ServerIdentityProof
proves the server name using the certificate chainAttestationProof
proves the Notary signature and attestation bodyTranscriptProof
proves subsequences of the transcript using the encoding or hash commitments.CryptoProvider
(inspired byrustls
).PlaintextHash
commitment type (not fully exposed yet) which will be implemented with authdecode. See Authdecode #479prover
andverifier
crates related to attestations.Not Included
There are some items that I initially wanted in scope but decided to remove as they proved to be larger than anticipated.