Skip to content

feat: tls #261

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

Merged
merged 10 commits into from
Dec 23, 2022
Merged

feat: tls #261

merged 10 commits into from
Dec 23, 2022

Conversation

salmad3
Copy link
Member

@salmad3 salmad3 commented Dec 5, 2022

Context

Latest preview

Please view the latest Fleek preview here.

@salmad3 salmad3 marked this pull request as draft December 5, 2022 05:37
@salmad3 salmad3 linked an issue Dec 5, 2022 that may be closed by this pull request
6 tasks
@salmad3 salmad3 linked an issue Dec 5, 2022 that may be closed by this pull request
3 tasks
@salmad3 salmad3 marked this pull request as ready for review December 5, 2022 16:07
@salmad3 salmad3 added the ready for review PR is ready for review label Dec 5, 2022
@p-shahi
Copy link
Member

p-shahi commented Dec 6, 2022

@thomaseizinger can you help review this?

(Took liberty of unassigning Marten to help spread the load.)

@p-shahi p-shahi removed the request for review from marten-seemann December 6, 2022 05:05
@salmad3 salmad3 added change request PR requires changes. and removed ready for review PR is ready for review labels Dec 6, 2022
Copy link

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Who is the audience of this document?

It starts out very high-level ("What is TLS") but gets detailed fairly quickly, explaining the messages in the handshake. Hence, for someone who's got no idea what TLS is, most of this document is probably overwhelming.

Given that libp2p is just using libp2p in a particular configuration, (i.e. we didn't build it from scratch) I'd recommend the following:

  • Retain the super high-level description (i.e. "What is TLS"). This provides some context on what we are talking about.
  • From there, link to useful material for people that want to understand TLS in detail (RFC, perhaps Wikipedia for the historical bit)
  • Describe that libp2p uses a particular subset, most notably TLS 1.3+
  • For any details, we can link to our spec: https://github.com/libp2p/specs/blob/master/tls/tls.md

Hope this helps :)

@salmad3 salmad3 added ready for review PR is ready for review and removed change request PR requires changes. labels Dec 12, 2022
salmad3 and others added 2 commits December 12, 2022 16:50
Co-authored-by: Marten Seemann <martenseemann@gmail.com>
Copy link

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for incorporating the review :)

@salmad3
Copy link
Member Author

salmad3 commented Dec 15, 2022

thanks @thomaseizinger and @marten-seemann!

available to re-review @marten-seemann

Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

It would be nice to have an introduction sentence saying that TLS is one of the security handshakes used to secure transports that don't have built-in security (TCP, WebSocket), and also mention that Noise is an alternative option. That would motivate the rest of this document.

Other than that, this looks good to me.

@salmad3
Copy link
Member Author

salmad3 commented Dec 19, 2022

Added an introductory statement. Based on the feedback here, I'll also simplify and update the Noise document I worked.

@salmad3 salmad3 added ready for review PR is ready for review and removed ready for review PR is ready for review labels Dec 23, 2022
@salmad3 salmad3 dismissed marten-seemann’s stale review December 23, 2022 12:41

additional statement added in the introduction and checked off by reviewers

@salmad3 salmad3 removed the ready for review PR is ready for review label Dec 23, 2022
@salmad3 salmad3 merged commit 9c2a856 into master Dec 23, 2022
@salmad3 salmad3 deleted the secure/tls branch January 3, 2023 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Populate secure communication section: Noise & TLS
4 participants