Closed raymoo closed 5 years ago
I should note that how it currently works is probably fine if you want to play with a group of friends that trust each other not to cheat. These are mainly issues where you might want to play with untrusted clients.
Any peer can send a slave RPC, even if they aren't the server.
That is the whole point of the EDIT: I exchanged the slave
keyword.master
and slave
keyword, sorry!
Anyway, that shouldn't be the case, see #7853, clients should only run the RPC when it is called from the master sets with set_network_master
.
Any peer can send a master RPC as well
That shouldn't be the case, see #7853. EDIT: I exchanged the master
and slave
keyword, sorry!
This is the purpose of the master
keyword, which means you should check the caller with get_tree().get_rpc_sender_id()
if you want to know who called it.
Did you test it? Last time I checked with my "cheating demo" it was working fine.
Peers could send RPCs with arguments of the wrong type, requiring every RPC call with arguments to check their arguments' types at the beginning, or risk crashing.
This is a good point, but something we can't really fix unless/until we introduce typing in GDScript. Documenting it will be nice though.
@Faless I thought that the slave
keyword just means the RPC is received by slaves, and the master
keyword means it is received by the master of a node?
@raymoo you are right, I exchanged the two keywords, my bad. I updated my comment.
@Faless I thought that the server routed slave
RPCs between different clients? Or was that changed?
As for master
RPCs calls, I did say that you can check the sender. I also said that this was a problem because it requires a boilerplate check at the top of every RPC that is meant to come from the server. But if slave
RPCs can really only be sent by peers that are the master of the node, I guess this could be solved by having a separate "server messages" node that the server is always a master of. It would still be annoying, but you would only need to split the node once, instead of needing a check at the beginning of every RPC.
EDIT: Oh, I guess that would mean the server still routes messages, but clients will only accept a slave
RPC from a peer that is a master
of the node.
thought that the server routed slave RPCs between different clients?
That is correct, though the original sender is preserved, so the client which receives it knows that the caller is not the server itself but another peer (and which one).
I also said that this was a problem because it requires a boilerplate check at the top of every RPC that is meant to come from the server
I see your point here, but can't come up with a solution.
If you want the check done for you just use the slave
keyword and set the correct master.
i.e.:
use NodeA.send_message (slave
function, owned by server id: 1) for server -> client communication.
use NodeB.send_message (slave
function, owned by a specific client) for client -> server and client->client communication.
Still, if you are worried that a client can cheat by sending different messages to different clients (i.e. you want to implement server checks) then you actually have to set the master=1 in the other clients on NodeB, and do the relay manually within your server (but most of the time disallowing client->client communication is what you actually want to do, as the proper way to do multiplayer with no cheating is to have the clients only send input, the server run the simulation, and then send results back to the clients).
@Faless Makes sense to me.
Peers could send RPCs with arguments of the wrong type
This may soon be possible to handle on the networking side once we got type hints with @vnen's GDScript type improvements :)
Nice, I would add that I just realized that the sync
keyword do not
perform the checks on who's the network master, but I think it should. I'll
make a PR for that, so we can discuss it in further details.
On Sun, May 13, 2018, 02:05 Max Hilbrunner notifications@github.com wrote:
Peers could send RPCs with arguments of the wrong type
This may soon be possible to handle on the networking side once we got type hints with @vnen https://github.com/vnen's GDScript type improvements :)
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/16733#issuecomment-388591673, or mute the thread https://github.com/notifications/unsubscribe-auth/ABnBbgM1W_Yl8dLXsLxa78de6E3obqFoks5tx3jmgaJpZM4SHyNJ .
This may soon be possible to handle on the networking side once we got type hints [...]
Adding reference to #19264
Type checking is now enforced if you use typed GDScript:
E.g.: In the multiplayer bomber
# gamestate.gd
func _connected_ok():
# [...]
# Sending a number instead of the player name
rpc("register_player", get_tree().get_network_unique_id(), 42)
# [...]
remote func register_player(id, new_player_name : String): # Setting string type
if get_tree().is_network_server():
# [...]
Will result in the server not calling the function and displaying the following error instead.
ERROR: _process_rpc: RPC - 'Node(gamestate.gd)::register_player': Cannot convert argument 2 from int to String.
At: core/io/multiplayer_api.cpp:295.
Is it possible to detect programatically when such an error happens? (For example so a client program can display a message to the user and quit when it receives bad data from a server)
Is it possible to detect programatically when such an error happens?
No :cry: . We plan to add support to programmatically handle MultiplayerAPI's errors after 3.1
.
Godot version:
3.0
Issue description:
RPCs have some issues right now that make them annoying to secure:
slave
RPC, even if they aren't the server. This means every RPC that is meant to come from the server needs to include a check at the beginning to ensure it actually is the server. I suggest that in addition to theremote
,sync
,master
, andslave
keywords that specify the recipients of an RPC call, there should also be a keyword to specify that a message is meant to only be sent by the server.EDIT: Any peer can send a
master
RPC as well, which is a problem for similar reasons toslave
RPCs.