search

SpigotMC / BungeeCord

BungeeCord, the 6th in a generation of server portal suites. Efficiently proxies and maintains connections and transport between multiple Minecraft servers.
https://www.spigotmc.org/go/bungeecord
Other
1.53k stars 1.09k forks source link

don't close health check directly #3690

Closed Outfluencer closed 3 weeks ago

Outfluencer commented 1 month ago

Fixes #3689 Just check if the connection is a health check if so don't log exceptions

md-5 commented 3 weeks ago

So you're just leaving a connection dangling because it started with a health check header?

This does not seem right and would be a DoS vector

Outfluencer commented 3 weeks ago

It can only be a health check if proxy protocol is enabled and if so, you should firewall the port, so i see no ddos risk

Outfluencer commented 3 weeks ago

Also the behaviour is now the same as before, only exceptions are not printed for healthchecks

md-5 commented 3 weeks ago

Also the behaviour is now the same as before, only exceptions are not printed for healthchecks

Right, so surely now the bug https://github.com/SpigotMC/BungeeCord/commit/6e1751733f6b3dafe824dcd7f00d5ed86572ba37 was designed to fix is back?

andreasdc commented 3 weeks ago

It can only be a health check if proxy protocol is enabled and if so, you should firewall the port, so i see no ddos risk

Everyone can send the health check if I'm not wrong.

Outfluencer commented 3 weeks ago

What do you mean? the problem is that if you send custom data and try to receive a custom response via haproxy it does not work with my old pr, now it does

Outfluencer commented 3 weeks ago

The bug is not back but we got this as problem with my change #3689

Outfluencer commented 3 weeks ago

It can only be a health check if proxy protocol is enabled and if so, you should firewall the port, so i see no ddos risk

Everyone can send the health check if I'm not wrong.

BungeeCord only decodes bytes to a health check if the HaProxyDecoder is in the pipe, that is only the case if proxy protocol is enabled, and if so you should firewall the port so only the right HAProxy can access it

andreasdc commented 3 weeks ago

-

It can only be a health check if proxy protocol is enabled and if so, you should firewall the port, so i see no ddos risk

Everyone can send the health check if I'm not wrong.

BungeeCord only decodes bytes to a health check if the HaProxyDecoder is in the pipe, that is only the case if proxy protocol is enabled, and if so you should firewall the port so only the right HAProxy can access it

That's right, but I think everyone can use the proxy to send such packets, but I'm not sure.

Outfluencer commented 3 weeks ago

-

It can only be a health check if proxy protocol is enabled and if so, you should firewall the port, so i see no ddos risk

Everyone can send the health check if I'm not wrong.

BungeeCord only decodes bytes to a health check if the HaProxyDecoder is in the pipe, that is only the case if proxy protocol is enabled, and if so you should firewall the port so only the right HAProxy can access it

That's right, but I think everyone can use the proxy to send such packets, but I'm not sure.

?

md-5 commented 3 weeks ago

What do you mean? the problem is that if you send custom data and try to receive a custom response via haproxy it does not work with my old pr, now it does

This change reverts https://github.com/SpigotMC/BungeeCord/commit/6e1751733f6b3dafe824dcd7f00d5ed86572ba37 which fixed https://github.com/SpigotMC/BungeeCord/issues/3608

How is #3608 not reintroduced in cases where HAProxy doesn't try to send custom data?

Not printing an exception to the console after the connection is already closed will not affect HAProxy

Outfluencer commented 3 weeks ago

What do you mean? the problem is that if you send custom data and try to receive a custom response via haproxy it does not work with my old pr, now it does

This change reverts https://github.com/SpigotMC/BungeeCord/commit/6e1751733f6b3dafe824dcd7f00d5ed86572ba37 which fixed https://github.com/SpigotMC/BungeeCord/issues/3608

How is #3608 not reintroduced in cases where HAProxy doesn't try to send custom data?

Not printing an exception to the console after the connection is already closed will not affect HAProxy

Why will it not affect it? We just dont want to log the exception that is caused by the connection close of an health check. Isnt this the solution for it?

md-5 commented 3 weeks ago

I see, so #3608 is purely cosmetic

Outfluencer commented 3 weeks ago

I see, so #3608 is purely cosmetic

Yes

andreasdc commented 3 weeks ago

Also the behaviour is now the same as before, only exceptions are not printed for healthchecks

How it's the same if channel is not being closed with the latest change?

Outfluencer commented 3 weeks ago

Also the behaviour is now the same as before, only exceptions are not printed for healthchecks

How it's the same if channel is not being closed with the latest change?

Because haProxy closes the connection by itself by default. I only closed the channel so we dont receive rst from the haProxy that causes the exception. Now we receive the rst und the exception is thrown but we dont print it for healthchecks. In both cases there are no exceptions spammed anymore