CP-308800: Dynamic control of firewalld service - part 1 #6629
Conversation
BengangY
changed the title
BengangY
changed the title
BengangY
force-pushed
the
private/bengangy/CP-308800
branch
3 times, most recently
from
46f6446 to
6fbb10e
Compare
There was a problem hiding this comment.
I really like this idea, it's similar to what I had in mind to remake the port management feature for the SDN, which seems very easy to add with this PR. I think it can be made even better to reduce complexity in the users' side
BengangY
force-pushed
the
private/bengangy/CP-308800
branch
from
6fbb10e to
ad68294
Compare
ocaml/xapi/firewall.ml
Outdated
| | Xapi_insecure -> | ||
| ("80", "TCP") | ||
| | _ -> | ||
| failwith | ||
| "service_type_to_port_and_protocol: Unsupported service type for \ | ||
| iptables" |
There was a problem hiding this comment.
This can be moved to line 48, and the other service can be implemented, it shouldn't be difficult to look up the port and protocol.
Doesn't SSH auto mode need this to work?
There was a problem hiding this comment.
The other services will be implemented in later PRs.
For SSH, I'm considering if we should implement dynamic firewall control in xapi, as SSH service can be changed not only by xapi, but also by xapi-monitor-ssh, and systemd command directly.
Maybe we can just simply leave it opening.
Another option is to add the firewall-cmd to ExecStartPre and ExecStopPost of sshd systemd definition file.
What's your thought? @psafont
There was a problem hiding this comment.
For SSH, I'm considering if we should implement dynamic firewall control in xapi, as SSH service can be changed not only by xapi, but also by xapi-monitor-ssh, and systemd command directly.
xapi-monitor-ssh needs to work even if xapi is not working, but there are rpc functions exposed in xapi that need to do the same. I think it makes sense that xapi uses this interface to open and close the SSH port; and ideally xapi-monitor-ssh would be done in ocaml and use the same library, otherwise it will need to duplicate the functionality somehow
There was a problem hiding this comment.
xapi-monitor-ssh is written in Python.
There was a problem hiding this comment.
I think the easiest way to dynamic update sshd port is to add ExecStartPre and ExecStopPost to systemd definition file, as both xapi and ssh-monitor-ssh calls systemd to manage sshd. This can avoid the duplication.
There was a problem hiding this comment.
I don't know the exact requirements from xenserver, but as far as I'm aware:
- Switching of and on a single system instead of 2 means there's less chance of getting recovery and updates wrong. This means that I prefer a solution that blocks the port to the solution that blocks the ports and starts and stops the ssh daemon.
- Sharing a module of code to open a close ports between different daemons means that it only needs one set of sets instead.
- Modifying upstream service files and code increases maintenance costs, and as such I would avoid using ExecStartPre and ExecStopPost
So my preference would be to use this module both on xapi and xapi-monitor-ssh as the single mechanism to interrupt and enable access to SSH. But as I said previously, this is without knowing xenserver's feature requirements
There was a problem hiding this comment.
The requirement comes from engineers instead of customers. It's to avoid the difference between a port being firewalled and not having a listening service when doing the port scan.
xapi-monitor-ssh is written by Python. I don't think we should rewrite it with Ocaml to use the same firewall module (or do you mean call this module in the Python code?). If we have to implement it in both xapi and xapi-monitor-ssh, I think we can just implement it in the existing Python code.
|
Only the 2 last comments are important, the others are cosmetic |
BengangY
force-pushed
the
private/bengangy/CP-308800
branch
2 times, most recently
from
cb836e3 to
bed29a2
Compare
BengangY
force-pushed
the
private/bengangy/CP-308800
branch
from
bed29a2 to
4479248
Compare
ocaml/xapi/firewall.ml
Outdated
|
|
||
| let service_type_to_service_info = function | ||
| | Dlm -> | ||
| ("dlm", !Xapi_globs.xapi_clusterd_port, "TCP") |
There was a problem hiding this comment.
It's probably better to use a record here.
There was a problem hiding this comment.
maybe a variant as well for TCP and UDP?
Signed-off-by: Bengang Yuan <[email protected]>
Signed-off-by: Bengang Yuan <[email protected]>
BengangY
force-pushed
the
private/bengangy/CP-308800
branch
from
4479248 to
3183690
Compare
| ; ( "firewall-backend" | ||
| , Arg.Set_string firewall_backend | ||
| , (fun () -> !firewall_backend) | ||
| , "Firewall backend. iptables (in XS 8) or firewalld (in XS 9 or later XS \ |
There was a problem hiding this comment.
Ideally we would not accept arbitratry strings here and reject illegal strings already. But this is the easiest solution and we have to reject illegal backends later and report them as internal error - which is not entirely true.
| @@ -1317,6 +1321,12 @@ let ssh_monitor_service = ref "xapi-ssh-monitor" | |||
|
|
|||
| let ssh_auto_mode_default = ref true | |||
|
|
|||
| (* Firewall backend to use. iptables in XS 8, firewalld in XS 9. *) | |||
| let firewall_backend = ref "firewalld" | |||
There was a problem hiding this comment.
I would prefer if this was iptables by default, with the configuration file for XS9 overriding the value to firewalld for the time being. And changing the default once XS9 is forked.
| let firewall_backend = ref "firewalld" | |
| let firewall_backend = ref "iptables" |
Implement dynamic control of firewalld service.
The first PR defines dynamic firewall control function, variable, and the http service's dynamic control.
The other services's dynamic control will be in the later PR.