Add transfer_player function to request client move to new server #14129

sfence · 2023-12-19T20:26:59Z

This is the adoption of #11175.

I have rebased the original @grapereader code. Is compilable, and looks to works well on my Linux system.

This is related to #5813 and #4428 at least.

To do

This PR is a Work in Progress.

Add test command or API to devtest game.
See feedback and apply potentially required changes

How to test

Create or reuse existing world in devtest game
Host server of it
Run the second Minetest instance, join the hosted server
run command /transfer_player 127.0.0.1 YOUR_PORT_NUMBER
you should be disconnected and reconnected

You can also host the game in the second Minetest instance with the same player_name and password as on the first server and reconnect by the same function from it.

On calling minetest.transfer_player(name, address, port), server sends a disconnect message of type SERVER_ACCESSDENIED_TRANSFER with the IP and port packed into customMessage. Clients that support this new message type will disconnect and reconnect to the new address:port, using the same username and password. Clients that do not support this new message will disconnect and display the destination server address in the disconnection message box.

lhofhansl · 2023-12-20T00:42:31Z

IMHO there needs to be an option to disallow this on a client. Or the user on the client needs to OK through some dialog.

sfence · 2023-12-20T03:25:28Z

IMHO there needs to be an option to disallow this on a client. Or the user on the client needs to OK through some dialog.

Do you mean the global option in minetest.conf or the game-based option in game.conf file?

If it will be disabled by default, upon the game allowing it, can it be a good solution?

Zughy · 2023-12-20T07:55:36Z

What happens if I'm registered in the first server but in the second one a different user registered an account with my same nane and different password?

sfence · 2023-12-20T15:03:30Z

What happens if I'm registered in the first server but in the second one a different user registered an account with my same nane and different password?

You will be disconnected from the first server and connecting to the second server returns an auth error, so the effect will be similar to disconnecting from the first server.

appgurueu

Convenient feature, however, given that Minetest uses unencrypted UDP packets, I'm mildly concerned about "redirect" attacks. For example, hypothetically, couldn't a malicious server first harvest player IPs, then just constantly spam "transfer player" requests to these IPs to "steal" the users?

I've also tried to review the C++ code. Hopefully one of the core devs can review my review.

appgurueu · 2023-12-28T19:25:57Z

builtin/fstk/ui.lua

@@ -61,7 +61,17 @@ function ui.update()
 	local formspec = {}

 	-- handle errors
-	if gamedata ~= nil and gamedata.reconnect_requested then
+	if gamedata ~= nil and gamedata.reconnect_requested and #gamedata.transfer_address > 0 then


#gamedata.transfer_address > 0 would be slightly more readable as gamedata.transfer_address ~= "" (also: why not use nil to indicate an absent transfer address?)

appgurueu · 2023-12-28T19:26:32Z

doc/lua_api.md

@@ -6589,6 +6589,9 @@ Bans
  optional reason, this will not prefix with 'Kicked: ' like kick_player.
  If no reason is given, it will default to 'Disconnected.'
    * Returns boolean indicating success (false if player nonexistent)
+* `minetest.transfer_player(name, address, port)`: Disconnect a player and
+  request the client re-connect at a new address and port. The connection
+  to the new server will occur using the same player name and password.


Nitpick: "will occur using the same player name" - what about the name parameter?

This is expected to be called by some mod on the server side. So that mod has to identify a player.

As there is name parameter for minetest.disconnect_player and minetest.kick_player, there is a name parameter for minetest.transfer_player.

Sorry, I framed that poorly. I understand the API. I just find the wording confusing, since (for obvious reasons) we don't pass a password parameter, yet it says "using the same player name and password". Suggestion: Something along the lines of "[...] will occur using name and the client-remembered password" maybe?

From my understanding: A password is remembered in some form of hash or something similar. This value is remembered and reused with the player's name to send the AUTH packet to the new server.

I think this PR will actually remember the password in plaintext, in client memory? (Which isn't optimal for security, btw.)

SRP still only sends hashes over the network though.

I have started working on it. I will create a PR to fix it as soon as possible.
This PR will be forced to wait for that PR to be merged, and I rebase this after it and fix the problem.

I have started working on it. I will create a PR to fix it as soon as possible. This PR will be forced to wait for that PR to be merged, and I rebase this after it and fix the problem.

Won't this PR have to remember the password though to do the SRP auth with the server without requiring the user to re-enter their password? Or is there a way to just remember a hash or something similar? (Which would be ideal.)

If there is a way to remember SRP auth without a password, it is an ideal situation. It looks promising.
Otherwise, the client will be forced to reenter the password.

See #14196.
Maybe, something more can be done in the main menu Lua scripts.

@appgurueu See #14196 (comment).

appgurueu · 2023-12-28T19:29:52Z

src/client/client.h

@@ -327,6 +327,9 @@ class Client : public con::PeerHandler, public InventoryManager, public IGameDef

 	bool reconnectRequested() const { return m_access_denied_reconnect; }

+	std::string getReconnectAddress() const { return m_access_denied_reconnect_address; }


Why not return const std::string &s here?

appgurueu · 2023-12-28T19:30:47Z

src/client/clientlauncher.cpp

@@ -228,11 +231,14 @@ bool ClientLauncher::run(GameStartData &start_data, const Settings &cmd_args)
 				core::rect<s32>(0, 0, 10000, 10000));

 			bool game_has_run = launch_game(error_message, reconnect_requested,


I feel at this point the three reconnect parameters might deserve their own struct? It seems to me this would make the code somewhat less verbose.

appgurueu · 2023-12-28T19:32:16Z

src/client/game.cpp

@@ -724,6 +724,8 @@ class Game {
 			const GameStartData &game_params,
 			std::string &error_message,
 			bool *reconnect,
+			std::string &reconnect_address,


Why not make these const?

appgurueu · 2023-12-28T19:34:26Z

src/client/game.cpp

@@ -4525,7 +4531,9 @@ void the_game(bool *kill,
 		const GameStartData &start_data,
 		std::string &error_message,
 		ChatBackend &chat_backend,
-		bool *reconnect_requested) // Used for local game
+		bool *reconnect_requested,
+		std::string &reconnect_address,


appgurueu · 2023-12-28T19:35:03Z

src/script/lua_api/l_server.cpp

+{
+	NO_MAP_LOCK_REQUIRED;
+
+	const char *playerName = lua_tolstring(L, 1, NULL);


Wouldn't readParam<std::string> work?

Probably will work. I have rebased it, not rewritten it.

appgurueu · 2023-12-28T19:37:43Z

src/network/clientpackethandler.cpp

+		std::string addressPack;
+		*pkt >> addressPack;
+
+		m_access_denied_reason = accessDeniedStrings[denyCode];


Being network-facing code, I think this should be more careful. Shouldn't there be an index check here?

I think that this is fine because of the condition above. denyCode has to be equal to SERVER_ACCESSDENIED_TRANSFER here.

Yep, that's true. (Though I don't see why we do the array lookup then - might as well hardcode the string here...)

appgurueu · 2023-12-28T19:38:03Z

src/network/clientpackethandler.cpp

+		m_access_denied_reason = accessDeniedStrings[denyCode];
+		m_access_denied_reconnect = true;
+
+		size_t splitAt = addressPack.find(';');


I suggest adding a check to deal with the case of ; not being found.

appgurueu · 2023-12-28T19:39:07Z

src/serverenvironment.cpp

+	auto addressPack = address + ";" + port;
+
+	for (RemotePlayer *player : m_players) {
+		if (name.compare(player->getName()) == 0) {


Isn't there some kind of "get player by name" function you could use?

I also think that this should ideally throw an error if no player with the given name is found (rather than failing silently).

It will be probably good to return false as l_disconnect_player in l_server.cpp does.

That depends on whether you would consider this a frequent, recoverable exception (that often does not need to be handled) or an "unrecoverable" "panic" / failure, IMO. I think passing a wrong player name is clearly a logic issue and hence of the latter kind, so should trigger an error. Modders can just check whether a player is online if they want the "lax" behavior (or they could use pcall).

sfence · 2023-12-30T00:18:04Z

@appgurueu
The problem of "redirecting" attacks and similar should be fixed with issue #10206.

sfence · 2023-12-31T23:06:02Z

This PR should be blocked by PR #14196 until it is merged or refused.

red-001 · 2024-08-30T05:44:53Z

I feel this really needs #14226 or at least a way to pass (signed) data to the other server.
Minetest already includes a SHA256 implementation, could fairly easily implement HMAC-SHA256 (should be ~30-50 LOC) and use that with a shared secret the server owners can agree upon. And at that point could add a flag to use an empty password for the other server.

I'm just thinking about possible use-cases for this, I don't really see many at all for transferring a player to a server unknown to the first. It in the worst case even come off as hostile from the point of view of the owner that's suddenly getting unexpected players that didn't join their server in the usual way. On the flip side, for servers where the owners know each other or the same person runs both servers, it might be very useful to pass meta-data or even skip login altogether since the first server already verified the user.

sfence · 2024-08-30T05:58:09Z

@red-001
The idea is that server-to-server communication can be done via the https lua API. Because of it, #14226 has been closed.

Transferring to another server can be used in worlds that have dimensions... like space and planets, normal world and nether... etc...
It can be good for performance requirements because every dimension can run on different HW.

red-001 · 2024-08-31T17:14:29Z

Transferring to another server can be used in worlds that have dimensions... like space and planets, normal world and nether... etc...
It can be good for performance requirements because every dimension can run on different HW.

Those all seem like examples were you would want to be able to pass some metadata with the player and skip login. Generic server to server communications are probably not related, I shouldn't have mentioned that PR.

sfence · 2024-08-31T17:48:51Z

Those all seem like examples were you would want to be able to pass some metadata with the player and skip login. Generic server to server communications are probably not related, I shouldn't have mentioned that PR.

It is not related until you want to sync inventories for example. (fly to moon, mine metals, return back to planet and build some technic staff for example)

appgurueu · 2024-09-03T18:37:01Z

I feel this really needs ~~#14226 or~~ at least a way to pass (signed) data to the other server.

That could be nice to have, but I don't think this is needed for this PR to be useful.

There are plenty of other options for server owners willing to exchange data between their servers. Via trusted mods and luasocket or similar, they can implement whatever they please. The HTTP API is also an option.

red-001 · 2024-09-03T18:47:01Z

Per discussion on IRC, I don't think there is that much bad that an attacker can do by redirecting the player and they would need to be able to either intercept packets, or guess the player peer id, IP address and port number.

Assuming the client won't attempt to register a new account on the server it's redirected to, it doesn't let an attacker than can read packets do anything new. Otherwise this could have allowed them to capture the verifier.
If the attacker can only blindly inject packets, this could open the door to upgrading that to an active packet interception attack, by redirecting the player to an attacker controlled server and forwarding packets to the "real" server.

Could reduce the risk of that by implementing #14988. Should be more difficult to correctly guess the peer_id, IP address, port number and sequence number.

That could be nice to have, but I don't think this is needed for this PR to be useful.

There are plenty of other options for server owners willing to exchange data between their servers. Via trusted mods and luasocket or similar, they can implement whatever they please. The HTTP API is also an option.

Yeah I agree, I just meant there should be some way to pass associated data with the client. Keeps everything nice and in-sync and would allow for more creative access control setups.

Could for instance have a separate realm that someone can join from the main server if they have the correct item (like the Nether in one of those other Voxel games) or a mini-games server you can be redirected to from the main one.
Can't do that as easily if you can't give the player a token they will give the server they are connecting to. If it's any easier could make the data unprotected and leave it to mods to implement JWT or whatever they want to use. I'm really just thinking of some small amount of metadata that can serve as a password replacement and some small amount of configuration data.

Actually could remove the concern around redirects from blind injection attacks mentioned above if a "token" was mandatory instead of a password.

MisterE123 · 2025-02-03T18:11:03Z

Could we get this for 5.12?

sfence · 2025-02-04T06:11:48Z

@MisterE123 Because of the change request here and the state of #14196, I do not expect this to move forward soon.

grapereader added 2 commits December 19, 2023 21:15

Doc update for minetest.transfer_player

287cfad

wsor4035 added @ Server / Client / Env. Feature ✨ PRs that add or enhance a feature Concept approved Approved by a core dev: PRs welcomed! labels Dec 19, 2023

Add transfer_player command to devetst game, rebase bug fixes.

3f7dbd9

appgurueu reviewed Dec 28, 2023

View reviewed changes

sfence mentioned this pull request Dec 31, 2023

Change the way how password is stored in C++ engine. #14196

Open

sfence mentioned this pull request Jan 7, 2024

Basic implementation of working server to server message. #14226

Closed

3 tasks

red-001 mentioned this pull request Aug 15, 2024

Network encryption #14970

Draft

Zughy added the Waiting (on dependency) Waiting on another PR or external circumstances (not for rebases/changes requested) label Aug 31, 2024

		@@ -327,6 +327,9 @@ class Client : public con::PeerHandler, public InventoryManager, public IGameDef

		bool reconnectRequested() const { return m_access_denied_reconnect; }

		std::string getReconnectAddress() const { return m_access_denied_reconnect_address; }

		@@ -228,11 +231,14 @@ bool ClientLauncher::run(GameStartData &start_data, const Settings &cmd_args)
		core::rect<s32>(0, 0, 10000, 10000));

		bool game_has_run = launch_game(error_message, reconnect_requested,

Add transfer_player function to request client move to new server #14129

Are you sure you want to change the base?

Add transfer_player function to request client move to new server #14129

Conversation

sfence commented Dec 19, 2023 • edited Loading

To do

How to test

lhofhansl commented Dec 20, 2023

sfence commented Dec 20, 2023

Zughy commented Dec 20, 2023 • edited Loading

sfence commented Dec 20, 2023

appgurueu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appgurueu Dec 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfence commented Dec 30, 2023

sfence commented Dec 31, 2023

red-001 commented Aug 30, 2024 • edited Loading

sfence commented Aug 30, 2024

red-001 commented Aug 31, 2024 • edited Loading

sfence commented Aug 31, 2024

appgurueu commented Sep 3, 2024

red-001 commented Sep 3, 2024 • edited Loading

MisterE123 commented Feb 3, 2025

sfence commented Feb 4, 2025

sfence commented Dec 19, 2023 •

edited

Loading

Zughy commented Dec 20, 2023 •

edited

Loading

appgurueu left a comment •

edited

Loading

appgurueu Dec 30, 2023 •

edited

Loading

red-001 commented Aug 30, 2024 •

edited

Loading

red-001 commented Aug 31, 2024 •

edited

Loading

red-001 commented Sep 3, 2024 •

edited

Loading