- Add the ability to disable TLS encryption (typically in order to circumvent #5349).
- Add the ability to enable TLS cert verification (in order to have full TLS security whenever users get a patched gloox from their system packages)
- Remove security claims from the Privacy Policy.
Details
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 6470 Build 10712: Vulcan Build Jenkins Build 10711: arc lint + arc unit
Event Timeline
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/796/
binaries/data/mods/public/gui/prelobby/common/terms/Privacy_Policy.txt | ||
---|---|---|
42 ↗ | (On Diff #7000) | Alternatively, one could replace is by can be, which would create extra translation work for little benefit, and would be dishonest considering TLS encryption is not fully implemented as long as certificate verification is broken. |
As mentioned in https://trac.wildfiregames.com/ticket/5349#comment:4 the question is whether gloox::TLSOptional should not rather become gloox::TLSDisabled in XmppClient.cpp if it crashes when trying the handshake.
I installed Fedora 29 on virtualbox yesterday, but I didn't succeed to compile yet (out of memory errors and bla).
(It seems gloox/TLS is so broken that I wonder if it's not better to disable TLS entirely and only have the interested people play with it.
But then again we wouldn't get feedback like the one from Fedora 29, so maybe it's a better compromise to add this option.)
binaries/data/mods/public/gui/options/options.json | ||
---|---|---|
382 ↗ | (On Diff #7000) | -Enable (all booleans options enable) |
388 ↗ | (On Diff #7000) | -Enable Experimental |
389 ↗ | (On Diff #7000) | As certificate verification is broken in gloox, it doesn't work on any platform currently, so not sure if this adds value other than the hypothetical use case where gloox is fixed upstream. (For #5344 eventually, it should probably be message boxes asking for confirmation whether to proceed with an illegal or not certificate if there isn't a certificate provided, rather than a config file, but I guess that's irrelevant for now) (entirely secure -> security seems binary, either it is or isn't) |
binaries/data/mods/public/gui/prelobby/common/terms/Privacy_Policy.txt | ||
42 ↗ | (On Diff #7000) | ack |
Testing on fedora 28: patch doesn't solve the segfault, with or without the patch both segfault on lobby join. If required I could get access to a fedora 29 machine for testing.
@bb Got a stacktrace for the lobby crash ? To see if it has anything in common with Mac ?
Stacktrace is on the trac ticket.
Tested on Fedora 29, with gloox::TLSDisabled it doesn't crash, with TLSOptional it does.
If this is going to be changed to encompass the TLSDisabled option, perhaps lobby.require_tls should be renamed (a three way choice would also be an option, or disabling this crap altogether).
Better would be a fix, investigating.
Fedora 29 uses GnuTLS 3.6.4 and gloox 1.0.14.
I've compiled with that GnuTLS version and the most recent gloox windows on Ubuntu 18.04 but can't reproduce there.
I suppose the checkbox is even relevant if the Fedora thing is fixed, because the next best platform might crash randomly as well.
Use TLSDisabled, rephrase, nuke cert verification option until we have at least one machine where it works.
Confirmed with wireshark that it works.
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/802/
Looks good to me! Please commit it if this allows Fedora users to work around the crash.
binaries/data/mods/public/gui/prelobby/common/terms/Privacy_Policy.txt | ||
---|---|---|
42 ↗ | (On Diff #7000) | Actually we can keep it as it's still enbled by default, if someone posts his password in the chat or disables encryption, not the fault of the service provider. (The sentence is more conflicting with the by default disabled certificate verification actually. Will be hard for someone to claim personal damages due to this fact, so whatever) |
Better would be a fix, investigating.
Compiling the entire GnuTLS / gloox / 0ad stack cleanly everytime in a VM takes 30min and makes my system unusuable or even freezes it, so it ended up being deleted...
especially because the next best platform can have broken gloox as well,
considering how many gloox/gnutls bugs are already reported,
and considering that I hardly know every detail about gloox, gnutls and fedora for every version.
While typing the commit message, one always stumbles upon thoughts one didn't have in the revision proposal:
I wonder if the options dialog is the right place.
Users don't know that they need to visit the options dialog and change an option to solve the crash. They will report it regardless as a bug and then we can inform them what to do.
The only difference that this patch introduces is that they don't need to edit a file but can do it in the GUI.
But if the "TLS Encryption" option checkbox were in the login/register dialog, the user might guess to disable that checkbox, without us having to inform him of the existence and receommendation of that option.
Incidentally it would also be closer to what we want in #5344.
Let's see whether the lobby/login/register rewrite made that easier...
It's getting more crowded. The terms buttons and feedback thing are uglily too large (but shrinking them would look differently awkward.
The new checkbox looks okay I think, but it still seems getting more confusing to the average user.
I guess it's better if the option wouldn't exist and if there was only a message box popping up, asking the user if he wants to proceed if the connection is unsafe, otherwise presume TLS.
But that would require a bit more rework - first attempting to login with TLS enabled and then asking if that failed - which is also not an option to circumvent any TLS handshake crashes...
(We could add an icon on the left and the buttons on the right, perhaps something like we find when doing a websearch for "gdpr icon".
Notice Article 12 https://gdpr-info.eu/art-12-gdpr/ speaks about "standardised icons", but I've never seen them.)
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Default... Executing section Source... Executing section JS... | | [NORMAL] ESLintBear (semi): | | Missing semicolon. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/prelobby/common/encryption/encryption.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/prelobby/common/encryption/encryption.js | 10| 10| | 11| 11| function updateEncryption(enabled) | 12| 12| { | 13| |- saveSettingAndWriteToUserConfig("lobby.tls", String(enabled)) | | 13|+ saveSettingAndWriteToUserConfig("lobby.tls", String(enabled)); | 14| 14| } binaries/data/mods/public/gui/prelobby/common/encryption/encryption.js | 13| » saveSettingAndWriteToUserConfig("lobby.tls",·String(enabled)) | | [NORMAL] JSHintBear: | | Missing semicolon.
Link to build: https://jenkins.wildfiregames.com/job/differential/803/
The problem of crashes still persists. The average user would not know (wont even try probably) the cause of the crash. And there would be a lot of “disable TLS” replies to reports. Pop-up or a tooltip would do maybe.
Arguably the previous diff was better, because it didn't confuse the uneducated player further in the login dialog. Arguably this diff is better, because it empowers the educated player.
As a third alternative one could commit only the cpp + default.cfg diff, as linux people are usually smart enough to edit a config file, and the other platforms, macOS and windows, seem tested by us.
Meh.
Yes, but since when do we inform users of bugs ingame to prevent duplicate bugreports?I think the reply to duplicate bugreports would just be "disable TLS until #5349 is fixed", no? And there don't seem to be too many Fedora / affected linux users, no?
There could be a random library crash everywhere, developers have to be able to rely on library functions as defined in the specs.
So using TLS by default and a message box if its unavailable otherwise should be possible - just not for all platforms in 2018.
0ad+gloox+gnutls is too buggy currently to implement that, so we gotta chose one of the three proposals, or a fourth one if you have a specific one.
I guess it’s low priority if it only affect a subset of a platform. Same goes for the glsl crash too.
We could add some text under the TLS encryption saying its broken, on WinXP + OsX < 10.12 + Fedora ?
A better idea.
Not sure it's possible though. Can we rely on the HWdetect to get the platform os version to disable (grey out) the enable tls (with a nice a message, such as not supported on your platform)
What about people on those platforms who have fixed it themselves. This is more of a library bug rather than the OS.
First of all, I can reproduce the crash on Fedora, and the current patch does fix it when the TLS box is unticked. So the C++ change is good ?
Secondarily, I do not have a strong opinion on Options vs. Login. I do not find the new login dialog ugly, I find however the XML/JS code a bit cluttered by the change (with respect to adding a single entry in the options JSON). I am not sure the login checkbox is that much more explicit for educated users than a config option. I think very few will risk unchecking that without someone suggesting them to do so.
Finally, adding detection of specific platforms via the hardware detection is equivalent to hardcoding the list of broken platforms in the game, so I think it's not a good idea. Just as smiley says, it's like GLSL: if someone says "my lobby crashes", the immediate proposal we can make will be "you can try disabling TLS". It is a new feature of A23b and it's unfortunately not our fault if gloox is broken in some places.
(Ah sorry I'm just paraphrasing your D1679#66713 comment elexis, I agree with you).
I understand your point. My concern was mainly about people who don't report bugs because it's not a common thing to do :)
Indeed. But we don't really need tons of bugreports here, we know what are the issues. We just have to follow the upstream work on gloox (for instance we could make it work on OSX 10.9 by making gloox support darwinssl), but that is more long-term.