[Freeipa-devel] [PATCH] 302 Stricter IP network validator in dnszone-add command

[Martin Kosek]

On 09/19/2012 05:30 PM, Rob Crittenden wrote:
> Martin Kosek wrote:
>> On 09/17/2012 09:35 PM, Rob Crittenden wrote:
>>> Martin Kosek wrote:
>>>> On 09/05/2012 01:02 PM, Jan Cholasta wrote:
>>>>> Dne 5.9.2012 12:48, Martin Kosek napsal(a):
>>>>>> On 09/05/2012 12:36 PM, Jan Cholasta wrote:
>>>>>>> Dne 5.9.2012 12:22, Petr Spacek napsal(a):
>>>>>>>> On 09/05/2012 11:30 AM, Jan Cholasta wrote:
>>>>>>>>> Dne 5.9.2012 10:04, Martin Kosek napsal(a):
>>>>>>>>>> We allowed IP addresses without network specification which lead
>>>>>>>>>> to unexpected results when the zone was being created. We should rather
>>>>>>>>>> strictly require the prefix/netmask specifying the IP network that
>>>>>>>>>> the reverse zone should be created for. This is already done in
>>>>>>>>>> Web UI.
>>>>>>>>>>
>>>>>>>>>> A unit test exercising this new validation was added.
>>>>>>>>>>
>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/2461
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I don't like this much. I would suggest using CheckedIPAddress and not
>>>>>>>>> forcing
>>>>>>>>> the user to enter the prefix length instead.
>>>>>>>>>
>>>>>>>>> CheckedIPAddress uses a sensible default prefix length if one is not
>>>>>>>>> specified
>>>>>>>>> (class-based for IPv4, /64 for IPv6) as opposed to IPNetwork (/32 for
>>>>>>>>> IPv4,
>>>>>>>>> /128 for IPv6 - this causes the erroneous reverse zones to be created as
>>>>>>>>> described in the ticket).
>>>>>>>>>
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> I don't like automatic netmask guessing. I have met class-based guessing
>>>>>>>> in Windows (XP?) and I was forced to overwrite default mask all the time
>>>>>>>> ...
>>>>>>>
>>>>>>> If there was no guessing, you would have to write the netmask anyway, so I
>>>>>>> don't see any harm in guessing here.
>>>>>>>
>>>>>>>>
>>>>>>>> IMHO there is no "sensible default prefix" in real world. I sitting on
>>>>>>>> network with /23 prefix right now. Also, I have never seen 10.x network
>>>>>>>> with /8 prefix.
>>>>>>>>
>>>>>>>
>>>>>>> While this might be true for IPv4 in some cases, /64 is perfectly sensible
>>>>>>> for
>>>>>>> IPv6. Also, I have never seen 192.168.x.x network with non-/24 prefix.
>>>>>>>
>>>>>>> Honza
>>>>>>>
>>>>>>
>>>>>> While this may be true for 192.168.x.x, it does not apply for 10.x.x.x
>>>>>> networks
>>>>>> as Petr already pointed out. I don't think that there will be many people
>>>>>> expecting that a reverse zone of 10.0.0.0/24 would be created.
>>>>>
>>>>> And they would be correct, because the default prefix length for a class A
>>>>> network is /8, not /24.
>>>>>
>>>>>>
>>>>>> And since FreeIPA is mainly deployed to internal networks, I assume this
>>>>>> will
>>>>>> be the case of most users.
>>>>>>
>>>>>> Martin
>>>>>>
>>>>>
>>>>> OK, but what about IPv6? Correct me if I'm wrong, but the prefix length is
>>>>> going to be /64 99% of the time for IPv6.
>>>>>
>>>>> The installer uses /24 for IPv4 addresses and /64 for IPv6 addresses, maybe
>>>>> this should be used as a default here as well.
>>>>>
>>>>> Honza
>>>>>
>>>>
>>>> In the end, I choose a more liberal approach and instead of defining a more
>>>> stricter validator for IPv4 only I rather used approach already implemented in
>>>> the installers, i.e. default length of network prefix is 24 for IPv4 and 64
>>>> for
>>>> IPv6.
>>>>
>>>> Updated patch attached.
>>>>
>>>> Martin
>>>
>>> Works for me. I wonder if this is a candidate for some more unit tests...
>>>
>>> rob
>>>
>>
>> One more test should not hurt. Updated patch attached.
>>
>> Martin
>>
> 
> ACK

Pushed to master, ipa-3-0.

Martin