Skip to content

ipaddress IPv4Network and IPv6Network need specialized reverse_pointer property #125641

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

Open
pemensik opened this issue Oct 17, 2024 · 12 comments
Open
Assignees
Labels
3.12 only security fixes 3.13 bugs and security fixes 3.14 new features, bugs and security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@pemensik
Copy link

pemensik commented Oct 17, 2024

Bug report

Bug description:

# Add a code block here, if required
from ipaddress import ip_network

# Generates now:
# '8.4./.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa'
assert(ip_network("2001:db8::/48").reverse_pointer == ['0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa'])

# Generates now:
# '0/29.2.0.192.in-addr.arpa'
assert(ip_network("192.0.2.0/29").reverse_pointer == [
'0.2.0.192.in-addr.arpa', '1.2.0.192.in-addr.arpa', '2.2.0.192.in-addr.arpa', '3.2.0.192.in-addr.arpa', '4.2.0.192.in-addr.arpa',
'5.2.0.192.in-addr.arpa', '6.2.0.192.in-addr.arpa', '7.2.0.192.in-addr.arpa'])

IPv4Network and IPv6Network get their implementation from _BaseAddress. But for network, it does not work as it should. It returns broken nonsense instead of helping data. Problem is unlike normal IP address, for a network range, it may return just single string for prefix lengths divisible by 8 for IPv4 and 4 for IPv6. But for other prefixes, it should return a list of domain names used.

I have implemented working generator for a list result at InfrastructureServices/dnsconfd#70
I think something similar should be used in base ipaddress directly. If reverse_pointer should not be fixed, it should be removed from networks instead.

Related to #123409, but that is not exactly about networks. Can be verified a bit using ipcalc --reverse-dns, but even that crashes on undivisible ipv6 prefixes.

CPython versions tested on:

3.13

Operating systems tested on:

Linux

@pemensik pemensik added the type-bug An unexpected behavior, bug, or error label Oct 17, 2024
@Zheaoli
Copy link
Contributor

Zheaoli commented Oct 17, 2024

Seems, we need to get more detail about the reverse pointer from the RFC.

@picnixz plz assign this issue to me, I will take handle of it .

@picnixz
Copy link
Member

picnixz commented Oct 17, 2024

You can take care of the issue, I'm not here for the next days. I didn't know that the reverse pointer for network was different (clearly not my field of research) so I'm happy if you can fix the bug!

@pemensik
Copy link
Author

There is no such thing as a reverse pointer for networks in dns itself. But it is related to how those domains can be delegated. Meaning where RFC 1035 can cut between zones operated by someone else. rfc2317 specifies recommended classless delegation using DNAME or CNAMEs in DNS.

Classless in-addr.arpa delegation describes how to delegate 1.0/25.2.0.192, therefore 192.0.2.0/25 network. Example from that page.

   $ORIGIN 2.0.192.in-addr.arpa.
   @       IN      SOA     my-ns.my.domain. hostmaster.my.domain. (...)
   ;...
   ;  <<0-127>> /25
   0/25            NS      ns.A.domain.
   0/25            NS      some.other.name.server.
   ;
   1               CNAME   1.0/25.2.0.192.in-addr.arpa.
   2               CNAME   2.0/25.2.0.192.in-addr.arpa.
   3               CNAME   3.0/25.2.0.192.in-addr.arpa.

This means that in 2.0.192.in-addr.arpa. zone, one needs to send ['0.2.0.192.in-addr.arpa.', '1.2.0.192.in-addr.arpa.', ... '127.2.0.192.in-addr.arpa.'] domains to some other domain. Those are the same, what I want generated by my proposal of . Special name 0/25.2.0.192.in-addr.arpa. is done to be able to delegate part of 2.0.192.in-addr.arpa. domain to DNS server operated by someone else. In that names 1.0/25.2.0.192.in-addr.arpa. to 127.0/25.2.0.192.in-addr.arpa. can be administered by different entity than original zone 2.0.192.in-addr.arpa..

There is no similar proposal for IPv6, because common IPv6 prefixes assigned to people are divided by 4 with zero remainder. That is /48, /52, /56, /60, /64. But prefix like /54 is still valid, but needs multiple domains to hold reverse addresses contained in that prefix.

@ZeroIntensity ZeroIntensity added stdlib Python modules in the Lib dir 3.12 only security fixes 3.13 bugs and security fixes 3.14 new features, bugs and security fixes labels Oct 17, 2024
@ZeroIntensity
Copy link
Member

This is affecting all bugfix+ versions (3.12+), right?

@Zheaoli
Copy link
Contributor

Zheaoli commented Oct 17, 2024

I think we just need to backport to 3.13 if this is necessary.

@Zheaoli
Copy link
Contributor

Zheaoli commented Oct 30, 2024

I might want to ask some questions about this issue

in your test code

# '8.4./.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa'
assert(ip_network("2001:db8::/48").reverse_pointer == ['0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa'])

I'm not sure the 0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa behavior is defined in the RFC. So I would like to hear about your thought about IPV6 part.

@pemensik
Copy link
Author

pemensik commented Nov 1, 2024

When I use ipcalc 2001:db8::/48, I get these ranges.

HostMin:	2001:db8::
HostMax:	2001:db8:0:ffff:ffff:ffff:ffff:ffff

Edit: the code is correct, '0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa' should be correct prefix common for all those reversed domains contained in that range. network_address and broadcast_address properties of _BaseNetwork should contain similar values.

>>> import ipaddress
>>> n = ipaddress.ip_network("2001:db8::/48")
>>> n.broadcast_address
IPv6Address('2001:db8:0:ffff:ffff:ffff:ffff:ffff')
>>> n.broadcast_address.exploded
'2001:0db8:0000:ffff:ffff:ffff:ffff:ffff'
>>> n.network_address.exploded
'2001:0db8:0000:0000:0000:0000:0000:0000'
>>> n.network_address.exploded.replace(':','')[:n.prefixlen//4][::-1]
'00008bd01002'
>>> '.'.join(n.network_address.exploded.replace(':','')[:n.prefixlen//4][::-1])
'0.0.0.0.8.b.d.0.1.0.0.2'

Because prefix is divisible by 4, just a single domain is enough to hold all possible addresses. Something like that was merged into our dnsconfd.

@pemensik
Copy link
Author

pemensik commented Nov 1, 2024

In case prefix is not divisible by 4, subnets contained in parent divisible by 4 need to be generated. In fact network_address.reverse_pointer can be reused and just proper number of label splitted from it. Our example is at:

https://github.com/InfrastructureServices/dnsconfd/blob/761c5fd37bd0d0b163a21fb6cabed14142004fcf/dnsconfd/network_objects/server_description.py#L190

cboltz pushed a commit to openSUSE/heroes-salt that referenced this issue Dec 1, 2024
The reverse_pointer functionality built into the ipaddress library
yields undesired leading zeroes if an IPv6 network is passed.

As this function is currently primarily utilized in the generation of our
configuration for DNS64, it causes reverse lookups to fail, as addresses
cannot be determined to belong to the given reverse domain.
Prior to implementing this functionality the issue was not present, as
the values were manually taken from `ipcalc`, which behaves as expected.

Good:
In  2a07:de40:b27e:64::/96
Out 0.0.0.0.0.0.0.0.4.6.0.0.e.7.2.b.0.4.e.d.7.0.a.2.ip6.arpa.

Bad:
In  2a07:de40:b27e:64::/96
Out 0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.4.6.0.0.e.7.2.b.0.4.e.d.7.0.a.2.ip6.arpa.

Correct the logic by replacing it with a contraption found in the related
upstream issue (thank you, @pemensik !):
python/cpython#125641

Fixes: 94220a9 ("Modularize PowerDNS recursor NAT64 configuration")
Fixes: a597f39 ("Add reverse_pointer function")
Signed-off-by: Georg Pfuetzenreuter <georg.pfuetzenreuter@suse.com>
@Forst
Copy link

Forst commented Feb 11, 2025

There's already a couple of issues with associated pull requests open for the same problem:

The first issue in particular has some more ideas and discussion on the matter, including a few other options on how to treat reverse_pointer for network objects:

  • Generate a single, more general reverse pointer (e.g. 192.0.2.128/25 -> 2.0.192.in-addr.arpa.)
  • Raise an exception when generating reverse pointers for networks that don't fall on the DNS record granularity boundary (8 bits for IPv4, 4 bits for IPv6)
  • Remove reverse_pointer for network objects altogether

The suggestion in this issue to return a list of one or more pointers is interesting, but a bit tricky in my opinion. For one, it may result in confusion where the address objects return a single string, but networks return a list for the same attribute. Also, the name reverse_pointer suggests it will return one pointer of some sort, rather than a list.

RFC 2317 mentioned above only covers prefixes larger than /24 in IPv4. It covers neither smaller prefixes nor IPv6, and that's something we should also think about.

@picnixz
Copy link
Member

picnixz commented Mar 2, 2025

@Zheaoli are you still working on this?

@pemensik
Copy link
Author

pemensik commented Mar 3, 2025

The first issue in particular has some more ideas and discussion on the matter, including a few other options on how to treat reverse_pointer for network objects:

* Generate a single, more general reverse pointer (e.g. `192.0.2.128/25` -> `2.0.192.in-addr.arpa.`)

Nope, that would be wrong. There might be a helper to this, but the given reverse pointer is for 192.0.2.0/24 network, not for the entry specified. Returning partially invalid data is not a good idea, especially if that desire were not explicitly requested. It may work for something until it creates visible errors. Wrong way IMO.

* Raise an exception when generating reverse pointers for networks that don't fall on the DNS record granularity boundary (8 bits for IPv4, 4 bits for IPv6)

And when one does have networks with non-clean divisible block, he should not be able to work with them, just because we may need change return type? Why? Is there anything wrong with prefixes not divisible by 8 or 4?

* Remove `reverse_pointer` for network objects altogether

Well, it it would mean we rename it to reverse_pointers instead, okay. After all, network is a group of addresses and that it needs a group of names for them, it should not be surprising. But just removing without any replacement, that is a bad idea.

The suggestion in this issue to return a list of one or more pointers is interesting, but a bit tricky in my opinion. For one, it may result in confusion where the address objects return a single string, but networks return a list for the same attribute.

Network is a group of address. Unlike single address it represents multiple of addresses. Well then multiple of reverse pointers should be documented, but not considered surprising.

Also, the name reverse_pointer suggests it will return one pointer of some sort, rather than a list.

I would be okay if reverse_pointer were kept and throwing Error if not divisible, but have reverse_pointers property providing always working list or set for any prefix.

RFC 2317 mentioned above only covers prefixes larger than /24 in IPv4. It covers neither smaller prefixes nor IPv6, and that's something we should also think about.

Yes, RFC 2317 is just a hint how to configure DNS server to delegate parts of IPv4 space to other servers. It addresses just too big chunks of 8 bits per IPv4 number, which lacks better granularity. It relies on code embedded in DNS servers to generate a list from network 192.0.2.128/26 on the server side. Not always that is enough and we want ability to generate that list ourselves in any case.

RFC 2317 did not have to deal with prefix smaller than 24, because those were possible to be gelegated just normal DNS delegation with common labels. But prefixes from 24 to 32, there are only final records, which should not be a CNAME.

@Forst
Copy link

Forst commented Mar 3, 2025

I would be okay if reverse_pointer were kept and throwing Error if not divisible, but have reverse_pointers property providing always working list or set for any prefix.

This sounds good to me! One could perhaps, for the sake of consistency, even implement reverse_pointers at the base class level to make it work for IP addresses as well, returning a list of a single item?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 only security fixes 3.13 bugs and security fixes 3.14 new features, bugs and security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

5 participants