Fix(secgroups): ICMP Echo Request being blocked

[Kumm-Kai]

This fixes a thing that never worked before 😄

Currently ICMP echo requests are blocked by the ingress security group rules for ICMP, which looks like this: type=1:code=8 (ICMP type 1 is not used for anything in the ICMP protocol).

After this PR the ingress security group rules will look like this: type=8 (code seems to be omitted by openstack if it is 0) and allows ICMP echo requests to reach the VM.

Egress isn't a problem because the egress security group rule doesn't block anything.

[nightlyone]

Wasn't that setting intentional that way to mitigate icmp based attacks like ping floods? ( https://www.cloudflare.com/learning/ddos/ping-icmp-flood-ddos-attack/)

tcptraceroute to the open ports of the load balancer can still be useful.

I would at least make it optional, so icmp floods can be quickly mitigated.

Niclas Schad @.***> schrieb am Fr., 8. Dez. 2023, 16:44:

@.**** approved this pull request.

— Reply to this email directly, view it on GitHub https://github.com/stackitcloud/yawol/pull/269#pullrequestreview-1772668855, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANPNTXFHCZDKHRUJHAINJDYIMYXDAVCNFSM6AAAAABAMY35HKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONZSGY3DQOBVGU . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[Kumm-Kai]

Wasn't that setting intentional that way to mitigate icmp based attacks like ping floods? ( https://www.cloudflare.com/learning/ddos/ping-icmp-flood-ddos-attack/) tcptraceroute to the open ports of the load balancer can still be useful. I would at least make it optional, so icmp floods can be quickly mitigated.

I'm very certain that this wasn't intentional, as the old values were entirely wrong. Even the comment a few lines above indicates that: https://github.com/stackitcloud/yawol/blob/f949e16b0ea40a05d6439aae31ec11ea0bb56057/internal/helper/loadbalancer.go#L204

If you need to mitigate this, just add a security group that blocks ICMP.

[dergeberl]

I would also say it is not intentional but also see the point that it can be problematic.

Just create a secgroup rule is not that easy because they get reconciled. Maybe we should add it with an setting in lb.spec.options to disable it and enable it by default.

wdyt @Kumm-Kai