URL: https://github.com/freeipa/freeipa/pull/881 Author: MartinBasti Title: #881: [4.5] fix ip address checks Action: opened
PR body: """ """ To pull the PR as Git branch: git remote add ghfreeipa https://github.com/freeipa/freeipa git fetch ghfreeipa pull/881/head:pr881 git checkout pr881
From 110b8c09454da75043948952cb0cc48f4756d360 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 3eb681b61cf51ab707db42f8ed99bfe34a0320c4 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 e07e6664308a198064f0e16c1c8c135c3e9caa4f 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 29f8202b44ac1f72968e5cde24dbb9e81e0731de 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 | 3 +-- 6 files changed, 8 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 3521d55591..01930c4de6 100644 --- a/ipaserver/install/installutils.py +++ b/ipaserver/install/installutils.py @@ -585,7 +585,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 f0e6c48f06..f01baf5155 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 6427935b16..9c351bd0ed 100644 --- a/ipatests/test_ipapython/test_ipautil.py +++ b/ipatests/test_ipapython/test_ipautil.py @@ -30,11 +30,10 @@ pytestmark = pytest.mark.tier0 - def make_ipaddress_checker(addr, words=None, prefixlen=None): def check_ipaddress(): try: - ip = ipautil.CheckedIPAddress(addr, match_local=False) + ip = ipautil.CheckedIPAddress(addr) assert ip.words == words and ip.prefixlen == prefixlen except Exception: assert words is None and prefixlen is None From bfe3ed4b8f1c1fbec935f8336b8d6b218ddc3203 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 892e9e8a80b462ca7216bfa5bf007fe953558ba4 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 ab3777a6f14fdd1a18cc193f11ce2c2c3fdf9d7d 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 b0de596bbf..c88061320c 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
