On 01/04/2021 09:26, Ilya Maximets wrote: > On 3/30/21 6:15 PM, Mark Gray wrote: >> On 30/03/2021 15:28, Aaron Conole wrote: >>> Mark Gray <[email protected]> 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 <[email protected]> >>>> --- >>>> 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.
Done and done. > >>> >>>> >>>> def unixctl_xfrm_policies(conn, unused_argv, unused_aux): >>>> global xfrm >>> >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
