Page MenuHomeWildfire Games

Hide ip and port from users until they want to join, add optional password
ClosedPublic

Authored by Angen on Dec 6 2020, 2:53 PM.

Details

Reviewers
wraitii
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP24728: Hide ip and port from users until they want to join, add optional password
Summary

Current issue with the lobby, is that we make ips of hosts public for anyone to read. This patch consists of 3 parts.
1.) Removing ips and ports from lobby javascript
2.) Removing need of script on the server to attach public ips to game stanza by asking the host using xmppclient as proxy.
3.) Implementing password protected matches, to deny this information to not trusted players.

Further description:
Do not send ports and stunip to the bots.

Removed from stanza.

Do not send ip to the lobby.

Removed from mapping gamelist from backend to gui (still on the backend side, because it is done by script on 0ad server).

Get ip and ports on request when trying to connect.

On the host side, ask stun server what is host's public ip and remember it.
On the client side, send iq through xmppclient to the hosting player and ask for ip, port and if Stun is used, then if answer is success, continue 
   with connecting, else fail.

Add optional password for matches.

Add password required identifier to the stanza. 
Allow host to setup password for the match. Hash it on the host side and store inside Netserver. If no password is given, matches will behave 
as it is not required.
On the client side, if password for the match is required, show additional window before trying to connect and ask for password, then hash it 
and send with iq request for ip, port and stun.
Server will answer with ip, port and stun only if passwords matches, else will asnwer with error string.

Some security:
Passwords are hashed before sending, so it is not easy to guess what users typed. (per wraitii)
Hashes are using different salt as lobby hashing and not using usernames as salt (as that is not doable), so they are different even typing the
same password as for the lobby account.
Client remembers which user was asked for connection data and iq's id of request. If answer doesn't match these things, it is ignored. (thnx
user1)
Every request for connection data is logged with hostname of the requester to the mainlog file (no ips).
If user gets iq to send connection data and is not hosting the match, will respond with error string "not_server".
If server gets iq::result with connection data, request is ignored.

Test Plan

Test connecting, rejoining with and without Stun.
Test local network with lobby. if works without patch.
Test LAN (no lobby).
Test it is not possible to connect with matches with incorrect password.
Test only matches that requires password (it was given by host) do require password for clients before trying to connect.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
SeverityLocationCodeMessage
Warningbinaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js:215ESLintBear (indent)ESLintBear (indent)
Warningbinaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js:404ESLintBear (no-trailing-spaces)ESLintBear (no-trailing-spaces)
Unit
No Unit Test Coverage
Build Status
Buildable 14204
Build 29801: Vulcan BuildJenkins
Build 29800: Vulcan Build (macOS)Jenkins
Build 29799: Vulcan Build (Windows)Jenkins
Build 29798: arc lint + arc unit

Unit TestsFailed

TimeTest
0 msJenkins > cxxtest_debug.xml::[failed-to-read]
Failed to read test report file E:\Jenkins\workspace\vs2015-differential\cxxtest_debug.xml org.dom4j.DocumentException: Error on line 347 of document : Content is not allowed in trailing section. at org.dom4j.io.SAXReader.read(SAXReader.java:511)
0 msJenkins > TestAllocators::Debug Build & Tests / test_da
0 msJenkins > TestAllocators::Release Build & Tests / test_da
0 msJenkins > TestAllocators::test_da
0 msJenkins > TestAllocators::test_da
View Full Test Results (1 Failed · 1,699 Passed)

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Stan added inline comments.Jan 12 2021, 12:04 PM
binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
108–129
157

I assume we can't reuse the translated message for the error? Why not reportConnectionFail(reason)

source/network/scripting/JSInterface_Network.cpp
56
source/lobby/XmppClient.cpp
96–97

No need to pass empty string to std::string, it has a valid default c-tor.

366

Strange naming.

source/network/NetClient.cpp
219–227 ↗(On Diff #14737)

No need to search for a port for the client. The code seems weird.

250 ↗(On Diff #14737)

Why so? And on which thread?

source/network/NetServer.h
154 ↗(On Diff #14737)

No need to copy the string.

source/network/StunClient.cpp
399 ↗(On Diff #14737)

It's better to check enet_address_get_host_ip for success.

source/network/tests/test_Net.h
77 ↗(On Diff #14737)

Range-based for.

Stan added a comment.Jan 12 2021, 12:42 PM

@Angen can you mark the inlines as done so it's easy to hide?

Stan added a comment.Jan 12 2021, 7:03 PM

@user1 says it's a release blocker, and that he will review it.

wraitii requested changes to this revision.Jan 13 2021, 3:22 PM

I think on the whole this looks pretty good & is a very worthwhile improvement.
There are two things I'd like to change, but one can be done later:

  • It seems to me you should always use SetupServerData+TryToConnect now instead of SetupConnection directly. This would also reduce duplication. This IMO needs to be changed.
  • The match password is only used to get the connection data. Once one has that, one could in principle 'illegally' connect to a game. Since the g_NetServer already knows the match password, you could add it to the netclient & validate on Authentication that the password is valid. This would reinforce the security. This can be done for A25 though IMO, as it's not critical (could also be merged with D3075 which is kinda similar).
source/lobby/XmppClient.cpp
361

Documentation: Request the Connection data (ip, port...) from the server.

837

Why is it needed to do this check?

841

I think it would be more logical to call PushGuiMessage directly.
Presumably you should also SAFE_DELETE(g_NetClient) here.

If you end up keeping the function, consider renaming it to HandleGetConnectionDataFailed, this makes it look like an FSM state but it's not one.

847

useless blank line

source/network/NetClient.cpp
179 ↗(On Diff #15163)

Likewise, I'd ENSURE(!m_serverAddress.empty())

190 ↗(On Diff #15163)

I would ENSURE (!m_Session), this is called from C++ code only.

209 ↗(On Diff #15163)

I think you should use this function elsewhere, as you're introducing duplication otherwise.

source/network/NetClient.h
117 ↗(On Diff #15163)

As written elsewhere, I think SetupConnection needs to be removed from the public interface now that TryToConnect exists. Add documentation to these functions too.

If you keep SetupConnection, move it below the other two so it reflects call order.

This revision now requires changes to proceed.Jan 13 2021, 3:22 PM
Angen marked 34 inline comments as done.Jan 13 2021, 5:18 PM
Angen marked 7 inline comments as done.Jan 13 2021, 7:18 PM

well, we do not need now bunch of code in JSI_Network::StartNetworkJoin

source/lobby/XmppClient.cpp
837

because iq is identified by sender and its id, so we react only to the ones we asked and no-one can break us just by sending random answer

841

no, js is closing connection like in case of other error messages

source/network/NetClient.cpp
179 ↗(On Diff #15163)

vs does not like it

219–227 ↗(On Diff #14737)

see JSI_Network::StartNetworkJoin

250 ↗(On Diff #14737)

Because we need to wait for answer
ref: JSI_Network::StartNetworkJoin

source/network/NetClient.h
117 ↗(On Diff #15163)

cant, JSI_Network::StartNetworkHost needs it and using try to connect is overkill even if doable

wraitii added inline comments.Jan 13 2021, 7:21 PM
source/lobby/XmppClient.cpp
837

👍

841

k I was wondering that.

source/network/NetClient.h
117 ↗(On Diff #15163)

AH you're right. Still, I'd move it below + doc.

Angen updated this revision to Diff 15266.Jan 13 2021, 7:53 PM

fix most of the comments

source/network/NetClient.h
117 ↗(On Diff #15163)

hmm, maybe it is actually doable anyway

Build has FAILED

builderr-debug-gcc7.txt
cc1plus: error: one or more PCH files were found, but they were invalid
cc1plus: error: use -Winvalid-pch for more information
cc1plus: fatal error: obj/network_Debug/precompiled.h: No such file or directory
compilation terminated.
make[1]: *** [network.make:145: obj/network_Debug/NetClient.o] Error 1
make: *** [Makefile:77: network] Error 2

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/4510/display/redirect
See console output for more information: https://jenkins.wildfiregames.com/job/docker-differential/4510/display/redirectconsole

Build is green

builderr-debug-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libnetwork_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/liblobby_dbg.a(precompiled.o) has no symbols
../../../source/network/scripting/JSInterface_Network.cpp:169:71: warning: unused parameter 'pCmptPrivate' [-Wunused-parameter]
void JSI_Network::StartNetworkJoinLobby(ScriptInterface::CmptPrivate* pCmptPrivate, const CStrW& playerName, const CStr& hostJID, const CStr& password)
                                                                      ^
1 warning generated.
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libengine_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

See https://jenkins.wildfiregames.com/job/macos-differential/2851/display/redirect for more details.

wraitii accepted this revision.Jan 15 2021, 4:34 PM

Overall I think this looks pretty good, and seemed to work in our tests. Given the time-frame, I'd rather commit and do some tweaks after, so accepting.

I'm not sure if we need to check the password in Authenticate or not. It might be possible for people to 'save' the IP of a host's public game, then use that to try and private games of that same host, but that seems like a rather minor concern maybe?
If not, it shouldn't be too difficult to add, but can be done after this patch IMO.

source/network/NetClient.h
115–116 ↗(On Diff #15266)
248 ↗(On Diff #15266)

Doc

This revision is now accepted and ready to land.Jan 15 2021, 4:34 PM
Stan added inline comments.Jan 15 2021, 4:47 PM
binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
236

Maybe the name should make it clear it's a boolean?

source/network/StunClient.cpp
395–400 ↗(On Diff #15266)
/** Gives the printable form of the IP address specified in the address parameter.
    @param address    address printed
    @param hostName   destination for name, must not be NULL
    @param nameLength maximum length of hostName.
    @returns the null-terminated name of the host in hostName on success
    @retval 0 on success
    @retval < 0 on failure
*/
ENET_API int enet_address_get_host_ip (const ENetAddress * address, char * hostName, size_t nameLength);
source/network/tests/test_Net.h
77 ↗(On Diff #15266)

Maybe.

Angen updated this revision to Diff 15413.Jan 17 2021, 11:19 AM

password -> hasPassword
const -> constexpr
b -> connectionData
some docs

Build has FAILED

builderr-debug-gcc7.txt
cc1plus: error: one or more PCH files were found, but they were invalid
cc1plus: error: use -Winvalid-pch for more information
cc1plus: fatal error: obj/network_Debug/precompiled.h: No such file or directory
compilation terminated.
make[1]: *** [network.make:145: obj/network_Debug/NetClient.o] Error 1
make: *** [Makefile:77: network] Error 2

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/4571/display/redirect
See console output for more information: https://jenkins.wildfiregames.com/job/docker-differential/4571/display/redirectconsole

Build has FAILED

builderr-debug-macos.txt
fatal errorfatal error: fatal errorfatal error: : file '/Users/wfg/Jenkins/workspace/macos-differential/build/workspaces/gcc/../../../libraries/source/spidermonkey/include-unix-debug/mozilla/StaticAnalysisFunctions.h' has been modified since the precompiled header 'obj/network_Debug/precompiled.h.gch' was builtfile '/Users/wfg/Jenkins/workspace/macos-differential/build/workspaces/gcc/../../../libraries/source/spidermonkey/include-unix-debug/mozilla/StaticAnalysisFunctions.h' has been modified since the precompiled header 'obj/network_Debug/precompiled.h.gch' was built: 

file '/Users/wfg/Jenkins/workspace/macos-differential/build/workspaces/gcc/../../../libraries/source/spidermonkey/include-unix-debug/mozilla/StaticAnalysisFunctions.h' has been modified since the precompiled header 'obj/network_Debug/precompiled.h.gch' was built
file '/Users/wfg/Jenkins/workspace/macos-differential/build/workspaces/gcc/../../../libraries/source/spidermonkey/include-unix-debug/m

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/2912/display/redirect
See console output for more information: https://jenkins.wildfiregames.com/job/macos-differential/2912/display/redirectconsole

Build is green

builderr-debug-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libnetwork_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libtinygettext_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libtinygettext_dbg.a(tinygettext.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/liblobby_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libglooxwrapper_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file

See https://jenkins.wildfiregames.com/job/macos-differential/2913/display/redirect for more details.

Build is unstable

builderr-debug-gcc7.txt
../../../source/network/scripting/JSInterface_Network.cpp: In function 'void JSI_Network::StartNetworkJoinLobby(ScriptInterface::CmptPrivate*, const CStrW&, const CStr8&, const CStr8&)':
../../../source/network/scripting/JSInterface_Network.cpp:169:71: warning: unused parameter 'pCmptPrivate' [-Wunused-parameter]
 void JSI_Network::StartNetworkJoinLobby(ScriptInterface::CmptPrivate* pCmptPrivate, const CStrW& playerName, const CStr& hostJID, const CStr& password)
                                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
../../../source/lib/res/h_mgr.cpp: In function 'void h_free_hd(HDATA*)':
../../../source/lib/res/h_mgr.cpp:560:27: warning: 'void* memset(void*, int, size_t)' clearing an object of type 'struct HDATA' with no trivial copy-assignment; use assignment or value-initialization instead [-Wclass-memaccess]
  memset(hd, 0, sizeof(*hd));
                           ^
../../../source/lib/res/h_mgr.cpp:132:8: note: 'struct HDATA' de

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/4572/display/redirect
See console output for more information: https://jenkins.wildfiregames.com/job/docker-differential/4572/display/redirectconsole

Stan added a comment.Jan 18 2021, 11:11 PM

Some more comments.

binaries/data/mods/public/gui/gamesetup/NetMessages/GameRegisterStanza.js
99 ↗(On Diff #14737)

Should "" be getDefaultPassword?

binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
19

Might be ServerHasPassword for consistency or the other way around

57

Nuke

215

Can you fix ? + Final dot

285

Is it hasPassword? Could have default value to false so the check below is simpler.

source/lobby/XmppClient.cpp
96–97

Invalid naming should be m_PascalCase

source/network/NetClient.cpp
250 ↗(On Diff #14737)

I assume there is no callback?

197 ↗(On Diff #15413)

This method and the onr below could be const I think

source/network/NetClient.h
119 ↗(On Diff #15413)

Not sure where EnetHost is pulled but you might be missing and include

source/network/NetServer.cpp
1576 ↗(On Diff #15413)

I think default port is 20595? Any reason you need this default?

source/network/scripting/JSInterface_Network.cpp
54

I think you need the UNUSED2 macro on pCmptPrivate to make the logs happy

Angen added inline comments.Jan 20 2021, 6:19 AM
binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
57

no, it is setting for host

285

no, this is actual password

source/network/NetClient.h
119 ↗(On Diff #15413)

it is wroking in svn

source/network/NetServer.cpp
1576 ↗(On Diff #15413)

not really, hmm I thought I removed all ""

Stan added inline comments.Jan 20 2021, 8:09 AM
binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
57

no, it is setting for host

I meant the removal of thz line abovz

source/network/NetClient.h
119 ↗(On Diff #15413)

Did you try no pch?

wraitii accepted this revision.Jan 20 2021, 9:12 AM

Still looking good'

source/network/NetClient.h
119 ↗(On Diff #15413)

The point is that this was already there, so it can't possibly break svn.
Regardless, it is forward declared and typedefed above so this works.

249–251 ↗(On Diff #15413)

(suggestion)

source/network/scripting/JSInterface_Network.cpp
54

It's used here, it's below

84

UNUSED

Angen added a comment.Jan 20 2021, 2:24 PM

ok, i ll do some renaming today and commit this as is.

binaries/data/mods/public/gui/gamesetup/NetMessages/GameRegisterStanza.js
99 ↗(On Diff #14737)

it is not related to default password, it is there to send empty string

binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
19

I wanted other way around, but requires is long word for stanza, has is a lot shorter :)

source/lobby/XmppClient.cpp
96–97

yes, but cc says follow style of the document trying to edit as the rule above cc guidelines

source/network/scripting/JSInterface_Network.cpp
84

L162 :)

Stan added inline comments.Jan 20 2021, 2:42 PM
source/lobby/XmppClient.cpp
96–97

Then m_PlayerMapUpdate is wrong.

Angen added inline comments.Jan 20 2021, 7:20 PM
source/network/NetClient.cpp
197 ↗(On Diff #15413)

cant, pushguimessage is function of CNetClient and cant be const

This revision was landed with ongoing or failed builds.Jan 20 2021, 7:31 PM
This revision was automatically updated to reflect the committed changes.