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

Wrong firewalld rule generated when publishing ports on specified ip #881

Open
karuboniru opened this issue Jan 2, 2024 · 3 comments · May be fixed by #885
Open

Wrong firewalld rule generated when publishing ports on specified ip #881

karuboniru opened this issue Jan 2, 2024 · 3 comments · May be fixed by #885
Assignees

Comments

@karuboniru
Copy link

karuboniru commented Jan 2, 2024

When specifying -p 10.52.0.2:1111:1111 to podman, my intention was to forward traffic coming from 10.52.0.2:1111 to container:1111. While when using firewalld as backend, the generated rule is

port=1111:proto=tcp:toport=1111:toaddr=10.52.0.2

which will forward any traffic to port 1111 to 10.52.0.2:1111


I think in this case, we should use rich rule in netavark_portfwd zone like

rule destination address="10.52.0.2" forward-port port=1111 protocol=tcp to-port=1111 to-addr="container-ip"

instead of the forward-ports rule.


rpm -q netavark podman
netavark-1.9.0-1.fc39.x86_64
podman-4.8.2-1.fc39.x86_64

reproduce

# podman run -p 10.52.0.2:1111:1111 -it --rm --log-level debug alpine
....
[DEBUG netavark::firewall::firewalld] Port is ("1111", "tcp", "1111", "10.52.0.2")
....
# firewall-cmd --info-policy=netavark_portfwd
netavark_portfwd (active)
  priority: -1
  target: CONTINUE
  ingress-zones: ANY
  egress-zones: ANY
  services: 
  ports: 
  protocols: 
  masquerade: no
  forward-ports: 
	port=1111:proto=tcp:toport=1111:toaddr=10.52.0.2
  source-ports: 
  icmp-blocks: 
  rich rules: 
@Luap99
Copy link
Member

Luap99 commented Jan 3, 2024

Thanks for the report, yes this looks like a valid bug.
However please keep in mind that we do not recommend using the firewalld driver (at the moment), see #722 for more problems

@baude
Copy link
Member

baude commented Jan 3, 2024

@mheon can this be scoped up into your firewalld work ?

@mheon
Copy link
Member

mheon commented Jan 3, 2024

This is definitely part of the remaining work for firewalld

mheon added a commit to mheon/netavark that referenced this issue Jan 15, 2024
There are two major changes here.

Firstly, this adds proper support for port forwarding from
localhost via a new policy accepting traffic from HOST. This is
the last bit we were missing from the original port-forwarding
implementation.

Secondly, this fixes a bug where we generated incorrect rules
when port-forwarding from a single IP. Instead of doing standard
port-forwarding rules, those need rich rules. This was reported
as containers#881.

There are also some small code cleanups in how we handle setting
up and tearing down port forwarding. It's still rather ugly, but
at least a little better than it was before.

Fixes containers#881

Signed-off-by: Matthew Heon <[email protected]>
@mheon mheon linked a pull request Jan 15, 2024 that will close this issue
mheon added a commit to mheon/netavark that referenced this issue Jan 15, 2024
There are two major changes here.

Firstly, this adds proper support for port forwarding from
localhost via a new policy accepting traffic from HOST. This is
the last bit we were missing from the original port-forwarding
implementation.

Secondly, this fixes a bug where we generated incorrect rules
when port-forwarding from a single IP. Instead of doing standard
port-forwarding rules, those need rich rules. This was reported
as containers#881.

There are also some small code cleanups in how we handle setting
up and tearing down port forwarding. It's still rather ugly, but
at least a little better than it was before.

Fixes containers#881

Signed-off-by: Matthew Heon <[email protected]>
mheon added a commit to mheon/netavark that referenced this issue Jan 15, 2024
There are two major changes here.

Firstly, this adds proper support for port forwarding from
localhost via a new policy accepting traffic from HOST. This is
the last bit we were missing from the original port-forwarding
implementation.

Secondly, this fixes a bug where we generated incorrect rules
when port-forwarding from a single IP. Instead of doing standard
port-forwarding rules, those need rich rules. This was reported
as containers#881.

There are also some small code cleanups in how we handle setting
up and tearing down port forwarding. It's still rather ugly, but
at least a little better than it was before.

Fixes containers#881

Signed-off-by: Matthew Heon <[email protected]>
mheon added a commit to mheon/netavark that referenced this issue Jan 15, 2024
There are two major changes here.

Firstly, this adds proper support for port forwarding from
localhost via a new policy accepting traffic from HOST. This is
the last bit we were missing from the original port-forwarding
implementation.

Secondly, this fixes a bug where we generated incorrect rules
when port-forwarding from a single IP. Instead of doing standard
port-forwarding rules, those need rich rules. This was reported
as containers#881.

There are also some small code cleanups in how we handle setting
up and tearing down port forwarding. It's still rather ugly, but
at least a little better than it was before.

Fixes containers#881

Signed-off-by: Matthew Heon <[email protected]>
mheon added a commit to mheon/netavark that referenced this issue Feb 15, 2024
There are two major changes here.

Firstly, this adds proper support for port forwarding from
localhost via a new policy accepting traffic from HOST. This is
the last bit we were missing from the original port-forwarding
implementation.

Secondly, this fixes a bug where we generated incorrect rules
when port-forwarding from a single IP. Instead of doing standard
port-forwarding rules, those need rich rules. This was reported
as containers#881.

There are also some small code cleanups in how we handle setting
up and tearing down port forwarding. It's still rather ugly, but
at least a little better than it was before.

Fixes containers#881

Signed-off-by: Matthew Heon <[email protected]>
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 a pull request may close this issue.

4 participants