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

net: add IP.IsPrivate (RFC 1918 & RFC 4193) #29146

Closed
aaranmcguire opened this issue Dec 7, 2018 · 41 comments
Closed

net: add IP.IsPrivate (RFC 1918 & RFC 4193) #29146

aaranmcguire opened this issue Dec 7, 2018 · 41 comments

Comments

@aaranmcguire
Copy link

@aaranmcguire aaranmcguire commented Dec 7, 2018

The net package seems to have many helpers to report what an IP is. e.g:

  • IsLoopback()
  • IsMulticast()
  • IsInterfaceLocalMulticast()

However there are no helpers to report if a IP address is in the private ranges (RFC 1918 & RFC 4193).

If no one has any objections I can make a method in net/ip.go named IsPrivate() that will check for the ranges listed in both RFCs using the same syntax the other helpers use.

@bradfitz bradfitz changed the title net: Support for RFC 1918 & RFC 4193 proposal: net: add IP.IsPrivate (RFC 1918 & RFC 4193) Dec 7, 2018
@gopherbot gopherbot added this to the Proposal milestone Dec 7, 2018
@gopherbot gopherbot added the Proposal label Dec 7, 2018
@rsc
Copy link
Contributor

@rsc rsc commented Dec 12, 2018

How many more kinds of groups are there besides loopback, multicast, private, etc? That is, what else might we be missing? I'd like to understand if this is the last one or just the next on a very long sequence. It seems OK if we're almost done.

@rsc
Copy link
Contributor

@rsc rsc commented Dec 12, 2018

/cc @mikioh

@bitfield
Copy link

@bitfield bitfield commented Dec 18, 2018

One problem with an isPrivate() method is that those private ranges only apply on the internet. Lots of code runs inside cloud VPCs and other private networks which use the whole IPv4 address space, so what would be a public network address on the internet is actually private.

@rsc
Copy link
Contributor

@rsc rsc commented Dec 19, 2018

ping @mikioh, @aaranmcguire: roughly speaking, are there 2 more of these IP address classes that we need to add, or 20 more?

@aaranmcguire
Copy link
Author

@aaranmcguire aaranmcguire commented Dec 21, 2018

I don't know any more classes of addresses, however I'm not a network person so there may be more I'm unaware of.

@rsc
Copy link
Contributor

@rsc rsc commented Jan 9, 2019

Since we think there are not many more of these, this seems OK to do.

IPv4 calls these Private; IPv6 calls these "Unique Local" at least in RFC 4193. One function should probably do both. Should it be IsPrivate or IsLocal?

@mikioh
Copy link
Contributor

@mikioh mikioh commented Jan 10, 2019

Will take a look, sorry my laptop is still broken (and my fingertips are not good for touch screens.)

@mikioh
Copy link
Contributor

@mikioh mikioh commented Jan 15, 2019

One roadblock is that we still have no concrete consensus on how to deal with IPv4 addresses including IPv4-IPv4(-IPv4) translation addresses and IPv6 addresses including IPv4-IPv6 transition addresses. For example, I'm still not sure whether reporting ::ffff:192.168.0.1 or <user-defined-translation-prefix>:c000:221:: as an IPv4 address is a good idea. A compromise might be:

// IsTranslator reports whether ip is IPv4 or IPv6 address translation
// available.
// When prefixes is nil, it deals with ip as an IPv4 address with the
// well-known prefix for IPv4-embedded IPv6 address defined in RFC
// 6052, an IPv4-mapped IPv6 address defined in RFC 4291, or an IPv4
// address in shared address space defined in RFC 6598.
method (IP) IsTranslator(prefixes []*IPNet) bool

// IsLocalUnicast reports whether ip is in the IPv4 private unicast
// address blocks, unique local IPv6 unicast address blocks, ...
method (IP) IsLocalUnicast() bool or
method (IP) IsLocalUnicast(prefixes []*IPNet) bool // including IsTranslator

If you think the above methods look confusing, that might be a clear signal that it's better to have an external package, like #11772.

The net package seems to have many helpers to report what an IP is: . e.g:

Yes, and the helper methods have an essential characteristic: each helper follows basic IP addressing architecture and never jumps across the line of complicated operational stuff; e.g., geographic things, translation things, and IANA-allocated special-purpose (e.g., for documentation, for AS112/honeypot) things.

IP address classes that we need to add, or 20 more?

People in some organization operating IPv4 private address blocks might want a method reporting organization-specific private/local IP addresses. I personally hope to solve this sort of issues with #18804.

@rsc
Copy link
Contributor

@rsc rsc commented Jan 16, 2019

It should be the case that net.IsPrivate(ip) == net.IsPrivate(ip.To16()) always, and net.IsPrivate(ip) == net.IsPrivate(ip.To4()) when ip.To4() != nil. We always treat IPv4-formatted and IPv4-in-IPv6-formatted addresss as equivalent in package net. That answers ::ffff:192.168.0.1.

I'm not familiar with <user-defined-translation-prefix>:c000:221:: but it seems like that would be outside the scope of IsPrivate. If you defined the prefix, it should be easy enough to write code to check for it (bytes.HasPrefix).

Does anyone object to adding net.IsPrivate?

@mikioh
Copy link
Contributor

@mikioh mikioh commented Jan 16, 2019

We always treat IPv4-formatted and IPv4-in-IPv6-formatted addresss

Are you talking about IPv4-mapped IPv6 address? Or something you or people in the Go team defined one? If the former, it's better to document why the prefix for IPv4-mapped IPv6 address ::ffff/96 is special and other translation addresses like IPv4-embedded addresses are ignored. If the latter, I have no clue.

Does anyone object to adding net.IsPrivate?

It should be IsLocalUnicast if it also covers IPv6 ULA.

@rsc
Copy link
Contributor

@rsc rsc commented Jan 23, 2019

It should be IsLocalUnicast if it also covers IPv6 ULA.

Why? We can easily include the "Unique Local Address" and "ULA" and "RFC 4193" in the docs, but is "private" an inaccurate description? These are addresses that don't route out to the internet, right? That's what private has always meant in V4.

@rsc
Copy link
Contributor

@rsc rsc commented Feb 6, 2019

It sounds like people are generally OK with IsPrivate, meaning either RFC 1918 (including with the standard IPv4-in-IPv6 embedding) or RFC 4193.

Let's stay away from IsTranslator and IsLocalUnicast for now.

@rsc rsc changed the title proposal: net: add IP.IsPrivate (RFC 1918 & RFC 4193) net: add IP.IsPrivate (RFC 1918 & RFC 4193) Feb 6, 2019
@rsc rsc modified the milestones: Proposal, Go1.13 Feb 6, 2019
@mikioh
Copy link
Contributor

@mikioh mikioh commented Feb 7, 2019

This is just a reply to @rsc for sharing the background.

Why? We can easily include the "Unique Local Address" and "ULA" and "RFC 4193" in the docs, but is "private" an inaccurate description?

Not so pedantic, two concerns:

  1. the existing methods for IP address identification are based on addressing architecture, and the blocks described in RFC 1918 and RFC 4193 can be considered as globally (unique, sometimes non-unique) local unicast addresses which are detached from global routing operation and are subtypes of global unicast addresses as described in RFC 4291 (please don't say it's just for IPv6 instantly, IPv6 cannot ignore the transition to IPv6 from IPv4 as its nature). I'm not sure whether introducing APIs from the operational point of view is a good idea because I don't see any difference between this proposal and a proposal like net: add IsAmazonAZ, IsGoogleExternal and IsAzureElastic to IP.
  2. privately use is one of the applications for the address blocks. I just wonder how do API users feel from !IP.IsPrivate().
@mikioh
Copy link
Contributor

@mikioh mikioh commented Feb 7, 2019

generally OK with IsPrivate

I'd recommend renaming IsPrivate with IsPrivateUnicast. IsPrivate looks a bit vague and we already have IsGlobalUnicast.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 17, 2019

Change https://golang.org/cl/162998 mentions this issue: net: add IP.IsLocal()

MaxSem added a commit to MaxSem/go that referenced this issue Feb 18, 2019
Fixes golang#29146

Change-Id: I69f51f943a684bdffaa9a32bea56eb2c5d881b33
@aaranmcguire
Copy link
Author

@aaranmcguire aaranmcguire commented Mar 31, 2019

What's the status of this? Can it be merged now?

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Apr 22, 2019

Kindly paging @MaxSem the author of CL https://go-review.googlesource.com/c/go/+/162998/, @mikioh left some comments on your CL sometime ago, please take a look. Thank you!

@MaxSem
Copy link

@MaxSem MaxSem commented Apr 23, 2019

As I see it, there were disagreements about the proposed function name among project members. I'm happy to change it, but I'd appreciate clarity as to the correct name :)

@jtherin
Copy link

@jtherin jtherin commented Mar 28, 2020

Totally agree with @as, today without the netmask, it's not possible to identify an address as unicast/broadcast/network.
But Private range, Multicast range, LinkLocal range and Lookback range are clearly identified.
So IsPrivate(), IsMulticast(), IsLinkLocal, IsLoopback() are nice!

@schmitmd
Copy link

@schmitmd schmitmd commented Nov 23, 2020

Is there anything still blocking this?

@cespare
Copy link
Contributor

@cespare cespare commented Nov 23, 2020

The CL was abandoned when the PR was closed. Someone will need to send a new CL and get it reviewed.

@6543
Copy link
Contributor

@6543 6543 commented Nov 23, 2020

I'm going to create a Pull ...

6543 added a commit to 6543-forks/go that referenced this issue Nov 24, 2020
Add IsPrivate() helper to check if an IP is private according to RFC 1918 & RFC 4193

Fixes golang#29146
@6543
Copy link
Contributor

@6543 6543 commented Nov 24, 2020

If I have to do something just tell me :)

-> #42793

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 24, 2020

Change https://golang.org/cl/272668 mentions this issue: net: add IP.IsPrivate

6543 added a commit to 6543-forks/go that referenced this issue Jan 5, 2021
Add IsPrivate() helper to check if an IP is private according to RFC 1918 & RFC 4193

Fixes golang#29146
6543 added a commit to 6543-forks/go that referenced this issue Feb 3, 2021
Add IsPrivate() helper to check if an IP is private according to RFC 1918 & RFC 4193

Fixes golang#29146
6543 added a commit to 6543-forks/go that referenced this issue Feb 16, 2021
Add IsPrivate() helper to check if an IP is private according to RFC 1918 & RFC 4193

Fixes golang#29146
@jkamdjou
Copy link

@jkamdjou jkamdjou commented Feb 24, 2021

would also love to see this merged 👍

6543 added a commit to 6543-forks/go that referenced this issue Feb 24, 2021
Add IsPrivate() helper to check if an IP is private according to RFC 1918 & RFC 4193

Fixes golang#29146
6543 added a commit to 6543-forks/go that referenced this issue Mar 1, 2021
Add IsPrivate helper to check if an IP is private according to
RFC 1918 & RFC 4193.

Fixes golang#29146
6543 added a commit to 6543-forks/go that referenced this issue Mar 12, 2021
Add IsPrivate() helper to check if an IP is private according to RFC 1918 & RFC 4193

Fixes golang#29146
6543 added a commit to 6543-forks/go that referenced this issue Mar 22, 2021
Add IsPrivate() helper to check if an IP is private according to RFC 1918 & RFC 4193

Fixes golang#29146
@odeke-em odeke-em modified the milestones: Backlog, Go1.17 Mar 22, 2021
@gopherbot gopherbot closed this in 1c9e587 Mar 22, 2021
@perillo
Copy link
Contributor

@perillo perillo commented Mar 22, 2021

I think the comment in the IP.IsPrivate method is wrong.

For the local IPv4 addresses, the comment cites RFC 4193, but this document only specifies local IPv6 addresses.

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Mar 22, 2021

@perillo, you are correct, thank you; it was just a typo. I had highlighted the change in my comment https://go-review.googlesource.com/c/go/+/272668/7..11/src/net/ip.go#b135
7EEEDDF4-94B5-4C18-8AB8-7122040058AD

But I missed it in the later revision. Please feel free to send a change and tag me and I’ll review it.

@perillo
Copy link
Contributor

@perillo perillo commented Mar 23, 2021

@odeke-em thanks to you for the contribution.

I have also noted another small issue. Unlike RFC 1918, RFC 4193 in section 3 does not say "The Internet Assigned Numbers Authority (IANA) has reserved ...". Instead in section 8 (IANA Considerations) it just says "The IANA has assigned the FC00::/7 prefix to "Unique Local Unicast".

See https://tools.ietf.org/html/rfc4193#section-3 and https://tools.ietf.org/html/rfc4193#section-8.

Thanks

@6543
Copy link
Contributor

@6543 6543 commented Mar 23, 2021

Did I miss something?!?

@6543
Copy link
Contributor

@6543 6543 commented Mar 23, 2021

@perillo sorry it was me geting these typos in :/

should I send a pull with your suggestions?

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Apr 6, 2021

@6543 yup, please send a PR for them. Thank you @perillo for the report, thank you @6543 for all your work and looking forward to more interactions with you when you send that change.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 11, 2021

Change https://golang.org/cl/309249 mentions this issue: net: reference the correct RFCs and sections for IP.IsPrivate

gopherbot pushed a commit that referenced this issue Apr 11, 2021
Properly cite RFC 1918 Section 3 for ipv4,
and RFC 4193 Section 8 for ipv6 comments.

Updates #29146

Change-Id: I8a2df0d7bef50444294bb3301fe09fb09f21ffaf
GitHub-Last-Rev: b034179
GitHub-Pull-Request: #45500
Reviewed-on: https://go-review.googlesource.com/c/go/+/309249
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
Run-TryBot: Emmanuel Odeke <emmanuel@orijtech.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.