search

GeyserMC / MCProtocolLib

A library for communication with a Minecraft client/server.
MIT License
724 stars 200 forks source link

Wrong TFO implementation #822

Closed xism4 closed 4 months ago

xism4 commented 4 months ago

It will never work, since the backend (Spigot/Paper) does not implement this option.

Add that if it is not active in this file (/proc/net/ipv4/tcp_fastopen enabled) it will never work, even if it is well implemented.

Server-side TCP FastOpen. Configures the maximum number of outstanding (waiting to be accepted) TFO connections

https://netty.io/wiki/tcp-fast-open.html

AlexProgrammerDE commented 4 months ago

Yes it works for backends that support TFO, like Velocity.

onebeastchris commented 4 months ago

mcpl's TCP fast open support is also opt-in, so I'm unsure what the issue here is? Is the implementation somehow wrong, or is it enabled when it shouldn't be?

xism4 commented 4 months ago

mcpl's TCP fast open support is also opt-in, so I'm unsure what the issue here is? Is the implementation somehow wrong, or is it enabled when it shouldn't be?

Velocity does not implement downstream TCP FAST CONNECT, only modes. This does not work either. Since not implemented on Paper

xism4 commented 4 months ago

mcpl's TCP fast open support is also opt-in, so I'm unsure what the issue here is? Is the implementation somehow wrong, or is it enabled when it shouldn't be?

It will simply never work, since it needs an implementation in the backend, besides the fact that it's approached in the wrong way as I said.

AlexProgrammerDE commented 4 months ago

mcpl's TCP fast open support is also opt-in, so I'm unsure what the issue here is? Is the implementation somehow wrong, or is it enabled when it shouldn't be?

Velocity does not implement downstream TCP FAST CONNECT, only modes. This does not work either. Since not implemented on Paper

No, it does implement it for both upstream and downstream connections. MCPL is both a mc server and client library and can utilize TFO both ways. https://github.com/PaperMC/Velocity/blob/71bb0246a8bf7507b570e6a82a99ce77a33ad19d/proxy/src/main/java/com/velocitypowered/proxy/network/ConnectionManager.java#L104 https://github.com/PaperMC/Velocity/blob/71bb0246a8bf7507b570e6a82a99ce77a33ad19d/proxy/src/main/java/com/velocitypowered/proxy/network/ConnectionManager.java#L167

xism4 commented 4 months ago

mcpl's TCP fast open support is also opt-in, so I'm unsure what the issue here is? Is the implementation somehow wrong, or is it enabled when it shouldn't be?

Velocity does not implement downstream TCP FAST CONNECT, only modes. This does not work either. Since not implemented on Paper

No, it does implement it for both upstream and downstream connections. MCPL is both a mc server and client library and can utilize TFO both ways. https://github.com/PaperMC/Velocity/blob/71bb0246a8bf7507b570e6a82a99ce77a33ad19d/proxy/src/main/java/com/velocitypowered/proxy/network/ConnectionManager.java#L104 https://github.com/PaperMC/Velocity/blob/71bb0246a8bf7507b570e6a82a99ce77a33ad19d/proxy/src/main/java/com/velocitypowered/proxy/network/ConnectionManager.java#L167

How about bungeecord?

AlexProgrammerDE commented 4 months ago

To my knowledge only Velocity. I'm thinking about making a PR to Paper this year to add TFO support.

xism4 commented 4 months ago

To my knowledge only Velocity. I'm thinking about making a PR to Paper this year to add TFO support.

That's my point, it is not well implemented to have a good use.

AlexProgrammerDE commented 4 months ago

The TFO implementation isn't wrong. It's for anyone interested in using TFO if available.

AlexProgrammerDE commented 4 months ago

It's niche, yes, but that's already known. It's also not what vanilla servers do, so it's as the discretion of the developers using MCPL.

xism4 commented 4 months ago

The TFO implementation isn't wrong. It's for anyone interested in using TFO if available.

At least not as "server" mode since TFO must have a number of connections (Server-side TCP FastOpen. Set the maximum number of pending TFO connections (waiting to be accepted).

AlexProgrammerDE commented 4 months ago

Yeah, that's why MCPL servers have that set. It's hardcoded as max 3 pending requests: https://github.com/GeyserMC/MCProtocolLib/blob/a724a8fdb4853291dfe357b36b5aee3926ab9abd/protocol/src/main/java/org/geysermc/mcprotocollib/network/tcp/TcpServer.java#L73

AlexProgrammerDE commented 4 months ago

Right now you can code both a MCPL server that supports TFO and a MCPL client that supports TFO. Like I do in my app for example.

xism4 commented 4 months ago

Right now you can code both a MCPL server that supports TFO and a MCPL client that supports TFO. Like I do in my app for example.

Right now you can code both a MCPL server that supports TFO and a MCPL client that supports TFO. Like I do in my app for example.

I see, but my question is that in BungeeCord, spigot and paper are not implemented. Only in forks.

AlexProgrammerDE commented 4 months ago

Yeah, that's known. Velocity isn't a fork btw. So what's the problem? It's opt-in at the developers discretion. Anyone who wants to use it in their app (like me) can opt into it. It lets me join Velocity servers faster.

xism4 commented 4 months ago

Right now you can code both a MCPL server that supports TFO and a MCPL client that supports TFO. Like I do in my app for example.

Right now you can code both a MCPL server that supports TFO and a MCPL client that supports TFO. Like I do in my app for example.

I see, but my question is that in BungeeCord, spigot and paper are not implemented. Only in forks.

I include that a large percentage of players go into Windows, which means that this feature no longer works. And it just opens the way for an attacker to use this to their advantage.

xism4 commented 4 months ago

Yeah, that's known. Velocity isn't a fork btw. So what's the problem? It's opt-in at the developers discretion. Anyone who wants to use it in their app (like me) can opt into it. It lets me join Velocity servers faster.

I was referring to the backends in this case, in Velocity it does not work directly.

AlexProgrammerDE commented 4 months ago

In MCPL it also does not work unless the developer enables it explicitly. Netty has a open issue for adding TFO support to NIO. Most servers run on linux + apps that run in docker containers like my app. If you think TFO has a security issue, you should report it to netty instead. It wouldn't be available in netty if it were a big security issue.

xism4 commented 4 months ago

In MCPL it also does not work unless the developer enables it explicitly. Netty has a open issue for adding TFO support to NIO. Most servers run on linux + apps that run in docker containers like my app. If you think TFO has a security issue, you should report it to netty instead. It wouldn't be available in netty if it were a big security issue.

This is a double-edged sword, as well as being a micro-improvement to get real connections in "faster", it can also be an advantage for other things.

xism4 commented 4 months ago

I understand the implementation now thank you