refactor: share Shadowsocks key search between TCP and UDP #189
Conversation
sbruens
force-pushed
the
sbruens/modularize-udp
branch
2 times, most recently
from
d74d1fd
to
bcf7e53
Compare
sbruens
force-pushed
the
sbruens/share-key-search
branch
from
f691d98
to
73143d3
Compare
sbruens
force-pushed
the
sbruens/share-key-search
branch
from
441d3de
to
b2134c8
Compare
sbruens
force-pushed
the
sbruens/share-key-search
branch
from
adee8a4
to
15cde23
Compare
sbruens
changed the title
There was a problem hiding this comment.
I'm not convinced of the value and correctness of this change. The API is more complex, and I don't think we can merge the logic. The formats are different. For UDP we need to unpack the whole package, while for TCP we need to unpack based on the cipher tag and IV size.
| metrics.AddTCPCipherSearch(false, 0) | ||
| return "", clientConn, onet.NewConnectionError("ERR_CIPHER", fmt.Sprintf("Reading header failed after %d bytes", n), err) | ||
| } | ||
|
|
||
| // Find the cipher and acess key id. |
There was a problem hiding this comment.
| // Find the cipher and acess key id. | |
| // Find the cipher and access key id. |
|
|
||
| unpackStart := time.Now() | ||
| // To hold the decrypted chunk length. | ||
| chunkLenBuf := make([]byte, bufferSize) |
There was a problem hiding this comment.
This is a very critical piece of code. It had to be heavily optimized to work well in production.
Could you please run the benchmarks to compare the performance and allocations?
| @@ -134,7 +112,6 @@ func (h *packetHandler) Handle(clientConn net.PacketConn) { | |||
|
|
|||
| func (h *packetHandler) authenticate(clientConn net.PacketConn) (net.Addr, *CipherEntry, []byte, int, *onet.ConnectionError) { | |||
| cipherBuf := make([]byte, serverUDPBufferSize) | |||
| textBuf := make([]byte, serverUDPBufferSize) | |||
There was a problem hiding this comment.
I believe there's an optimization we can do here. Instead of serverUDPBufferSize, I believe we can use clientProxyBytes, since the decrypted packet is always clientProxyBytes - key tag size (but we don't know the key yet).
|
|
||
| unpackStart := time.Now() | ||
| // To hold the decrypted chunk length. | ||
| chunkLenBuf := make([]byte, bufferSize) |
There was a problem hiding this comment.
Let's pass the buffer instead of the size, so the caller can own the allocation.
| // NewShadowsocksStreamAuthenticator creates a stream authenticator that uses Shadowsocks. | ||
| // TODO(fortuna): Offer alternative transports. | ||
| func NewShadowsocksStreamAuthenticator(ciphers CipherList, replayCache *ReplayCache, metrics ShadowsocksTCPMetrics) StreamAuthenticateFunc { | ||
| return func(clientConn transport.StreamConn) (string, transport.StreamConn, *onet.ConnectionError) { | ||
| firstBytes := make([]byte, bytesForKeyFinding) | ||
| if n, err := io.ReadFull(clientConn, firstBytes); err != nil { | ||
| metrics.AddTCPCipherSearch(false, 0) |
There was a problem hiding this comment.
This is a behavior change and will bias the metric. The point is to understand how slow the search is, there's no pointing in including the failures when read the data.
| chunkLenBuf := make([]byte, bufferSize) | ||
| for ci, elt := range ciphers { | ||
| entry := elt.Value.(*CipherEntry) | ||
| buf, err := shadowsocks.Unpack(chunkLenBuf, src, entry.CryptoKey) |
There was a problem hiding this comment.
I believe this is broken. src should have the exact size firstBytes[:cryptoKey.SaltSize()+2+cryptoKey.TagSize() for TCP and be the full packet for UDP.
We should have a test to catch that. Try different ciphers.
I remember trying to unify TCP and UDP, but they are different. I believe we are better off keeping the logic separate. It makes the signature of each method simpler, rather than a more complex API you have now. It's easier to use it wrong, vs the current code.
No description provided.