Provide ShadowsocksReader/Writer as a library

[fortuna]

This change does some refactoring that allows the use of go-shadowsocks2 as a library and customize its behavior.

Background

In Outline (https://getoutline.org) we need to customize the server to allow for multiple users, single port and to collect metrics. However, none of the existing servers provide that in a way we can use. And we will certainly need other changes in the future.

go-shadowsocks2 seemed to be the best implementation to base our server, since it's modern and the most modular. We will be migrating from shadowsocks-libev which we currently use, but is harder to turn into a library.

Even though we need to have our own server binary, we would like to diverge as little as possible from go-shadowsocks2. That's why I'm introducing this PR that will make it possible for us to keep most functionality in go-ss2.

We want to contribute back to the community, so it's our intention to upstream as many fixes and features as possible back to go-ss2. To us, the more of our features we have in go-ss2, the better, but we respect if you decide to not accept some of the features.

This Change

Going back to the PR, the change I'm making is providing the ShadowsocksReader and ShadowsocksWriter classes that encapsulate the Shadowsocks crypto logic.

Currently that logic is split across shadowaead.streamConn, shadowaead.Reader and shadowaead.Writer. While encrypting/decrypting is in the Reader and Writer, the salt initialization is in the streamConn, which prevents reuse.

This change moves the initialization logic to ShadowsocksReader and ShadowsocksWriter, making them standalone classes that can be easily reused.

As a bonus, I added a number of unit tests to make sure things continue to work.

Use

You may be interested in seeing how a server using go-shadowsocks2 as a library would look like. Here is a fully functioning server that I was able to deploy using the Outline Manager:
https://github.com/Jigsaw-Code/outline-ss-server.

It shows how we can add multi-user, single port and metrics tracking with with prometheus.io without having to modify go-shadowsocks2.

I'd be happy to upstream those features as a follow up if you'd like.

Please let me know what your thoughts on this are on our approach and how you would like to advance with this PR.

[riobard]

Hi @fortuna

Thanks for the PR and sorry for the delay. I have some free time to investigate recently. Since it's quite a big change set, I'll have to spend some time to study it first. Questions will be communicated here in the coming days.

[fortuna]

Thanks. Let me know if you have any questions. I can also walk you through the changes if it helps.

[riobard]

@fortuna The changes are way too aggressive for little benefit. Specifically, the entire PR seems to depend on the new DuplexConn interface, but it can be completely avoided by using type assertion, e.g.

type closeWriter interface { CloseWrite() error }
type closeReader interface { CloseRead() error }

func copyHalfClose(dst, src net.Conn) (int64, error) {
    n, err := io.Copy(dst, src)
    if w, ok := dst.(closeWriter); ok { w.CloseWrite() }
    if r, ok := src.(closeReader); ok { r.CloseRead() }
    return n, err
}

There are other minor issues which I won't go after here. I believe the PR can be significantly simplified without the unnecessary DuplexConn interface.

Additionally, I'd like to know what is missing for go-ss2 to be used as a library? I thought it's currently already usable as a library, and I failed to see what this PR improves upon that. Please explain a bit more about what you need. Thanks a lot!

[fortuna]

@riobard, I'm sorry the intent of my changes and our needs were not clear. I'll try to break them down and clarify.

DuplexConn

The goal of DuplexConn is to add API safety.

The suggestion you gave for copyHalfClose would break if dst and src don't support CloseWrite() and CloseRead(). The connections would remain open.

There's an implicit assumption throughout the code that the connections can have the Reader and Writer closed separately. By defining DuplexConn we make that implicit assumption explicit, and prevent illegal use. While it's fine in go-shadowsocks2, because the connection is always a net.TCPConn, that's not guaranteed when it becomes a library. The change makes the library more robust by making it harder to do the wrong thing.

I could change the code to use TCPConn and still get that guarantee, but then we would prevent the use of UnixConn. Introducing DuplexConn is a way to support any type of net.Conn that supports CloseRead() and CloseWrite() and still get the API safety guarantees.

Notice that while it may feel like a large change, besides the addition of adaptor.go it's only a string replacement in multiple places. Once you consider that, it's a straightforward and much smaller change.

Does that clarify what the DuplexConn trade-off is? I can revert it, but we would lose the API safety. What would you prefer?

stream.go

I'd say the biggest change was in stream.go. What I did there is consolidate all reading and write logic into ShadowsocksReader and ShadowsocksWriter. Before the change, the logic was part in reader and writer and part in streamConn, preventing me from accessing the read and write logic directly. And I couldn't really close them separately. The new classes not only makes tests easier and cleaner, but also allows me to replay reads for multi-user.

Does that make sense?

Needed APIs

The functionality I need can be found at https://github.com/Jigsaw-Code/outline-ss-server/blob/78993a32d867366f3fbf7bfa502cc0dba34bcf95/server.go#L35

This is already available in libraries ✔️ :

• core.ListCipher() and core.PickCipher()
• socks.ReadAddr() and socks.SplitAddr()
• shadowaead.Unpack() and shadowaead.NewPacketConn()

These are not available in libraries, or need change:

• ssnet.Relay()
  • Present in tcp.go, but needs change to properly close the connections and be placed in a library.
• shadowaead.NewShadowsocksWriter() and shadowaead.NewShadowsocksReader()
  • Present in shadowaead/stream.go, but needs to be consolidated with logic from streamConn
• ssnet.WrapDuplexConn()
  • Replaces streamConn. It joins a DuplexConn, Reader and Writer to make a new DuplexConn. This is needed in multi user to allow me to modify the Reader part of the client connection for replays.

Does this address your concerns? Do you have suggestions on how to achieve that in way that you are comfortable with?

[riobard]

@fortuna As library authors we should require as little as possible from library users. go-ss2 is specifically designed to minimize exposed API surface area, and use standard interfaces like net.Conn, io.Writer, and io.Reader whenever possible. The minimal API makes it easier to interact with other libraries.

My main objection against DuplexConn is that it's too much to ask for little benefit. If users give us a net.Conn we should make do with it, instead of forcing them to wrap it in an one-of-a-kind interface. Both net.TCPConn and net.UnixConn implement CloseRead() and CloseWrite(). It's perfectly fine to type assert for just those two methods and optionally half close.

What we should worry about is what to do if the net.Conn does not actually support half closing? How do we break out of the relay function in that case? It's a tricky problem. In practice, half close is pretty rare now that I don't think it's worth the cost to break a clean API for obsolete use cases.

---

As for the needed API, my take is that you think the building blocks currently exposed in go-ss2 are too primitive for Outline? The original plan is to only provide minimal API that handles Shadowsocks protocol and crypto, and the rest (e.g. relay and OS-dependent redirection) is left to library users to deal with. What you're asking seems to be higher-level and easier API? Is my understanding correct?

[fortuna]

I agree with the point of not asking too much from the user. To be clear, we are not asking more from the user: you can pass a TCPConn or a UnixConn and it just works.

But if you are not comfortable with that, I'll look into reverting it back to net.Conn. I'll let you know when I'm done with that.

---

On the needed API, yes, I'd like to have a higher level and easier to use API. You provide a "AEAD Reader/Writer", but I need a "Shadowsocks Reader/Writer", which is higher level. It should take care of the IV, counter, and the details of the SS crypto protocol.

Currently that logic is in streamConn, making it inaccessible. So what I did was to move the Shadowsocks crypto logic to the ShadowsocksReader/Writer classesand keep streamConn super lightweight. One nice thing about the new classes is that it allows you to speak Shadowsocks crypto in any data stream.

Does it make sense? Are you ok with that part of the change? I think this is actually the most important part of the change.

[riobard]

Yes, please revert back to use net.Conn interface. We can discuss how to support half closing later. IIRC, the original relay implementation tried to support half close but ended up with lots of zombie connections that never got properly closed due to network issues. The obscure parts of TCP are not really well supported by many network devices, unfortunately :(

---

go-ss2 adopts a 2-layer approach: lower layer is crypto (AEAD and stream ciphers), and higher layer is Shadowsocks protocol (handshake and addressing). Now that you pointed it out, it's clear to me that we have a solid crypto layer (as in package shadowstream and shadowaead) but almost no Shadowsocks layer (which was supposed to be in package core). I guess it's partly because Shadowsocks protocol is so simple that I just parse addresses directly, and also partly because it's tightly coupled with OS-dependnt redirection mechanism.

Indeed it does make sense to have a higher-level API that combines the two layers so library users can invoke directly without worrying too much about the internals here. I'll look thru the Outline use case and the example you sent to see what to do. Now that we agree to drop DuplexConn so there's no need for the net subpackage, maybe package core is the right place to put the API.

One question: does Outline intend to provide backward compatibility with the deprecated shadowstream crypto, or do you want to enforce AEAD ciphers? It seems to complicate the multi-user hack mentioned in another place. Please let me know what's your intention here. I guess it'll affect the design of the higher-level API.

[fortuna]

I believe I addressed most of your concern, but there are still some open questions.

DuplexConn

I reverted back to use net.Conn. The change touches a lot less files now.
Note, however, that I still need code to wrap the ShadowsocksReader/Writer into a connection.

API

I'm not sure I understand what you'd like to do here. Do you have any objection to the ShadowsocksReader and ShadowsocksWriter? They encapsulate and expose the Shadowsocks encryption and do what we need.

Stream ciphers

One question: does Outline intend to provide backward compatibility with the deprecated shadowstream crypto, or do you want to enforce AEAD ciphers?

We will not support the old non-AEAD ciphers. I see no valid reason to use them in new servers.

[fortuna]

FYI, I realized I don't need to keep the new relay code in go-shadowsocks2, so I moved to outline-ss-server and reverted that part of the change as you wanted.

[riobard]

Sorry for the delay. Got busy last week.

Now with the DuplexConn interface gone, I'm not sure what else do we get from this PR? The proposed ShadowsocksWriter and ShadowsocksReader interfaces are functionally the same as the original Writer/Reader. Did I miss anything here? Please advise.

[fortuna]

I've updated the PR description to clarify:

I'm providing the ShadowsocksReader and ShadowsocksWriter classes that encapsulate the Shadowsocks crypto logic.

Currently that logic is split across shadowaead.streamConn, shadowaead.Reader and shadowaead.Writer. While encrypting/decrypting is in the Reader and Writer, the salt initialization is in the streamConn, which prevents reuse.

This change moves both the crypto and initialization logic to ShadowsocksReader and ShadowsocksWriter, making them standalone classes that can be easily reused for our purposes.

As a bonus, I added a number of unit tests to make sure things continue to work.

Does that make sense? This change is a lot smaller now.

[fortuna]

@riobard Does the explanation above make sense to you? I need the IV logic in the reader/writer.
Also, the tests are valuable too.

[riobard]

@fortuna I see how you want to reorganize the code. What I don't understand is why not directly use NewConn, which combines all crypto and initialization you need? Or maybe you have a concrete use case to separate the reader and writer pair out of the combined Conn?

[fortuna]

That's right, In the multi-user implementation I need the Reader separate from the Writer so I can wrap it in different ways before creating a connection later.

I can't take a connection as input directly because I need to be able to replay the bytes when trying each cipher. I wrap the original net.TCPConn with a TeeReader to pipe any bytes read to a replay buffer, and then I create a MultiReader that reads from the replay buffer before reading from the connection in the TeeReader.

Once I find the cipher, I have to stop buffering for replay, so I create a new MultiReader just to consume any bytes already read before continuing to consume the connection.

See the findAccessKey code at
https://github.com/Jigsaw-Code/outline-ss-server/blob/3060d1308b32a5d31bef7f70631287ed8604eb25/tcp.go#L54

[fortuna]

@riobard Any thoughts? Are you ok with proceeding with refactoring the Reader/Writer so they independently contain the full logic I need for multi user and testing?

If not, I guess I can proceed with my own fork of the code, but it would be a pity to not make the contributions upstreamed.

[riobard]

@fortuna I now understand your concern. I tend to agree with the change, but I need to consider several factors together, namely

• API stability: this PR changes the exposed API so it comes with compatibility issues. We hadn't run into situations like this before, and I'm still thinking about what promises we'll make henceforth.
• Multiuser support: AFAIK this PR is primarily driven by the need to support multiuser without changing the underlying protocol. There are several proposals to address the issue (e.g. Secure single-port multi-user authentication shadowsocks-org#54 and Proposal: Multi-user support in Shadowsocks protocol shadowsocks-org#128), but so far no consensus has been reached yet.
• UDP mode: it's not clear to me how you're gonna support multiuser for UDP mode, which raises more questions about the symmetry between shadowstream and shadowaead that is broken by this PR.

I'm on the move this week and I'd like to spend some time on the implications. Please let me know if you have further feedback regarding issues raised here. Thanks! :)

[fortuna]

Here are my thoughts on the points you raised:

API stability.

It seems the non-backward compatible change would be the removal of shadowaead.NewReader() and shadowaead.NewWriter.
I went to Sourcegraph and searched for uses of those methods, and did not find any: NewReader, NewWriter

Of course, that's not foolproof, but may give you some piece of mind. Their search is pretty good. You can see, for instance, all the references for shadowaead.NewConn, which we are keeping.

My guess is that those methods don't actually address existing developer needs, making them not useful. Maybe because they are not easy to use, since you need to call them when you have the salt, which requires some sort of connection interception. The new writer and reader I'm introducing are a lot easier to use, since it does the interception and initialization for you.

Multi-user support

I think the multi-user discussion should happen separately. While I think it would be great if a new version of the protocol supports it, our users need the existing protocol to support multi-users right now, so we'll proceed with our multi-user solution regardless of the new protocol. We will certainly consider the new protocol when it's agreed upon.

UDP Mode

This is our multi-user code for UDP: https://github.com/Jigsaw-Code/outline-ss-server/blob/d29f83117efeaa294b02bd8fc68ee0ed7db46774/udp.go#L37

I simply call shadowaead.Unpack() with each cipher until I find the right one.
For UDP we don't need to replay a connection stream without consuming it, which removes the need for all the wrapping we have in TCP.

Shadowstream - Shadowaead symmetry

This change provides standalone reader and writer for shadowaead, but not shadowstream, so you're right it introduces an asymmetry. I figured that implementing them for shadowstream would be of no value, since I wouldn't use it.

Final thoughts

My initial intent with the go-shadowsocks2 changes was to have it contain as many of the functionality of outline-ss-server as possible, so we would benefit the community and maintain only a little bit of code in our fork.

I realize I'm proposing significant changes, and I understand if you're not comfortable with that. Given the situation, I believe it will be best if I also move the ShadowsocksReader/Writer to our code, so we won't need to change go-shadowsocks2 at all. We'll still depend on the cipher primitives, so that's code we won't need to maintain.

[fortuna]

FYI, I moved the change to outline-ss-server:
https://github.com/Jigsaw-Code/outline-ss-server/blob/master/shadowsocks/stream.go
https://github.com/Jigsaw-Code/outline-ss-server/blob/master/shadowsocks/stream_test.go

No need for this change anymore.

[riobard]

@fortuna Understood. We'll consider an API redesign in the next major release to address the issues :)