Skip to content
This repository has been archived by the owner on Jan 29, 2021. It is now read-only.

Connection validation with shared key #14

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Connection validation with shared key #14

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4bddaff
Allow users to override default shared key used when validating disco…
robinnorth Nov 25, 2019
3bfa98d
Split shared key into separate discovery and connection keys that can…
robinnorth Nov 29, 2019
020e85b
Match existing indent style
robinnorth Nov 29, 2019
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
4 changes: 2 additions & 2 deletions ...ib4Mirror/Assets/Mirror/Runtime/Transport/LiteNetLib4Mirror/LiteNetLib4MirrorDiscovery.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public static void StopDiscovery()

private static void OnDiscoveryResponse(IPEndPoint remoteendpoint, NetPacketReader reader, UnconnectedMessageType messagetype)
{
if (messagetype == UnconnectedMessageType.BasicMessage && reader.TryGetString(out string application) && application == Application.productName)
if (messagetype == UnconnectedMessageType.BasicMessage && reader.TryGetString(out string key) && key == LiteNetLib4MirrorUtils.SharedKey)
{
Singleton.onDiscoveryResponse.Invoke(remoteendpoint, LiteNetLib4MirrorUtils.FromBase64(reader.GetString()));
}
Expand All @@ -82,7 +82,7 @@ private static void OnDiscoveryResponse(IPEndPoint remoteendpoint, NetPacketRead

internal static void OnDiscoveryRequest(IPEndPoint remoteendpoint, NetPacketReader reader, UnconnectedMessageType messagetype)
{
if (messagetype == UnconnectedMessageType.Broadcast && reader.TryGetString(out string application) && application == Application.productName && Singleton.ProcessDiscoveryRequest(remoteendpoint, LiteNetLib4MirrorUtils.FromBase64(reader.GetString()), out string response))
if (messagetype == UnconnectedMessageType.Broadcast && reader.TryGetString(out string key) && key == LiteNetLib4MirrorUtils.SharedKey && Singleton.ProcessDiscoveryRequest(remoteendpoint, LiteNetLib4MirrorUtils.FromBase64(reader.GetString()), out string response))
{
LiteNetLib4MirrorCore.Host.SendUnconnectedMessage(LiteNetLib4MirrorUtils.ReusePutDiscovery(DataWriter, response, ref _lastDiscoveryMessage), remoteendpoint);
}
Expand Down
2 changes: 1 addition & 1 deletion ...ib4Mirror/Assets/Mirror/Runtime/Transport/LiteNetLib4Mirror/LiteNetLib4MirrorTransport.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ internal void InitializeTransport()

private static string GetConnectKey()
{
return LiteNetLib4MirrorUtils.ToBase64(Application.productName + Application.companyName + Application.unityVersion + LiteNetLib4MirrorCore.TransportVersion + Singleton.authCode);
return LiteNetLib4MirrorUtils.ToBase64(LiteNetLib4MirrorUtils.SharedKey + Application.unityVersion + LiteNetLib4MirrorCore.TransportVersion + Singleton.authCode);
}

#region Unity Functions
Expand Down
12 changes: 6 additions & 6 deletions ...NetLib4Mirror/Assets/Mirror/Runtime/Transport/LiteNetLib4Mirror/LiteNetLib4MirrorUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ namespace Mirror.LiteNetLib4Mirror
public static class LiteNetLib4MirrorUtils
{
internal static ushort LastForwardedPort;
internal static readonly string ApplicationName;
public static string SharedKey { get; set; }
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

OK, sure thing. I will implement it as you describe.

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Owner

Choose a reason for hiding this comment

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

GetConnectData which uses GetConnectKey is already overridable there.

Copy link
Author

Choose a reason for hiding this comment

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

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).

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, it does call that, then please make that overridable.

public static bool UpnpFailed { get; private set; }
public static IPAddress ExternalIp { get; private set; }

static LiteNetLib4MirrorUtils()
{
ApplicationName = Application.productName;
SharedKey = Application.productName + Application.companyName;
}

public static string ToBase64(string text)
Expand All @@ -52,11 +52,11 @@ public static NetDataWriter ReusePut(NetDataWriter writer, string text, ref stri

public static NetDataWriter ReusePutDiscovery(NetDataWriter writer, string text, ref string lastText)
{
if (ApplicationName + text != lastText)
if (SharedKey + text != lastText)
{
lastText = ApplicationName + text;
lastText = SharedKey + text;
writer.Reset();
writer.Put(ApplicationName);
writer.Put(SharedKey);
writer.Put(ToBase64(text));
}

Expand Down Expand Up @@ -155,7 +155,7 @@ private static async Task ForwardPortInternalAsync(ushort port, int milisecondsD
}

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);
Copy link
Owner

Choose a reason for hiding this comment

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

This won't work, as this is a task and that property can only be fetched from the main thread.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, i forgot that unity always runs async code on the main thread, still, having it fetched outside is safer.

LastForwardedPort = port;
Debug.Log($"Port {port.ToString()} forwarded successfully!");
}
Expand Down