Connection validation with shared key#14
Connection validation with shared key#14robinnorth wants to merge 3 commits intoMichalPetryka:masterfrom
Conversation
…very and connection requests
|
|
||
| ExternalIp = await device.GetExternalIPAsync(); | ||
| await device.CreatePortMapAsync(new Mapping(networkProtocolType, IPAddress.None, port, port, 0, ApplicationName)).ConfigureAwait(false); | ||
| await device.CreatePortMapAsync(new Mapping(networkProtocolType, IPAddress.None, port, port, 0, Application.productName)).ConfigureAwait(false); |
There was a problem hiding this comment.
This won't work, as this is a task and that property can only be fetched from the main thread.
There was a problem hiding this comment.
That does execute on the main thread in my test project for me, but I will change it to be thread-safe, as it probably can't be guaranteed that it will always run on the main thread.
There was a problem hiding this comment.
Oh, i forgot that unity always runs async code on the main thread, still, having it fetched outside is safer.
| { | ||
| internal static ushort LastForwardedPort; | ||
| internal static readonly string ApplicationName; | ||
| public static string SharedKey { get; set; } |
There was a problem hiding this comment.
I'd rather have that shared key as a static protected variable in LiteNetLib4MirrorDiscovery that'd be set by a protected virtual method that could be overriden.
There was a problem hiding this comment.
OK, sure thing. I will implement it as you describe.
There was a problem hiding this comment.
Actually, because LiteNetLib4MirrorTransport currently uses LiteNetLib4MirrorUtils.ApplicationName to generate a connection key, LiteNetLib4MirrorDiscovery might not be the best place for this, as for the use case this PR addresses, both network discovery and connection need to be independent of using Application.productName, and you probably wouldn't want to have LiteNetLib4MirrorTransport reference LiteNetLib4MirrorDiscovery.SharedKey in its GetConnectKey method. That is why I initially went with adding this to LiteNetLib4MirrorUtils.
Perhaps this shared key could be a static protected property in the transport instead, with a protected virtual method to set it? Or even just a serialized variable in the transport, in the same way that authKey currently is?
There was a problem hiding this comment.
GetConnectData which uses GetConnectKey is already overridable there.
There was a problem hiding this comment.
I think GetConnectKey itself needs to be overridable, as ServerStart calls it directly. Or LiteNetLib4MirrorTransport needs a similar protected ConnectKey property and overridable setter method (which would match the proposed change to LiteNetLib4MirrorDiscovery).
There was a problem hiding this comment.
Oh, it does call that, then please make that overridable.
2 participants
This PR renames
LiteNetLib4MirrorUtils.ApplicationNametoLiteNetLib4MirrorUtils.SharedKeyand makes use of it everywhere discovery and connection requests are validated.Users can override the default value of the key (
"Application.productName + Application.companyName") before initialising their network code, as follows:This removes the reliance on client and server apps to have the same product name to connect to one another, as discussed in #13, but leaves the default behaviour of the transport unchanged.