Uses SSL for the UserReport.
Details
Details
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Event Timeline
| binaries/data/config/default.cfg | ||
|---|---|---|
| 477 | The v1/ suffix was removed, because it's duplicated by the version in the UserReport. | |
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/695/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/696/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/697/display/redirect
Comment Actions
Thanks for writing the patch, it's much better with SSL for the UserReporter.
I could not test whether the patch works as advertized if someone passed HTTP instead of HTTPS for mod.io, because the curl error are not relayed and it only displays "Could not parse JSON".
So I can't full verify the correctness unless we have some cheap LOGWARNING or whatever to display the error code.
I can't imagine it not working, but I find myself surprised sometimes with library functions. So you can edit the error code or commit without my verification if you are dead serious about the correctness.
(also some less relevant / verbose comments in irc at date of first publication)
We should also have it for the lobby, I guess me or Jan should generate a certificate.
| binaries/data/config/default.cfg | ||
|---|---|---|
| 477 | Ok. This could be a backend version (v1) while the version that the UserReporter sends could be considered the client version (11). Perhaps the idea is that old clients can still report at v1 and newer at v2. | |
| source/ps/UserReport.cpp | ||
| 98 | nuke (as agreed on irc since one may want to have the verbose output option without adding possible but unlikely security risks) | |
| 121 | Checked all options at https://curl.haxx.se/libcurl/c/curl_easy_setopt.html, couldn't find anything else that would be useful, maybe dunno probably not CURLOPT_FAILONERROR | |
| binaries/data/config/default.cfg | ||
|---|---|---|
| 477 | HTTP Test negative: When I apply this patch and keep the "http" in default.cfg, the UserReporter returns the http status code 301. But this seems wrong, because the SSL/TLS negotiation should have been rejected before the HTTP status code is received, no? HTTPS Test positive: The current certificate is invalid (cert for wrong host), and the UserReporter shows "SSL" as an error reason - which is correct and sufficiently informative. | |
Comment Actions
The backend is not ready to use this patch:
- feedback.wildfiregames.com still resolves to Philips IP.
- Philips server does not support SSL.
- Our server currently passes the certificate from svn.wildfiregames.com, i.e. is invalid.
- Since gloox rejects the lobby certificate with SChannel on windows and since curl uses SChannel too, the UserReporter might reject the proper Lets Encrypt certificate too.
Comment Actions
So I can't full verify the correctness unless we have some cheap LOGWARNING or whatever to display the error code.
Proper © error display was implemented in rP21892, so the patch can be tested since.
I can't imagine it not working, but I find myself surprised sometimes with library functions.
And that's why we require to test things before committing them.
Patch doesn't actually work for me and still allows upload with http. I could see with Wireshark that the up and download was done using HTTP, though it seems zlib encrypted (and an absence of any TLS initiation within the http cleartext data, also that would be a protocol new to me).
Actually it seems this only uses SSL/TLS for FTP and other protocols, but not HTTP!
https://curl.haxx.se/libcurl/c/CURLOPT_USE_SSL.html
https://curl.haxx.se/libcurl/c/libcurl-errors.html#CURLEUSESSLFAILED
So if it should be enforced in C++, it'd have to be a rejection if the URL starts with http:// Im afraid.
But we wanted to protect from malicious JS mods changing the URL anyway by adding protected config entries for such sensitive data.
Btw the curl UserAgent exposes possibly sensitive data
0ad libcurl/7.47.0 GnuTLS/3.4.10 zlib/1.2.8 libidn/1.32 librtmp/2.3 (http://play0ad.com/)
Comment Actions
But the url still can be changed anyway, no?
The frontend and the backend should be kept in sync:
https://code.wildfiregames.com/source/0ad/browse/ps/trunk/source/tools/webservices/userreport/
Comment Actions
The repository should still be kept in sync with what we installed on the server.
Also http > https doesn't matter for backend.
The frontend rejects servers that don't support https (keeping the promise of the terms and conditions).
Comment Actions
Maybe it would be nice to have a separate repository ? So you can pull to keep it in sync automatically.