[Freeipa-devel] [PATCH] 19 Do stricter checking of IP addressed passed to server install

Mon May 30 11:44:40 UTC 2011

On Fri, 2011-05-27 at 16:50 +0200, Jan Cholasta wrote:
> On 25.5.2011 09:46, Martin Kosek wrote:
> > On Tue, 2011-05-24 at 15:42 +0200, Jan Cholasta wrote:
> >> On 24.5.2011 14:44, Jan Cholasta wrote:
> >>> On 24.5.2011 14:43, Martin Kosek wrote:
> >>>> On Fri, 2011-05-20 at 20:34 +0200, Jan Cholasta wrote:
> >>>>> On 18.5.2011 10:51, Martin Kosek wrote:
> >>>>>> On Mon, 2011-05-16 at 19:15 +0200, Jan Cholasta wrote:
> >>>>>>> On 16.5.2011 17:26, Martin Kosek wrote:
> >>>>>>>> On Tue, 2011-05-10 at 20:11 +0200, Jan Cholasta wrote:
> >>>>>>>>> Split from patch 3, requires patch 18.
> >>>>>>>>>
> >>>>>>>>> https://fedorahosted.org/freeipa/ticket/1213
> >>>>>>>>>
> >>>>>>>>> Honza
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> I tested all patches (3.6, 18, 19), but I think some work still
> >>>>>>>> needs to
> >>>>>>>> be done:
> >>>>>>>>
> >>>>>>>> 1) What about adding /sbin/ip package to Requires in spec? I thought
> >>>>>>>> there was an agreement to do it.
> >>>>>>>
> >>>>>>> Will do.
> >>>>>>
> >>>>>> Ok.
> >>>>>>
> >>>>>>>
> >>>>>>>>
> >>>>>>>> 2) When I run `ipa-server-install --ip-address=$ADDR`, and $ADDR is
> >>>>>>>> invalid address (e.g. $ADDR==foo), loopback address (e.g.
> >>>>>>>> $ADDR==127.0.0.1) or just another that the local address (e.g.
> >>>>>>>> $ADDR==123.123.123.123) the installer always fails with "the hostname
> >>>>>>>> resolves to an IP address that is different from the one provided
> >>>>>>>> on the
> >>>>>>>> command line".
> >>>>>>>>
> >>>>>>>> I think we may want a different error message in those 3 cases - it
> >>>>>>>> should be easy to do it now, with the improved IP handling.
> >>>>>>>
> >>>>>>> It looks like the print statements from verify_ip_address doesn't
> >>>>>>> actually print anything to the user. Will look onto that.
> >>>>>>
> >>>>>> Ok.
> >>>>>>
> >>>>>>>
> >>>>>>>>
> >>>>>>>> 3) When I pass netmask to ipa-server-install --ip-address=$ADDR, the
> >>>>>>>> installation always fails with the above message. Even though I
> >>>>>>>> took the
> >>>>>>>> addr+netmask from "/sbin/ip address" output.
> >>>>>>>
> >>>>>>> Works for me. Please make sure you've added your hostname to
> >>>>>>> /etc/hosts.
> >>>>>>
> >>>>>> I think I had. But I will recheck when you send a fix.
> >>>>>>
> >>>>>>>
> >>>>>>>>
> >>>>>>>> 4) I miss IP address checks in --ip-address and --forwarder
> >>>>>>>> parameters
> >>>>>>>> of ipa-dns-install script. I can pass invalid or local addresses to
> >>>>>>>> these parameters. This breaks Bind configuration.
> >>>>>>>
> >>>>>>> --ip-address is checked, but --forwarder is not. Will fix that.
> >>>>>>
> >>>>>> Ok, I will recheck both of them when you do.
> >>>>>>
> >>>>>>>
> >>>>>>>>
> >>>>>>>> 5) I think we may want to check also for local address in
> >>>>>>>> #ipa host-add $HOST --ip-address=127.0.0.1
> >>>>>>>>
> >>>>>>>> 6) I couldn't add IP address with netmask in host module:
> >>>>>>>> # ipa host-add $HOST --ip-address=10.16.78.102/22
> >>>>>>>> ipa: ERROR: invalid 'ip_address': invalid IP address
> >>>>>>>
> >>>>>>> The patches are for the installer, as are the tickets they fix, so
> >>>>>>> these
> >>>>>>> issues are out of scope. A new ticket should be opened for them.
> >>>>>>>
> >>>>>>
> >>>>>> You touched this parameter in your patches, that's why I tested it. I
> >>>>>> created a new ticket for it:
> >>>>>>
> >>>>>> https://fedorahosted.org/freeipa/ticket/1234
> >>>>>>
> >>>>>> Ticket 1234, yey :-)
> >>>>>>
> >>>>>>>>
> >>>>>>>> 7) Why is the _ParsedIPAddress named with a leading underscore?
> >>>>>>>> It's not
> >>>>>>>> really an internal use since it is returned by new IP handling
> >>>>>>>> functions
> >>>>>>>> and used in other modules.
> >>>>>>>
> >>>>>>> _ParsedIPAddress is not for public use. The fact that object of this
> >>>>>>> class is returned by parse_ip_address doesn't really matter - this is
> >>>>>>> Python, not C++ or Java.
> >>>>>>
> >>>>>> Hm, snappy... And I was wondering why my /usr/bin/java doesn't want to
> >>>>>> run FreeIPA, now I know - it's because its Python.
> >>>>>>
> >>>>>> Martin
> >>>>>>
> >>>>>
> >>>>> Patch updated. Requires patch 18.1
> >>>>>
> >>>>> Honza
> >>>>>
> >>>>
> >>>> All reported issues were fixed, good idea with a new type for our
> >>>> IPAOptionParser.
> >>>>
> >>>> Still, NACK from me:
> >>>>
> >>>> ipa-replica-install doesn't use IPAOptionParser, but the good old
> >>>> OptionParser which doesn't know the new type. This makes
> >>>> ipa-replica-prepare crash all the time. I know, I am nitpicker :-)
> >>>>
> >>>> Martin
> >>>>
> >>>
> >>> Thanks, I missed that.
> >>>
> >>> Honza
> >>>
> >>
> >> Fixed and added a unit test.
> >>
> >
> > NACK. Please test your patches before you send them for a review. It
> > saves reviewer's time.
> 
> Sorry, I'll do better next time.
> 
> >
> > 1) Unwanted warning about unmatching network interface when replica is
> > installed:
> >
> > # ipa-replica-prepare vm-059.idm.lab.bos.redhat.com
> > --ip-address=10.16.78.59
> > Warning: No network interface matches IP address 10.16.78.59
> > Directory Manager (existing master) password:
> > ...
> 
> Fixed.
> 
> >
> > 2) ipa-replica-install crashes
> > # ipa-replica-install /home/mkosek/replica-info-vm-059.idm.lab.bos.redhat.com.gpg
> > Directory Manager (existing master) password:
> >
> > Configuring ntpd
> >    [1/4]: stopping ntpd
> >    [2/4]: writing configuration
> >    [3/4]: configuring ntpd to start on boot
> >    [4/4]: starting ntpd
> > done configuring ntpd.
> > creation of replica failed: unsupported operand type(s) for /: 'NoneType' and 'int'
> >
> > Your system may be partly configured.
> > Run /usr/sbin/ipa-server-install --uninstall to clean up.
> >
> >
> > ipa-replica-install log:
> > 2011-05-25 03:36:18,503 DEBUG unsupported operand type(s) for /: 'NoneType' and 'int'
> >    File "/usr/sbin/ipa-replica-install", line 550, in<module>
> >      main()
> >
> >    File "/usr/sbin/ipa-replica-install", line 496, in main
> >      install_dns_records(config, options)
> >
> >    File "/usr/sbin/ipa-replica-install", line 329, in install_dns_records
> >      options.conf_ntp)
> >
> >    File "/usr/lib/python2.7/site-packages/ipaserver/install/bindinstance.py", line 469, in add_master_dns_records
> >      self.__add_self()
> >
> >    File "/usr/lib/python2.7/site-packages/ipaserver/install/bindinstance.py", line 399, in __add_self
> >      if dns_zone_exists(get_reverse_zone(self.ip_address, self.ip_prefix_len)[0]):
> >
> >    File "/usr/lib/python2.7/site-packages/ipaserver/install/bindinstance.py", line 106, in get_reverse_zone
> >      pos = 4 - ip_prefix_len / 8
> 
> Also fixed.
> 
> >
> >
> > Martin
> >
> 
> Honza
> 

ACK, pushed to master.

Martin