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

refactor(core): alpha.7 rewrite #574

Merged
merged 16 commits into from
Sep 19, 2024
Merged

refactor(core): alpha.7 rewrite #574

merged 16 commits into from
Sep 19, 2024

Conversation

sinui0
Copy link
Member

@sinui0 sinui0 commented Aug 27, 2024

This PR completely rewrites tlsn-core for our alpha.7 release. Closes #438

There 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

  • Improved the structure of attestations
    • Header has a static smol size and now includes a version tag and unique identifier
    • Body fields are now merkelized so that only fields necessary to prove desired statements need to be included
    • Body is forward-compatible with new kinds of fields (for example, new kinds of commitments)
  • Added a Request pattern which will allow a Prover to configure aspects of the attestation depending on what the Notary supports.
  • Further encapsulated various kinds of proofs
    • ServerIdentityProof proves the server name using the certificate chain
    • AttestationProof proves the Notary signature and attestation body
    • TranscriptProof proves subsequences of the transcript using the encoding or hash commitments.
  • Extensible cryptographic algorithms via CryptoProvider (inspired by rustls).
    • Supports multiple hash algorithms, including user provided custom.
    • Supports multiple signature algorithms, including user provided custom.
  • Added a new PlaintextHash commitment type (not fully exposed yet) which will be implemented with authdecode. See Authdecode #479
  • Encapsulated more logic into this crate in the form of builders, reducing API surface area and making it more difficult to misuse. There should now be very little implementation code in prover and verifier 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.

@sinui0 sinui0 requested review from themighty1 and th4s August 27, 2024 05:46
Copy link
Member

@th4s th4s left a 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 todos and clippy comments I think the only thing needed is more unit tests.

crates/data-fixtures/data/http/response_json Outdated Show resolved Hide resolved
crates/core/src/attestation.rs Show resolved Hide resolved
crates/core/src/hash.rs Outdated Show resolved Hide resolved
crates/core/src/index.rs Outdated Show resolved Hide resolved
crates/core/src/index.rs Outdated Show resolved Hide resolved
crates/core/src/signing.rs Outdated Show resolved Hide resolved
crates/core/src/transcript/commit.rs Show resolved Hide resolved
crates/core/src/transcript/commit.rs Show resolved Hide resolved
crates/core/tests/api.rs Outdated Show resolved Hide resolved
@yuroitaki yuroitaki self-requested a review September 9, 2024 03:13
@themighty1
Copy link
Member

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.
This PR goes back to using the "Prover==TLS Client" terminology, e.g. by calling the type "ServerCertCommitment" instead of "TLSPeerCertCommitment".
Is that intended?

@maceip
Copy link
Collaborator

maceip commented Sep 11, 2024

  • ServerIdentityProof proves the server name using the certificate chain
  • AttestationProof proves the Notary signature and attestation body
  • TranscriptProof proves subsequences of the transcript using the encoding or hash commitments.

👨‍🍳

crates/core/src/signing.rs Show resolved Hide resolved
crates/core/src/signing.rs Show resolved Hide resolved
crates/core/src/connection.rs Show resolved Hide resolved
crates/core/src/transcript.rs Outdated Show resolved Hide resolved
@sinui0
Copy link
Member Author

sinui0 commented Sep 16, 2024

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. This PR goes back to using the "Prover==TLS Client" terminology, e.g. by calling the type "ServerCertCommitment" instead of "TLSPeerCertCommitment". Is that intended?

For now I think those semantics will only contribute to confusion. In the future can always add a field for client certificate commitment.

* 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
@heeckhau heeckhau added this to the Alpha7 milestone Sep 18, 2024
@sinui0
Copy link
Member Author

sinui0 commented Sep 18, 2024

Let's get this merged! Only examples are failing AFAICT, will fix in follow up PRs

Copy link
Member

@th4s th4s left a comment

Choose a reason for hiding this comment

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

🚀 nice

@sinui0 sinui0 merged commit 53ff873 into dev Sep 19, 2024
3 of 5 checks passed
@sinui0 sinui0 deleted the refactor/core-rewrite branch September 19, 2024 14:57
@sinui0 sinui0 mentioned this pull request Oct 10, 2024
Copy link
Member

@themighty1 themighty1 left a 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.
Copy link
Member

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.

Copy link
Member Author

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

@themighty1
Copy link
Member

themighty1 commented Oct 21, 2024

ACK, including our rs-merkle fork, (will open a PR with some fixes)
@sinui0 could you pls answer my q above.

@themighty1 themighty1 restored the refactor/core-rewrite branch October 24, 2024 12:44
@themighty1 themighty1 deleted the refactor/core-rewrite branch October 24, 2024 13:12
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.

6 participants