Skip to content

misc: Don't use 0.0.0.0 as the default listen address and warn if using a loopback #2406

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

Closed
simon-lemay-unity wants to merge 6 commits into from
Closed

misc: Don't use 0.0.0.0 as the default listen address and warn if using a loopback #2406

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e0edda0
Revert "feat: Change default listen address to 0.0.0.0 (#2307)"
simon-lemay-unity Feb 9, 2023
382bfa4
Bring back the good bits from that revert
simon-lemay-unity Feb 9, 2023
e1ab404
Warn if using 127.0.0.1 as the default listen address
simon-lemay-unity Feb 9, 2023
1bf1e7b
Avoid the warning in many tests
simon-lemay-unity Feb 9, 2023
c16ba4e
Minor change to a log line
simon-lemay-unity Feb 9, 2023
eb41ad5
Add PR number to CHANGELOG entry
simon-lemay-unity Feb 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion com.unity.netcode.gameobjects/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@ Additional documentation and release notes are available at [Multiplayer Documen

### Changed

- `UnityTransport` will now emit a warning if binding to a loopback address with an empty 'Server Listen Address' setting. The warning can be silenced by entering a valid IP address (127.0.0.1 is recommended for local development, 0.0.0.0 if accepting remote connections is required).
Copy link
Copy Markdown
Collaborator

@ShadauxCat ShadauxCat Feb 9, 2023

Choose a reason for hiding this comment

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

Please add the PR number here. (A couple others are missing them as well but let's try not to make that a pattern please <3.)

- Updated `UnityTransport` dependency on `com.unity.transport` to 1.3.1.
- `NetworkShow()` of `NetworkObject`s are delayed until the end of the frame to ensure consistency of delta-driven variables like `NetworkList`.
- Dirty `NetworkObject` are reset at end-of-frame and not at serialization time.
- `NetworkHide()` of an object that was just `NetworkShow()`n produces a warning, as remote clients will _not_ get a spawn/despawn pair.
- The default listen address of `UnityTransport` is now 0.0.0.0. (#2307)
- Renamed the NetworkTransform.SetState parameter `shouldGhostsInterpolate` to `teleportDisabled` for better clarity of what that parameter does. (#2228)
- Network prefabs are now stored in a ScriptableObject that can be shared between NetworkManagers, and have been exposed for public access. By default, a Default Prefabs List is created that contains all NetworkObject prefabs in the project, and new NetworkManagers will default to using that unless that option is turned off in the Netcode for GameObjects settings. Existing NetworkManagers will maintain their existing lists, which can be migrated to the new format via a button in their inspector. (#2322)

Expand Down
17 changes: 9 additions & 8 deletions com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,9 +303,9 @@ public struct ConnectionAddressData
public ushort Port;

/// <summary>
/// IP address the server will listen on. If not provided, will use 0.0.0.0.
/// IP address the server will listen on. If not provided, will use 'Address'.
/// </summary>
[Tooltip("IP address the server will listen on. If not provided, will use 0.0.0.0.")]
[Tooltip("IP address the server will listen on. If not provided, will use 'Address'.")]
[SerializeField]
public string ServerListenAddress;

Expand Down Expand Up @@ -336,16 +336,17 @@ public NetworkEndpoint ListenEndPoint
{
if (string.IsNullOrEmpty(ServerListenAddress))
{
var ep = NetworkEndpoint.AnyIpv4;
var ep = ParseNetworkEndpoint(Address, Port);

// If an address was entered and it's IPv6, switch to using :: as the
// default listen address. (Otherwise we always assume IPv4.)
if (!string.IsNullOrEmpty(Address) && ServerEndPoint.Family == NetworkFamily.Ipv6)
if (ep.IsLoopback)
{
ep = NetworkEndpoint.AnyIpv6;
Debug.LogWarning("Server/host will only listen for local connections. To allow connections " +
"from remote devices, set the 'Server Listen Address' setting of the 'Unity Transport' " +
"component to 0.0.0.0. To preserve the current behavior and silence this warning, set " +
Copy link
Copy Markdown
Collaborator

@ShadauxCat ShadauxCat Feb 9, 2023

Choose a reason for hiding this comment

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

Should we be specifically promoting use of 0.0.0.0? Seems like this message should encourage the user to think about their choice. Like "set the Server Listen Address (use 0.0.0.0 to accept all incoming connections)". I feel like the way it's worded right now has the same security concern as before because a lot of users are just going to read this and then set 0.0.0.0 while wondering why we don't just do that automatically.

Copy link
Copy Markdown
Contributor Author

@simon-lemay-unity simon-lemay-unity Feb 9, 2023

Choose a reason for hiding this comment

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

I agree, but realistically there are only two choices here: bind to 127.0.0.1 or bind to 0.0.0.0. The key differentiator between the two being that 0.0.0.0 allows connections from remote devices. Sure, it's possible to set it to the IP address of a specific network adapter, but that's for people who know what they're doing (and they're not the audience of this warning).

The only reason to use 127.0.0.1 is security, and I'm wary of stating that in the warning because it implies that 0.0.0.0 is less secure in some way (when in fact if you want to accept remote connections you'll be opening a port to the outside anyway). To me it really boils down to whether the user needs to accept connections from remote devices or not. (The answer to that question being yes most of the time is why I originally made 0.0.0.0 the default.)

I'm open to suggestions on how to rephrase the warning, however.

Copy link
Copy Markdown
Collaborator

@ShadauxCat ShadauxCat Feb 9, 2023

Choose a reason for hiding this comment

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

I suppose, but that makes me wonder if we should even have an IP entry by default or if this should be a checkbox for "Accept remote connections" and put the text box under a collapsed "advanced settings" section. Then the warning could just say "Warning: Running in local mode. Connections from remote devices will not be accepted. To change this, check 'accept remote connections' in NetworkManager settings".

If we know that most users will only be choosing between two possible values for that field, why not just make it a boolean field and hide the advanced configuration so only advanced users need to think about it at all?

Copy link
Copy Markdown
Collaborator

@ShadauxCat ShadauxCat Feb 9, 2023 •
edited by NoelStephensUnity
Loading

Choose a reason for hiding this comment

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

  • Accept Remote Connections
--- Advanced Settings ---
Bind IP: [_____________]

Copy link
Copy Markdown
Contributor Author

@simon-lemay-unity simon-lemay-unity Feb 9, 2023

Choose a reason for hiding this comment

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

I agree, and in hindsight a checkbox really would have been the better option here. I'm not sure about adding this at this point, though. I'm afraid this will just add more complexity to a component (UnityTransport) which already doesn't exactly shine by how straightforward it is to use.

Adding this without breaking the API would probably require adding a custom editor component for the settings (we can't just add a separate advanced settings structure to ConnectionData because ServerListenAddress is already a member and I've seen users set it manually from code). We'll also need to add logic to harmonize the checkbox and the text field. For example, what happens if I check the box but set the bind IP to 127.0.0.1?

All of that for an option which, as you astutely point out, users are just going to blindly enable and wonder why it's not the default behavior. Ultimately the IP the server binds on is a relatively minor consideration in the grand scheme of creating a multiplayer game. I think the amount of effort we put into handling it should reflect that.

Copy link
Copy Markdown
Collaborator

@ShadauxCat ShadauxCat Feb 9, 2023

Choose a reason for hiding this comment

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

Yeah, a custom editor component was what I had in mind actually. No changes to the actual runtime code, just a custom editor component that fills that box with 127.0.0.1 or 0.0.0.0 as necessary based on the state of the checkbox, probably disabling the checkbox if the user's manually entered anything there.

Might be too much for a quick bugfix to go into an already-cut release branch, but since we seem to be in agreement that that'd be a better solution, maybe we could make a task for later to adjust that for 1.3.1?

Copy link
Copy Markdown
Contributor Author

@simon-lemay-unity simon-lemay-unity Feb 9, 2023

Choose a reason for hiding this comment

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

Yes, that sounds good! I think it would make a great improvement for the next release.

"it to 127.0.0.1 instead.");
}

return ep.WithPort(Port);
return ep;
}
else
{
Expand Down
3 changes: 3 additions & 0 deletions com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTestHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,9 @@ private static void AddUnityTransport(NetworkManager networkManager)
unityTransport.MaxConnectAttempts = 4;
unityTransport.ConnectTimeoutMS = 500;

// Prevent the warning about binding to localhost.
unityTransport.SetConnectionData("127.0.0.1", 7777, "127.0.0.1");

// Set the NetworkConfig
networkManager.NetworkConfig ??= new NetworkConfig();
networkManager.NetworkConfig.NetworkTransport = unityTransport;
Expand Down
1 change: 1 addition & 0 deletions com.unity.netcode.gameobjects/TestHelpers/Runtime/NetworkManagerHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ public static bool StartNetworkManager(out NetworkManager networkManager, Networ
Debug.Log($"{nameof(NetworkManager)} Instantiated.");

var unityTransport = NetworkManagerGameObject.AddComponent<UnityTransport>();
unityTransport.SetConnectionData("127.0.0.1", 7777, "127.0.0.1");
if (networkConfig == null)
{
networkConfig = new NetworkConfig
Expand Down
17 changes: 2 additions & 15 deletions com.unity.netcode.gameobjects/Tests/Editor/Transports/UnityTransportTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ public void UnityTransport_RestartSucceedsAfterFailure()
UnityTransport transport = new GameObject().AddComponent<UnityTransport>();
transport.Initialize();

transport.SetConnectionData("127.0.0.", 4242, "127.0.0.");
transport.SetConnectionData("127.0.0.", 4242);
Assert.False(transport.StartServer());

LogAssert.Expect(LogType.Error, "Invalid network endpoint: 127.0.0.:4242.");
Expand All @@ -124,20 +124,7 @@ public void UnityTransport_RestartSucceedsAfterFailure()
#endif
LogAssert.Expect(LogType.Error, "Server failed to bind. This is usually caused by another process being bound to the same port.");

transport.SetConnectionData("127.0.0.1", 4242, "127.0.0.1");
Assert.True(transport.StartServer());

transport.Shutdown();
}

// Check that leaving all addresses empty is valid.
[Test]
public void UnityTransport_StartServerWithoutAddresses()
{
UnityTransport transport = new GameObject().AddComponent<UnityTransport>();
transport.Initialize();

transport.SetConnectionData(string.Empty, 4242);
transport.SetConnectionData("127.0.0.1", 4242);
Assert.True(transport.StartServer());

transport.Shutdown();
Expand Down
6 changes: 5 additions & 1 deletion com.unity.netcode.gameobjects/Tests/Runtime/Transports/UnityTransportTestHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,11 @@ public static void InitializeTransport(out UnityTransport transport, out List<Tr

if (family == NetworkFamily.Ipv6)
{
transport.SetConnectionData("::1", 7777);
transport.SetConnectionData("::1", 7777, "::1");
}
else
{
transport.SetConnectionData("127.0.0.1", 7777, "127.0.0.1");
}

transport.Initialize();
Expand Down