| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 7 Jul 2023 12:12:51 +0200
From: Adrian Moreno <amorenoz@...hat.com>
To: Aaron Conole <aconole@...hat.com>, netdev@...r.kernel.org
Cc: dev@...nvswitch.org, Ilya Maximets <i.maximets@....org>,
Eric Dumazet <edumazet@...gle.com>, linux-kselftest@...r.kernel.org,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
shuah@...nel.org, "David S. Miller" <davem@...emloft.net>
Subject: Re: [ovs-dev] [PATCH net-next 4/4] selftests: openvswitch: add ct-nat
test case with ipv4
On 6/28/23 18:27, Aaron Conole wrote:
> Building on the previous work, add a very simplistic NAT case
> using ipv4. This just tests dnat transformation
>
> Signed-off-by: Aaron Conole <aconole@...hat.com>
Hi Aaron,
I know that the goal is not to support the full syntax, and that nat is a
specially convoluted action, so I'm just commenting on the low-hanging fruits
(see below).
> ---
> .../selftests/net/openvswitch/openvswitch.sh | 64 +++++++++++++++++++
> .../selftests/net/openvswitch/ovs-dpctl.py | 60 +++++++++++++++++
> 2 files changed, 124 insertions(+)
>
> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> index 40a66c72af0f0..dced4f612a78c 100755
> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> @@ -14,6 +14,7 @@ tests="
> arp_ping eth-arp: Basic arp ping between two NS
> ct_connect_v4 ip4-ct-xon: Basic ipv4 tcp connection using ct
> connect_v4 ip4-xon: Basic ipv4 ping between two NS
> + nat_connect_v4 ip4-nat-xon: Basic ipv4 tcp connection via NAT
> netlink_checks ovsnl: validate netlink attrs and settings
> upcall_interfaces ovs: test the upcall interfaces"
>
> @@ -300,6 +301,69 @@ test_connect_v4 () {
> return 0
> }
>
> +# nat_connect_v4 test
> +# - client has 1500 byte MTU
> +# - server has 1500 byte MTU
> +# - use ICMP to ping in each direction
> +# - only allow CT state stuff to pass through new in c -> s
> +test_nat_connect_v4 () {
> + which nc >/dev/null 2>/dev/null || return $ksft_skip
> +
> + sbx_add "test_nat_connect_v4" || return $?
> +
> + ovs_add_dp "test_nat_connect_v4" nat4 || return 1
> + info "create namespaces"
> + for ns in client server; do
> + ovs_add_netns_and_veths "test_nat_connect_v4" "nat4" "$ns" \
> + "${ns:0:1}0" "${ns:0:1}1" || return 1
> + done
> +
> + ip netns exec client ip addr add 172.31.110.10/24 dev c1
> + ip netns exec client ip link set c1 up
> + ip netns exec server ip addr add 172.31.110.20/24 dev s1
> + ip netns exec server ip link set s1 up
> +
> + ip netns exec client ip route add default via 172.31.110.20
> +
> + ovs_add_flow "test_nat_connect_v4" nat4 \
> + 'in_port(1),eth(),eth_type(0x0806),arp()' '2' || return 1
> + ovs_add_flow "test_nat_connect_v4" nat4 \
> + 'in_port(2),eth(),eth_type(0x0806),arp()' '1' || return 1
> + ovs_add_flow "test_nat_connect_v4" nat4 \
> + "ct_state(-trk),in_port(1),eth(),eth_type(0x0800),ipv4(dst=192.168.0.20)" \
> + "ct(commit,nat(dst=172.31.110.20)),recirc(0x1)"
> + ovs_add_flow "test_nat_connect_v4" nat4 \
> + "ct_state(-trk),in_port(2),eth(),eth_type(0x0800),ipv4()" \
> + "ct(commit,nat),recirc(0x2)"
> +
> + ovs_add_flow "test_nat_connect_v4" nat4 \
> + "recirc_id(0x1),ct_state(+trk-inv),in_port(1),eth(),eth_type(0x0800),ipv4()" "2"
> + ovs_add_flow "test_nat_connect_v4" nat4 \
> + "recirc_id(0x2),ct_state(+trk-inv),in_port(2),eth(),eth_type(0x0800),ipv4()" "1"
> +
> + # do a ping
> + ovs_sbx "test_nat_connect_v4" ip netns exec client ping 192.168.0.20 -c 3 || return 1
> +
> + # create an echo server in 'server'
> + echo "server" | \
> + ovs_netns_spawn_daemon "test_nat_connect_v4" "server" \
> + nc -lvnp 4443
> + ovs_sbx "test_nat_connect_v4" ip netns exec client nc -i 1 -zv 192.168.0.20 4443 || return 1
> +
> + # Now test in the other direction (should fail)
> + echo "client" | \
> + ovs_netns_spawn_daemon "test_nat_connect_v4" "client" \
> + nc -lvnp 4443
> + ovs_sbx "test_nat_connect_v4" ip netns exec client nc -i 1 -zv 172.31.110.10 4443
> + if [ $? == 0 ]; then
> + info "connect to client was successful"
> + return 1
> + fi
> +
> + info "done..."
> + return 0
> +}
> +
> # netlink_validation
> # - Create a dp
> # - check no warning with "old version" simulation
> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> index 704cb4adf79a9..12ba5265b88fb 100644
> --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> @@ -511,6 +511,66 @@ class ovsactions(nla):
> else:
> ctact["attrs"].append([scan[1], None])
> actstr = actstr[strspn(actstr, ", ") :]
> + # it seems strange to put this here, but nat() is a complex
> + # sub-action and this lets it sit anywhere in the ct() action
> + if actstr.startswith("nat"):
> + actstr = actstr[3:]
> + natact = ovsactions.ctact.natattr()
> +
> + if actstr.startswith("("):
> + t = None
> + actstr = actstr[1:]
> + if actstr.startswith("src"):
> + t = "OVS_NAT_ATTR_SRC"
> + actstr = actstr[3:]
> + elif actstr.startswith("dst"):
> + t = "OVS_NAT_ATTR_DST"
> + actstr = actstr[3:]
> +
> + actstr, ip_block_min = parse_extract_field(
> + actstr, "=", "([0-9a-fA-F:\.]+)", str, False
> + )
> + actstr, ip_block_max = parse_extract_field(
> + actstr, "-", "([0-9a-fA-F:\.]+)", str, False
> + )
Having the ":" character here makes this line parse the port as well (i.e:
1.1.1.1:6789 as ip_block_max) which then makes ip address parsing fail.
> + actstr, proto_min = parse_extract_field(
> + actstr, ":", "(\d+)", int, False
> + )
> + actstr, proto_max = parse_extract_field(
> + actstr, "-", "(\d+)", int, False
> + )
> + if t is not None:
> + natact["attrs"].append([t, None])
> +
> + if ip_block_min is not None:
> + natact["attrs"].append(
> + ["OVS_NAT_ATTR_IP_MIN", ip_block_min]
> + )
> + if ip_block_max is not None:
> + natact["attrs"].append(
> + ["OVS_NAT_ATTR_IP_MAX", ip_block_max]
> + )
> + if proto_min is not None:
> + natact["attrs"].append(
> + ["OVS_NAT_ATTR_PROTO_MIN", proto_min]
> + )
> + if proto_max is not None:
> + natact["attrs"].append(
> + ["OVS_NAT_ATTR_PROTO_MAX", proto_max]
> + )
> +
> + for natscan in (
> + ("persist", "OVS_NAT_ATTR_PERSISTENT"),
> + ("hash", "OVS_NAT_ATTR_PROTO_HASH"),
> + ("random", "OVS_NAT_ATTR_PROTO_RANDOM"),
> + ):
I think this is not taking into account the comma that separates ip:port from
these keywords. A possible solution would be to add it to the natscan (e.g:
s/persist/,persist/).
> + if actstr.startswith(natscan[0]):
> + actstr = actstr[len(natscan[0]) :]
> + natact["attrs"].append([natscan[1], None])
> + actstr = actstr[strspn(actstr, ", ") :]
> +
> + ctact["attrs"].append(["OVS_CT_ATTR_NAT", natact])
> + actstr = actstr[strspn(actstr, ",) ") :]
>
> self["attrs"].append(["OVS_ACTION_ATTR_CT", ctact])
> actstr = actstr[strspn(actstr, "), ") :]
--
Adrián Moreno
Powered by blists - more mailing lists