4:15 p.m.
On 04/08/2014 11:38 AM, Daniel P. Berrange wrote:
The network and nwfilter drivers both have a need to update
firewall rules. The currently share no code for interacting
with iptables / firewalld. The nwfilter driver is fairly
tied to the concept of creating shell scripts to execute
which makes it very hard to port to talk to firewalld via
DBus APIs.
This patch introduces a virFirewallPtr object which is able
to represent a complete sequence of rule changes, with the
ability to have multiple transactional checkpoints with
rollbacks. By formally separating the definition of the rules
to be applied from the mechanism used to apply them, it is
also possible to write a firewall engine that uses firewalld
DBus APIs natively instead of via the slow firewalld-cmd.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
[...]
+
# util/virhash.h
virHashAddEntry;
virHashCreate;
diff --git a/src/util/virerror.c b/src/util/virerror.c
index cbbaa83..e0bc970 100644
--- a/src/util/virerror.c
+++ b/src/util/virerror.c
@@ -129,6 +129,7 @@ VIR_ENUM_IMPL(virErrorDomain, VIR_ERR_DOMAIN_LAST,
"Systemd",
"Bhyve",
"Crypto",
+ "Firewall",
)
+static int
+virFirewallValidateBackend(virFirewallBackend backend)
+{
+ VIR_DEBUG("Validating backend %d", backend);
+#if WITH_DBUS
+ if (backend == VIR_FIREWALL_BACKEND_AUTOMATIC ||
+ backend == VIR_FIREWALL_BACKEND_FIREWALLD) {
+ int rv = virDBusIsServiceRegistered(VIR_FIREWALL_FIREWALLD_SERVICE);
Nit: empty line after var. decl. ?
+ VIR_DEBUG("Firewalled is registered ? %d", rv);
+ if (rv < 0) {
+ if (rv == -2) {
+ if (backend == VIR_FIREWALL_BACKEND_FIREWALLD) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("firewalld firewall backend requested, but
service is not running"));
+ return -1;
+ } else {
+ VIR_DEBUG("firewalld service not running, trying direct
backend");
+ backend = VIR_FIREWALL_BACKEND_DIRECT;
+ }
+ } else {
+ return -1;
+ }
+ } else {
+ VIR_DEBUG("firewalld service running, using firewalld backend");
+ backend = VIR_FIREWALL_BACKEND_FIREWALLD;
+ }
+ }
+#else
+ if (backend == VIR_FIREWALL_BACKEND_AUTOMATIC) {
+ VIR_DEBUG("DBus support disabled, trying direct backend");
+ backend = VIR_FIREWALL_BACKEND_DIRECT;
+ } else if (backend == VIR_FIREWALL_BACKEND_FIREWALLD) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("firewalld firewall backend requested, but DBus support
disabled"));
+ return -1;
+ }
+#endif
+
+ if (backend == VIR_FIREWALL_BACKEND_DIRECT) {
+ const char *commands[] = {
+ IPTABLES_PATH, IP6TABLES_PATH, EBTABLES_PATH
+ };
+ size_t i;
Also here?
+
+#define VIR_FIREWALL_RETURN_IF_ERROR(firewall) \
+ if (!firewall || firewall->err) \
+ return;
+
+#define VIR_FIREWALL_RULE_RETURN_IF_ERROR(firewall, ruel)\
ruel -> rule -- seems unused since it compiles
+
+int
+virFirewallApply(virFirewallPtr firewall)
+{
+ size_t i, j;
+ int ret = -1;
+
+ virMutexLock(&ruleLock);
+ if (virFirewallInitialize() < 0)
+ goto cleanup;
+
+ if (!firewall || firewall->err == ENOMEM) {
+ virReportOOMError();
+ goto cleanup;
+ }
+ if (firewall->err) {
+ virReportSystemError(firewall->err, "%s",
+ _("Unable to create rule"));
+ goto cleanup;
+ }
+
+ VIR_DEBUG("Applying groups for %p", firewall);
+ for (i = 0; i < firewall->ngroups; i++) {
+ if (virFirewallApplyGroup(firewall, i) < 0) {
+ VIR_DEBUG("Rolling back groups upto %zu for %p", i, firewall);
+ size_t first = i;
+ virErrorPtr saved_error = virSaveLastError();
+
+ /*
+ * Look at any inheritance markers to figure out
+ * what the first rollback group we need to apply is
+ */
+ for (j = 0; j <= i; j++) {
+ VIR_DEBUG("Checking inheritance of group %zu", i - j);
+ if (firewall->groups[i - j]->rollbackFlags &
+ VIR_FIREWALL_ROLLBACK_INHERIT_PREVIOUS)
+ first = (i - j) - 1;
if j = i then first = -1. This doesn't seem right. Limit the loop test
to j < i ?
+ }
+ /*
+ * Now apply all rollback groups in order
+ */
+ for (j = first; j <= i; j++) {
+ VIR_DEBUG("Rolling back group %zu", j);
+ virFirewallRollbackGroup(firewall, j);
+ }
+
+ virSetError(saved_error);
+ virFreeError(saved_error);
+ VIR_DEBUG("Done rolling back groups for %p", firewall);
+ goto cleanup;
+ }
+ }
+ VIR_DEBUG("Done applying groups for %p", firewall);
+
+ ret = 0;
+ cleanup:
+ virMutexUnlock(&ruleLock);
+ return ret;
+}
+
+static int
+testFirewallChainedRollback(const void *opaque ATTRIBUTE_UNUSED)
+{
+ virBuffer cmdbuf = VIR_BUFFER_INITIALIZER;
+ virFirewallPtr fw = NULL;
+ int ret = -1;
+ const char *actual = NULL;
+ const char *expected =
+ IPTABLES_PATH " -A INPUT --source-host 192.168.122.1 --jump ACCEPT\n"
+ IPTABLES_PATH " -A INPUT --source-host 192.168.122.127 --jump
REJECT\n"
+ IPTABLES_PATH " -A INPUT --source-host '!192.168.122.1' --jump
REJECT\n"
+ IPTABLES_PATH " -A INPUT --source-host 192.168.122.255 --jump
REJECT\n"
+ IPTABLES_PATH " -D INPUT --source-host 192.168.122.127 --jump
REJECT\n"
+ IPTABLES_PATH " -D INPUT --source-host '!192.168.122.1' --jump
REJECT\n"
+ IPTABLES_PATH " -D INPUT --source-host 192.168.122.255 --jump
REJECT\n"
+ IPTABLES_PATH " -D INPUT --source-host '!192.168.122.1' --jump
REJECT\n";
+ const struct testFirewallData *data = opaque;
+
+ fwDisabled = data->fwDisabled;
+ if (virFirewallSetBackend(data->tryBackend) < 0)
+ goto cleanup;
+
+ if (data->expectBackend == VIR_FIREWALL_BACKEND_DIRECT) {
+ virCommandSetDryRun(&cmdbuf, testFirewallRollbackHook, NULL);
+ } else {
+ fwBuf = &cmdbuf;
+ fwError = true;
+ }
+
+ fw = virFirewallNew();
+
+ virFirewallStartTransaction(fw, 0);
+
+ virFirewallAddRule(fw, VIR_FIREWALL_LAYER_IPV4,
+ "-A", "INPUT",
+ "--source-host", "192.168.122.1",
+ "--jump", "ACCEPT", NULL);
+
+ virFirewallStartRollback(fw, 0);
+
+ virFirewallAddRule(fw, VIR_FIREWALL_LAYER_IPV4,
+ "-D", "INPUT",
+ "--source-host", "192.168.122.1",
+ "--jump", "ACCEPT", NULL);
+
Hm, wondering why this rule doesn't make it into the expected output...
+
+ virFirewallStartTransaction(fw, 0);
+
+ virFirewallAddRule(fw, VIR_FIREWALL_LAYER_IPV4,
+ "-A", "INPUT",
+ "--source-host", "192.168.122.127",
+ "--jump", "REJECT", NULL);
+
+ virFirewallAddRule(fw, VIR_FIREWALL_LAYER_IPV4,
+ "-A", "INPUT",
+ "--source-host", "!192.168.122.1",
+ "--jump", "REJECT", NULL);
+
+ virFirewallStartRollback(fw, 0);
+
+ virFirewallAddRule(fw, VIR_FIREWALL_LAYER_IPV4,
+ "-D", "INPUT",
+ "--source-host", "192.168.122.127",
+ "--jump", "REJECT", NULL);
+
+ virFirewallAddRule(fw, VIR_FIREWALL_LAYER_IPV4,
+ "-D", "INPUT",
+ "--source-host", "!192.168.122.1",
+ "--jump", "REJECT", NULL);
+
+
+ virFirewallStartTransaction(fw, 0);
+
+ virFirewallAddRule(fw, VIR_FIREWALL_LAYER_IPV4,
+ "-A", "INPUT",
+ "--source-host", "192.168.122.255",
+ "--jump", "REJECT", NULL);
+
+ virFirewallAddRule(fw, VIR_FIREWALL_LAYER_IPV4,
+ "-A", "INPUT",
+ "--source-host", "!192.168.122.1",
+ "--jump", "REJECT", NULL);
+
+ virFirewallStartRollback(fw, VIR_FIREWALL_ROLLBACK_INHERIT_PREVIOUS);
+
+ virFirewallAddRule(fw, VIR_FIREWALL_LAYER_IPV4,
+ "-D", "INPUT",
+ "--source-host", "192.168.122.255",
+ "--jump", "REJECT", NULL);
+
+ virFirewallAddRule(fw, VIR_FIREWALL_LAYER_IPV4,
+ "-D", "INPUT",
+ "--source-host", "!192.168.122.1",
+ "--jump", "REJECT", NULL);
+
+ if (virFirewallApply(fw) == 0) {
+ fprintf(stderr, "Firewall apply unexpectedly worked\n");
+ goto cleanup;
+ }
+
+ if (virtTestOOMActive())
+ goto cleanup;
+
+ if (virBufferError(&cmdbuf))
+ goto cleanup;
+
+ actual = virBufferCurrentContent(&cmdbuf);
+
+ if (STRNEQ_NULLABLE(expected, actual)) {
+ fprintf(stderr, "Unexected command execution\n");
+ virtTestDifference(stderr, expected, actual);
+ goto cleanup;
+ }
+
+ ret = 0;
+ cleanup:
+ virBufferFreeAndReset(&cmdbuf);
+ fwBuf = NULL;
+ virCommandSetDryRun(NULL, NULL, NULL);
+ virFirewallFree(fw);
+ return ret;
+}
+
ACK with nits addressed.