-
Notifications
You must be signed in to change notification settings - Fork 60
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
kv/memberlist: fix incorrect TCP transport host parsing #396
Conversation
8c6da91
to
e324871
Compare
for { | ||
select { | ||
case <-start: | ||
start = nil | ||
|
||
// let's join the first member | ||
if portToConnect > 0 { | ||
_, err := kv.kv.JoinMembers([]string{fmt.Sprintf("127.0.0.1:%d", portToConnect)}) | ||
_, err := kv.kv.JoinMembers([]string{localhostIP.String() + fmt.Sprintf(":%d", portToConnect)}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likely going to be a conflict between this and the "join host port" PR, I will resolve once that is merged.
e324871
to
75e44ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for spotting and fixing this @kevinburkesegment!
Previously, if you passed "localhost" as the bind address for a TCPTransport, we would attempt to parse this as an IP address, fail, and then begin listening on 0.0.0.0. This is a security issue since 0.0.0.0 binds to all interfaces, including public ones, while "localhost" is a shortcut for an IP address that is typically is only accessible from the local machine, not the wider Internet. Fix this by returning an error if you attempt to pass a BindAddr to TCPTransport that is not actually an IP address. Also, fix the tests to resolve "localhost" to an IP address before proceeding - typically, but not always, this is 127.0.0.1, which is why we try to parse the loopback address instead of hardcoding it. I can confirm that on a Mac, with this patch applied I no longer get dialog boxes warning me that the tests are attempting to listen on 0.0.0.0. Updates grafana#381.
75e44ac
to
a684960
Compare
Co-authored-by: Charles Korn <charleskorn@users.noreply.github.com>
kv/memberlist/tcp_transport_test.go
Outdated
if err == nil { | ||
t.Fatal("expected non-nil err, got nil") | ||
} | ||
want := `could not parse bind addr "localhost" as IP address` | ||
if err.Error() != want { | ||
t.Fatalf("NewTCPTransport with non-IP address BindAddr: got %v, want %v", err, want) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use the require
package in other tests to make assertions like these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
kv/memberlist/tcp_transport_test.go
Outdated
require.Error(t, err) | ||
require.Equal(t, err.Error(), `could not parse bind addr "localhost" as IP address`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to simplify this even further:
require.Error(t, err) | |
require.Equal(t, err.Error(), `could not parse bind addr "localhost" as IP address`) | |
require.EqualError(t, err, `could not parse bind addr "localhost" as IP address`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Thanks @kevinburkesegment! |
* kv/memberlist: fix incorrect TCP transport host parsing Previously, if you passed "localhost" as the bind address for a TCPTransport, we would attempt to parse this as an IP address, fail, and then begin listening on 0.0.0.0. This is a security issue since 0.0.0.0 binds to all interfaces, including public ones, while "localhost" is a shortcut for an IP address that is typically is only accessible from the local machine, not the wider Internet. Fix this by returning an error if you attempt to pass a BindAddr to TCPTransport that is not actually an IP address. Also, fix the tests to resolve "localhost" to an IP address before proceeding - typically, but not always, this is 127.0.0.1, which is why we try to parse the loopback address instead of hardcoding it. I can confirm that on a Mac, with this patch applied I no longer get dialog boxes warning me that the tests are attempting to listen on 0.0.0.0. Updates #381. * kv/memberlist: add test for IP address rejection * Update CHANGELOG.md Co-authored-by: Charles Korn <charleskorn@users.noreply.github.com> * fix more tests * rework sync.Once usage * use require instead of manual tests * require.EqualError * require.Error no longer necessary --------- Co-authored-by: Charles Korn <charleskorn@users.noreply.github.com>
Previously, if you passed "localhost" as the bind address for a TCPTransport, we would attempt to parse this as an IP address, fail, and then begin listening on 0.0.0.0.
This is a security issue since 0.0.0.0 binds to all interfaces, including public ones, while "localhost" is a shortcut for an IP address that is typically is only accessible from the local machine, not the wider Internet. A user who intended to bind only to the loopback interface might inadvertently expose a server to the public Internet.
Fix this by returning an error if you attempt to pass a BindAddr to TCPTransport that is not actually an IP address. Also, fix the tests to resolve "localhost" to an IP address before proceeding - typically, but not always, this is 127.0.0.1, which is why we try to parse the loopback address instead of hardcoding it.
I can confirm that on a Mac, with this patch applied I no longer get dialog boxes warning me that the tests are attempting to listen on 0.0.0.0.
Updates #381.