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

Lots of tests listen on 0.0.0.0 #381

Closed
kevinburkesegment opened this issue Sep 15, 2023 · 4 comments · Fixed by #609
Closed

Lots of tests listen on 0.0.0.0 #381

kevinburkesegment opened this issue Sep 15, 2023 · 4 comments · Fixed by #609
Assignees
@julienduchesne
Labels
bug Something isn't working

Comments

@kevinburkesegment
Copy link
Contributor

Many of the tests in the test suite attempt to bind to a random port on 0.0.0.0, which means anything can make a connection to the server, including external devices.

On Mac, this is very annoying because each invocation triggers a dialog box asking to allow connections to the test binary. I counted about ten or so of these dialog boxes in one run of the test suite.

To track down this issue, edit $GOROOT/src/net/dial.go, and add the following patch to ListenConfig.Listen:

 // See func Listen for a description of the network and address
 // parameters.
 func (lc *ListenConfig) Listen(ctx context.Context, network, address string) (Listener, error) {
+	if address == ":0" {
+		fmt.Println("ListenConfig.Listen", network, address)
+		debug.PrintStack()
+	}
 	addrs, err := DefaultResolver.resolveAddrList(ctx, "listen", network, address, nil)

Then rerun the tests. This will likely show you which test file is responsible for opening a socket to 0.0.0.0.
@pracucci
Copy link
Contributor

Would listening on 127.0.0.1 instead of 0.0.0.0 fix the issue?

@kevinburkesegment
Copy link
Contributor Author

Yes - here's an example CL submitting a patch: grafana/loki#10411

Some machines map "localhost" to a different IP than 127.0.0.1 but it's sufficient for most machines to dial this host.

@kevinburkesegment
Copy link
Contributor Author

Basically - I'm happy to fix these just wanted some guidance that this is something you'd be interested in reviewing and merging.

@aknuds1
Copy link
Collaborator

aknuds1 commented Sep 26, 2023

Yep, please do open a PR 👍

@aknuds1 aknuds1 added the bug Something isn't working label Sep 26, 2023
kevinburkesegment added a commit to kevinburkesegment/dskit that referenced this issue Oct 9, 2023
@kevinburkesegment
kv/memberlist: fix incorrect TCP transport host parsing
70d8283 
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.
@kevinburkesegment kevinburkesegment mentioned this issue Oct 9, 2023
Merged
kevinburkesegment added a commit to kevinburkesegment/dskit that referenced this issue Oct 9, 2023
@kevinburkesegment
kv/memberlist: fix incorrect TCP transport host parsing
8c6da91 
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.
kevinburkesegment added a commit to kevinburkesegment/dskit that referenced this issue Oct 9, 2023
@kevinburkesegment
kv/memberlist: fix incorrect TCP transport host parsing
e324871 
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.
@kevinburkesegment kevinburkesegment mentioned this issue Oct 9, 2023
Merged
kevinburkesegment added a commit to kevinburkesegment/dskit that referenced this issue Oct 10, 2023
@kevinburkesegment
kv/memberlist: fix incorrect TCP transport host parsing
75e44ac 
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.
kevinburkesegment added a commit to kevinburkesegment/dskit that referenced this issue Oct 10, 2023
@kevinburkesegment
kv/memberlist: fix incorrect TCP transport host parsing
a684960 
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.
charleskorn added a commit that referenced this issue Oct 12, 2023
@kevinburkesegment @charleskorn
kv/memberlist: fix incorrect TCP transport host parsing (#396)
3b80e3b 
* 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>
ying-jeanne pushed a commit that referenced this issue Nov 2, 2023
@kevinburkesegment @charleskorn @ying-jeanne
kv/memberlist: fix incorrect TCP transport host parsing (#396)
7941696 
* 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>
julienduchesne added a commit that referenced this issue Oct 10, 2024
@julienduchesne
Tests: Listen on localhost instead of 0.0.0.0
c4c47e1 
Closes #381

The MacOS network dialog is a bit annoying
@julienduchesne julienduchesne mentioned this issue Oct 10, 2024
Merged
@julienduchesne julienduchesne self-assigned this Oct 11, 2024
julienduchesne added a commit that referenced this issue Oct 11, 2024
@julienduchesne
Tests: Listen on localhost instead of 0.0.0.0
2e567b0 
Closes #381

The MacOS network dialog is a bit annoying
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@julienduchesne julienduchesne

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Tests: Listen on localhost instead of 0.0.0.0 grafana/dskit
4 participants
@aknuds1 @pracucci @julienduchesne @kevinburkesegment