Explicitly drop non-echo ICMP in SNAT action #734
Conversation
Our SNAT layer has limited support for ICMP traffic, and is limited to using the identifier of Echo Request/Reply traffic as a form of pseudo-port. For other ICMP traffic, there is no comparable field and the packet must be dropped. This PR changes how this is requested, from a `GenDescError` (ordinarily used for programming errors or resource exhaustion) into a standard drop. This is more appropriate given that guests are free to (attempt to) send these other ICMP variants at their leisure, including in response to an ibound frame on a recently closed port. Closes #727.
FelixMcFelix
changed the title
|
As a high level comment regarding #727. Can we make it so that OPTE logs do not land on the root console at all? |
| let icmp6 = | ||
| meta.inner_icmp6().ok_or(GenIcmpErr::MetaNotFound)?; | ||
|
|
||
| Ok(if icmp6.ty() == u8::from(Icmpv6Message::EchoRequest) { |
There was a problem hiding this comment.
Are we not handling echo reply here because this is for initializing a few flow only?
There was a problem hiding this comment.
In general for SNAT, Echo Reply messages should all be handled by the LFT entry created as part of a stateful allow here.
The other thing is that this code is only used in the outbound direction. The only time a guest should send an echo reply would be unsolicited (?), or if the other side quickly follows up with its own echo request on the same identifier (which is, again, handled by the pair of LFT entries created by the first outbound packet).
|
Thanks for the review, Ry.
We can, I believe that downgrading the error level for
I don't really see offhand that it would, but I might be wrong -- #730 would only really factor in if we wanted to set up an explicit SNAT-incompatible-ICMP rule for the purpose of tracking dedicated stats. #369 we're most likely to solve using a body transform on the NAT (EIP/FIP) action, since the affected packet types are incompatible with how we perform SNAT. |
That sounds like something we should do. As this can completely destroy one's ability to manage a sled from the root login. |
Our SNAT layer has limited support for ICMP traffic, and is limited to using the identifier of Echo Request/Reply traffic as a form of pseudo-port. For other ICMP traffic, there is no comparable field and the packet must be dropped.
This PR changes how this is requested, from a
GenDescError(ordinarily used for programming errors or resource exhaustion) into a standard drop. This is more appropriate given that guests are free to (attempt to) send these other ICMP variants at their leisure, including in response to an ibound frame on a recently closed port.Closes #727.