[Freeipa-devel] [PATCH] 0006 Remove check for alphabetic only characters from domain name validation

Fri Mar 15 11:54:55 UTC 2013

On 03/15/2013 12:11 PM, Martin Kosek wrote:
> On 03/15/2013 12:08 PM, Petr Spacek wrote:
>> On 14.3.2013 15:08, Martin Kosek wrote:
>>> On 03/11/2013 01:02 PM, Ana Krivokapic wrote:
>>>> On 02/27/2013 10:58 AM, Martin Kosek wrote:
>>>>> On 02/22/2013 04:02 PM, Ana Krivokapic wrote:
>>>>>> On 02/22/2013 10:19 AM, Petr Spacek wrote:
>>>>>>> On 20.2.2013 11:03, Ana Krivokapic wrote:
>>>>>>>> On 02/18/2013 01:08 PM, Martin Kosek wrote:
>>>>>>>>> On 02/18/2013 12:47 PM, Sumit Bose wrote:
>>>>>>>>>> On Mon, Feb 18, 2013 at 12:27:35PM +0100, Petr Spacek wrote:
>>>>>>>>>>> On 15.2.2013 15:22, Ana Krivokapic wrote:
>>>>>>>>>>>> Hello,
>>>>>>>>>>>>
>>>>>>>>>>>> The .isalpha() check in validate_domain_name() was too strict,
>>>>>>>>>>>> causing some commands like ipa dnsrecord-add to fail.
>>>>>>>>>>>>
>>>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/3385
>>>>>>>>>>> I would add --force option rather than removing whole check, if
>>>>>>>>>>> it's possible.
>>>>>>>>>>>
>>>>>>>>>>> Would it be possible to mention RFC in the error message? Something
>>>>>>>>>>> like _('top level domain label must be alphabetic (RFC 1123 section
>>>>>>>>>>> 2.1)')
>>>>>>>>>>> ?
>>>>>>>>>>>
>>>>>>>>>>> IMHO it is handy, because it educates users.
>>>>>>>>>> The problem is that this check is always done on the last component of
>>>>>>>>>> the domain_name even if it is just a sub-domain of the FreeIPA domain,
>>>>>>>>>> where e.g. numbers are valid characters.
>>>>>>>>>>
>>>>>>>>>> At the beginning of validate_domain_name() a trailing '.' is stripped
>>>>>>>>>> away. iirc the trailing '.' is an indication for a complete, fully
>>>>>>>>>> qualified name. Would it work if the presence of the trailing '.' is
>>>>>>>>>> saved and the check is only done if there was a '.'?
>>>>>>>>>>
>>>>>>>>>> bye,
>>>>>>>>>> Sumit
>>>>>>>>>>
>>>>>>>>> Sure. Though I am now not 100% sure that some IPA functions do not
>>>>>>>>> use this
>>>>>>>>> validator with a fqdn hostname without trailing dot. If not, I am
>>>>>>>>> for fixing
>>>>>>>>> this function as Sumit and Petr suggested.
>>>>>>>>>
>>>>>>>>> Martin
>>>>>>>>>
>>>>>>>>> _______________________________________________
>>>>>>>>> Freeipa-devel mailing list
>>>>>>>>> Freeipa-devel at redhat.com
>>>>>>>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>>>>>>>> After some thought, I decided to change the approach.
>>>>>>>>
>>>>>>>> As pointed out by Sumit, the problem was that the validate_domain_name()
>>>>>>>> function did not distinguish between fqdn and non-fqdn domains
>>>>>>>> (subdomains of the IPA domain). The trailing dot is not a clear
>>>>>>>> indication either, because some IPA functions use this validator with an
>>>>>>>> fqdn without the trailing dot.
>>>>>>>>
>>>>>>>> To fix this, I introduced an additional parameter to this function - a
>>>>>>>> flag which indicates whether the domain name is an fqdn or not. The is
>>>>>>>> .isalpha() check is then performed only in the case of an fqdn.
>>>>>>>>
>>>>>>>> I also improved the error message to mention the relevant RFC, as
>>>>>>>> suggested by Petr.
>>>>>>> Please don't forget to add --force switch. It could be handy.
>>>>>>>
>>>>>> I added the --force switch to ipa dnsrecord-add and opened a new ticket
>>>>>> to handle the rest of the ipa commands that use domain name validation:
>>>>>> https://fedorahosted.org/freeipa/ticket/3455
>>>>>>
>>>>>> Updated patch is attached.
>>>>>>
>>>>> This patch fixed validation only partially. The --force flag you made
>>>>> available
>>>>> will not allow admin to for example add a zone "example.zone1" which
>>>>> technically will be resolvable, it is just not a good practice:
>>>>>
>>>>> # ipa dnszone-add example.zone1 --name-server `hostname`. --force
>>>>> ipa: ERROR: invalid 'name': top level domain label must be alphabetic (RFC
>>>>> 1123
>>>>> section 2.1)
>>>>>
>>>>> To enable this, I think you would need to not postpone the validation to DNS
>>>>> zone pre_callback as you could not check --force flag presence right in the
>>>>> idnsname parameter validator.
>>>>>
>>>>> We may also want to change --force flag label, it now talks only about NS
>>>>> record validation, but we now expanded it a bit, so the label would need
>>>>> to be
>>>>> more general.
>>>>>
>>>>> Martin
>>>>
>>>> I added the fix for dnszone-add and edited the label of the --force flag
>>>> to make it more general.
>>>>
>>>
>>> The zone with this name can be created even without the --force flag.
>>>
>>> # ipa dnszone-add example.zone1 --name-server `hostname`.
>>> Administrator e-mail address [hostmaster.example.zone1.]:
>>>>>> Administrator e-mail address: top level domain label must be alphabetic
>>> (RFC 1123 section 2.1)
>>> Administrator e-mail address [hostmaster.example.zone1.]:
>>> hostmaster.example.zone.
>>>    Zone name: example.zone1
>>>    Authoritative nameserver: vm-037.idm.lab.bos.redhat.com.
>>>    Administrator e-mail address: hostmaster.example.zone.
>>>    SOA serial: 1363268183
>>>    SOA refresh: 3600
>>>    SOA retry: 900
>>>    SOA expire: 1209600
>>>    SOA minimum: 3600
>>>    BIND update policy: grant IDM.LAB.BOS.REDHAT.COM krb5-self * A; grant
>>> IDM.LAB.BOS.REDHAT.COM krb5-self
>>>                        * AAAA; grant IDM.LAB.BOS.REDHAT.COM krb5-self * SSHFP;
>>>    Active zone: TRUE
>>>    Dynamic update: FALSE
>>>    Allow query: any;
>>>    Allow transfer: none;
>>>
>>> I thought that the check would only be surpassed with the --force flag.
>>>
>>> All this struggle with this patch makes me thinking if we are not making it too
>>> complicated. I am rather inclined to drop this particular check altogether. I
>>> do not see that much harm if user creates an IPA managed zone "example.zone1".
>>> Petr, what is your opinion on dropping this check in IPA? Is it OK to drop it?
>>
>> IMHO we can drop the whole check, it should be safe. I did some basic tests
>> with latest bind-dyndb-ldap and it worked for me.
>>
>
> Ok, I agree. Ana, can you please send a revised patch which rather drops this
> check altogether?
>
> Thanks,
> Martin
>

As Ana pointed out on IRC, this is actually what her first revision did.

ACK for the first incarnation of the patch, pushed to master, ipa-3-1.

Thanks,
Martin