URL: https://github.com/freeipa/freeipa/pull/875 Author: MartinBasti Title: #875: Fix ip address checks Action: opened
PR body: """ Fix various checks of IP address in installers, removal of some unneeded checks that are not working correctly, and mainly causes only false positive errors. This PR also fixes regressions caused by bf9886a84393d1d1546db7e49b102e08a16a83e7 https://pagure.io/freeipa/issue/4317 """ To pull the PR as Git branch: git remote add ghfreeipa https://github.com/freeipa/freeipa git fetch ghfreeipa pull/875/head:pr875 git checkout pr875
From f342625aa0da367792cfbd5c4f1a164bf878ee8c Mon Sep 17 00:00:00 2001 From: Martin Basti <mba...@redhat.com> Date: Tue, 13 Jun 2017 17:03:30 +0200 Subject: [PATCH 1/7] Fix local IP address validation Previously bf9886a84393d1d1546db7e49b102e08a16a83e7 match_local has undesirable side effect that CheckedIPAddress object has set self._net from local interface. However with the recent changes, match_local is usually set to False, thus this side effect stops happening and default mask per address class is used. This causes validation error because mask on interface and mask used for provided IP addresses differ (reporducible only with classless masks). FreeIPA should compare only IP addresses with local addresses without masks https://pagure.io/freeipa/issue/4317 --- ipapython/ipautil.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py index a277ed8747..647ee833ae 100644 --- a/ipapython/ipautil.py +++ b/ipapython/ipautil.py @@ -216,10 +216,10 @@ def get_matching_interface(self): addr=ifaddr, netmask=ifdata['netmask'] )) - if ifnet == self._net or ( - self._net is None and ifnet.ip == self): - self._net = ifnet + + if ifnet.ip == self: iface = interface + self._net = ifnet break return iface From 446d8fbfa0a912f993191c1447fb4f8002ea065d Mon Sep 17 00:00:00 2001 From: Martin Basti <mba...@redhat.com> Date: Wed, 14 Jun 2017 14:45:03 +0200 Subject: [PATCH 2/7] ipa-dns-install: remove check for local ip address This check was forgotten and will be removed now. https://pagure.io/freeipa/issue/4317 --- install/tools/ipa-dns-install | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/install/tools/ipa-dns-install b/install/tools/ipa-dns-install index 5bd0ba6d77..cb6c5d887f 100755 --- a/install/tools/ipa-dns-install +++ b/install/tools/ipa-dns-install @@ -47,7 +47,9 @@ def parse_options(): default=False, help="print debugging information") parser.add_option("--ip-address", dest="ip_addresses", metavar="IP_ADDRESS", default=[], action="append", - type="ip", ip_local=True, help="Master Server IP Address. This option can be used multiple times") + type="ip", + help="Master Server IP Address. This option can be used " + "multiple times") parser.add_option("--forwarder", dest="forwarders", action="append", type="ip", help="Add a DNS forwarder. This option can be used multiple times") parser.add_option("--no-forwarders", dest="no_forwarders", action="store_true", From 082ff655fd44b82e26b675f1a20fc4be5a3abc05 Mon Sep 17 00:00:00 2001 From: Martin Basti <mba...@redhat.com> Date: Wed, 14 Jun 2017 14:47:23 +0200 Subject: [PATCH 3/7] refactor CheckedIPAddress class Make methods without side effects (setting mask) https://pagure.io/freeipa/issue/4317 --- ipapython/ipautil.py | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py index 647ee833ae..2c020e3ecb 100644 --- a/ipapython/ipautil.py +++ b/ipapython/ipautil.py @@ -62,6 +62,12 @@ socket.SOCK_DGRAM: 'udp' } +InterfaceDetails = collections.namedtuple( + 'InterfaceDetails', [ + 'name', # interface name + 'ifnet' # network details of interface + ]) + class UnsafeIPAddress(netaddr.IPAddress): """Any valid IP address with or without netmask.""" @@ -161,9 +167,12 @@ def __init__(self, addr, match_local=False, parse_netmask=True, raise ValueError("cannot use multicast IP address {}".format(addr)) if match_local: - if not self.get_matching_interface(): + intf_details = self.get_matching_interface() + if not intf_details: raise ValueError('no network interface matches the IP address ' 'and netmask {}'.format(addr)) + else: + self.set_ip_net(intf_details.ifnet) if self._net is None: if self.version == 4: @@ -193,7 +202,8 @@ def is_broadcast_addr(self): def get_matching_interface(self): """Find matching local interface for address - :return: Interface name or None if no interface has this address + :return: InterfaceDetails named tuple or None if no interface has + this address """ if self.version == 4: family = netifaces.AF_INET @@ -204,7 +214,6 @@ def get_matching_interface(self): "Unsupported address family ({})".format(self.version) ) - iface = None for interface in netifaces.interfaces(): for ifdata in netifaces.ifaddresses(interface).get(family, []): @@ -218,11 +227,17 @@ def get_matching_interface(self): )) if ifnet.ip == self: - iface = interface - self._net = ifnet - break + return InterfaceDetails(interface, ifnet) - return iface + def set_ip_net(self, ifnet): + """Set IP Network details for this address. IPNetwork is valid only + locally, so this should be set only for local IP addresses + + :param ifnet: netaddr.IPNetwork object with information about IP + network where particula address belongs locally + """ + assert isinstance(ifnet, netaddr.IPNetwork) + self._net = ifnet def valid_ip(addr): From ec86fcebc2686ef36559a8c56d7f4cde16f87099 Mon Sep 17 00:00:00 2001 From: Martin Basti <mba...@redhat.com> Date: Wed, 14 Jun 2017 14:54:43 +0200 Subject: [PATCH 4/7] CheckedIPAddress: remove match_local param This parameter is unused in code. We are no longer testing if IP address matches an interface in constructor. https://pagure.io/freeipa/issue/4317 --- ipapython/config.py | 5 ++--- ipapython/ipautil.py | 10 +--------- ipaserver/install/installutils.py | 2 +- ipaserver/plugins/dns.py | 4 ++-- ipaserver/plugins/host.py | 2 +- ipatests/test_ipapython/test_ipautil.py | 4 ++-- 6 files changed, 9 insertions(+), 18 deletions(-) diff --git a/ipapython/config.py b/ipapython/config.py index 9db2dcd4db..6349892fe8 100644 --- a/ipapython/config.py +++ b/ipapython/config.py @@ -68,10 +68,9 @@ def format_usage(self, usage): def check_ip_option(option, opt, value): from ipapython.ipautil import CheckedIPAddress - ip_local = option.ip_local is True ip_netmask = option.ip_netmask is True try: - return CheckedIPAddress(value, parse_netmask=ip_netmask, match_local=ip_local) + return CheckedIPAddress(value, parse_netmask=ip_netmask) except Exception as e: raise OptionValueError("option %s: invalid IP address %s: %s" % (opt, value, e)) @@ -86,7 +85,7 @@ class IPAOption(Option): optparse.Option subclass with support of options labeled as security-sensitive such as passwords. """ - ATTRS = Option.ATTRS + ["sensitive", "ip_local", "ip_netmask"] + ATTRS = Option.ATTRS + ["sensitive", "ip_netmask"] TYPES = Option.TYPES + ("ip", "dn") TYPE_CHECKER = copy(Option.TYPE_CHECKER) TYPE_CHECKER["ip"] = check_ip_option diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py index 2c020e3ecb..5a6bf5a27d 100644 --- a/ipapython/ipautil.py +++ b/ipapython/ipautil.py @@ -135,7 +135,7 @@ class CheckedIPAddress(UnsafeIPAddress): Reserved or link-local addresses are never accepted. """ - def __init__(self, addr, match_local=False, parse_netmask=True, + def __init__(self, addr, parse_netmask=True, allow_loopback=False, allow_multicast=False): try: super(CheckedIPAddress, self).__init__(addr) @@ -166,14 +166,6 @@ def __init__(self, addr, match_local=False, parse_netmask=True, if not allow_multicast and self.is_multicast(): raise ValueError("cannot use multicast IP address {}".format(addr)) - if match_local: - intf_details = self.get_matching_interface() - if not intf_details: - raise ValueError('no network interface matches the IP address ' - 'and netmask {}'.format(addr)) - else: - self.set_ip_net(intf_details.ifnet) - if self._net is None: if self.version == 4: self._net = netaddr.IPNetwork( diff --git a/ipaserver/install/installutils.py b/ipaserver/install/installutils.py index 5f4bf1553f..5f7a346eb0 100644 --- a/ipaserver/install/installutils.py +++ b/ipaserver/install/installutils.py @@ -590,7 +590,7 @@ def get_server_ip_address(host_name, unattended, setup_dns, ip_addresses): if len(hostaddr): for ha in hostaddr: try: - ips.append(ipautil.CheckedIPAddress(ha, match_local=False)) + ips.append(ipautil.CheckedIPAddress(ha)) except ValueError as e: root_logger.warning("Invalid IP address %s for %s: %s", ha, host_name, unicode(e)) diff --git a/ipaserver/plugins/dns.py b/ipaserver/plugins/dns.py index b075f0bf97..38747c0b09 100644 --- a/ipaserver/plugins/dns.py +++ b/ipaserver/plugins/dns.py @@ -567,7 +567,7 @@ def add_records_for_host_validation(option_name, host, domain, ip_addresses, che for ip_address in ip_addresses: try: ip = CheckedIPAddress( - ip_address, match_local=False, allow_multicast=True) + ip_address, allow_multicast=True) except Exception as e: raise errors.ValidationError(name=option_name, error=unicode(e)) @@ -599,7 +599,7 @@ def add_records_for_host(host, domain, ip_addresses, add_forward=True, add_rever for ip_address in ip_addresses: ip = CheckedIPAddress( - ip_address, match_local=False, allow_multicast=True) + ip_address, allow_multicast=True) if add_forward: add_forward_record(domain, host, unicode(ip)) diff --git a/ipaserver/plugins/host.py b/ipaserver/plugins/host.py index 1e1f9d82df..364e5be600 100644 --- a/ipaserver/plugins/host.py +++ b/ipaserver/plugins/host.py @@ -245,7 +245,7 @@ def validate_ipaddr(ugettext, ipaddr): Verify that we have either an IPv4 or IPv6 address. """ try: - CheckedIPAddress(ipaddr, match_local=False) + CheckedIPAddress(ipaddr) except Exception as e: return unicode(e) return None diff --git a/ipatests/test_ipapython/test_ipautil.py b/ipatests/test_ipapython/test_ipautil.py index 409cf2ddc0..75011d508f 100644 --- a/ipatests/test_ipapython/test_ipautil.py +++ b/ipatests/test_ipapython/test_ipautil.py @@ -64,9 +64,9 @@ def test_ip_address(addr, words, prefixlen): if words is None: pytest.raises( - ValueError, ipautil.CheckedIPAddress, addr, match_local=False) + ValueError, ipautil.CheckedIPAddress, addr) else: - ip = ipautil.CheckedIPAddress(addr, match_local=False) + ip = ipautil.CheckedIPAddress(addr) assert ip.words == words assert ip.prefixlen == prefixlen From d6b93298e0ffba027260c07df5e5df4ffd76b820 Mon Sep 17 00:00:00 2001 From: Martin Basti <mba...@redhat.com> Date: Wed, 14 Jun 2017 15:02:21 +0200 Subject: [PATCH 5/7] Remove ip_netmask from option parser ipa-dns-install uses ip_netmask=False --> parse_netmask=False, other installers uses default (parse_netmask=True). Use this consistent accross all installers. Also this option is unused (and shouldn't be used). https://pagure.io/freeipa/issue/4317 --- ipapython/config.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/ipapython/config.py b/ipapython/config.py index 6349892fe8..19abfc51ee 100644 --- a/ipapython/config.py +++ b/ipapython/config.py @@ -68,9 +68,8 @@ def format_usage(self, usage): def check_ip_option(option, opt, value): from ipapython.ipautil import CheckedIPAddress - ip_netmask = option.ip_netmask is True try: - return CheckedIPAddress(value, parse_netmask=ip_netmask) + return CheckedIPAddress(value) except Exception as e: raise OptionValueError("option %s: invalid IP address %s: %s" % (opt, value, e)) @@ -85,7 +84,7 @@ class IPAOption(Option): optparse.Option subclass with support of options labeled as security-sensitive such as passwords. """ - ATTRS = Option.ATTRS + ["sensitive", "ip_netmask"] + ATTRS = Option.ATTRS + ["sensitive"] TYPES = Option.TYPES + ("ip", "dn") TYPE_CHECKER = copy(Option.TYPE_CHECKER) TYPE_CHECKER["ip"] = check_ip_option From ffd799e8314519ddc05b99618de0dccf044f229e Mon Sep 17 00:00:00 2001 From: Martin Basti <mba...@redhat.com> Date: Thu, 15 Jun 2017 10:26:03 +0200 Subject: [PATCH 6/7] replica install: add missing check for non-local IP address Add missing warning for used non-local IP address. https://pagure.io/freeipa/issue/4317 --- ipaserver/install/server/replicainstall.py | 1 + 1 file changed, 1 insertion(+) diff --git a/ipaserver/install/server/replicainstall.py b/ipaserver/install/server/replicainstall.py index 6620f0222f..9e328bf83b 100644 --- a/ipaserver/install/server/replicainstall.py +++ b/ipaserver/install/server/replicainstall.py @@ -854,6 +854,7 @@ def install_check(installer): # check addresses here, dns module is doing own check network_ip_address_warning(config.ips) broadcast_ip_address_warning(config.ips) + no_matching_interface_for_ip_address_warning(config.ips) if options.setup_adtrust: adtrust.install_check(False, options, remote_api) From 9956e31a25ac1a290b2199d71e1c1ef57310af80 Mon Sep 17 00:00:00 2001 From: Martin Basti <mba...@redhat.com> Date: Thu, 15 Jun 2017 10:27:55 +0200 Subject: [PATCH 7/7] Remove network and broadcast address warnings We cannot reliably determine when an IP Address is network or broadcast. We allowed to use non-local IP addresses due container use cases, we don't know subnets of used IP addresses. https://pagure.io/freeipa/issue/4317 --- ipaclient/install/client.py | 4 ---- ipalib/util.py | 20 -------------------- ipaserver/install/dns.py | 2 -- ipaserver/install/server/install.py | 4 ---- ipaserver/install/server/replicainstall.py | 10 +--------- 5 files changed, 1 insertion(+), 39 deletions(-) diff --git a/ipaclient/install/client.py b/ipaclient/install/client.py index 6f23c271cf..46d57b0dba 100644 --- a/ipaclient/install/client.py +++ b/ipaclient/install/client.py @@ -38,8 +38,6 @@ from ipalib.install.service import enroll_only, prepare_only from ipalib.rpc import delete_persistent_client_session_data from ipalib.util import ( - broadcast_ip_address_warning, - network_ip_address_warning, normalize_hostname, no_matching_interface_for_ip_address_warning, verify_host_resolvable, @@ -1299,8 +1297,6 @@ def update_dns(server, hostname, options): root_logger.info("Failed to determine this machine's ip address(es).") return - network_ip_address_warning(update_ips) - broadcast_ip_address_warning(update_ips) no_matching_interface_for_ip_address_warning(update_ips) update_txt = "debug\n" diff --git a/ipalib/util.py b/ipalib/util.py index 1bd8495a49..31e73230da 100644 --- a/ipalib/util.py +++ b/ipalib/util.py @@ -1110,26 +1110,6 @@ def check_principal_realm_in_trust_namespace(api_instance, *keys): 'namespace')) -def network_ip_address_warning(addr_list): - for ip in addr_list: - if ip.is_network_addr(): - root_logger.warning("IP address %s might be network address", ip) - # fixme: once when loggers will be fixed, we can remove this - # print - print("WARNING: IP address {} might be network address".format(ip), - file=sys.stderr) - - -def broadcast_ip_address_warning(addr_list): - for ip in addr_list: - if ip.is_broadcast_addr(): - root_logger.warning("IP address %s might be broadcast address", ip) - # fixme: once when loggers will be fixed, we can remove this - # print - print("WARNING: IP address {} might be broadcast address".format( - ip), file=sys.stderr) - - def no_matching_interface_for_ip_address_warning(addr_list): for ip in addr_list: if not ip.get_matching_interface(): diff --git a/ipaserver/install/dns.py b/ipaserver/install/dns.py index 090b794936..1c1aac06a1 100644 --- a/ipaserver/install/dns.py +++ b/ipaserver/install/dns.py @@ -264,8 +264,6 @@ def install_check(standalone, api, replica, options, hostname): ip_addresses = get_server_ip_address(hostname, options.unattended, True, options.ip_addresses) - util.network_ip_address_warning(ip_addresses) - util.broadcast_ip_address_warning(ip_addresses) util.no_matching_interface_for_ip_address_warning(ip_addresses) if not options.forward_policy: diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py index 7eb291e07c..dced253e7f 100644 --- a/ipaserver/install/server/install.py +++ b/ipaserver/install/server/install.py @@ -27,8 +27,6 @@ from ipalib.constants import DOMAIN_LEVEL_0 from ipalib.util import ( validate_domain_name, - network_ip_address_warning, - broadcast_ip_address_warning, no_matching_interface_for_ip_address_warning, ) import ipaclient.install.ntpconf @@ -616,8 +614,6 @@ def install_check(installer): options.ip_addresses) # check addresses here, dns module is doing own check - network_ip_address_warning(ip_addresses) - broadcast_ip_address_warning(ip_addresses) no_matching_interface_for_ip_address_warning(ip_addresses) if options.setup_adtrust: diff --git a/ipaserver/install/server/replicainstall.py b/ipaserver/install/server/replicainstall.py index 9e328bf83b..4f28de25bd 100644 --- a/ipaserver/install/server/replicainstall.py +++ b/ipaserver/install/server/replicainstall.py @@ -32,11 +32,7 @@ from ipaplatform.paths import paths from ipalib import api, constants, create_api, errors, rpc, x509 from ipalib.config import Env -from ipalib.util import ( - network_ip_address_warning, - broadcast_ip_address_warning, - no_matching_interface_for_ip_address_warning, -) +from ipalib.util import no_matching_interface_for_ip_address_warning from ipaclient.install.client import configure_krb5_conf, purge_host_keytab from ipaserver.install import ( adtrust, bindinstance, ca, certs, dns, dsinstance, httpinstance, @@ -852,8 +848,6 @@ def install_check(installer): options.ip_addresses) # check addresses here, dns module is doing own check - network_ip_address_warning(config.ips) - broadcast_ip_address_warning(config.ips) no_matching_interface_for_ip_address_warning(config.ips) if options.setup_adtrust: @@ -1285,8 +1279,6 @@ def promote_check(installer): False, options.ip_addresses) # check addresses here, dns module is doing own check - network_ip_address_warning(config.ips) - broadcast_ip_address_warning(config.ips) no_matching_interface_for_ip_address_warning(config.ips) if options.setup_adtrust:
_______________________________________________ FreeIPA-devel mailing list -- freeipa-devel@lists.fedorahosted.org To unsubscribe send an email to freeipa-devel-le...@lists.fedorahosted.org
