[Freeipa-devel] [PATCH] 120 Improve DNS record data validation

[Rob Crittenden]

Martin Kosek wrote:
> On Fri, 2011-11-04 at 16:53 -0400, Rob Crittenden wrote:
>> Martin Kosek wrote:
>>> On Wed, 2011-10-19 at 15:38 -0400, Adam Young wrote:
>>>> On 10/19/2011 08:15 AM, Martin Kosek wrote:
>>>>> On Wed, 2011-09-07 at 15:18 +0200, Martin Kosek wrote:
>>>>>> On Wed, 2011-09-07 at 15:05 +0200, Martin Kosek wrote:
>>>>>>> This is 3.0 Core Effort Backlog patch.
>>>>>>>
>>>>>>> The changes to API may look scary, but it should be OK, I just added
>>>>>>> validators and normalizers. I found a lot of RR types unsupported by
>>>>>>> bind-dyndb-ldap. I implemented a validator telling this information to
>>>>>>> the user. I think the message is more user-friendly than the previous
>>>>>>> LDAP schema error.
>>>>>>>
>>>>>>> Enjoy the RFCs! :-)
>>>>>>>
>>>>>>> Martin
>>>>>>>
>>>>>>> ---
>>>>>>> Implement missing validators for DNS RR types so that we can capture
>>>>>>> at least basic user errors. Additionally, a normalizer creating
>>>>>>> a fully-qualified domain name has been implemented for several RRs
>>>>>>> to prevent this common user error.
>>>>>>>
>>>>>>> https://fedorahosted.org/freeipa/ticket/1106
>>>>>>>
>>>>>> I noticed a typo in format description for LOC record validation. A
>>>>>> fixed patch attached.
>>>>>>
>>>>>> Martin
>>>>> Rebased for current master.
>>>>>
>>>>> This patch is still waiting for review. As I would like to base my next
>>>>> DNS work (structured DNS commands) on this patch I would like to have it
>>>>> reviewed soon.
>>>>>
>>>>> Thanks,
>>>>> Martin
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> Freeipa-devel mailing list
>>>>> Freeipa-devel at redhat.com
>>>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>>>>
>>>>
>>>> I've just given it a visual review, but it looks right.  Probably
>>>> should have some unit tests to go with it for some of the more
>>>> commonly used types.
>>>
>>> Good idea. A, AAAA, NS records are already being checked, I added tests
>>> for MX and SRV records too.
>>>
>>> I also refactored DNS tests a little, there were many repeatedly using
>>> hard-coded values (like default zone manager) which would be hard to fix
>>> of anything changes.
>>>
>>> Martin
>>
>> I can't tell what your intention is with the split for cname and dname
>> but it seems to allow just about any value.
>>
>> I know there are a ton of data types but is it worthwhile to have a
>> positive and negative case for each to avoid regressions?
>>
>> rob
>
> I just wanted to make sure that user enters one value and not more. This
> effectively tests if there is no whitespace in the value, but you are
> right that this is a silly way of testing it.
>
> I rather implemented a hostname validator to common ipalib module
> util.py following the RFCs for hostname. It is intended to replace
> various hostname validators we use across IPA code. But I will leave
> this step for future. For now, I just replaced the dumb validator with
> split() for CNAME, DNAME and PTR validators to this new function which
> actually validates the hostname value.
>
> As you advised, I implemented some more unit tests with positive,
> negative cases. Now, we should have most of the common types covered (A,
> AAAA, NS, MX, SRV, LOC, CNAME, KX, PTR). I did not implement tests for
> DNSSEC RR types as I expect some changes there in the future.
>
> Martin
>

ACK, pushed to master.

rob