Transit Refactor (no changes in cryptography and framing) #80
Conversation
piegamesde
force-pushed
the
piegames/noise-transit
branch
2 times, most recently
from
befefe7 to
9ca5fa6
Compare
|
|
piegamesde
force-pushed
the
piegames/noise-transit
branch
from
9ca5fa6 to
21af98c
Compare
|
The latest commit now also fixes OP#2246. The new behavior is sufficiently compatible with the current relay server, so I think we can already merge this and do not depend on the relay server getting fixed too. |
piegamesde
force-pushed
the
piegames/noise-transit
branch
from
21af98c to
dd2766a
Compare
go.mod
Outdated
| @@ -9,6 +9,7 @@ require ( | |||
| github.com/klauspost/compress v1.15.0 | |||
| github.com/mdp/qrterminal/v3 v3.0.0 | |||
| github.com/spf13/cobra v1.5.0 | |||
| gitlab.com/yawning/nyquist.git v0.0.0-20211010104234-909e7e222b8f | |||
There was a problem hiding this comment.
I we confident to use this repo? Seems like this is on low usage / activity repo.
Also go.dev suggest another repo of the same person if I am correct: https://pkg.go.dev/search?q=nyquist&m=
Was this noise library analysed to use: flynn/noise ? Seems quit active and popular.
There was a problem hiding this comment.
In general it would be good to carefully assess and discuss new external libraries inclusion as it might raise certain security risks and can lead to compromising whole solution.
There was a problem hiding this comment.
Addition some comments from internal discussion:
flynn/noise
- less code
- less external dependencies
- seems to be more popular and active (29 closed PRs from many contributors vs. one person project)
yawning/nyquist
- better structure
- slightly higher coverage (85.4% vs 83%, basically similar)
- author is known for contribution to Tor and low-level cryptographic primitives
Alternative:
(wireguard-go case) mostly we use one of the protocol from the noise family, we could think of implementing that as a module within the project later and get rid of the library at some point later. This helps in keeping the dependencies low
| readHandshakeMsg(expected []byte) error | ||
|
|
||
| Close() error | ||
| } |
There was a problem hiding this comment.
Since we are refactoring this part, I am wondering if it makes sense to have it as close to Go's net.Conn interface. We only care about Read(), Write() and Close(). The handshake read/write need not be exposed as part of the interface because it is part of the code that establishes a connection and return a valid Conn. Perhaps a better example would be net/tls module in Go stdlib. The Dial() function encapsulates all the connection handshake etc and returns a Conn type that implements net.Conn interface.
There was a problem hiding this comment.
No, the handshake is done later on by the transitCryptor abstraction layer. I don't see a good way around this.
wormhole/transit.go
Outdated
| } | ||
|
|
||
| func (self *wsConnection) writeMsg(msg []byte) error { | ||
| return self.conn.Write(self.ctx, websocket.MessageBinary, msg) |
There was a problem hiding this comment.
So, here, no extra length field added into the record and instead the websocket framing takes care of the length, right?
|
Wasm module is not building up: |
|
Why are we changing the Transit protocol? This won't be compatible with anything else... |
|
This is currently not active in the Transit layer, but was still added there in order to profit from as many synergies between Transit and Dilation as possible. Also, it allowed me to test the code without having the initial Dilation setup. But in the end, I'd like to see it officially used in Transit, see magic-wormhole/magic-wormhole-protocols#25 for brainstorming.
The same questions also apply to why the cryptography was changed for Dilation, since NaCl could have been reused there as well. The only advantage I see is that it takes over the nonce handling, making implementing it securely even more idiot-proof than NaCl's secretbox. Also, we could use authenticated data for the unencrypted length field more or less for free, although I haven't spend a lot of time thinking about this (I don't think this easily works when relay servers are involved) |
|
So we're not going to deploy this, ever? (I don't see the point of changing the existing Transit protocol, when there's Dilation. New protocols should use that. Having two incompatible Transit protocols just means more work for all implementations.) There's nothing "wrong" with NaCl but when designing a completely new protocol (Dilation) it made sense to use the work Noise had done. However, it doesn't necessarily follow that transit needs to immediately change too. |
|
Unless Noise for Transit becomes a thing in the spec, the code path that enables it in Wormhole William will never go live, no. |
|
All the secretbox code appears to be deleted, though; how can we use a version with this code but still be a secretbox-based transit connection? (i.e. we want the proper framing, but existing Transit protocol using NaCl) |
I just wanted to add that, it would be better to handle each tickets in its own PR. |
|
The secretbox code was not deleted, it was moved as part of the refactoring: https://github.com/LeastAuthority/wormhole-william/pull/80/files#diff-12b6490102104ebe1fbeb8c4bb576802a73d7466d5913c05dcf92195086e0694R156
I think we can all agree on that, but unfortunately there are hard interdependencies between the commits due to the refactoring I had to do in order to properly implement the features. I can split off the last commit in its own PR, but that wouldn't change the fact that it strongly depends on the previous commits. |
piegamesde
force-pushed
the
piegames/noise-transit
branch
from
75a8eab to
f3d9244
Compare
|
Okay, I've squashed all changes and then committed everything except for the |
piegamesde
force-pushed
the
piegames/noise-transit
branch
2 times, most recently
from
8169621 to
79b127c
Compare
piegamesde
force-pushed
the
piegames/noise-transit
branch
from
79b127c to
ac19096
Compare
|
Now wasm is building up |
piegamesde
changed the title
meejah
dismissed
their stale review
i'm fine as long as we dont deploy non-standard transit
|
It would be a lot better to land just the framing fixes without all the re-factoring. IIUC, the point of the refactoring is to try out Noise -- but landing that on master here means that bringing in future updates from upstream (or pushing changes there) becomes harder, forever. If the intent is different, please describe that :) I don't anticipate ever wanting to support two different (but nearly identical) Transit protocols -- but that discussion can stay on the protocols repository. |
|
Apart from an unnecessary interface indirection (since the interface has only one implementor and one could directly use the implementing struct instead), I think this refactoring is still a net improvement for the code. I wouldn't have done it if not my noise cryptography experiments but now that it is there I would not like to go back. |
|
I'm not making a value judgement on the refactoring, but given that we still lack a plan at all for dealing with this forked code, any extensive changes to existing code will make synchronizing with upstream harder (whether that's pulling code or pushing code). Since Transit is a stable protocol (i.e. I don't anticipate any changes to it) and thus unlikely to need future development except for security or bug fixes, those are more great reasons to keep the repositories closer together. That is, a future security fix can be easily pushed to upstream or pulled in from upstream. |
|
Oh neat. I'm biased but I think nyquist is pretty great. I haven't really promoted it much, nor have I worked on it since the Noise spec hasn't changed since I wrote it. As far as builds failing when targeting WASM. My more recent crypto packages explicitly do not support WASM, as it is impossible to guarantee that things that should be constant time, are in fact constant time. |
|
Although this branch doesn't use it, I went and fixed nyquist so that it works on WASM, under the rationale that "well, everyone else under the sun is ignoring the specification's lack of timing characteristics". Sorry for the inconvenience. |
The next-generation "Dilation" protocol for magic-wormhole does specify Noise-based encryption for the subchannels (and we do have plans to implement that). See e.g. https://github.com/magic-wormhole/magic-wormhole/blob/master/docs/dilation-protocol.md#l2-protocol |
There was a problem hiding this comment.
I would like to first figure out what our plans are with the upstream Go repository.
Given that we've decided the "redundant" framing is actually the way to go, what's in this branch that's useful is the refactoring.
But, depending on our plans with upstream, this could make it far harder to pull in changes -- and given that Transit isn't going to change or grow a second specified encryption method we really need to understand the implications of even more divergence.
Do we have a ticket anywhere for the fork handling?
We should not wait for what will happen with upstream for this PR. Upstreaming questions is going separately and we have equally the same risk with any other already merged changes. Additionally this PR cannot go to upstream anyway without other changes included. As we dropped initial cryptography change and framing fix, so this PR left only as code refactor. As for initial websocket library trouble, seems we are now clear what was the problem at it very much related with Go specific about values, pointer usage. Good article describe why we need to be careful: https://eli.thegreenplace.net/2018/beware-of-copying-mutexes-in-go/ Before merging, I will verify changes once more if they work with Winden and Destiny app. |
donpui
changed the title
Yes we should wait; this PR just diverges further. I'm not convinced any changes in Transit help with Dilation in any way -- can you please describe how? I'm still -1 on merging this (until the upstream questions are answered). Or, consider answering: what problem do these 500 lines of changes solve? (i.e. what ticket is fixed or issue solved?) |
OP#1896, OP#1895, OP#2246
Code Review Checklist (to be filled out by reviewer)