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.

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

[MP] Add peer authentication support to the default MultiplayerAPI. #67917

Merged
merged 2 commits into from Nov 2, 2022

Conversation

Faless
Copy link
Collaborator

@Faless Faless commented Oct 26, 2022

This PR adds support for pre-authenticating peers in SceneMultiplayer before letting them join the multiplayer session (i.e. before RPCs and replication starts).

Workflow explanation:

  • Use set_auth_callback(callback) to enable the authentication features.
    The callback must be a Callable that accepts an int (the peer ID), and a PackedByteArray of data which will be called when authentication data is received.
  • The peer_authenticating(id) signal will be emitted instead of peer_connected when a new peer connects.
  • Use send_auth(id: int, data: PackedByteArray) to send authentication data.
  • Call complete_auth(id: int) when the authentication process is complete and you expect to start receiving game data.
  • The peer_connected signal will be emitted as soon as both parties complete the authentication.
  • Use disconnect_peer(id) to disconnect a connected peer.
  • If the peer_connected signal didn't fire for that peer (i.e. it was still in the authentication phase), the peer_auth_failed signal will be emitted instead of peer_disconnected.

Closes godotengine/godot-proposals#548

Here a demo using it to implement

Draft status:

@Faless
Copy link
Collaborator Author

Faless commented Oct 28, 2022

Made auth_callback a property, added the auth_timeout property to automatically disconnect peers after some time if the authentication fails to complete, and a get_authenticating_peers exposed method to list peers in the authenticating state.

@dsnopek
Copy link
Contributor

dsnopek commented Oct 28, 2022

This looks awesome!

I've started messing around with it, and it would be nice if the connected_to_server signal wasn't emitted until peer 1 has actually managed to authenticate. Of course, connecting to peer_connected and checking if it's peer 1 works, but it's sort of confusing that connected_to_server can be emitted, but you still can't actually send any RPCs.

@Faless
Copy link
Collaborator Author

Faless commented Oct 31, 2022

I've started messing around with it, and it would be nice if the connected_to_server signal wasn't emitted until peer 1 has actually managed to authenticate. Of course, connecting to peer_connected and checking if it's peer 1 works, but it's sort of confusing that connected_to_server can be emitted, but you still can't actually send any RPCs.

I agree, and I think there has historically been some more confusion here since connected_to_server is fired after peer_connected(1) (because it is tied to MultiplayerPeer.connection_established which had to be fired after peer_connected or sending would not work while emitting connected_to_server).

I believe we have a chance to improve this by firing connected_to_server directly in _admit_peer if peer == 1, after adding it to the list of peers but before firing peer_connected(1) (so RPCs to broadcast or server will still work during connected_to_server).

In any case, if we fire connected_to_server from the multiplayer API, we should probably remove the connection_established signal from MultiplayerPeer.
And I believe we could similarly drop server_disconnected and connection_failed and implement them in the Multiplayer API (when polling/updating the connection state). This should remove some overhead when implementing custom MultiplayerPeer, and also remove some unnecessary bindings.

WDYT?

@dsnopek
Copy link
Contributor

dsnopek commented Oct 31, 2022

That all makes sense to me!

@Faless
Copy link
Collaborator Author

Faless commented Oct 31, 2022

That all makes sense to me!

I've done the changes as a separate commit, and added the "breaks compat" tag since it changes the MultiplayerPeer interface (removing the signals), and the order in which connected_to_server and server_disconnected are emitted.

@dsnopek
Copy link
Contributor

dsnopek commented Oct 31, 2022

I've done the changes as a separate commit, and added the "breaks compat" tag since it changes the MultiplayerPeer interface (removing the signals), and the order in which connected_to_server and server_disconnected are emitted.

These changes look great to me!

I tried it with my test project, and now connected_to_server() only happens after the client has managed to authenticate as expected :-)

@Faless Faless marked this pull request as ready for review November 1, 2022 21:38
@Faless Faless requested review from a team as code owners November 1, 2022 21:38
Copy link
Member

@mhilbrunner mhilbrunner left a comment

Choose a reason for hiding this comment

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

Code looks good, giant YESS!! to the feature, left a few docs thoughts.

Add few methods to allow peers to exchange authentication information.

- `set_auth_callback(callback)`: Enable the authentication features.
  The callback is a `Callable` that accepts an `int` (the peer ID), and
  a `PackedByteArray` of data.
- The `peer_authenticating(id)` signal will be emitted instead of
  `peer_connected` when a new peer connects.
- Use `send_auth(id: int, data: PackedByteArray)` to exchange data.
- Call `complete_auth(id: int)` when the authentication process is
  complete and you expect to start receiving game data.
- The `peer_connected` signal will be emitted as soon as both parties
  complete the authentication.
- Use `disconnect_peer(id)` to disconnect a connected peer.
- If the `peer_connected` signal didn't fire for that peer (i.e. it was
  still in the authentication phase), the `peer_auth_failed` signal will
  be emitted instead of `peer_disconnected`.
Now handled directly by the MultiplayerAPI implementation.
@akien-mga akien-mga merged commit 30e4e7c into godotengine:master Nov 2, 2022
@akien-mga
Copy link
Member

Thanks!

@Damnae
Copy link

Damnae commented Mar 20, 2023

Is there any working test for this?

It doesn't appear to work because send_auth expects the peer to be CONNECTION_CONNECTED. The client can't send its authentication because it isn't in connected state until authentication is completed.

Trying to use send_auth causes ERR_UNCONFIGURED.
Waiting for the peer state to be CONNECTION_CONNECTED reaches timeout.

ERR_FAIL_COND_V(multiplayer_peer.is_null() || multiplayer_peer->get_connection_status() != MultiplayerPeer::CONNECTION_CONNECTED, ERR_UNCONFIGURED);

@xaqbr
Copy link

xaqbr commented Oct 16, 2023

Is there any working test for this?

It doesn't appear to work because send_auth expects the peer to be CONNECTION_CONNECTED. The client can't send its authentication because it isn't in connected state until authentication is completed.

Trying to use send_auth causes ERR_UNCONFIGURED. Waiting for the peer state to be CONNECTION_CONNECTED reaches timeout.

ERR_FAIL_COND_V(multiplayer_peer.is_null() || multiplayer_peer->get_connection_status() != MultiplayerPeer::CONNECTION_CONNECTED, ERR_UNCONFIGURED);

Call send_auth in your peer_authenticating(id, data) signal callback after creating the client. Both peers will receive the signal when authentication starts and they must successfully authenticate each other before connection proceeds. Make sure the data parameter isn't empty or it will silently fail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the ability to send authentication information in before accepting RPC calls
6 participants