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

ipaddress should make it easy to identify rfc6598 addresses #61602

Closed
leim mannequin opened this issue Mar 12, 2013 · 31 comments
Closed

ipaddress should make it easy to identify rfc6598 addresses #61602

leim mannequin opened this issue Mar 12, 2013 · 31 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@leim
Copy link
Mannequin

leim mannequin commented Mar 12, 2013

BPO 17400
Nosy @terryjreedy, @jcea, @ncoghlan, @pitrou, @macfreek, @tiran
Files
  • issue.17400.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2013-11-03.12:37:43.282>
    created_at = <Date 2013-03-12.01:36:56.910>
    labels = ['type-feature', 'library']
    title = 'ipaddress should make it easy to identify rfc6598 addresses'
    updated_at = <Date 2013-11-03.12:37:43.281>
    user = 'https://bugs.python.org/leim'

    bugs.python.org fields:

    activity = <Date 2013-11-03.12:37:43.281>
    actor = 'ncoghlan'
    assignee = 'none'
    closed = True
    closed_date = <Date 2013-11-03.12:37:43.282>
    closer = 'ncoghlan'
    components = ['Library (Lib)']
    creation = <Date 2013-03-12.01:36:56.910>
    creator = 'leim'
    dependencies = []
    files = ['31851']
    hgrepos = []
    issue_num = 17400
    keywords = ['patch']
    message_count = 31.0
    messages = ['184002', '184019', '184031', '184052', '184249', '184258', '184260', '184334', '195550', '195568', '195607', '195608', '195612', '195852', '195979', '196081', '196320', '196323', '198326', '200847', '200852', '200853', '200854', '200938', '200967', '200984', '200985', '200989', '200990', '201157', '202020']
    nosy_count = 10.0
    nosy_names = ['terry.reedy', 'jcea', 'ncoghlan', 'pitrou', 'macfreek', 'christian.heimes', 'pmoody', 'santoso.wijaya', 'python-dev', 'leim']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue17400'
    versions = ['Python 3.4']

    @leim
    Copy link
    Mannequin Author

    leim mannequin commented Mar 12, 2013

    Currently: ipaddress.IPv4Network('100.64.1.0/24').is_private == False

    Given RFC6598, 100.64.0.0/10 is now approved for use as CGN space, and also for rfc1918-like private usage. Could the code be altered so that is_private will return true for 100.64.0.0/10 as well??

    @leim leim mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Mar 12, 2013
    @tiran
    Copy link
    Member

    tiran commented Mar 12, 2013

    According to Wikipedia [1] even more address ranges are reserved and non-routable. But only three address ranges are marked as private. So 100.64.0.0/10 is reserved and non-routable but not considered a private address range.

    [1] http://en.wikipedia.org/wiki/Reserved_IP_addresses

    @pmoody
    Copy link
    Mannequin

    pmoody mannequin commented Mar 12, 2013

    I don't see anyway to actually assign this bug to myself, but I'll get a patch for this.

    @leim
    Copy link
    Mannequin Author

    leim mannequin commented Mar 12, 2013

    Thanks Peter.

    On 13 March 2013 03:35, pmoody <report@bugs.python.org> wrote:

    pmoody added the comment:

    I don't see anyway to actually assign this bug to myself, but I'll get a
    patch for this.

    ----------
    nosy: +pmoody


    Python tracker <report@bugs.python.org>
    <http://bugs.python.org/issue17400\>


    @pmoody
    Copy link
    Mannequin

    pmoody mannequin commented Mar 15, 2013

    Is the request that is_private should return true for all reserved/non-routable addresses? The docstrings refer to specific rfcs which don't cover most of the addresses listed in the wikipedia page. I haven't done a lot of network programming in the last couple of years, so what do folks think the least surprising result here would be?

    @terryjreedy
    Copy link
    Member

    Peter, 'Assigned To' is a developer who intends to push (or has pushed) a patch. Anyone can write and attach one. And it is nice to give notice that you intend to.

    @leim
    Copy link
    Mannequin Author

    leim mannequin commented Mar 15, 2013

    is_private should return true for all prefixes that are intended for
    *private* use, hence it should include rfc1918 and rfc6598. rfc6598
    stipulates 100.64.0.0/10

    On 16 March 2013 06:34, pmoody <report@bugs.python.org> wrote:

    pmoody added the comment:

    Is the request that is_private should return true for all
    reserved/non-routable addresses? The docstrings refer to specific rfcs
    which don't cover most of the addresses listed in the wikipedia page. I
    haven't done a lot of network programming in the last couple of years, so
    what do folks think the least surprising result here would be?

    ----------


    Python tracker <report@bugs.python.org>
    <http://bugs.python.org/issue17400\>


    @pmoody
    Copy link
    Mannequin

    pmoody mannequin commented Mar 16, 2013

    So I'm not convinced that 6598 space should be treated like 1918 space. Specifically, the second paragraph of the rfc states:

    Shared Address Space is distinct from RFC 1918 private address space
    because it is intended for use on Service Provider networks.
    However, it may be used in a manner similar to RFC 1918 private
    address space on routing equipment that is able to do address
    translation across router interfaces when the addresses are identical
    on two different interfaces. Details are provided in the text of
    this document.

    which I read as, "It's not private like rfc1918 space, but sometimes certain people can treat it similarly." Are there more convincing arguments for treating 6598 like 1918?

    @macfreek
    Copy link
    Mannequin

    macfreek mannequin commented Aug 18, 2013

    I was about to make the same suggestion as the OP.

    Most users think of "private IP" addresses as NATed IP addresses. I think the technical term is "forwardable, but not globally unique". Thus, the method of least surprise would be that indeed the is_private() method returns True for 100.64.0.0/10.

    As for the RFC, these addresses are indeed the same, that they are both NATted. They are different that for RFC 1918 addresses, it is the end-site (home network, or office network) that does the NATing, while for RFC 6598, it is the ISP that does the NATing.

    I think the confusing comes from the term is_private(). Formally, this only applies to RFC 1918 addresses, but it seems that this library does not take a formal but pragmatic approach. Otherwise, they would have added the methods is_forwardable(), is_global() and is_reserved() in line with what is the official specification at http://www.iana.org/assignments/iana-ipv4-special-registry/iana-ipv4-special-registry.xhtml. I prefer a pragmatic approach, and the term is_natted() or is_private() because that is what most programmers are interested in. Those few programmers that truly understand the difference between all these IP ranges (e.g. those who write bogon filter software), will simply avoid these methods and just use the rest of the library.

    So +1 for this request.

    @pmoody
    Copy link
    Mannequin

    pmoody mannequin commented Aug 18, 2013

    I'm still not convinced. The rfc still says in essence "It's not private like rfc1918 space, but sometimes certain people can treat it similarly." and I think it would be pretty surprising for ipaddress to return True if it's not a network operator running the query. Since we have no way of knowing that, I'm extremely disinclined to make this change. A more formal solution would be all of the possible "is_RFCXXXX" methods, but that doesn't seem to be worth the effort.

    @pmoody pmoody mannequin closed this as completed Aug 18, 2013
    @macfreek
    Copy link
    Mannequin

    macfreek mannequin commented Aug 19, 2013

    I don't understand your remark "I think it would be pretty surprising for ipaddress to return True if it's not a network operator running the query."

    @macfreek
    Copy link
    Mannequin

    macfreek mannequin commented Aug 19, 2013

    Edit: could you rephrase?

    @macfreek
    Copy link
    Mannequin

    macfreek mannequin commented Aug 19, 2013

    A bit odd questions: What is the is_private() function intended to accomplish?

    I have been wondering what is_private() means, and how users of the library are going to use this function. I've actually failed to come up with any sensible use-case with the current implementation. So in the current state, without the modifications, my vote would be to remove the method, as it is more likely to add confusion than the be of a particular use.

    The most useful method (which I originally thought it was meant to do) is a function that indicates if a certain IP range is NATted (only an indication, since it requires a network to reliable test). However, that's not what the current function entails: it misses the 100.64.0.0/10 range, which is NATted, but includes the fc00::/7 unique local block, which is not NATted.

    I would be all in favour of such a is_natted() function, but that's not what this is.

    The is_private() function also does not simply list "private" IP addresses, when looking at the formal IETF definitions, since it includes fc00::/7, which are unique local addresses, which in practice used in a rather different way: IPv4 private IP addresses are often NATted (and routed after translation), while IPv6 unique local addresses are typically used for local non-routed networks.

    Also, the is_private() function does not list all non-globally assigned addresses. That should includes a lot more ranges, as listed on http://www.iana.org/assignments/iana-ipv4-special-registry/iana-ipv4-special-registry.xhtml and http://www.iana.org/assignments/iana-ipv6-special-registry/iana-ipv6-special-registry.xhtml.

    So far, the is_private() function seems to return True for addresses which are:

    • non-globally assigned (as opposed to regular unicast and multicast addresses)
    • available for manual assignment (as opposed to link-local addresses)
    • may be assigned by end-sites, but not by ISPs (as opposed to shared IP addresses and the small DS-Lite block)
    • is not intended for benchmarking purposes (as opposed to benchmarking IP addresses)

    Frankly, I wonder if this very particular information of enough interest to warrant a function on its own.

    In that case, I much rather see more generic (and hopefully more useful) functions such as is_natted() and for is_global(), is_forwardable() and is_reserved(), as defined by IANA.

    @pmoody
    Copy link
    Mannequin

    pmoody mannequin commented Aug 22, 2013

    is_private was, as you note, basically shorthand for is_RFC1918 (and is_RFC4193 for v6). It's not a particularly well-named method, but at the time that I wrote it (~5 years ago?), it did what I needed it to do.

    I'm not sure what you mean by an 'is_natted()' method; there's nothing in particular preventing someone from natting a globally unique address. is_global() makes some sense to me, and it appears that I most likely have to update is_reserved, but I don't understand is_forwardable().

    @macfreek
    Copy link
    Mannequin

    macfreek mannequin commented Aug 23, 2013

    Peter, first of all, thanks for your library. I didn't mention that before, but should have.

    I'm in favour of a pragmatic approach. I've only come across NATing for RFC 1918 and RFC 6598 addresses. While it can technically be done for other addresses, and is allowed by RFC 3022 section 5.1, I have never seen that in practice.

    is_natted() (or perhaps: is_nattable()?) could be used by a SIP client to decide if it should include a VIA header or not, without need to do a resource-expensive NAT check at all times.

    To be clear: I'm not a great fan of is_natted() myself, but I fear that keeping is_private() the way it is, people will use it as if it meant is_natted() and will end up with unintended errors in their code.

    is_forward and is_global -if deemed useful- should just follow the columns at http://www.iana.org/assignments/iana-ipv4-special-registry and http://www.iana.org/assignments/iana-ipv6-special-registry. Perhaps functions like is_valid_source(), is_valid_destination() and is_reserved() may be included too.

    The meaning of these columns is explained in [http://tools.ietf.org/html/rfc6890#section-2.2.1 RFC 6890]. I interpret forwardable as "should be forwarded by a router" and global as "may be seen on the Internet / should be forwarded beyond administrative domain boundaries". For example, private IP addresses or benchmarking IP addresses may be routed just fine, as long as they're never seen on the global Internet.

    PS: There is a typo in the documentation. is_unspecified mentions RFC 5375, but that should be RFC 5735, which in turn is obsoleted by RFC 6890. I'll see if I can make a patch, but that will be after my holiday.

    @ncoghlan
    Copy link
    Contributor

    Reopening this - rewording the issue title to cover the problem to be solved (i.e. accounting for RFC 6598 addresses) rather than a specific solution (which isn't appropriate, since the RFC *explicitly* states that shared addresses and private addresses aren't the same thing)

    It seems to me that the simplest solution would be to use the terminology from RFC 6598 and add a new "is_shared" attribute to addresses.

    @ncoghlan ncoghlan reopened this Aug 24, 2013
    @ncoghlan ncoghlan changed the title ipaddress.is_private needs to take into account of rfc6598 ipaddress should make it easy to identify rfc6598 addresses Aug 24, 2013
    @pmoody
    Copy link
    Mannequin

    pmoody mannequin commented Aug 27, 2013

    The problem is that 'shared' describes exactly one network, unless you mean that we should try to start 'private' as 'shared'. That's something I really don't want to do because it leads to confusion like this.

    Do you not think that is_global or is_forwardable (per the iana registry) is worthwhile?

    @ncoghlan
    Copy link
    Contributor

    I'd also be fine with "is_carrier_private", or, as you say, the inverse
    "is_global" for "not is_private and not is_carrier_private and not (any of
    the other private addresses)" (assuming I understood that suggestion
    correctly).

    I guess the "is_global" one is the most useful, since that lets you know if
    you can send or store that address directly, or if you need to translate it
    to a persistent global address somehow.

    @pmoody
    Copy link
    Mannequin

    pmoody mannequin commented Sep 23, 2013

    ok, here's an is_global/is_private patch using the iana special registry for ipv4 and ipv6.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 21, 2013

    New changeset 2e8dd5c240b7 by Peter Moody in branch 'default':
    bpo-17400; ipaddress should make it easy to identify rfc6598 addresses
    http://hg.python.org/cpython/rev/2e8dd5c240b7

    @tiran
    Copy link
    Member

    tiran commented Oct 21, 2013

    About 2e8dd5c240b7
    It might be a good idea to cache the two lists in a class or module variable in order to speed things up. It might also be a good idea to move the most common networks like 192.168.0.0/16 to the top of the list.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 21, 2013

    New changeset 07a5610bae9d by Peter Moody in branch 'default':
    bpo-17400; NEWS and ipaddress.rst change
    http://hg.python.org/cpython/rev/07a5610bae9d

    @pmoody
    Copy link
    Mannequin

    pmoody mannequin commented Oct 21, 2013

    I have a change that needs to be submitted for the parser then I'll come back to the caching.

    The pedant in me would like like to keep the addresses ordered because that makes it clear where to add new networks as iana changes classifications, but it may just make more sense to put rfc1918 at the top.

    @ncoghlan
    Copy link
    Contributor

    The docs patch doesn't look quite right - Peter, did you mean to copy the "is_private" docs before modifying them?

    As far as caching goes, perhaps we can just drop functools.lru_cache into the relevant property implementations?

        @property
        @lru_cache()
        def is_global(self):
            """Test if this address is allocated for public networks."""

    (also in copying that example method header, I noticed the docstring for the IPv4 is_global currently still says "private" instead of "public")

    @pitrou
    Copy link
    Member

    pitrou commented Oct 22, 2013

    Sorry for chiming in a bit late, but what's the rationale for including 100.64.0.0/10 in the "is_private" set, rather than *only* excluding it from the "is_global" set?

    The rationale for RFC 6598 is precisely that 100.64.0.0/10 is *not* private in the common sense, so it would deserve a different treatment in the ipaddress module as well.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 22, 2013

    New changeset 365fd677856f by Peter Moody in branch 'default':
    bpo-17400: fix documentation, add cache to is_global and correctly handle 100.64.0.0/10
    http://hg.python.org/cpython/rev/365fd677856f

    @pmoody
    Copy link
    Mannequin

    pmoody mannequin commented Oct 22, 2013

    antoine, quite right. I've updated is_global.
    Nick, I've added lru_cache() to is_private and updated the docs (hope it's right this time).

    @ncoghlan
    Copy link
    Contributor

    Thanks Peter, just a couple more tweaks:

    • tests need to ensure the carrier private range is neither global *nor*
      private (it looks to me like they will still show up as private at this
      point)
    • looks like the docs for is_private still need to be restored (and mention
      the subtlety about deliberately excluding "carrier private").
    • this is probably worthy of a What's New entry mentioning the new attribute

    My apologies for not finding time to review the original patch more
    closely, as that could have saved you a bit of the subsequent running
    around...

    @pitrou
    Copy link
    Member

    pitrou commented Oct 22, 2013

    antoine, quite right. I've updated is_global.
    Nick, I've added lru_cache() to is_private and updated the docs (hope it's right this time).

    Mmmh... I actually meant the reverse. IIUC, 100.64.0.0/10 isn't global
    (i.e. globally routable) and isn't private either.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 24, 2013

    New changeset b9623fa5a0dd by Peter Moody in branch 'default':
    bpo-17400: correct handling of 100.64.0.0/10, fixing the docs and updating NEWS
    http://hg.python.org/cpython/rev/b9623fa5a0dd

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Nov 3, 2013

    Just updating the issue state to reflect the fact Peter committed this a while ago.

    @ncoghlan ncoghlan closed this as completed Nov 3, 2013
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants