Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send iv/salt, target addr with payload altogether #164

Closed

Conversation

lixin9311
Copy link
Collaborator

Alternative patch of #160

Only tested by CI

@lixin9311
Copy link
Collaborator Author

@Anonymous-Someneese Thank you for raising the issue. Please check if this patch could fix your problem.

oif added a commit to oif/go-shadowsocks2 that referenced this pull request Feb 6, 2020
payloadBuf = payloadBuf[:nr]
buf[0], buf[1] = byte(nr>>8), byte(nr) // big-endian payload size
readAndEnctypt := func(buf []byte) (n int, err error) {
payloadBuf := buf[2+w.Overhead():]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this may results a panic due to index out of range(L49).

max(n) known as payloadBuf size in L47 = bufLength - 2 - overhead, so to get bytes in this line will be 2 + overhead + buf length - 2 - overhead + overhead = overhead + buf length has a extra overhead.

so here should be payloadBuf := buf[2+w.Overhead() : 2+w.Overhead()+payloadSizeMask]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the feedback, I fixed this issue in the latest commit.

@riobard
Copy link

riobard commented Feb 15, 2020

@lixin9311 Since this PR changes the public interface of exported packages, maybe we should version it somehow? So we don't break code using this as dependency.

@lixin9311
Copy link
Collaborator Author

@lixin9311 Since this PR changes the public interface of exported packages, maybe we should version it somehow? So we don't break code using this as dependency.

I think if we generate salt/iv in initWriter/newWrite, it will be ok. But the behavior of NewWriter will be changed.
Create & send salt/iv before creating the Writer -> Writer will automatically create and send salt/iv.

Nevertheless, no method for creating a StreamCipher Writer was exported, and that will be the right behavior of a Writer I think.

@lixin9311
Copy link
Collaborator Author

Forget it, I can't see a better way without changing NewWriter(w io.Writer, aead cipher.AEAD), as long as it takes cipher.AEAD as a parameter.

@Anonymous-Someneese
Copy link

My question still remains. If a client connects but expect the server to send the first byte. Wouldn't the proxy stuck at readerWithHeader.Read()?

@lixin9311
Copy link
Collaborator Author

@riobard I've been thinking about it. Although manually fixing it or enabling Nagle's algorithm (SetNoDelay(false)) will fix the issue, there would be a significant latency penalty, and it's bad for some games using TCP (e.g., Minecraft).
I don't think we should fix the signature problem here. Instead, I would recommend using plugins to avoid such an issue.

@lixin9311 lixin9311 closed this Feb 17, 2020
@riobard
Copy link

riobard commented Feb 17, 2020

How much extra delay will be introduced by enabling Nagel's algo just for the first few packets?

@lixin9311
Copy link
Collaborator Author

https://en.wikipedia.org/wiki/Nagle%27s_algorithm#Interactions_with_real-time_systems

Very depending on the use case, and packet size.

If we set a read timeout for a few milliseconds to the underlying TCP socket, and cancel it after the first successful read. There would be little penalty if the server was supposed to send a message first, and we don't have to use sync package (syncing goroutines will introduce more latency).

Anyway, I don't think it is the right place to solve the signature problem in the proxy.

@riobard
Copy link

riobard commented Feb 17, 2020

We could probably turn off nodelay initially, and turn it back on when we receive the first piece of data from user clients.

@riobard
Copy link

riobard commented Feb 17, 2020

Actually I'm wondering if turning off TCP_NODELAY works for the first packet at all?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants