On 3/30/21 6:15 PM, Mark Gray wrote: > On 30/03/2021 15:28, Aaron Conole wrote: >> Mark Gray <mark.d.g...@redhat.com> writes: >> >>> When configuring IPsec, "ovs-monitor-ipsec" honours >>> the 'local_ip' option in the 'Interface' table by configuring >>> the 'left' side of the Libreswan connection with 'local_ip'. >>> If 'local_ip' is not specified, "ovs-monitor-ipsec" sets >>> 'left' to '%defaultroute' which is interpreted as the IP >>> address of the default gateway interface. >>> >>> However, when 'remote_ip' is an IPv6 address, Libreswan >>> still interprets '%defaultroute' as the IPv4 address on the >>> default gateway interface (see: >>> https://github.com/libreswan/libreswan/issues/416) giving >>> an "address family inconsistency" error. >>> >>> This patch resolves this issue by specifying the >>> connection as IPv6 when the 'remote_ip' is IPv6 and >>> 'local_ip' has not been set. >>> >>> Signed-off-by: Mark Gray <mark.d.g...@redhat.com> >>> --- >>> ipsec/ovs-monitor-ipsec.in | 54 +++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 53 insertions(+), 1 deletion(-) >>> >>> diff --git a/ipsec/ovs-monitor-ipsec.in b/ipsec/ovs-monitor-ipsec.in >>> index 9f412aaaf25a..b8cfb0a8ae79 100755 >>> --- a/ipsec/ovs-monitor-ipsec.in >>> +++ b/ipsec/ovs-monitor-ipsec.in >>> @@ -14,10 +14,11 @@ >>> # limitations under the License. >>> >>> import argparse >>> +import copy >> >> I think it's okay to get things in alphabetical order, but it's not >> related. > > I'll change back > >> >>> +import ipaddress >>> import re >>> import subprocess >>> import sys >>> -import copy >>> import os >>> from string import Template >>> >>> @@ -413,6 +414,11 @@ conn prevent_unencrypted_vxlan >>> leftprotoport=udp/4789 >>> mark={0} >>> >>> +""" >>> + >>> + IPV6_CONN = """\ >>> + hostaddrfamily=ipv6 >>> + clientaddrfamily=ipv6 >>> """ >>> >>> auth_tmpl = {"psk": Template("""\ >>> @@ -528,6 +534,9 @@ conn prevent_unencrypted_vxlan >>> else: >>> auth_section = self.auth_tmpl["pki_ca"].substitute(tunnel.conf) >>> >>> + if tunnel.conf["address_family"] == "IPv6": >>> + auth_section = self.IPV6_CONN + auth_section >>> + >>> vals = tunnel.conf.copy() >>> vals["auth_section"] = auth_section >>> vals["version"] = tunnel.version >>> @@ -795,6 +804,7 @@ class IPsecTunnel(object): >>> Tunnel Type: $tunnel_type >>> Local IP: $local_ip >>> Remote IP: $remote_ip >>> + Address Family: $address_family >>> SKB mark: $skb_mark >>> Local cert: $certificate >>> Local name: $local_name >>> @@ -836,6 +846,9 @@ class IPsecTunnel(object): >>> "tunnel_type": row.type, >>> "local_ip": options.get("local_ip", "%defaultroute"), >>> "remote_ip": options.get("remote_ip"), >>> + "address_family": self._get_conn_address_family( >>> + >>> options.get("remote_ip"), >>> + >>> options.get("local_ip")), >>> "skb_mark": monitor.conf["skb_mark"], >>> "certificate": monitor.conf["pki"]["certificate"], >>> "private_key": monitor.conf["pki"]["private_key"], >>> @@ -904,6 +917,24 @@ class IPsecTunnel(object): >>> >>> return header + conf + status + spds + sas + cons + "\n" >>> >>> + def _get_conn_address_family(self, remote_ip, local_ip): >>> + remote = address_family(remote_ip) >>> + local = address_family(local_ip) >>> + >>> + if local == "IPv4" and remote == "IPv4": >>> + return "IPv4" >>> + elif local == "IPv6" and remote == "IPv6": >>> + return "IPv6" >>> + elif remote == "IPv4" and local_ip is None: >>> + return "IPv4" >>> + elif remote == "IPv6" and local_ip is None: >>> + return "IPv6" >>> + elif remote != local: >>> + # remote family and local family are mismatched >>> + return None >>> + else: >>> + return None >>> + >> >> I think we can shrink this whole section to: >> >> >> def _get_conn_address_family(self, remote_ip, local_ip): >> remote = address_family(remote_ip) >> local = address_family(local_ip) >> >> if local is None: >> return remote >> elif local != remote: >> return None >> >> return remote > > Yes, you are right. Simpler. >> >> >>> def _is_valid_tunnel_conf(self): >>> """This function verifies if IPsec tunnel has valid configuration >>> set in 'conf'. If it is valid, then it returns True. Otherwise, >>> @@ -1160,6 +1191,27 @@ class IPsecMonitor(object): >>> >>> return m.group(1) >>> >>> +def is_ipv4(address): >>> + try: >>> + ipaddress.IPv4Address(address) >>> + except ipaddress.AddressValueError: >>> + return False >>> + return True >>> + >>> +def is_ipv6(address): >>> + try: >>> + ipaddress.IPv6Address(address) >>> + except ipaddress.AddressValueError: >>> + return False >>> + return True >>> + >>> +def address_family(address): >>> + if is_ipv4(address): >>> + return "IPv4" >>> + elif is_ipv6(address): >>> + return "IPv6" >>> + else: >>> + return None >> >> Maybe the above 3 functions can look like: >> >> >> def address_family(address): >> try: >> ip = ipaddress.ip_address(address) >> ipstr = str(type(ip)) >> except ValueError: >> return None >> >> if ipstr.find('v6') != -1: >> return "IPv6" >> return "IPv4" >> > Yes, better. >> >> Note that I use ValueError above, because when I tried to test locally, >> I got the following from ipaddress: >> >> ValueError: 'asdffdsa' does not appear to be an IPv4 or IPv6 address > > IIRC I did try an invalid IP address. Maybe the behavior depends on the > version of Python. I tested this on my machine and got the same behavior > as you so I will accept that change >> >> And the block: >> >> >>> try: >> ... ip6=ipaddress.ip_address('asdffdsa') >> ... except ipaddress.AddressValueError: >> ... print("nope") >> ... >> Traceback (most recent call last): >> File "<stdin>", line 2, in <module> >> File "/usr/lib64/python3.7/ipaddress.py", line 54, in ip_address >> address) >> ValueError: 'asdffdsa' does not appear to be an IPv4 or IPv6 address >> >>> try: >> ... ip6=ipaddress.ip_address('asdffdsa') >> ... except ValueError: >> ... print("nope") >> ... >> nope >> >> So, I guess even though it implies that it will always throw >> AddressValueException in the documentation, it doesn't :-/
Shouldn't we catch both types of exception, the documented one and the real one? I mean, since it's documented, library might actually throw them or start throwing in the future. This also worth a comment in the code. >> >>> >>> def unixctl_xfrm_policies(conn, unused_argv, unused_aux): >>> global xfrm >> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
