Page MenuHomeWildfire Games

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

Authored by Silier 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.

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
170

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

source/network/scripting/JSInterface_Network.cpp
59
source/lobby/XmppClient.cpp
99–100

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

374

Strange naming.

source/network/NetClient.cpp
219–227

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

250

Why so? And on which thread?

source/network/NetServer.h
154

No need to copy the string.

source/network/StunClient.cpp
399

It's better to check enet_address_get_host_ip for success.

source/network/tests/test_Net.h
77

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
369–383

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

844

Why is it needed to do this check?

848

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.

854

useless blank line

source/network/NetClient.cpp
173

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

183

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

202

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

source/network/NetClient.h
115

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
Silier marked 34 inline comments as done.Jan 13 2021, 5:18 PM
Silier 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
844

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

848

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

source/network/NetClient.cpp
173

vs does not like it

219–227

see JSI_Network::StartNetworkJoin

250

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

source/network/NetClient.h
115

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
844

👍

848

k I was wondering that.

source/network/NetClient.h
115

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

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

fix most of the comments

source/network/NetClient.h
115

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
107–109
240

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
299

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

source/network/StunClient.cpp
395–400
/** 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
78–81

Maybe.

Silier 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

Should "" be getDefaultPassword?

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

Might be ServerHasPassword for consistency or the other way around

60

Nuke

265

Can you fix ? + Final dot

357

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

source/lobby/XmppClient.cpp
99–100

Invalid naming should be m_PascalCase

source/network/NetClient.cpp
190

This method and the onr below could be const I think

250

I assume there is no callback?

source/network/NetClient.h
111

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

source/network/NetServer.cpp
1584

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

source/network/scripting/JSInterface_Network.cpp
81

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

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

no, it is setting for host

357

no, this is actual password

source/network/NetClient.h
111

it is wroking in svn

source/network/NetServer.cpp
1584

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
60

no, it is setting for host

I meant the removal of thz line abovz

source/network/NetClient.h
111

Did you try no pch?

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

Still looking good'

source/network/NetClient.h
111

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.

240–242

(suggestion)

source/network/scripting/JSInterface_Network.cpp
81

It's used here, it's below

148

UNUSED

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

binaries/data/mods/public/gui/gamesetup/NetMessages/GameRegisterStanza.js
99

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–21

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

source/lobby/XmppClient.cpp
99–100

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

source/network/scripting/JSInterface_Network.cpp
148

L162 :)

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

Then m_PlayerMapUpdate is wrong.

Silier added inline comments.Jan 20 2021, 7:20 PM
source/network/NetClient.cpp
190

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.