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.

Sign up for GitHub

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

Jump to bottom

net: add IP.IsLocal #30278

Closed
wants to merge 1 commit into from
Closed

net: add IP.IsLocal #30278

wants to merge 1 commit into from

Conversation

MaxSem
Copy link

@MaxSem MaxSem commented Feb 17, 2019

Fixes #29146

@googlebot googlebot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Feb 17, 2019
@gopherbot
Copy link
Contributor

This PR (HEAD: 4bedd5d) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/162998 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.

Please don’t reply on this GitHub thread. Visit golang.org/cl/162998.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 1:

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/162998.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Max Semenik:

Patch Set 3:

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/162998.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Max Semenik:

Patch Set 3:

Uh, the bot reverted my PS2?

Please don’t reply on this GitHub thread. Visit golang.org/cl/162998.
After addressing review feedback, remember to publish your drafts!

@MaxSem
net: add IP.IsLocal
424bea4 
Fixes golang#29146

Change-Id: I69f51f943a684bdffaa9a32bea56eb2c5d881b33
@gopherbot
Copy link
Contributor

This PR (HEAD: 424bea4) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/162998 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Max Semenik:

Patch Set 6:

Okay, back to pushing to GitHub :D This is ready for review.

Please don’t reply on this GitHub thread. Visit golang.org/cl/162998.
After addressing review feedback, remember to publish your drafts!

@MaxSem MaxSem changed the title net: add IP.IsLocal() net: add IP.IsLocal Feb 21, 2019
@gopherbot
Copy link
Contributor

Message from Mikio Hara:

Patch Set 7:

(5 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/162998.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Mikio Hara:

Patch Set 7:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/162998.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Juan Antonio Osorio:

Patch Set 7:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/162998.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Mikio Hara:

Patch Set 7:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/162998.
After addressing review feedback, remember to publish your drafts!

msoedov
msoedov reviewed May 14, 2019
View reviewed changes
src/net/ip.go
// Local IPv4 addresses are defined in https://tools.ietf.org/html/rfc1918
return ip4[0] == 10 ||
(ip4[0] == 172 && ip4[1]&0xf0 == 16) ||
(ip4[0] == 192 && ip4[1] == 168)
Copy link
Contributor

@msoedov msoedov May 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. IsLocal name is miss-leading. Did you mean IsPrivate? I would interpret IsLocal as a private address in the same network
  2. How to handle cases 0.0.0.0 and 127.0.0.1 and ::1?

Copy link

@cypres cypres Jun 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I also think IsPrivate would be a better name.
  2. We do at least have IsUnspecified() and IsLoopback() to cover those cases with IsLocal you can do IsLocal() || IsUnspecified() || IsLoopback() if you want to cover all.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speaking about private addresses - 100.64.0.0/10 is private, 198.18.0.0/15 is private too.

This implementation is true only for RFC 1918 (IPv4 addresses) and RFC 4193 (IPv6 addresses).

Copy link

@cypres cypres Jul 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure but that doesn't mean it would still be useful too have a group for RFC 1918 and RFC 4193... We're using this to filter webhook calls, so people can't setup a webhook that calls a private address in our internal networks. The carrier grade nat scope 100.64.0.0/10 and/or the benchmark test net 198.18.0.0/15 is not really any concern for us, protecting internal networks is.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was not clear, my points are:

  • IsLocal is a bad name, i am with you on it.
  • IsPrivate is ok name, but changing the name brings new problem - current implementation includes only part of private ip address blocks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a second thought about it. IsPrivate wont do, because there are a lot of other ip block in the private scope.

IP blocks from rfc1918 fall into private category but they have special purpose. rfc1918 doesnt name them local tho. But the gist is - local communication (inside enterprise). So the grouping is ok, naming is a little bit miss-leading. Proof - our comments 😄

But IsLocal makes more sense then IsPrivate and this logical block shouldnt cover

How to handle cases 0.0.0.0 and 127.0.0.1 and ::1?

Only blocks from RFC 1918 (IPv4 addresses) and RFC 4193 (IPv6 addresses) should be there.

@gopherbot
Copy link
Contributor

Message from Alex Miasoedov:

Patch Set 7:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/162998.
After addressing review feedback, remember to publish your drafts!

@gopherbot gopherbot force-pushed the master branch 3 times, most recently from 53bd915 to 6139019 Compare October 3, 2019 21:09
@gopherbot gopherbot force-pushed the master branch 6 times, most recently from 4a7ed1f to 0f992b9 Compare November 5, 2019 02:56
@MaxSem MaxSem closed this Mar 27, 2020
@MaxSem MaxSem deleted the IP.IsLocal branch March 27, 2020 20:25
@6543 6543 mentioned this pull request Nov 24, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilyam8 ilyam8 ilyam8 left review comments

@msoedov msoedov msoedov left review comments

@cypres cypres cypres left review comments

Assignees
No one assigned
Labels
cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

net: add IP.IsPrivate (RFC 1918 & RFC 4193)
6 participants
@MaxSem @gopherbot @cypres @msoedov @ilyam8 @googlebot