Transit Refactor (no changes in cryptography and framing) by piegamesde · Pull Request #80 · LeastAuthority/wormhole-william

piegamesde · 2022-09-12T13:24:54Z

OP#1896, OP#1895, OP#2246

Code Review Checklist (to be filled out by reviewer)

Description accurately reflects what changes are being made.
Description explains why the changes are being made (or references an issue containing one).
The PR appropriately sized.
New code has enough tests.
New code has enough documentation to answer "how do I use it?" and "what does it do?".
Existing documentation is up-to-date, if impacted.

piegamesde · 2022-09-16T13:02:47Z

~~Ready for review, but not for merging yet~~

piegamesde · 2022-09-21T10:36:34Z

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.

donpui · 2022-09-23T06:34:32Z

 	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


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.

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.

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

vu3rdd

Looks very good. Very nice use of Go's interfaces.

Left some comments. But I suggest we get it in and then address anything else (if at all) as a separate task later.

vu3rdd · 2022-09-26T06:52:32Z

+	readHandshakeMsg(expected []byte) error
+
+	Close() error
+}


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.

No, the handshake is done later on by the transitCryptor abstraction layer. I don't see a good way around this.

vu3rdd · 2022-09-26T09:20:10Z

+}
+
+func (self *wsConnection) writeMsg(msg []byte) error {
+	return self.conn.Write(self.ctx, websocket.MessageBinary, msg)


So, here, no extra length field added into the record and instead the websocket framing takes care of the length, right?

donpui · 2022-09-26T11:26:29Z

Wasm module is not building up:

➜  wormhole-william git:(piegames/noise-transit) GOOS=js GOARCH=wasm go build -o ../../dist/wormhole.wasm ./wasm/module
# gitlab.com/yawning/x448.git/internal/toolchain
../../../../../go/pkg/mod/gitlab.com/yawning/x448.git@v0.0.0-20211010103852-8630503de309/internal/toolchain/constraints.go:45:6: undefined: __SOFTWARE_REQUIRES_GOARCH_NOT_WASM__
# gitlab.com/yawning/x448.git/internal/field
../../../../../go/pkg/mod/gitlab.com/yawning/x448.git@v0.0.0-20211010103852-8630503de309/internal/field/field.go:26:22: undefined: TightFieldElement
../../../../../go/pkg/mod/gitlab.com/yawning/x448.git@v0.0.0-20211010103852-8630503de309/internal/field/field.go:33:22: undefined: TightFieldElement
../../../../../go/pkg/mod/gitlab.com/yawning/x448.git@v0.0.0-20211010103852-8630503de309/internal/field/field.go:33:42: undefined: LooseFieldElement
../../../../../go/pkg/mod/gitlab.com/yawning/x448.git@v0.0.0-20211010103852-8630503de309/internal/field/field.go:37:30: undefined: LooseFieldElement
../../../../../go/pkg/mod/gitlab.com/yawning/x448.git@v0.0.0-20211010103852-8630503de309/internal/field/field.go:37:50: undefined: TightFieldElement
../../../../../go/pkg/mod/gitlab.com/yawning/x448.git@v0.0.0-20211010103852-8630503de309/internal/field/field.go:41:27: undefined: TightFieldElement
../../../../../go/pkg/mod/gitlab.com/yawning/x448.git@v0.0.0-20211010103852-8630503de309/internal/field/field.go:41:51: undefined: LimbUint
../../../../../go/pkg/mod/gitlab.com/yawning/x448.git@v0.0.0-20211010103852-8630503de309/internal/field/field.go:52:25: undefined: TightFieldElement
../../../../../go/pkg/mod/gitlab.com/yawning/x448.git@v0.0.0-20211010103852-8630503de309/internal/field/field.go:52:50: undefined: LooseFieldElement
../../../../../go/pkg/mod/gitlab.com/yawning/x448.git@v0.0.0-20211010103852-8630503de309/internal/field/field.go:52:74: undefined: LimbUint
../../../../../go/pkg/mod/gitlab.com/yawning/x448.git@v0.0.0-20211010103852-8630503de309/internal/field/field.go:52:74: too many errors
# github.com/oasisprotocol/curve25519-voi/internal/field
../../../../../go/pkg/mod/github.com/oasisprotocol/curve25519-voi@v0.0.0-20210831082354-38e59a871ca9/internal/field/field.go:69:11: undefined: Element
../../../../../go/pkg/mod/github.com/oasisprotocol/curve25519-voi@v0.0.0-20210831082354-38e59a871ca9/internal/field/field.go:75:11: undefined: Element
../../../../../go/pkg/mod/github.com/oasisprotocol/curve25519-voi@v0.0.0-20210831082354-38e59a871ca9/internal/field/field.go:84:11: undefined: Element
../../../../../go/pkg/mod/github.com/oasisprotocol/curve25519-voi@v0.0.0-20210831082354-38e59a871ca9/internal/field/field.go:93:11: undefined: Element
../../../../../go/pkg/mod/github.com/oasisprotocol/curve25519-voi@v0.0.0-20210831082354-38e59a871ca9/internal/field/field.go:102:11: undefined: Element
../../../../../go/pkg/mod/github.com/oasisprotocol/curve25519-voi@v0.0.0-20210831082354-38e59a871ca9/internal/field/field.go:108:11: undefined: Element
../../../../../go/pkg/mod/github.com/oasisprotocol/curve25519-voi@v0.0.0-20210831082354-38e59a871ca9/internal/field/field.go:120:11: undefined: Element
../../../../../go/pkg/mod/github.com/oasisprotocol/curve25519-voi@v0.0.0-20210831082354-38e59a871ca9/internal/field/field.go:132:11: undefined: Element
../../../../../go/pkg/mod/github.com/oasisprotocol/curve25519-voi@v0.0.0-20210831082354-38e59a871ca9/internal/field/field.go:174:11: undefined: Element
../../../../../go/pkg/mod/github.com/oasisprotocol/curve25519-voi@v0.0.0-20210831082354-38e59a871ca9/internal/field/field.go:180:11: undefined: Element
../../../../../go/pkg/mod/github.com/oasisprotocol/curve25519-voi@v0.0.0-20210831082354-38e59a871ca9/internal/field/field.go:180:11: too many errors
# github.com/oasisprotocol/curve25519-voi/curve/scalar
../../../../../go/pkg/mod/github.com/oasisprotocol/curve25519-voi@v0.0.0-20210831082354-38e59a871ca9/curve/scalar/scalar.go:551:28: undefined: unpackedScalar
../../../../../go/pkg/mod/github.com/oasisprotocol/curve25519-voi@v0.0.0-20210831082354-38e59a871ca9/curve/scalar/scalar.go:555:27: undefined: unpackedScalar
../../../../../go/pkg/mod/github.com/oasisprotocol/curve25519-voi@v0.0.0-20210831082354-38e59a871ca9/curve/scalar/scalar_unpacked.go:35:27: undefined: unpackedScalar
../../../../../go/pkg/mod/github.com/oasisprotocol/curve25519-voi@v0.0.0-20210831082354-38e59a871ca9/curve/scalar/scalar_unpacked.go:40:10: undefined: unpackedScalar
../../../../../go/pkg/mod/github.com/oasisprotocol/curve25519-voi@v0.0.0-20210831082354-38e59a871ca9/curve/scalar/scalar_unpacked.go:48:10: undefined: unpackedScalar
../../../../../go/pkg/mod/github.com/oasisprotocol/curve25519-voi@v0.0.0-20210831082354-38e59a871ca9/curve/scalar/scalar_unpacked.go:57:10: undefined: unpackedScalar
../../../../../go/pkg/mod/github.com/oasisprotocol/curve25519-voi@v0.0.0-20210831082354-38e59a871ca9/curve/scalar/scalar_unpacked.go:64:10: undefined: unpackedScalar
../../../../../go/pkg/mod/github.com/oasisprotocol/curve25519-voi@v0.0.0-20210831082354-38e59a871ca9/curve/scalar/scalar_unpacked.go:71:10: undefined: unpackedScalar
../../../../../go/pkg/mod/github.com/oasisprotocol/curve25519-voi@v0.0.0-20210831082354-38e59a871ca9/curve/scalar/scalar_unpacked.go:76:10: undefined: unpackedScalar
../../../../../go/pkg/mod/github.com/oasisprotocol/curve25519-voi@v0.0.0-20210831082354-38e59a871ca9/curve/scalar/scalar_unpacked.go:127:10: undefined: unpackedScalar
../../../../../go/pkg/mod/github.com/oasisprotocol/curve25519-voi@v0.0.0-20210831082354-38e59a871ca9/curve/scalar/scalar_unpacked.go:127:10: too many errors

meejah · 2022-09-26T20:14:34Z

Why are we changing the Transit protocol?

This won't be compatible with anything else...

meejah

If I understand this correctly, this will make Transit incompatible with all other implementations and completely changes the cryptography.

Why would we do this? What's the motivation here? What's wrong with the extremely standard NaCl cryptography?

piegamesde · 2022-09-26T20:21:49Z

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.

Why would we do this? What's the motivation here? What's wrong with the extremely standard NaCl cryptography?

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)

meejah · 2022-09-26T21:05:54Z

So we're not going to deploy this, ever?
Or, what do you mean by "not active in the Transit layer"?

(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.

piegamesde · 2022-09-26T21:40:35Z

Unless Noise for Transit becomes a thing in the spec, the code path that enables it in Wormhole William will never go live, no.

meejah · 2022-09-27T02:11:31Z

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)

vu3rdd · 2022-09-27T02:16:12Z

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.

I just wanted to add that, it would be better to handle each tickets in its own PR.

piegamesde · 2022-09-27T10:31:56Z

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

it would be better to handle each tickets in its own PR

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 · 2022-09-27T10:54:18Z

Okay, I've squashed all changes and then committed everything except for the transportCryptorNoise struct. Let's see if it compiles …

donpui · 2022-09-27T11:40:21Z

Now wasm is building up

i'm fine as long as we dont deploy non-standard transit

meejah · 2022-09-28T19:44:53Z

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.

piegamesde · 2022-09-28T23:00:05Z

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.

meejah · 2022-09-29T20:38:40Z

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.

Yawning · 2022-09-30T09:21:56Z

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.

Yawning · 2022-10-03T10:35:27Z

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.

meejah · 2022-10-03T17:04:33Z

Although this branch doesn't use it, I went and fixed nyquist [..]

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

meejah

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?

donpui · 2022-11-17T09:39:49Z

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.
I think it is good code refactor and makes code clean, should help us further while implementing dilation.

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.

meejah · 2022-11-21T04:16:05Z

We should not wait for what will happen with upstream for this PR.

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?)

piegamesde force-pushed the piegames/noise-transit branch 2 times, most recently from befefe7 to 9ca5fa6 Compare September 12, 2022 13:52

piegamesde marked this pull request as ready for review September 16, 2022 13:02

piegamesde requested review from donpui and vu3rdd September 16, 2022 13:11

piegamesde force-pushed the piegames/noise-transit branch from 9ca5fa6 to 21af98c Compare September 21, 2022 10:34

piegamesde force-pushed the piegames/noise-transit branch from 21af98c to dd2766a Compare September 21, 2022 10:39

donpui reviewed Sep 23, 2022

View reviewed changes

vu3rdd approved these changes Sep 26, 2022

View reviewed changes

donpui mentioned this pull request Sep 26, 2022

update transit url to use WebSocket over TLS with standard port LeastAuthority/destiny#174

Closed

6 tasks

meejah previously requested changes Sep 26, 2022

View reviewed changes

CI: drop Go 1.15

4ed47f0

piegamesde force-pushed the piegames/noise-transit branch from 75a8eab to f3d9244 Compare September 27, 2022 10:51

piegamesde force-pushed the piegames/noise-transit branch 2 times, most recently from 8169621 to 79b127c Compare September 27, 2022 11:00

Transit refactor

ac19096

piegamesde force-pushed the piegames/noise-transit branch from 79b127c to ac19096 Compare September 27, 2022 11:02

piegamesde changed the title ~~Transit: add noise cryptography~~ Transit: don't add noise cryptography yet Sep 27, 2022

Donatas Puidokas and others added 2 commits October 25, 2022 14:58

Fixed empty readBuf issue

5802757

Back to good old framing

fc5ee1e

meejah requested changes Oct 25, 2022

View reviewed changes

donpui changed the title ~~Transit: don't add noise cryptography yet~~ Transit Refactor (no changes in cryptography and framing) Nov 17, 2022

donpui self-requested a review November 17, 2022 09:40

donpui approved these changes Jan 31, 2023

View reviewed changes

Conversation

piegamesde commented Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Checklist (to be filled out by reviewer)

Uh oh!

piegamesde commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

piegamesde commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

donpui Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donpui Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

donpui Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

vu3rdd left a comment

Choose a reason for hiding this comment

Uh oh!

vu3rdd Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

piegamesde Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

vu3rdd Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

donpui commented Sep 26, 2022

Uh oh!

meejah commented Sep 26, 2022

Uh oh!

meejah left a comment

Choose a reason for hiding this comment

Uh oh!

piegamesde commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meejah commented Sep 26, 2022

Uh oh!

piegamesde commented Sep 26, 2022

Uh oh!

meejah commented Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vu3rdd commented Sep 27, 2022

Uh oh!

piegamesde commented Sep 27, 2022

Uh oh!

piegamesde commented Sep 27, 2022

Uh oh!

donpui commented Sep 27, 2022

Uh oh!

meejah commented Sep 28, 2022

Uh oh!

piegamesde commented Sep 28, 2022

Uh oh!

meejah commented Sep 29, 2022

Uh oh!

Yawning commented Sep 30, 2022

Uh oh!

Yawning commented Oct 3, 2022

Uh oh!

meejah commented Oct 3, 2022

Uh oh!

meejah left a comment

Choose a reason for hiding this comment

Uh oh!

donpui commented Nov 17, 2022

Uh oh!

meejah commented Nov 21, 2022

Uh oh!

Reviewers

Assignees

piegamesde commented Sep 12, 2022 •

edited

Loading

piegamesde commented Sep 16, 2022 •

edited

Loading

piegamesde commented Sep 21, 2022 •

edited

Loading

donpui Sep 23, 2022 •

edited

Loading

piegamesde commented Sep 26, 2022 •

edited

Loading

meejah commented Sep 27, 2022 •

edited

Loading