On 02/11/2013 09:54 AM, Natanael Copa wrote:

Support setting which public ip to use for NAT via attribute address in subelement <nat> in <forward>:

... <forward mode='nat'> <address start='1.2.3.4' end='1.2.3.10'/> </forward> ...

This will construct an iptables line using:

'-j SNAT --to-source <start>-<end>'

instead of:

'-j MASQUERADE'

Signed-off-by: Natanael Copa <ncopa@alpinelinux.org> --- docs/formatnetwork.html.in | 18 ++++++ src/conf/network_conf.c | 152 ++++++++++++++++++++++++++++++++++++++++++-- src/conf/network_conf.h | 3 + src/network/bridge_driver.c | 16 +++++ src/util/viriptables.c | 56 +++++++++++++--- src/util/viriptables.h | 4 ++ 6 files changed, 235 insertions(+), 14 deletions(-)

diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 7b42529..5fbd0a9 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -136,6 +136,24 @@ network, and to/from the host to the guests, are unrestricted and not NATed.<span class="since">Since 0.4.2</span> + + <p><span class="since">Since 1.0.3</span> it is possible to + specify a public IPv4 address range to be used for the NAT by + using the <code><nat></code> and + <code><address></code> subelements. + <pre> +... + <forward mode='nat'> + <nat> + <address start='1.2.3.4' end='1.2.3.10'/> + </nat> + </forward> +... + </pre> + An singe IPv4 address can be set by setting + <code>start</code> and <code>end</code> attributes to + the same value. + </p> </dd>

<dt><code>route</code></dt> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 3604ff7..61d086a 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1325,6 +1325,80 @@ cleanup: }

static int +virNetworkForwardNatDefParseXML(const char *networkName, + xmlNodePtr node, + xmlXPathContextPtr ctxt, + virNetworkForwardDefPtr def) +{ + int ret = -1; + xmlNodePtr *natAddrNodes = NULL; + int nNatAddrs; + char *addr_start = NULL; + char *addr_end = NULL;

Although you'll find both underscore_separated variable names and studlyCapped variable names in libvirt, there seems to be more of a preference for the latter. I'm changing all addr_start to addrStart, and all addr_end to addrEnd.

+ xmlNodePtr save = ctxt->node; + + ctxt->node = node; + + if (def->type != VIR_NETWORK_FORWARD_NAT) { + virReportError(VIR_ERR_XML_ERROR, + _("The <nat> element can only be used when <forward> 'mode' is 'nat' in network %s"), + networkName); + goto cleanup; + } + + /* addresses for SNAT */ + nNatAddrs = virXPathNodeSet("./address", ctxt, &natAddrNodes); + if (nNatAddrs < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid <address> element found in <forward> of " + "network %s"), networkName); + goto cleanup; + } else if (nNatAddrs > 1) { + virReportError(VIR_ERR_XML_ERROR, + _("Only one <address> element is allowed in <nat> in " + "<forward> in network %s"), networkName); + goto cleanup; + } else if (nNatAddrs == 1) { + addr_start = virXMLPropString(*natAddrNodes, "start"); + if (addr_start == NULL) { + virReportError(VIR_ERR_XML_ERROR, + _("missing 'start' attribute in <address> element in <nat> in " + "<forward> in network %s"), networkName); + goto cleanup; + } + addr_end = virXMLPropString(*natAddrNodes, "end"); + if (addr_end == NULL) { + virReportError(VIR_ERR_XML_ERROR, + _("missing 'end' attribute in <address> element in <nat> in " + "<forward> in network %s"), networkName); + goto cleanup; + } + } + + if (addr_start && virSocketAddrParse(&def->addr_start, addr_start, AF_INET) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Bad ipv4 start address '%s' in <nat> in <forward> in " + "network '%s'"), addr_start, networkName); + goto cleanup; + } + + if (addr_end && virSocketAddrParse(&def->addr_end, addr_end, AF_INET) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Bad ipv4 end address '%s' in <nat> in <forward> in " + "network '%s'"), addr_end, networkName); + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FREE(addr_start); + VIR_FREE(addr_end);

You forgot to free natAddrNodes.

+ ctxt->node = save; + return ret; +} + +static int virNetworkForwardDefParseXML(const char *networkName, xmlNodePtr node, xmlXPathContextPtr ctxt, @@ -1334,7 +1408,8 @@ virNetworkForwardDefParseXML(const char *networkName, xmlNodePtr *forwardIfNodes = NULL; xmlNodePtr *forwardPfNodes = NULL; xmlNodePtr *forwardAddrNodes = NULL; - int nForwardIfs, nForwardAddrs, nForwardPfs; + xmlNodePtr *forwardNatNodes = NULL; + int nForwardIfs, nForwardAddrs, nForwardPfs, nForwardNats; char *forwardDev = NULL; char *forwardManaged = NULL; char *type = NULL; @@ -1384,6 +1459,24 @@ virNetworkForwardDefParseXML(const char *networkName, goto cleanup; }

+ nForwardNats = virXPathNodeSet("./nat", ctxt, &forwardNatNodes); + if (nForwardNats < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid <nat> element found in <forward> of network %s"), + networkName); + goto cleanup; + } else if (nForwardNats > 1) { + virReportError(VIR_ERR_XML_ERROR, + _("Only one <nat> element is allowed in <forward> of network %s"), + networkName); + goto cleanup; + } else if (nForwardNats == 1) { + if (virNetworkForwardNatDefParseXML(networkName, + *forwardNatNodes, + ctxt, def) < 0) + goto cleanup;

I think the official rules for libvirt may only say that braces are necessary if the body of a compound statement is > 1 line, but I prefer to also add braces when the conditional is > 1 line.

+ } + if (((nForwardIfs > 0) + (nForwardAddrs > 0) + (nForwardPfs > 0)) > 1) { virReportError(VIR_ERR_XML_ERROR, _("<address>, <interface>, and <pf> elements in <forward> " @@ -1525,6 +1618,7 @@ cleanup: VIR_FREE(forwardPfNodes); VIR_FREE(forwardIfNodes); VIR_FREE(forwardAddrNodes); + VIR_FREE(forwardNatNodes); ctxt->node = save; return ret; } @@ -2079,13 +2173,54 @@ virPortGroupDefFormat(virBufferPtr buf, }

static int +virNatDefFormat(virBufferPtr buf, + const virNetworkForwardDefPtr fwd)

This should be named consistent with the parse function - virNetworkForwardNatDefFormat().

+{ + char *addr_start = NULL; + char *addr_end = NULL; + int ret = -1; + + if (VIR_SOCKET_ADDR_VALID(&fwd->addr_start)) { + addr_start = virSocketAddrFormat(&fwd->addr_start); + if (!addr_start) + goto cleanup; + } + + if (VIR_SOCKET_ADDR_VALID(&fwd->addr_end)) { + addr_end = virSocketAddrFormat(&fwd->addr_end); + if (!addr_end) + goto cleanup; + } + + if (!addr_end && !addr_start) + return 0; + + virBufferAddLit(buf, "<nat>\n"); + virBufferAdjustIndent(buf, 2); + + virBufferAsprintf(buf, "<address start='%s'", addr_start); + if (addr_end) + virBufferAsprintf(buf, " end='%s'", addr_end); + virBufferAsprintf(buf, "/>\n"); + + virBufferAdjustIndent(buf, -2); + virBufferAsprintf(buf, "</nat>\n"); + ret = 0; + +cleanup: + VIR_FREE(addr_start); + VIR_FREE(addr_end); + return ret; +} + +static int virNetworkDefFormatInternal(virBufferPtr buf, const virNetworkDefPtr def, unsigned int flags) { unsigned char *uuid; char uuidstr[VIR_UUID_STRING_BUFLEN]; - int ii; + int ii, shortforward;

virBufferAddLit(buf, "<network"); if (!(flags & VIR_NETWORK_XML_INACTIVE) && (def->connections > 0)) { @@ -2122,10 +2257,17 @@ virNetworkDefFormatInternal(virBufferPtr buf, else virBufferAddLit(buf, " managed='no'"); } - virBufferAsprintf(buf, "%s>\n", - (def->forward.nifs || def->forward.npfs) ? "" : "/"); + shortforward = !(def->forward.nifs || def->forward.npfs + || VIR_SOCKET_ADDR_VALID(&def->forward.addr_start) + || VIR_SOCKET_ADDR_VALID(&def->forward.addr_end));

Yes, nice refactor to eliminate the duplicated conditional below.

+ virBufferAsprintf(buf, "%s>\n", shortforward ? "/" : ""); virBufferAdjustIndent(buf, 2);

+ if (def->forward.type == VIR_NETWORK_FORWARD_NAT) { + if (virNatDefFormat(buf, &def->forward) < 0) + goto error; + } + /* For now, hard-coded to at most 1 forward.pfs */ if (def->forward.npfs) virBufferEscapeString(buf, "<pf dev='%s'/>\n", @@ -2155,7 +2297,7 @@ virNetworkDefFormatInternal(virBufferPtr buf, } } virBufferAdjustIndent(buf, -2); - if (def->forward.npfs || def->forward.nifs) + if (!shortforward) virBufferAddLit(buf, "</forward>\n"); }

diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 4c634ed..1a598e3 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -174,6 +174,9 @@ struct _virNetworkForwardDef {

size_t nifs; virNetworkForwardIfDefPtr ifs; + + /* adresses for SNAT */ + virSocketAddr addr_start, addr_end; };

typedef struct _virPortGroupDef virPortGroupDef; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index c834f83..6d74c1f 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1587,6 +1587,8 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver, &ipdef->address, prefix, forwardIf, + &network->def->forward.addr_start, + &network->def->forward.addr_end, NULL) < 0) { virReportError(VIR_ERR_SYSTEM_ERROR, forwardIf ? @@ -1601,6 +1603,8 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver, &ipdef->address, prefix, forwardIf, + &network->def->forward.addr_start, + &network->def->forward.addr_end, "udp") < 0) { virReportError(VIR_ERR_SYSTEM_ERROR, forwardIf ? @@ -1615,6 +1619,8 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver, &ipdef->address, prefix, forwardIf, + &network->def->forward.addr_start, + &network->def->forward.addr_end, "tcp") < 0) { virReportError(VIR_ERR_SYSTEM_ERROR, forwardIf ? @@ -1631,12 +1637,16 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver, &ipdef->address, prefix, forwardIf, + &network->def->forward.addr_start, + &network->def->forward.addr_end, "udp"); masqerr4: iptablesRemoveForwardMasquerade(driver->iptables, &ipdef->address, prefix, forwardIf, + &network->def->forward.addr_start, + &network->def->forward.addr_end, NULL); masqerr3: iptablesRemoveForwardAllowRelatedIn(driver->iptables, @@ -1667,16 +1677,22 @@ networkRemoveMasqueradingIptablesRules(struct network_driver *driver, &ipdef->address, prefix, forwardIf, + &network->def->forward.addr_start, + &network->def->forward.addr_end, "tcp"); iptablesRemoveForwardMasquerade(driver->iptables, &ipdef->address, prefix, forwardIf, + &network->def->forward.addr_start, + &network->def->forward.addr_end, "udp"); iptablesRemoveForwardMasquerade(driver->iptables, &ipdef->address, prefix, forwardIf, + &network->def->forward.addr_start, + &network->def->forward.addr_end, NULL);

iptablesRemoveForwardAllowRelatedIn(driver->iptables, diff --git a/src/util/viriptables.c b/src/util/viriptables.c index 41fe780..3f0dcf0 100644 --- a/src/util/viriptables.c +++ b/src/util/viriptables.c @@ -805,11 +805,15 @@ iptablesForwardMasquerade(iptablesContext *ctx, virSocketAddr *netaddr, unsigned int prefix, const char *physdev, + virSocketAddr *addr_start, + virSocketAddr *addr_end, const char *protocol, int action) { - int ret; - char *networkstr; + int ret = -1; + char *networkstr = NULL; + char *addr_start_str = NULL; + char *addr_end_str = NULL; virCommandPtr cmd = NULL;

if (!(networkstr = iptablesFormatNetwork(netaddr, prefix))) @@ -820,8 +824,18 @@ iptablesForwardMasquerade(iptablesContext *ctx, virReportError(VIR_ERR_INTERNAL_ERROR, _("Attempted to NAT '%s'. NAT is only supported for IPv4."), networkstr); - VIR_FREE(networkstr); - return -1; + goto cleanup; + } + + if (VIR_SOCKET_ADDR_IS_FAMILY(addr_start, AF_INET)) { + addr_start_str = virSocketAddrFormat(addr_start); + if (!addr_start_str) + goto cleanup; + if (VIR_SOCKET_ADDR_IS_FAMILY(addr_end, AF_INET)) { + addr_end_str = virSocketAddrFormat(addr_end); + if (!addr_end_str) + goto cleanup;

The indentation went wrong here.

+ } }

cmd = iptablesCommandNew(ctx->nat_postrouting, AF_INET, action); @@ -835,12 +849,32 @@ iptablesForwardMasquerade(iptablesContext *ctx, if (physdev && physdev[0]) virCommandAddArgList(cmd, "--out-interface", physdev, NULL);

- virCommandAddArgList(cmd, "--jump", "MASQUERADE", NULL); + /* Use --jump SNAT if public addr is specified */ + if (addr_start_str && addr_start_str[0]) { + char tmpstr[sizeof("123.123.123.123-123.123.123.123:65535-65535")];

Rather than doing this and calling snprintf, we prefer to call virAsprintf(), which allocates the memory as required and doesn't consume space on the stack (of course then you have to free the memory afterwards). I changed tmpstr into addrRangeStr, declared at the toplevel of the function, changed the snprintf's to virAsprintfs() (with proper checking for error return), and freed addrRangeStr during the function's cleanup.

+ const char *portstr = ""; + + memset(tmpstr, 0, sizeof(tmpstr)); + if (protocol && protocol[0]) + portstr = ":1024-65535"; + if (addr_end_str && addr_end_str[0]) { + snprintf(tmpstr, sizeof(tmpstr), "%s-%s%s", + addr_start_str, addr_end_str, portstr); + } else { + snprintf(tmpstr, sizeof(tmpstr), "%s%s", addr_start_str, portstr); + }

- if (protocol && protocol[0]) - virCommandAddArgList(cmd, "--to-ports", "1024-65535", NULL); + virCommandAddArgList(cmd, "--jump", "SNAT", + "--to-source", tmpstr, NULL); + } else { + virCommandAddArgList(cmd, "--jump", "MASQUERADE", NULL); + + if (protocol && protocol[0]) + virCommandAddArgList(cmd, "--to-ports", "1024-65535", NULL); + }

ret = iptablesCommandRunAndFree(cmd); +cleanup: VIR_FREE(networkstr);

You didn't free addr_start_str or addr_end_str.

return ret; } @@ -863,9 +897,11 @@ iptablesAddForwardMasquerade(iptablesContext *ctx, virSocketAddr *netaddr, unsigned int prefix, const char *physdev, + virSocketAddr *addr_start, + virSocketAddr *addr_end, const char *protocol) { - return iptablesForwardMasquerade(ctx, netaddr, prefix, physdev, protocol, ADD); + return iptablesForwardMasquerade(ctx, netaddr, prefix, physdev, addr_start, addr_end, protocol, ADD); }

/** @@ -886,9 +922,11 @@ iptablesRemoveForwardMasquerade(iptablesContext *ctx, virSocketAddr *netaddr, unsigned int prefix, const char *physdev, + virSocketAddr *addr_start, + virSocketAddr *addr_end, const char *protocol) { - return iptablesForwardMasquerade(ctx, netaddr, prefix, physdev, protocol, REMOVE); + return iptablesForwardMasquerade(ctx, netaddr, prefix, physdev, addr_start, addr_end, protocol, REMOVE); }

diff --git a/src/util/viriptables.h b/src/util/viriptables.h index d7fa731..4241380 100644 --- a/src/util/viriptables.h +++ b/src/util/viriptables.h @@ -107,11 +107,15 @@ int iptablesAddForwardMasquerade (iptablesContext *ctx, virSocketAddr *netaddr, unsigned int prefix, const char *physdev, + virSocketAddr *addr_start, + virSocketAddr *addr_end, const char *protocol); int iptablesRemoveForwardMasquerade (iptablesContext *ctx, virSocketAddr *netaddr, unsigned int prefix, const char *physdev, + virSocketAddr *addr_start, + virSocketAddr *addr_end, const char *protocol); int iptablesAddOutputFixUdpChecksum (iptablesContext *ctx, const char *iface,

If I'd seen the need to change snprintf() to virAsprintf() when I started, I might have just marked up this diff and asked for a V3, but by the time I got there, I'd already done the other trivial changes I listed above, so I made the virAsprintf() change as well. Can someone check the interdiff I'm attaching to this message. If that gets an ACK, then I'll ACK the combination of that with the original (because the interdiff addresses everything I mention in my review) and push.

On 02/11/2013 09:54 AM, Natanael Copa wrote:

Let users set the port range to be used for forward mode NAT:

... <forward mode='nat'> <nat> <port start='1024' end='65535'/> </nat> </forward> ...

Signed-off-by: Natanael Copa <ncopa@alpinelinux.org> --- docs/formatnetwork.html.in | 21 ++++++++++++++--- src/conf/network_conf.c | 57 +++++++++++++++++++++++++++++++++++++++------ src/conf/network_conf.h | 3 ++- src/network/bridge_driver.c | 16 +++++++++++++ src/util/viriptables.c | 39 ++++++++++++++++++++++++------- src/util/viriptables.h | 4 ++++ 6 files changed, 120 insertions(+), 20 deletions(-)

diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 5fbd0a9..adb5bb9 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -138,9 +138,11 @@ 0.4.2</span>

<p><span class="since">Since 1.0.3</span> it is possible to - specify a public IPv4 address range to be used for the NAT by - using the <code><nat></code> and - <code><address></code> subelements. + specify a public IPv4 address and port range to be used for + the NAT by using the <code><nat></code> subelement. + The address range is set with the <code><address></code> + subelements and <code>start</code> and <code>stop</code> + attributes: <pre> ... <forward mode='nat'> @@ -154,6 +156,19 @@ <code>start</code> and <code>end</code> attributes to the same value. </p> + <p> + The port range to be used for the <code><nat></code> can + be set via the subelement <code><port></code>: + <pre> +... + <forward mode='nat'> + <nat> + <port start='500' end='1000'/> + </nat> + </forward> +... + </pre> + </p> </dd>

<dt><code>route</code></dt> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 61d086a..5725800 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1332,7 +1332,8 @@ virNetworkForwardNatDefParseXML(const char *networkName, { int ret = -1; xmlNodePtr *natAddrNodes = NULL; - int nNatAddrs; + xmlNodePtr *natPortNodes = NULL; + int nNatAddrs, nNatPorts; char *addr_start = NULL; char *addr_end = NULL; xmlNodePtr save = ctxt->node; @@ -1389,6 +1390,36 @@ virNetworkForwardNatDefParseXML(const char *networkName, goto cleanup; }

+ /* ports for SNAT and MASQUERADE */ + nNatPorts = virXPathNodeSet("./port", ctxt, &natPortNodes); + if (nNatPorts < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid <port> element found in <forward> of " + "network %s"), networkName); + goto cleanup; + } else if (nNatPorts > 1) { + virReportError(VIR_ERR_XML_ERROR, + _("Only one <port> element is allowed in <nat> in " + "<forward> in network %s"), networkName); + goto cleanup; + } else if (nNatPorts == 1) { + if (virXPathUInt("string(./port[1]/@start)", ctxt, &def->port_start) < 0 + || def->port_start > 65535) { + + virReportError(VIR_ERR_XML_DETAIL, + _("Missing or invalid 'start' attribute in <port> " + "in <nat> in <forward> in network %s"), + networkName); + goto cleanup; + } + if (virXPathUInt("string(./port[1]/@end)", ctxt, &def->port_end) < 0 + || def->port_end > 65535 || def->port_end < def->port_start) { + virReportError(VIR_ERR_XML_DETAIL, + _("Missing or invalid 'end' attribute in <port> in " + "<nat> in <forward> in network %s"), networkName); + goto cleanup; + } + } ret = 0;

cleanup: @@ -2179,6 +2210,7 @@ virNatDefFormat(virBufferPtr buf, char *addr_start = NULL; char *addr_end = NULL; int ret = -1; + int longdef;

"longdef" must be leftover from an earlier version of your patch, as it's now unused.

if (VIR_SOCKET_ADDR_VALID(&fwd->addr_start)) { addr_start = virSocketAddrFormat(&fwd->addr_start); @@ -2192,16 +2224,25 @@ virNatDefFormat(virBufferPtr buf, goto cleanup; }

- if (!addr_end && !addr_start) + if (!addr_start && !addr_end && !fwd->port_start && !fwd->port_end) return 0;

virBufferAddLit(buf, "<nat>\n"); virBufferAdjustIndent(buf, 2);

- virBufferAsprintf(buf, "<address start='%s'", addr_start); - if (addr_end) - virBufferAsprintf(buf, " end='%s'", addr_end); - virBufferAsprintf(buf, "/>\n"); + if (addr_start) { + virBufferAsprintf(buf, "<address start='%s'", addr_start); + if (addr_end) + virBufferAsprintf(buf, " end='%s'", addr_end); + virBufferAsprintf(buf, "/>\n"); + } + + if (fwd->port_start || fwd->port_end) { + virBufferAsprintf(buf, "<port start='%d'", fwd->port_start); + if (fwd->port_end) + virBufferAsprintf(buf, " end='%d'", fwd->port_end); + virBufferAsprintf(buf, "/>\n"); + }

virBufferAdjustIndent(buf, -2); virBufferAsprintf(buf, "</nat>\n"); @@ -2259,7 +2300,9 @@ virNetworkDefFormatInternal(virBufferPtr buf, } shortforward = !(def->forward.nifs || def->forward.npfs || VIR_SOCKET_ADDR_VALID(&def->forward.addr_start) - || VIR_SOCKET_ADDR_VALID(&def->forward.addr_end)); + || VIR_SOCKET_ADDR_VALID(&def->forward.addr_end) + || def->forward.port_start + || def->forward.port_end); virBufferAsprintf(buf, "%s>\n", shortforward ? "/" : ""); virBufferAdjustIndent(buf, 2);

diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 1a598e3..7df2426 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -175,8 +175,9 @@ struct _virNetworkForwardDef { size_t nifs; virNetworkForwardIfDefPtr ifs;

- /* adresses for SNAT */ + /* ranges for NAT */ virSocketAddr addr_start, addr_end; + unsigned int port_start, port_end;

As with addr_*, since there is a preference for using Caps instead of _ between words in variable names, I'm going to head this one off at the beginning and change all the port_starts to portStart and portEnds to port_end.

};

typedef struct _virPortGroupDef virPortGroupDef; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 6d74c1f..5c83085 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1589,6 +1589,8 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver, forwardIf, &network->def->forward.addr_start, &network->def->forward.addr_end, + network->def->forward.port_start, + network->def->forward.port_end, NULL) < 0) { virReportError(VIR_ERR_SYSTEM_ERROR, forwardIf ? @@ -1605,6 +1607,8 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver, forwardIf, &network->def->forward.addr_start, &network->def->forward.addr_end, + network->def->forward.port_start, + network->def->forward.port_end, "udp") < 0) { virReportError(VIR_ERR_SYSTEM_ERROR, forwardIf ? @@ -1621,6 +1625,8 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver, forwardIf, &network->def->forward.addr_start, &network->def->forward.addr_end, + network->def->forward.port_start, + network->def->forward.port_end, "tcp") < 0) { virReportError(VIR_ERR_SYSTEM_ERROR, forwardIf ? @@ -1639,6 +1645,8 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver, forwardIf, &network->def->forward.addr_start, &network->def->forward.addr_end, + network->def->forward.port_start, + network->def->forward.port_end, "udp"); masqerr4: iptablesRemoveForwardMasquerade(driver->iptables, @@ -1647,6 +1655,8 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver, forwardIf, &network->def->forward.addr_start, &network->def->forward.addr_end, + network->def->forward.port_start, + network->def->forward.port_end, NULL); masqerr3: iptablesRemoveForwardAllowRelatedIn(driver->iptables, @@ -1679,6 +1689,8 @@ networkRemoveMasqueradingIptablesRules(struct network_driver *driver, forwardIf, &network->def->forward.addr_start, &network->def->forward.addr_end, + network->def->forward.port_start, + network->def->forward.port_end, "tcp"); iptablesRemoveForwardMasquerade(driver->iptables, &ipdef->address, @@ -1686,6 +1698,8 @@ networkRemoveMasqueradingIptablesRules(struct network_driver *driver, forwardIf, &network->def->forward.addr_start, &network->def->forward.addr_end, + network->def->forward.port_start, + network->def->forward.port_end,

There's so many of these, I'm starting to wonder if it's worth having a virPortAddrRange data type (but not wondering enough to actually do it myself :-)

"udp"); iptablesRemoveForwardMasquerade(driver->iptables, &ipdef->address, @@ -1693,6 +1707,8 @@ networkRemoveMasqueradingIptablesRules(struct network_driver *driver, forwardIf, &network->def->forward.addr_start, &network->def->forward.addr_end, + network->def->forward.port_start, + network->def->forward.port_end, NULL);

iptablesRemoveForwardAllowRelatedIn(driver->iptables, diff --git a/src/util/viriptables.c b/src/util/viriptables.c index 3f0dcf0..aa48520 100644 --- a/src/util/viriptables.c +++ b/src/util/viriptables.c @@ -807,6 +807,8 @@ iptablesForwardMasquerade(iptablesContext *ctx, const char *physdev, virSocketAddr *addr_start, virSocketAddr *addr_end, + unsigned int port_start, + unsigned int port_end, const char *protocol, int action) { @@ -815,6 +817,7 @@ iptablesForwardMasquerade(iptablesContext *ctx, char *addr_start_str = NULL; char *addr_end_str = NULL; virCommandPtr cmd = NULL; + char port_str[sizeof(":65535-65535")] = "";

Again, port_str (which I'm renaming portRangeStr) should be a char* initialized to NULL, and we should call virAsprintf rather than snprintf (with all the associated checks for OOM).

if (!(networkstr = iptablesFormatNetwork(netaddr, prefix))) return -1; @@ -849,19 +852,27 @@ iptablesForwardMasquerade(iptablesContext *ctx, if (physdev && physdev[0]) virCommandAddArgList(cmd, "--out-interface", physdev, NULL);

+ if (protocol && protocol[0]) { + if (port_start == 0 && port_end == 0) { + port_start = 1024; + port_end = 65535; + } + + if (port_start < port_end && port_end < 65536) + snprintf(port_str, sizeof(port_str), ":%d-%d", + port_start, port_end);

1) port_* are unsigned, so the format string should use %u rather than %d 2) If the conditional *isn't* true, that's an error and should be logged.

+ } + /* Use --jump SNAT if public addr is specified */ if (addr_start_str && addr_start_str[0]) { char tmpstr[sizeof("123.123.123.123-123.123.123.123:65535-65535")]; - const char *portstr = "";

memset(tmpstr, 0, sizeof(tmpstr)); - if (protocol && protocol[0]) - portstr = ":1024-65535"; if (addr_end_str && addr_end_str[0]) { snprintf(tmpstr, sizeof(tmpstr), "%s-%s%s", - addr_start_str, addr_end_str, portstr); + addr_start_str, addr_end_str, port_str); } else { - snprintf(tmpstr, sizeof(tmpstr), "%s%s", addr_start_str, portstr); + snprintf(tmpstr, sizeof(tmpstr), "%s%s", addr_start_str, port_str); }

virCommandAddArgList(cmd, "--jump", "SNAT", @@ -869,8 +880,8 @@ iptablesForwardMasquerade(iptablesContext *ctx, } else { virCommandAddArgList(cmd, "--jump", "MASQUERADE", NULL);

- if (protocol && protocol[0]) - virCommandAddArgList(cmd, "--to-ports", "1024-65535", NULL); + if (port_str[0]) + virCommandAddArgList(cmd, "--to-ports", &port_str[1], NULL); }

ret = iptablesCommandRunAndFree(cmd); @@ -899,9 +910,14 @@ iptablesAddForwardMasquerade(iptablesContext *ctx, const char *physdev, virSocketAddr *addr_start, virSocketAddr *addr_end, + unsigned int port_start, + unsigned int port_end, const char *protocol) { - return iptablesForwardMasquerade(ctx, netaddr, prefix, physdev, addr_start, addr_end, protocol, ADD); + return iptablesForwardMasquerade(ctx, netaddr, prefix, physdev, + addr_start, addr_end, + port_start, port_end, + protocol, ADD); }

/** @@ -924,9 +940,14 @@ iptablesRemoveForwardMasquerade(iptablesContext *ctx, const char *physdev, virSocketAddr *addr_start, virSocketAddr *addr_end, + unsigned int port_start, + unsigned int port_end, const char *protocol) { - return iptablesForwardMasquerade(ctx, netaddr, prefix, physdev, addr_start, addr_end, protocol, REMOVE); + return iptablesForwardMasquerade(ctx, netaddr, prefix, physdev, + addr_start, addr_end, + port_start, port_end, + protocol, REMOVE); }

diff --git a/src/util/viriptables.h b/src/util/viriptables.h index 4241380..f2db368 100644 --- a/src/util/viriptables.h +++ b/src/util/viriptables.h @@ -109,6 +109,8 @@ int iptablesAddForwardMasquerade (iptablesContext *ctx, const char *physdev, virSocketAddr *addr_start, virSocketAddr *addr_end, + unsigned int port_start, + unsigned int port_end, const char *protocol); int iptablesRemoveForwardMasquerade (iptablesContext *ctx, virSocketAddr *netaddr, @@ -116,6 +118,8 @@ int iptablesRemoveForwardMasquerade (iptablesContext *ctx, const char *physdev, virSocketAddr *addr_start, virSocketAddr *addr_end, + unsigned int port_start, + unsigned int port_end, const char *protocol); int iptablesAddOutputFixUdpChecksum (iptablesContext *ctx, const char *iface,

ACK with those nits fixed. I was making them as I went along, and had to rebase the patch anyway, so I'll post a new version that you can review; if you ACK, I'll push.

From: Natanael Copa <ncopa@alpinelinux.org> Let users set the port range to be used for forward mode NAT: ... <forward mode='nat'> <nat> <port start='1024' end='65535'/> </nat> </forward> ... Signed-off-by: Natanael Copa <ncopa@alpinelinux.org> Signed-off-by: Laine Stump <laine@laine.org> --- Differences from V1: * rebased and resolved conflicts due to changes in PATCH 1/2 * renamed port_(start|end) to port(Start|End). * fixed nits listed in review docs/formatnetwork.html.in | 22 +++++++++++++--- src/conf/network_conf.c | 59 +++++++++++++++++++++++++++++++++++------- src/conf/network_conf.h | 3 ++- src/network/bridge_driver.c | 16 ++++++++++++ src/util/viriptables.c | 62 +++++++++++++++++++++++++++++++-------------- src/util/viriptables.h | 4 +++ 6 files changed, 134 insertions(+), 32 deletions(-) diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index cad47d0..b85d9f6 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -139,9 +139,11 @@ <p> <span class="since">Since 1.0.3</span> it is possible to - specify a public IPv4 address range to be used for NAT - by using the <code><nat></code> element and - its <code><address></code> subelement. + specify a public IPv4 address and port range to be used for NAT + by using the <code><nat></code> element. + The address range is set with the <code><address></code> + subelement and <code>start</code> and <code>stop</code> + attributes: </p> <pre> ... @@ -160,6 +162,20 @@ egress, just don't specify any <code><nat></code> element. </p> + <p> + The port range to be used for NAT can also be set via + the <code><port></code> subelement + of <code><nat></code>: + </p> + <pre> +... + <forward mode='nat'> + <nat> + <port start='500' end='1000'/> + </nat> + </forward> +... + </pre> </dd> <dt><code>route</code></dt> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index d7b31cf..581f885 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1332,7 +1332,8 @@ virNetworkForwardNatDefParseXML(const char *networkName, { int ret = -1; xmlNodePtr *natAddrNodes = NULL; - int nNatAddrs; + xmlNodePtr *natPortNodes = NULL; + int nNatAddrs, nNatPorts; char *addrStart = NULL; char *addrEnd = NULL; xmlNodePtr save = ctxt->node; @@ -1390,6 +1391,36 @@ virNetworkForwardNatDefParseXML(const char *networkName, goto cleanup; } + /* ports for SNAT and MASQUERADE */ + nNatPorts = virXPathNodeSet("./port", ctxt, &natPortNodes); + if (nNatPorts < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid <port> element found in <forward> of " + "network %s"), networkName); + goto cleanup; + } else if (nNatPorts > 1) { + virReportError(VIR_ERR_XML_ERROR, + _("Only one <port> element is allowed in <nat> in " + "<forward> in network %s"), networkName); + goto cleanup; + } else if (nNatPorts == 1) { + if (virXPathUInt("string(./port[1]/@start)", ctxt, &def->portStart) < 0 + || def->portStart > 65535) { + + virReportError(VIR_ERR_XML_DETAIL, + _("Missing or invalid 'start' attribute in <port> " + "in <nat> in <forward> in network %s"), + networkName); + goto cleanup; + } + if (virXPathUInt("string(./port[1]/@end)", ctxt, &def->portEnd) < 0 + || def->portEnd > 65535 || def->portEnd < def->portStart) { + virReportError(VIR_ERR_XML_DETAIL, + _("Missing or invalid 'end' attribute in <port> in " + "<nat> in <forward> in network %s"), networkName); + goto cleanup; + } + } ret = 0; cleanup: @@ -2194,16 +2225,25 @@ virNetworkForwardNatDefFormat(virBufferPtr buf, goto cleanup; } - if (!addrEnd && !addrStart) + if (!addrEnd && !addrStart && !fwd->portStart && !fwd->portEnd) return 0; virBufferAddLit(buf, "<nat>\n"); virBufferAdjustIndent(buf, 2); - virBufferAsprintf(buf, "<address start='%s'", addrStart); - if (addrEnd) - virBufferAsprintf(buf, " end='%s'", addrEnd); - virBufferAsprintf(buf, "/>\n"); + if (addrStart) { + virBufferAsprintf(buf, "<address start='%s'", addrStart); + if (addrEnd) + virBufferAsprintf(buf, " end='%s'", addrEnd); + virBufferAsprintf(buf, "/>\n"); + } + + if (fwd->portStart || fwd->portEnd) { + virBufferAsprintf(buf, "<port start='%d'", fwd->portStart); + if (fwd->portEnd) + virBufferAsprintf(buf, " end='%d'", fwd->portEnd); + virBufferAsprintf(buf, "/>\n"); + } virBufferAdjustIndent(buf, -2); virBufferAsprintf(buf, "</nat>\n"); @@ -2259,9 +2299,10 @@ virNetworkDefFormatInternal(virBufferPtr buf, else virBufferAddLit(buf, " managed='no'"); } - shortforward = !(def->forward.nifs || def->forward.npfs - || VIR_SOCKET_ADDR_VALID(&def->forward.addrStart) - || VIR_SOCKET_ADDR_VALID(&def->forward.addrEnd)); + shortforward = !(def->forward.nifs || def->forward.npfs || + VIR_SOCKET_ADDR_VALID(&def->forward.addrStart) || + VIR_SOCKET_ADDR_VALID(&def->forward.addrEnd) || + def->forward.portStart || def->forward.portEnd); virBufferAsprintf(buf, "%s>\n", shortforward ? "/" : ""); virBufferAdjustIndent(buf, 2); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 11d6c9c..515115b 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -175,8 +175,9 @@ struct _virNetworkForwardDef { size_t nifs; virNetworkForwardIfDefPtr ifs; - /* adresses for SNAT */ + /* ranges for NAT */ virSocketAddr addrStart, addrEnd; + unsigned int portStart, portEnd; }; typedef struct _virPortGroupDef virPortGroupDef; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 9f502a5..cf47ec4 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1589,6 +1589,8 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver, forwardIf, &network->def->forward.addrStart, &network->def->forward.addrEnd, + network->def->forward.portStart, + network->def->forward.portEnd, NULL) < 0) { virReportError(VIR_ERR_SYSTEM_ERROR, forwardIf ? @@ -1605,6 +1607,8 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver, forwardIf, &network->def->forward.addrStart, &network->def->forward.addrEnd, + network->def->forward.portStart, + network->def->forward.portEnd, "udp") < 0) { virReportError(VIR_ERR_SYSTEM_ERROR, forwardIf ? @@ -1621,6 +1625,8 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver, forwardIf, &network->def->forward.addrStart, &network->def->forward.addrEnd, + network->def->forward.portStart, + network->def->forward.portEnd, "tcp") < 0) { virReportError(VIR_ERR_SYSTEM_ERROR, forwardIf ? @@ -1639,6 +1645,8 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver, forwardIf, &network->def->forward.addrStart, &network->def->forward.addrEnd, + network->def->forward.portStart, + network->def->forward.portEnd, "udp"); masqerr4: iptablesRemoveForwardMasquerade(driver->iptables, @@ -1647,6 +1655,8 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver, forwardIf, &network->def->forward.addrStart, &network->def->forward.addrEnd, + network->def->forward.portStart, + network->def->forward.portEnd, NULL); masqerr3: iptablesRemoveForwardAllowRelatedIn(driver->iptables, @@ -1679,6 +1689,8 @@ networkRemoveMasqueradingIptablesRules(struct network_driver *driver, forwardIf, &network->def->forward.addrStart, &network->def->forward.addrEnd, + network->def->forward.portStart, + network->def->forward.portEnd, "tcp"); iptablesRemoveForwardMasquerade(driver->iptables, &ipdef->address, @@ -1686,6 +1698,8 @@ networkRemoveMasqueradingIptablesRules(struct network_driver *driver, forwardIf, &network->def->forward.addrStart, &network->def->forward.addrEnd, + network->def->forward.portStart, + network->def->forward.portEnd, "udp"); iptablesRemoveForwardMasquerade(driver->iptables, &ipdef->address, @@ -1693,6 +1707,8 @@ networkRemoveMasqueradingIptablesRules(struct network_driver *driver, forwardIf, &network->def->forward.addrStart, &network->def->forward.addrEnd, + network->def->forward.portStart, + network->def->forward.portEnd, NULL); iptablesRemoveForwardAllowRelatedIn(driver->iptables, diff --git a/src/util/viriptables.c b/src/util/viriptables.c index f8a09bf..b0951b5 100644 --- a/src/util/viriptables.c +++ b/src/util/viriptables.c @@ -807,6 +807,8 @@ iptablesForwardMasquerade(iptablesContext *ctx, const char *physdev, virSocketAddr *addrStart, virSocketAddr *addrEnd, + unsigned int portStart, + unsigned int portEnd, const char *protocol, int action) { @@ -815,6 +817,7 @@ iptablesForwardMasquerade(iptablesContext *ctx, char *addrStartStr = NULL; char *addrEndStr = NULL; char *addrRangeStr = NULL; + char *portRangeStr = NULL; virCommandPtr cmd = NULL; if (!(networkstr = iptablesFormatNetwork(netaddr, prefix))) @@ -850,32 +853,44 @@ iptablesForwardMasquerade(iptablesContext *ctx, if (physdev && physdev[0]) virCommandAddArgList(cmd, "--out-interface", physdev, NULL); + if (protocol && protocol[0]) { + if (portStart == 0 && portEnd == 0) { + portStart = 1024; + portEnd = 65535; + } + if (portStart > portEnd || portEnd > 65535) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Bad NAT port range '%u-%u'"), + portStart, portEnd); + goto cleanup; + } + if (virAsprintf(&portRangeStr, ":%u-%u", portStart, portEnd) < 0) { + virReportOOMError(); + goto cleanup; + } + } + /* Use --jump SNAT if public addr is spec1ified */ if (addrStartStr && addrStartStr[0]) { - const char *portstr = ""; - - if (protocol && protocol[0]) - portstr = ":1024-65535"; if (addrEndStr && addrEndStr[0]) { - if (virAsprintf(&addrRangeStr, "%s-%s%s", - addrStartStr, addrEndStr, portstr) < 0) { - virReportOOMError(); - goto cleanup; - } - } else { - if (virAsprintf(&addrRangeStr, "%s%s", addrStartStr, portstr) < 0) { + if (virAsprintf(&addrRangeStr, "%s-%s%s", addrStartStr, addrEndStr, + portRangeStr ? portRangeStr : "") < 0) { virReportOOMError(); goto cleanup; } + } else if (virAsprintf(&addrRangeStr, "%s%s", addrStartStr, + portRangeStr ? portRangeStr : "") < 0) { + virReportOOMError(); + goto cleanup; } virCommandAddArgList(cmd, "--jump", "SNAT", - "--to-source", addrRangeStr, NULL); - } else { - virCommandAddArgList(cmd, "--jump", "MASQUERADE", NULL); + "--to-source", addrRangeStr, NULL); + } else { + virCommandAddArgList(cmd, "--jump", "MASQUERADE", NULL); - if (protocol && protocol[0]) - virCommandAddArgList(cmd, "--to-ports", "1024-65535", NULL); - } + if (portRangeStr && portRangeStr[0]) + virCommandAddArgList(cmd, "--to-ports", &portRangeStr[1], NULL); + } ret = iptablesCommandRunAndFree(cmd); cleanup: @@ -883,6 +898,7 @@ cleanup: VIR_FREE(addrStartStr); VIR_FREE(addrEndStr); VIR_FREE(addrRangeStr); + VIR_FREE(portRangeStr); return ret; } @@ -906,10 +922,14 @@ iptablesAddForwardMasquerade(iptablesContext *ctx, const char *physdev, virSocketAddr *addrStart, virSocketAddr *addrEnd, + unsigned int portStart, + unsigned int portEnd, const char *protocol) { return iptablesForwardMasquerade(ctx, netaddr, prefix, physdev, - addrStart, addrEnd, protocol, ADD); + addrStart, addrEnd, + portStart, portEnd, + protocol, ADD); } /** @@ -932,10 +952,14 @@ iptablesRemoveForwardMasquerade(iptablesContext *ctx, const char *physdev, virSocketAddr *addrStart, virSocketAddr *addrEnd, + unsigned int portStart, + unsigned int portEnd, const char *protocol) { return iptablesForwardMasquerade(ctx, netaddr, prefix, physdev, - addrStart, addrEnd, protocol, REMOVE); + addrStart, addrEnd, + portStart, portEnd, + protocol, REMOVE); } diff --git a/src/util/viriptables.h b/src/util/viriptables.h index 05362da..ca6adcc 100644 --- a/src/util/viriptables.h +++ b/src/util/viriptables.h @@ -109,6 +109,8 @@ int iptablesAddForwardMasquerade (iptablesContext *ctx, const char *physdev, virSocketAddr *addrStart, virSocketAddr *addrEnd, + unsigned int portStart, + unsigned int portEnd, const char *protocol); int iptablesRemoveForwardMasquerade (iptablesContext *ctx, virSocketAddr *netaddr, @@ -116,6 +118,8 @@ int iptablesRemoveForwardMasquerade (iptablesContext *ctx, const char *physdev, virSocketAddr *addrStart, virSocketAddr *addrEnd, + unsigned int portStart, + unsigned int portEnd, const char *protocol); int iptablesAddOutputFixUdpChecksum (iptablesContext *ctx, const char *iface, -- 1.7.11.7