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

[Martin Kosek]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mkosek-120-5-improve-dns-record-data-validation.patch
Type: text/x-patch
Size: 91294 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20111109/cfae3acb/attachment.bin>