Skip to content

Conversation

@salmad3
Copy link
Contributor

@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
Contributor 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
Contributor 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

5 participants