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

Add support for Proxy Protocol #7710

Merged
merged 5 commits into from
May 24, 2022
Merged

Add support for Proxy Protocol #7710

merged 5 commits into from
May 24, 2022

Conversation

PanSzelescik
Copy link
Contributor

This PR adds support for Proxy Protocol. With this option you can use for example TCPShield or other proxy with enabled Proxy Protocol without Bungeecord or Velocity.

I think this implementation requires also better warning if Proxy Protocol is enabled, because without any proxy you cannot enter or ping the server.

@PanSzelescik PanSzelescik requested a review from a team as a code owner April 7, 2022 14:22
@Netherwhal
Copy link

Netherwhal commented Apr 7, 2022

What does this change mean? Will this expose the "realip" in the logs (which I think would be quite useful) or will this switch the connection internally between server <> client to be direct? Would that not bypass the protection and allow clients to get the real-IP of the server?

@electronicboy
Copy link
Member

it's haproxy, this PR would just add support for understanding the proxy header which provides the info of the connection source, etc

@Netherwhal
Copy link

I meant this part:

connection.address = socketaddr;

but thats just because I don't know what happens to that object later on :) - was just making sure this will not bypass the connection via proxy, but just expose additional info via the proxy protocol...

@PanSzelescik
Copy link
Contributor Author

Yes, it decodes proxy header, which includes the player's IP. And line:

connection.address = socketaddr;

sets a variable to correct player's IP address. Without it, it will be visible in the logs that the player entered from the proxy IP (for example from one of TCPShield servers or localhost). That's what Proxy Protocol (HAProxy) is for.

@tomcraft
Copy link

tomcraft commented Apr 8, 2022

As a comparision, that's also what happens when running with BungeeCord or Velocity support (also proxies), so don't worry "nothing new" that's just the way HAProxy gives this information.

@PanSzelescik
Copy link
Contributor Author

PanSzelescik commented Apr 9, 2022

And what about query? Add support there too? And whether to do a separate config for it? But this is DatagramSocket (not Netty), so the implementation would be more difficult

@lhridder
Copy link

It might be useful to have a config option to whitelist subnets that are allowed to connect when proxyprotocol is enabled with something like:

proxy-protocol-whitelist: ["1.2.3.4/32", "1.1.1.1/24"]

on line 44 of the patch (would still require cidr matching)

if (com.destroystokyo.paper.PaperConfig.proxyProtocolWhitelist.contains(channel.remoteAddress().getAddress()) {

As these kinds of single server setups behind for example tcpshield are mostly done on shared hosting the end user probably won't be able to add firewall rules themselves and leaving proxy protocol wide open allows for more direct attacks and general ip spoofing.

@electronicboy
Copy link
Member

such a feature being limited to just the proxy protocol stuff would be such a pointlessly limited config vs a wider thing, but, afaik, there's already firewall emulating plugins for resolving that specific case

I'll probably be looking into merging this as soon as it's rebased or sometime this week, currently dead, however

@lhridder
Copy link

Why would you need a wider thing? For multi server networks Bungeeguard or the velocity key system is probably already used to validate between servers or just running on private networking.

@electronicboy
Copy link
Member

wouldn't need it, just existing plugin based solutions here already exist, if something was added to Paper for this, I'd much prefer it cover a wider surface vs just this one thing as it seems pointless to have a patch which could cover something wider only cover one thing

@lhridder
Copy link

Ah alright, yeah a global firewall would work too but it probably won't be used that often.

@electronicboy electronicboy merged commit 31ccc57 into PaperMC:master May 24, 2022
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.

None yet

5 participants