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.

Sign up for GitHub

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

Jump to bottom

Noise: skeleton of transport and connection #405

Merged
merged 10 commits into from Feb 28, 2020
Merged

Noise: skeleton of transport and connection #405

merged 10 commits into from Feb 28, 2020

Conversation

mhchia
Copy link
Contributor

@mhchia mhchia commented Feb 7, 2020 •
edited

What's inside this PR

  • security.noise
    • transport
      • Transport(ISecureTransport)
        • Note: Given options, use the desired pattern to upgrade an IRawConnection to NoiseConnection.
    • patterns
      • IPattern(ABC)
        • Note: Handle the whole handshake process.
        • PatternXX(IPattern)
          • Note: Not complete yet. Will finish authentication of libp2p pubkey in the next PR.
        • (Other patterns in the future)
    • connection
      • NoiseConnection(ISecureConn)
        • Note: It is just a fake implementation now. Will support [en|de]crypt in the latter PRs.
    • pb
      • Note: The wire message described in protobuf. It is not used currently. Will be used in the next PR.
  • tests
    • noise_conn_factory
      • Note: upgrading IRawConnection to NoiseConnection.
    • test_noise
      • Note: Add a simple test for both noise.Transport and NoiseConnection.

@mhchia mhchia changed the title [WIP] Add the skeletons of noise transport and conn [WIP] Add the skeleton of noise transport and conn Feb 7, 2020
ralexstokes
ralexstokes reviewed Feb 7, 2020
View reviewed changes
Copy link
Contributor

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

i know this is still a draft, just taking a quick look :)

libp2p/security/noise/transport.py Outdated Show resolved Hide resolved
@mhchia @ralexstokes
Update libp2p/security/noise/transport.py
1152f9b 
Co-Authored-By: Alex Stokes <r.alex.stokes@gmail.com>
@mhchia
Copy link
Contributor Author

mhchia commented Feb 8, 2020

@ralexstokes No problem! Thanks for the review. 🙏

@mhchia mhchia marked this pull request as ready for review February 15, 2020 04:37
@mhchia mhchia changed the title [WIP] Add the skeleton of noise transport and conn Add the skeleton of noise transport and conn Feb 15, 2020
@mhchia mhchia mentioned this pull request Feb 15, 2020
Merged
3 tasks
@mhchia mhchia changed the title Add the skeleton of noise transport and conn Noise: skeleton of transport and connection Feb 15, 2020
@mhchia
Copy link
Contributor Author

mhchia commented Feb 26, 2020 •
edited

This one is ready for review. Most of the details are implemented in #406 .

ralexstokes
ralexstokes approved these changes Feb 26, 2020
View reviewed changes
Copy link
Contributor

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

looks good at a high-level; i'll go look at the other PR now

libp2p/network/stream/net_stream.py
@@ -51,14 +51,14 @@ def set_protocol(self, protocol_id: TProtocol) -> None:
except MuxedStreamReset as error:
raise StreamReset() from error

async def write(self, data: bytes) -> int:
async def write(self, data: bytes) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if we can't write all of data to the underlying stream? the idea w/ this interface is to allow a caller to detect this scenario and re-attempt the write if that happens

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'm changing this because I didn't see this use case(returns before all of the data is written) present in our codebase and also the underlying transport e.g. trio.SocketStream.send_all. write is asynchronous and we probably can guarantee that write returns only when all of the data is written. I can also change it back if we would prefer to allow write to return if it only writes part of the data.

Copy link
Contributor

Choose a reason for hiding this comment

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

we should avoid underlying implementations from guiding our interface choices but i suppose we can leave this alone for now just to bias towards speed :)

libp2p/security/noise/patterns.py
def __init__(
self, local_peer: ID, libp2p_privkey: PrivateKey, noise_static_key: PrivateKey
) -> None:
self.protocol_name = b"Noise_XX_25519_ChaChaPoly_SHA256"
Copy link
Contributor

Choose a reason for hiding this comment

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

is this the specific noise suite we are using in libp2p-noise according to that spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's the protocol we use in the XX pattern. Ref: https://github.com/libp2p/specs/tree/master/noise#valid-noise-protocol-names

@mhchia mhchia merged commit 1f881e0 into libp2p:master Feb 28, 2020
@mhchia mhchia deleted the feature/noise branch February 28, 2020 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ralexstokes ralexstokes ralexstokes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mhchia @ralexstokes