Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dns resolver for nameservers with ports #6408

Conversation

t-woerner
Copy link
Member

DNSResolver: Fix use of nameservers with ports

IPA DNS zone and forwardzone commands allow to use nameservers with ports
as "SERVER_IP port PORT_NUMBER". bind is supporting this syntax, but the
Resolver in dnspython that is used to verify the list of forwarders
(nameservers) is only allowing to have IP addresses in this list. With
dnspython version 2.20 there is a new validator in dns.resolver.BaseResolver
that ensures this.

Refs:

ipapython/dnsutil.DNSResolver derives from dns.resolver.Resolver. The setter
for nameservers has been overloaded in the DNSResolver class to split out the
port numbers into the nameserver_ports dict { SERVER_IP: PORT_NUMBER }.
After the setter for nameservers succeeded, nameserver_ports is set.

Additional tests have been added to verify that nameservers and also
nameserver_ports are properly set and also valid.

Fixes: https://pagure.io/freeipa/issue/9158

@t-woerner t-woerner force-pushed the fix_dns_resolver_for_nameservers_with_ports branch from 2e67c98 to 5314ece Compare August 11, 2022 11:19
IPA DNS zone and forwardzone commands allow to use nameservers with ports
as "SERVER_IP port PORT_NUMBER". bind is supporting this syntax, but the
Resolver in dnspython that is used to verify the list of forwarders
(nameservers) is only allowing to have IP addresses in this list. With
dnspython version 2.20 there is a new validator in dns.resolver.BaseResolver
that ensures this.

Refs:
- https://bind9.readthedocs.io/en/v9_18_4/reference.html#zone-statement-grammar
- https://github.com/rthalley/dnspython/blob/master/dns/resolver.py#L1094

ipapython/dnsutil.DNSResolver derives from dns.resolver.Resolver. The setter
for nameservers has been overloaded in the DNSResolver class to split out
the port numbers into the nameserver_ports dict { SERVER_IP: PORT_NUMBER }.
After the setter for nameservers succeeded, nameserver_ports is set.
nameserver_ports is used in the resolve() method of dns.resolver.Resolver.

Additional tests have been added to verify that nameservers and also
nameserver_ports are properly set and also valid.

Fixes: https://pagure.io/freeipa/issue/9158

Signed-off-by: Thomas Woerner <twoerner@redhat.com>
@abbra
Copy link
Contributor

abbra commented Aug 15, 2022

LGTM. Thanks for extensive comments -- when I reviewed this PR last week, it was harder to find why nameserver_ports was modified but not used. Now it is much clearer.

Please remove the temp commit and we can ack this PR.

@abbra abbra added ipa-4-9 Mark for backport to ipa 4.9 ipa-4-10 Mark for backport to ipa 4.10 labels Aug 15, 2022
@t-woerner t-woerner force-pushed the fix_dns_resolver_for_nameservers_with_ports branch from b600251 to bb2161f Compare August 15, 2022 08:41
@t-woerner
Copy link
Member Author

t-woerner commented Aug 15, 2022

You are welcome.

Please remove the temp commit and we can ack this PR.

I have removed the Temp commit.

@abbra abbra added the ack Pull Request approved, can be merged label Aug 16, 2022
@flo-renaud flo-renaud added the pushed Pull Request has already been pushed label Aug 16, 2022
@flo-renaud
Copy link
Contributor

master:

  • 7780358 DNSResolver: Fix use of nameservers with ports

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged ipa-4-9 Mark for backport to ipa 4.9 ipa-4-10 Mark for backport to ipa 4.10 pushed Pull Request has already been pushed
Projects
None yet
3 participants