transports/tls: Add `libp2p-tls` as per spec #2945

thomaseizinger · 2022-09-26T06:02:54Z

Description

This code is mostly extracted from #2289. I've tagged everyone who made contributions to that PR as co-authors (cc @kpp @dvc94ch @elenaf9 @mxinden) in the first commit.

On top of that, this also adds an implementation of the Upgrade{Inbound,Outbound} traits so that TLS can be used with other transports. Finally, this PR also makes an effort towards libp2p/specs#459 by including a few static tests (cc @marten-seemann).

Links to any relevant issues

Open Questions

Open tasks

Reference https://github.com/DemiMarie as co-author: Co-authored-by: Demi Marie Obenour <demiobenour@gmail.com>
Reference https://github.com/tomaka as co-author: Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com>

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

This code is extracted from #2289. Co-authored-by: David Craven <david@craven.ch> Co-authored-by: Roman Proskuryakov <r.proskuryakoff@gmail.com> Co-authored-by: Max Inden <mail@max-inden.de Co-authored-by: Elena Frank <elena.frank@protonmail.com>

We shouldn't rely on our callers to call `verify` here.

This will be useful for testing.

We are already leaking the `rustls` dependency to our clients through the `make_{server,client}_config` functions which have to be public for QUIC. Might as well reduce boilerplate and not wrap `TlsStream`.

thomaseizinger · 2022-09-26T06:10:31Z

Unfortunately, it seems ring is not WASI friendly https://github.com/libp2p/rust-libp2p/actions/runs/3125423794/jobs/5069777479#step:9:36.

kpp · 2022-10-06T16:15:00Z

The mapping looks sound to me but we may be handling too many cases.

I had to dig into RFC for TLS 1.3 to get the list of algorithms. These are not "too many", these are actual list of supported algos.

transports/tls/src/certificate.rs

thomaseizinger · 2022-10-06T21:39:21Z

The mapping looks sound to me but we may be handling too many cases.

I had to dig into RFC for TLS 1.3 to get the list of algorithms. These are not "too many", these are actual list of supported algos.

Yep, I've been down that road too when working on the code :)
What are your thoughts in regards to the snippet from rustls that I linked above? That only handles a much smaller set of algos.

kpp · 2022-10-06T23:50:50Z

What are your thoughts in regards to the snippet from rustls that I linked above?

I could use https://github.com/barebones-x509/barebones-x509/ and reuse their code to minify the code in the tls module. But when I tried to dial to a go implementation it didn't work. (the crate was/is OK, we didn't use it properly).

So I carefully rewrote it controlling every part I could and trying not to bloat the code but to keep it reasonable. It took me a month or so to figure out what's broken. So the code can be compatible with the go impl and does not break on a weird but acceptable by RFC configuration.

You are free to refactor as you like leaving the authorship and copyrights. Feel free to ping me for an extra code review. I will be happy to assist.

I could take all existing commits and put another one on top that deletes all the QUIC code. Given that we squash-merge, I am not sure it makes a big difference?

I don't know what's the point of co-authorship with squash-merge. The code was originally written by @Demi-Marie in #1334 and rewritten by me looking at their code and at the barebones-x509 crate. So their co-authorship is not an open question.

thomaseizinger · 2022-10-07T06:48:35Z

What are your thoughts in regards to the snippet from rustls that I linked above?

I could use barebones-x509/barebones-x509 and reuse their code to minify the code in the tls module. But when I tried to dial to a go implementation it didn't work. (the crate was/is OK, we didn't use it properly).

So I carefully rewrote it controlling every part I could and trying not to bloat the code but to keep it reasonable. It took me a month or so to figure out what's broken. So the code can be compatible with the go impl and does not break on a weird but acceptable by RFC configuration.

You are free to refactor as you like leaving the authorship and copyrights. Feel free to ping me for an extra code review. I will be happy to assist.

I am not planning to as I don't think I have the necessary knowledge at this point. Thanks for your prior work!

I could take all existing commits and put another one on top that deletes all the QUIC code. Given that we squash-merge, I am not sure it makes a big difference?

I don't know what's the point of co-authorship with squash-merge. The code was originally written by @Demi-Marie in #1334 and rewritten by me looking at their code and at the barebones-x509 crate. So their co-authorship is not an open question.

I am planning to attribute co-authorship to all people involved in this code on the squash-merge commit. I think that is fair. Please let me know if you disagree!

In particular, am planning on including the following (in alphabetical order):

Co-authored-by: David Craven <david@craven.ch>
Co-authored-by: Demi Marie Obenour <demiobenour@gmail.com>
Co-authored-by: Elena Frank <elena.frank@protonmail.com>
Co-authored-by: Max Inden <mail@max-inden.de
Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com>
Co-authored-by: Roman Proskuryakov <r.proskuryakoff@gmail.com>

thomaseizinger · 2022-10-17T04:32:56Z

This is ready to merge from my end. @mxinden @elenaf9 Waiting for your input / approval :)

mxinden

This is good to merge from my end. Please go ahead @thomaseizinger unless @elenaf9 has any objections.

I am in favor of adding the above list of co-authors to the squash commit.

mxinden · 2022-10-17T08:37:32Z

transports/tls/Cargo.toml

@@ -0,0 +1,29 @@
+[package]
+name = "libp2p-tls"
+version = "0.1.0"


Suggested change

version = "0.1.0"

version = "0.1.0-alpha"

To the best of my knowledge, the code below has not run in any production systems.

To communicate expectations, I suggest releasing this as an alpha first.

See also same suggestion on QUIC pull request #2289 (comment)

On it!

Most users depend on libp2p though so they are not going to see this version. IMO we are effectively removing that "alpha" label again through the re-export.

Do you think we should perhaps not re-export the TLS implementation yet so users have to depend on it manually?

Most users depend on libp2p though so they are not going to see this version. IMO we are effectively removing that "alpha" label again through the re-export.

That is true. Though I would explicitly call it out in the changelog.

Do you think we should perhaps not re-export the TLS implementation yet so users have to depend on it manually?

I don't have an opinion here. I think we should be consistent with libp2p-quic. Chatted with @elenaf9 quickly. She prefers not removing libp2p-quic from libp2p.

What do you think about annotating the re-export with the unstable attribute macro @thomaseizinger ?

What do you think about annotating the re-export with the unstable attribute macro @thomaseizinger ?

That is specific to the Rust compiler from what I know, I don't think you can use that actually?

Ah yes you are right! Don't have a strong on opinion on whether to re-export tls and quic or not. I did not consider that when re-exporting we "hide" the alpha-flag, so maybe not re-exporting is the best solution after all.

I think not exporting it would be the better solution but I don't want to block this PR on it. We can always change that until the next release.

thomaseizinger · 2022-10-20T22:38:33Z

Currently blocked on libp2p/test-plans#60.

transports/tls/src/lib.rs

…p-tls

Given that libp2p#2945 merged, we can remove the TLS section here.

thomaseizinger and others added 19 commits September 26, 2022 14:55

Add skeleton for libp2p-tls

06a77c2

This code is extracted from #2289. Co-authored-by: David Craven <david@craven.ch> Co-authored-by: Roman Proskuryakov <r.proskuryakoff@gmail.com> Co-authored-by: Max Inden <mail@max-inden.de Co-authored-by: Elena Frank <elena.frank@protonmail.com>

Minor reformatting

5364b43

Fix compile errors and remove Error type

71b2fe8

Remove unnecessary pub(crate)

80c0d1c

Implement Upgrade traits to allow use of TLS as authentication

03a2aed

Add changelog files

c5b1558

Fix docs

2316505

Minor doc and name adjustments

334544d

Verify p2p extension as part of parsing certificate

4d6db82

We shouldn't rely on our callers to call `verify` here.

Fix compile error

e4547a5

Extract private fn that allows to control the cert keypair

3267ccd

This will be useful for testing.

fixup! Verify p2p extension as part of parsing certificate

9a9ac2b

Add static test vectors

b810bcc

Re-order items by importance

7c6e6cf

Re-order functions in impl block and make private where possible

a8d965f

Tidy up manifest

6d68ffa

Sort hex dependencies

21b4319

Fix clippy

f0cac36

Remove Stream type

39fdf41

We are already leaking the `rustls` dependency to our clients through the `make_{server,client}_config` functions which have to be public for QUIC. Might as well reduce boilerplate and not wrap `TlsStream`.

thomaseizinger requested review from mxinden and elenaf9 September 26, 2022 06:02

thomaseizinger mentioned this pull request Sep 26, 2022

transports/quic: Add implementation based on quinn-proto #2289

Merged

7 tasks

Fmt

3b76e39

thomaseizinger mentioned this pull request Sep 26, 2022

Include test vectors in specifications libp2p/specs#459

Closed

Exclude libp2p-tls from wasm build

3ddf442

thomaseizinger added 4 commits September 26, 2022 16:11

Add license to libp2p-tls

d0bb3fa

Don't leak rcgen dependency in public API

01ecdbc

Don't leak webpki dependency

67eda94

Add note about exposed dependency to manifest

eedbefc

mxinden added difficulty:hard in progress labels Oct 6, 2022

kpp approved these changes Oct 6, 2022

View reviewed changes

bkchr reviewed Oct 6, 2022

View reviewed changes

transports/tls/src/certificate.rs Outdated Show resolved Hide resolved

Restore original copy-right

f11c54e

thomaseizinger requested review from mxinden and kpp and removed request for kpp October 11, 2022 12:11

Merge branch 'master' into libp2p-tls

bf1b30a

mxinden approved these changes Oct 17, 2022

View reviewed changes

thomaseizinger added 4 commits October 19, 2022 09:40

Merge branch 'master' into libp2p-tls

59c43ce

Set libp2p-tls to 0.1.0-alpha

3fe6386

Fix clippy error in latest rustls release

ed98810

Merge branch 'master' into libp2p-tls

f5bf544

Merge branch 'master' into libp2p-tls

eae9b5f

elenaf9 reviewed Oct 23, 2022

View reviewed changes

transports/tls/src/lib.rs Show resolved Hide resolved

thomaseizinger added 2 commits October 24, 2022 10:34

Use doc_auto_cfg for libp2p-tls

27b58f7

Merge branch 'libp2p-tls' of github.com:libp2p/rust-libp2p into libp2…

b01bff4

…p-tls

thomaseizinger merged commit 159a10b into master Oct 24, 2022

thomaseizinger deleted the libp2p-tls branch October 24, 2022 00:54

thomaseizinger mentioned this pull request Oct 24, 2022

CHANGELOG.md: Move libp2p-tls to correct version #3057

Merged

4 tasks

mxinden added a commit to mxinden/rust-libp2p that referenced this pull request Oct 25, 2022

ROADMAP: Remove TLS

9d47284

Given that libp2p#2945 merged, we can remove the TLS section here.

This was referenced Oct 28, 2022

Tracking issue for QUIC support paritytech/polkadot-sdk#536

Open

Upgrade x509-parser kpp/rust-libp2p#20

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transports/tls: Add `libp2p-tls` as per spec #2945

transports/tls: Add `libp2p-tls` as per spec #2945

thomaseizinger commented Sep 26, 2022 •

edited

Loading

thomaseizinger commented Sep 26, 2022

kpp commented Oct 6, 2022

thomaseizinger commented Oct 6, 2022

kpp commented Oct 6, 2022 •

edited

Loading

thomaseizinger commented Oct 7, 2022 •

edited

Loading

thomaseizinger commented Oct 17, 2022

mxinden left a comment

mxinden Oct 17, 2022

thomaseizinger Oct 18, 2022

mxinden Oct 20, 2022

elenaf9 Oct 20, 2022

thomaseizinger Oct 20, 2022

elenaf9 Oct 22, 2022 •

edited

Loading

thomaseizinger Oct 23, 2022

thomaseizinger commented Oct 20, 2022

transports/tls: Add libp2p-tls as per spec #2945

transports/tls: Add libp2p-tls as per spec #2945

Conversation

thomaseizinger commented Sep 26, 2022 • edited Loading

Description

Links to any relevant issues

Open Questions

Open tasks

Change checklist

thomaseizinger commented Sep 26, 2022

kpp commented Oct 6, 2022

thomaseizinger commented Oct 6, 2022

kpp commented Oct 6, 2022 • edited Loading

thomaseizinger commented Oct 7, 2022 • edited Loading

thomaseizinger commented Oct 17, 2022

mxinden left a comment

Choose a reason for hiding this comment

mxinden Oct 17, 2022

Choose a reason for hiding this comment

thomaseizinger Oct 18, 2022

Choose a reason for hiding this comment

mxinden Oct 20, 2022

Choose a reason for hiding this comment

elenaf9 Oct 20, 2022

Choose a reason for hiding this comment

thomaseizinger Oct 20, 2022

Choose a reason for hiding this comment

elenaf9 Oct 22, 2022 • edited Loading

Choose a reason for hiding this comment

thomaseizinger Oct 23, 2022

Choose a reason for hiding this comment

thomaseizinger commented Oct 20, 2022

transports/tls: Add `libp2p-tls` as per spec #2945

transports/tls: Add `libp2p-tls` as per spec #2945

thomaseizinger commented Sep 26, 2022 •

edited

Loading

kpp commented Oct 6, 2022 •

edited

Loading

thomaseizinger commented Oct 7, 2022 •

edited

Loading

elenaf9 Oct 22, 2022 •

edited

Loading