refactor: async await #14

vasco-santos · 2020-01-08T14:46:17Z

This PR refactors libp2p-stardust and its server to have an async await API, which is compliant with the new interface-transport, interface-connection and interface-peer-discovery.

In addition, the stardust server was also completely refactor to work with the new transport.

There is some stuff that I would like to be different in this module, such as what I described in the next comment. However, I think that we should move that into a new issue to be considered later if relevant.

Needs:

Update docs
Connection encryption redundancy

Follow up tasks (create issues):

interface-transports tests should be refactored to be used in this module (as well as for circuit relay)
new protobuf definition?

[`libp2p-websocket-star`](libp2p/js-libp2p-websocket-star#57) has a security vulnerability and was not converted in the refactor. This PR removes the `libp2p-websocket-star` information until libp2p/js-libp2p-stardust#14 lands and we can recommend using it instead. refs #2600

vasco-santos · 2020-02-10T17:46:44Z

@mkg20001 @jacobheun

I currently have the register and discovery flows working with the new js-libp2p dialer + new transport/discovery interfaces. I am currently working on the dial flow.

Since starting to tackle this, I was thinking on suggesting that we change the naming of the messages in a more meaningful way. However, I wanted to discuss this with both of you after getting an initial version working.

Taking into account the current state for the dial flow, I got to a point when I really want to change the protobuf definition.

Have a single top level messages instead of a bunch of different ones. Considering for example libp2p-circuit, or libp2p-daemon. This way, we should have a Stardust message which includes all the other messages inside, like the daemon example. The main advantage of this consists on being easy to identify the type of the message from the start, which is particularly nice when the server needs to understand if a message from a new stream should go to the register or dial flows, without needing to decode and inspect the content of the message to infer its type.
Simplify the messages naming and parameters:

JoinInit {random128, peerId} => Register { data, peerId }
JoinChallenge { error, saltEncrypted } => Challenge { data } / Error { msg }
JoinChallengeSolution { solution } => Solution { data }
JoinVerify { error } => Ok { } / Error { msg }

Discovery { ids } => Discovery { ids }
DiscoveryAck { } => Ok { }

DIalRequest { target } => Dial { }
DialResponse { } => Ok { }

I will start working on changing the first part while I wait for your feedback, as it will ease a lot the implementation of the dial flow. Anyway, I would like feedback from you on both this points before getting this ready to review.

vasco-santos · 2020-02-11T19:53:54Z

FYI, I did not do the protobuf changes above, and I think that I will pass them into an issue to be discussed, instead of being included in this PR.

mkg20001 · 2020-02-11T19:59:56Z

My reasoning for not wanting to use a single message is that it doesn't matter if you put in a valid but wrongly typed message, it's still invalid. Except there's more stuff to check for the fields existing. And more code to be executed per parsed message since it needs to parse all the other non-existing messages aswell.
And more code is more bugs. So unless there would be any advantage for the actual code itself, let's just not add complexity for no reason.

Edit: Also the type is explicitly specified by the order, so no checking required.

mkg20001

Not sold on using an extra SECIO layer, due to e2e-secio and wss being assumed

README.md

src/index.js

mkg20001 · 2020-02-12T09:50:59Z

src/server/index.js

+   * @param {Array<multiaddr>} options.addresses
+   * @param {Array<Transport>} options.transports
+   * @param {Array<Multiplexer>} options.muxers
+   * @param {Array<Encryption>} options.encryption


The idea was to use stardust over wss:// protocol, making it encrypted already, and dropping the extra secio layer.
Connections between peers are always encrypted end-to-end encrypted and authed by secio, so even plaintext stardust isn't posing a security risk (aside from allowing praying eyes to view metadata)

Since it was planned to be used over wss:// the SECIO layer will only slow things down.

@mkg20001 yeah, I know that not using the extra encryption layer was one the ideas here. I focused on getting this to work with the new libp2p api first. Now that things are getting shaped, I will understand if we will be able to not use the SECIO layer

Ok

Possibly it could just use a switch with plaintext, otherwise making a new micro-switch would also be possible should the libp2p switch be either too complicated to run without a swarm or cause significant performance impacts

We should also look out to prevent the user from having to enter an id, even if it's just a placeholder for plaintext-crypto. If not possible to remove from the switch a fake id could always be appended by the listener in that case.

We should make the libp2p dialer working for all types of use cases, so I think that we should not go on creating a new "switch" to get this and understand what can be done without compromising security. In this way, we cannot use the Plaintext crypto protocol as peers would have to add it to use Stardust 🙅‍♂

We can potentially have an upgrader here, that would basically allow us to upgrade the connection manually instead of using the provided upgrader (this would an improvement over the micro-switch idea).

I was discussing this with @jacobheun earlier today and he pointed out an important aspect. Our websocket transport is relying on the underlying ws module to verify if the data is encrypted and it is not considering wss / ws. This creates a risk for this.

We should probably think a bit about the alternatives that we have and understand what will be the best decision.

We can potentially have an upgrader here,

Sounds good

ws module to verify if the data is encrypted and it is not considering wss / ws. This creates a risk for this.

As I already mentioned earlier the actual p2p connections are encrypted and authed by SECIO

What is possible with using a non-encrypted stardust connection is just DOSing the client with false discovery with a MITM information (this would only work if a MITM would happen and additionally would create at most a minor inconvenience) and the leakage of some metadata (who talks to whom) which would be leaked with regular TCP anyways as the SECIO-handshake is unencrypted.

Stardust is also mainly intended for websites that can't directly dial to peers so the website is most likely using HTTPS which means that we're not able to use unencrypted ws

So unless I'm missing something it appears to be a wrong assumption that this creates a problem.

Though checking if the underlying connection is ws or wss isn't hard as the transport is the one that's dialing so it should be enough to match it the multiaddr in question uses ws or wss

So if we insist on encryption we could do:

if ws: custom upgrader with SECIO (or user-specified) as only crypto

if wss: custom upgrader with plaintext as only crypto

The server would use both plaintext and SECIO then

One thing we need to keep in mind is that stardust allows for any reliable address, which includes tcp, webrtc, http, etc. We are also not even checking that, just that the stardust multiaddr codec exists in the address. If we remove the potential additional encryption layer, we should be more strict about the addresses we allow. As @mkg20001 mentioned, the general use case is websockets in browser, which will mandate wss. However, I don't see anything in the dialer/listener preventing me from connecting to an insecure tcp stardust server if I discover the address, but as already mentioned it's a low threat attack vector.

We could enforce wss and use plaintext, but the fact that stardust potentially supports any reliable multiaddr is pretty nice. If we want to support broader use cases like tcp > stardust > websockets broadly using secio to reduce the complexity might be worth the throughput hit. If we just care about wss > stardust > websockets, I think we can enforce that and reasonably use a plaintext upgrader.

I suggest .that we take this out of the scope of the refactor PR and discuss this on an issue. I think we should be able to provide options to the transport/server that would allow the users to operate with both of those cases. What do you think? Can you create an issue @mkg20001 ?

I think it would be reasonable to take the double encryption hit for now and do a followup release for performance tuning for transports with built in encryption. It's not ideal but mitigates any risk/complexity of multiple reliable multiaddrs and gets the transport usable for folks.

src/server/index.js

jacobheun

Just two minor things. I think once we merged this it would be good to do an RC and test it out in js-ipfs before doing a final release.

jacobheun · 2020-03-05T12:31:08Z

src/listener.js

+
+    // close stream and connection with the server
+    const wrappedStream = this.wrappedStream.unwrap()
+    wrappedStream.sink([])


This should be unnecessary. The call below to this.serverConnection.close() is terminating the entire connection, so this.wrappedStream, which is a stream of that connection should automatically terminate.

Also, just a note, wrappedStream.sink([]) ONLY closes the write side of the stream, the read would still be open. source and sink must both be closed. We should document this better in the libp2p and connection interface docs, with examples of how to properly close streams to help people avoid halfopen streams.

We should document this better in the libp2p and connection interface docs, with examples of how to properly close streams to help people avoid halfopen streams.

Yeah, it makes sense

jacobheun · 2020-03-05T12:36:57Z

test/discovery.spec.js

+      }, conn => pipe(conn, conn)))
+      await Promise.all(listeners.map(listener => listener.listen(SERVER_URL)))
+
+      await pWaitFor(() => discovered.length >= 3)


We should validate the discovered peer list after the wait for. Also, it might be good to make discovered a key:peerId Map to avoid discovered being larger than 3. We should not find more than 3 peers, the excess is likely due to "rediscovering" peers we already know.

mkg20001

Nitpicks mostly, otherwise good to go

README.md

src/index.js

src/listener.js

src/server/bin.js

src/server/index.js

vasco-santos · 2020-03-06T15:50:14Z

@mkg20001 and @jacobheun made a few changes to this. Listen should take into account multiaddr like /ip4/0.0.0.0/tcp/5892/ws/p2p-stardust/p2p/${stardustServerId}/p2p/${ownId}.

Also tested this with browser examples of js-libp2p and js-ipfs

mkg20001

Last batch of nitpicks to apply, after that LGTM

src/server/index.js

src/utils.js

Co-Authored-By: Maciej Krüger <mkg20001@gmail.com>

vasco-santos · 2020-03-06T18:01:07Z

As @jacobheun suggested, we should probably release this as a rc while the examples are reviewed. What do you think @mkg20001 ?

jacobheun

LGTM 🚀

mkg20001 · 2020-03-06T18:18:14Z

@vasco-santos ok

Since I'm lead I should merge this now? (Haven't done a merge in eons)

vasco-santos · 2020-03-06T18:22:49Z

Since I'm lead I should merge this now? (Haven't done a merge in eons)

Yeah, go for it 🚢

For releasing, you can have a look at ipfs/aegir/#releasing. There is an example for the pre release! 🙂

vasco-santos · 2020-03-06T18:26:07Z

You should just need to run:

npm run release -- --type prerelease --preid rc --dist-tag next

vasco-santos force-pushed the refactor/async-await branch 9 times, most recently from 129da5e to cc3366d Compare January 9, 2020 13:09

refactor: async await

4a79284

vasco-santos force-pushed the refactor/async-await branch from cc3366d to 4a79284 Compare January 9, 2020 14:19

chore: use libp2p

a6724b6

This was referenced Feb 7, 2020

docs: remove websocket-star docs ipfs/js-ipfs#2754

Merged

feat: replace websocket-star with stardust ipfs/js-ipfs#2600

Closed

vasco-santos added 2 commits February 7, 2020 18:45

chore: not use micro-switch anymore

2b2c183

chore: use classes instead

7706136

vasco-santos force-pushed the refactor/async-await branch from e0dd797 to 7706136 Compare February 10, 2020 13:57

vasco-santos added 2 commits February 11, 2020 18:15

chore: add functional dial flow

3e87ae7

chore: update tests

1e69f54

chore: enable browser tests

f1d8b52

vasco-santos force-pushed the refactor/async-await branch from 54f91de to f1d8b52 Compare February 11, 2020 21:12

vasco-santos added 2 commits February 12, 2020 09:55

chore: update dependencies

e2fcd73

chore: use libp2p utils

48ca3eb

mkg20001 suggested changes Feb 12, 2020

View reviewed changes

chore: add bin env to server

c612a5c

vasco-santos force-pushed the refactor/async-await branch from aa7a793 to c612a5c Compare March 4, 2020 20:47

vasco-santos mentioned this pull request Mar 5, 2020

chore: update browser example with stardust libp2p/js-libp2p#579

Closed

2 tasks

jacobheun reviewed Mar 5, 2020

View reviewed changes

chore: remove redundant finish of sink

bc1e8e0

vasco-santos requested review from jacobheun, mkalinin and mkg20001 and removed request for mkalinin and mkg20001 March 5, 2020 14:08

mkg20001 suggested changes Mar 5, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

src/index.js Show resolved Hide resolved

src/listener.js Outdated Show resolved Hide resolved

src/server/bin.js Show resolved Hide resolved

src/server/index.js Outdated Show resolved Hide resolved

chore: address review

d7abfde

vasco-santos force-pushed the refactor/async-await branch from 0c3d407 to d7abfde Compare March 5, 2020 16:22

vasco-santos mentioned this pull request Mar 5, 2020

Should stardust be a reliable address #22

Open

vasco-santos added 3 commits March 5, 2020 18:01

chore: address review

021babe

fix: use correct listen addr

71e13b7

chore: merge

002f831

vasco-santos mentioned this pull request Mar 6, 2020

chore: add stardust to libp2p config and updated instructions on browser example ipfs/js-ipfs#2831

Closed

3 tasks

mkg20001 approved these changes Mar 6, 2020

View reviewed changes

src/server/index.js Outdated Show resolved Hide resolved

src/utils.js Outdated Show resolved Hide resolved

chore: apply suggestions from code review

044cf3f

Co-Authored-By: Maciej Krüger <mkg20001@gmail.com>

jacobheun approved these changes Mar 6, 2020

View reviewed changes

mkg20001 merged commit f7393db into master Mar 6, 2020

mkg20001 deleted the refactor/async-await branch March 6, 2020 18:23

jacobheun mentioned this pull request Mar 26, 2020

Update bin.js #16

Closed

lidel mentioned this pull request Mar 31, 2020

Getting project back on track 🏗️👨‍🏭 ipfs-shipyard/ipfs-share-files#75

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: async await #14

refactor: async await #14

vasco-santos commented Jan 8, 2020 •

edited

Loading

vasco-santos commented Feb 10, 2020

vasco-santos commented Feb 11, 2020

mkg20001 commented Feb 11, 2020 •

edited

Loading

mkg20001 left a comment

mkg20001 Feb 12, 2020

vasco-santos Feb 12, 2020

mkg20001 Feb 12, 2020

vasco-santos Feb 12, 2020

mkg20001 Feb 16, 2020

mkg20001 Feb 16, 2020

jacobheun Feb 26, 2020

vasco-santos Feb 28, 2020

jacobheun Feb 28, 2020

jacobheun left a comment

jacobheun Mar 5, 2020

vasco-santos Mar 5, 2020

jacobheun Mar 5, 2020

mkg20001 left a comment

vasco-santos commented Mar 6, 2020

mkg20001 left a comment

vasco-santos commented Mar 6, 2020

jacobheun left a comment

mkg20001 commented Mar 6, 2020

vasco-santos commented Mar 6, 2020

vasco-santos commented Mar 6, 2020

refactor: async await #14

refactor: async await #14

Conversation

vasco-santos commented Jan 8, 2020 • edited Loading

vasco-santos commented Feb 10, 2020

vasco-santos commented Feb 11, 2020

mkg20001 commented Feb 11, 2020 • edited Loading

mkg20001 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobheun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkg20001 left a comment

Choose a reason for hiding this comment

vasco-santos commented Mar 6, 2020

mkg20001 left a comment

Choose a reason for hiding this comment

vasco-santos commented Mar 6, 2020

jacobheun left a comment

Choose a reason for hiding this comment

mkg20001 commented Mar 6, 2020

vasco-santos commented Mar 6, 2020

vasco-santos commented Mar 6, 2020

vasco-santos commented Jan 8, 2020 •

edited

Loading

mkg20001 commented Feb 11, 2020 •

edited

Loading