Skip to content

Commit

Permalink
Revert "fix #4949:"
Browse files Browse the repository at this point in the history 
This reverts commit 002857e.
  • Loading branch information
jK committed Sep 10, 2015
1 parent 3f37498 commit 08ee557
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion rts/Net/GameServer.cpp
View file
Expand Up @@ -70,7 +70,7 @@ using boost::format;
CONFIG(int, AutohostPort).defaultValue(0);
CONFIG(int, SpeedControl).defaultValue(1).minimumValue(1).maximumValue(2)
.description("Sets how server adjusts speed according to player's load (CPU), 1: use average, 2: use highest");
CONFIG(bool, AllowSpectatorJoin).defaultValue(false).description("allow any unauthenticated clients to join as spectator with any unchecked name, thus permitting impersonation");
CONFIG(bool, AllowSpectatorJoin).defaultValue(true).description("allow any unauthenticated clients to join as spectator with any unchecked name, thus permitting impersonation");
CONFIG(bool, WhiteListAdditionalPlayers).defaultValue(true);
CONFIG(bool, ServerRecordDemos).defaultValue(false).dedicatedValue(true);
CONFIG(bool, ServerLogInfoMessages).defaultValue(false);
Expand Down

22 comments on commit 08ee557

@abma
Copy link
Contributor

@abma abma commented on 08ee557 Sep 10, 2015

Choose a reason for hiding this comment

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

wtf? why? thats an insane default value! feel free to talk to all autohost admins. who/what needs a default value of true here?

@abma
Copy link
Contributor

@abma abma commented on 08ee557 Sep 10, 2015

Choose a reason for hiding this comment

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

isn't the default value a value which fits for most people?

most people are here: players / autohosts vs a few devs (i don't see why a normal user wants/needs true here)

afaik currently ALL autohosts haven't set this to false which allows breaking for trolls to break a game.

@abma
Copy link
Contributor

@abma abma commented on 08ee557 Sep 10, 2015

Choose a reason for hiding this comment

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

-> https://springrts.com/phpbb/viewtopic.php?f=12&t=33869

@jk3064
Copy link
Contributor

@jk3064 jk3064 commented on 08ee557 Sep 10, 2015

Choose a reason for hiding this comment

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

16:11 [LCC]jK: BD stfu
16:12 [LCC]jK: default enabling DID NOT LIMIT autohost to disable it and implementing proper whitespacing (I haven't seen ANY autohost doing so)
16:12 hokomoko: *whitelisting
16:13 [LCC]jK: oops ^^
16:13 [LCC]jK: -> autohost can send cmds via the autohost interface to add new users WITH a password (and even a team)
16:13 [LCC]jK: but NONE autohost is using that
16:14 [LCC]jK: so blame them, not the default value of a tag

@abma
Copy link
Contributor

@abma abma commented on 08ee557 Sep 10, 2015

Choose a reason for hiding this comment

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

bibim said, spads default config makes usage of it, so why disable the check then?

-> spring should default to "secure" values

@jk3064
Copy link
Contributor

@jk3064 jk3064 commented on 08ee557 Sep 10, 2015

Choose a reason for hiding this comment

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

It's the autohost which uses the username for permission levels, so it is the autohosts job to make it secure!

And when an user hosts a game without using an autohost (& permission levels etc.), you want to be able to specjoin it!

So yes, you expect it to be enabled by default.

@jk3064
Copy link
Contributor

@jk3064 jk3064 commented on 08ee557 Sep 10, 2015

Choose a reason for hiding this comment

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

| bibim said, spads default config makes usage of it, so why disable the check then?
Obviously it doesn't make "usage of it" (what ever that means), cause then it would default disable the tag.

@abma
Copy link
Contributor

@abma abma commented on 08ee557 Sep 10, 2015

Choose a reason for hiding this comment

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

spads doesn't change springs config, its up to the autohost owner to change it!

@abma
Copy link
Contributor

@abma abma commented on 08ee557 Sep 10, 2015

Choose a reason for hiding this comment

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

security is always uncomfortable, so it must be enforced.

-> default it a more secure value which is false

@gajop
Copy link
Member

@gajop gajop commented on 08ee557 Sep 10, 2015

Choose a reason for hiding this comment

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

Is this going to disallow us to join single player games as a spectator by default or something weird like that?

@jk3064
Copy link
Contributor

@jk3064 jk3064 commented on 08ee557 Sep 10, 2015

Choose a reason for hiding this comment

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

yep, that's exactly what the tag does when disabled.

@abma
Copy link
Contributor

@abma abma commented on 08ee557 Sep 10, 2015

Choose a reason for hiding this comment

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

do we need sth. like an exploit first until we fix this? thats very bad handling of security. :-|

@gajop
Copy link
Member

@gajop gajop commented on 08ee557 Sep 10, 2015

Choose a reason for hiding this comment

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

I'd lime this to be enabled only in multiplayer if at all possible. It should at least never work for hosts (single player)?!

@abma
Copy link
Contributor

@abma abma commented on 08ee557 Sep 10, 2015

Choose a reason for hiding this comment

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

side note: this problem was already used by some troll which killed at least one game with it.

@ashdnazg
Copy link
Member

Choose a reason for hiding this comment

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

it used to be set to false, and all autohost owners changed it to true, because they prioritised ingame join above this issue.

The problem isn't in this specific setting, it's the fact that spring's security is either "everything is open" or "everything is blocked".
In other words there should be a different solution.

@abma
Copy link
Contributor

@abma abma commented on 08ee557 Sep 10, 2015

Choose a reason for hiding this comment

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

sadly i can only repeat bibim (i hope i understood him correctly):

spads adds lobby spectators ingame as players and so a password check can be done. so no need for a different setting, it should work well with AllowSpectatorJoin disabled.

@Yaribz is that true?

i don't have a spads configured, so i can't easily test that.

@Yaribz
Copy link
Contributor

@Yaribz Yaribz commented on 08ee557 Sep 10, 2015

Choose a reason for hiding this comment

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

yes

@Yaribz
Copy link
Contributor

@Yaribz Yaribz commented on 08ee557 Sep 10, 2015

Choose a reason for hiding this comment

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

Is this going to disallow us to join single player games as a spectator by default or something weird like that?

What? I don't see how this could be related to the startscript having one or several players...

@Yaribz
Copy link
Contributor

@Yaribz Yaribz commented on 08ee557 Sep 10, 2015

Choose a reason for hiding this comment

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

16:13 [LCC]jK: -> autohost can send cmds via the autohost interface to add new users WITH a password (and even a team)
16:13 [LCC]jK: but NONE autohost is using that

What? You actually meant all autohosts are using that since years, didn't you?
FYI SPADS is using that since version 0.9.3b, which is more than 5 years old...

@silentwings
Copy link
Contributor

Choose a reason for hiding this comment

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

Its unrealistic to change+rename this setting and then blame ill effects on autohost owners not reacting. Autohosts owners are not developers, they are not likely to read https://github.com/spring/spring/blob/develop/doc/changelog.txt#L241 and even less likely to deign that it suggests they might do anything ... and just in case that wasn't enough difficulty for them, from the perspective of any SPADS owner (no clue about ZK), the name AllowSpectatorJoin is very misleading.

edit: Also, I'm told that line of the changelog was inaccurate and the tags meaning was changed as well as renamed.

@springjools
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we instead set spoofprotection setting to kick players whose ip-address in game and battle lobby do not match? That can be done in spads.

@silentwings
Copy link
Contributor

Choose a reason for hiding this comment

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

@jools: because with AllowSpectatorJoin=true spectators who have never visited the battleroom can join a running game.

Please sign in to comment.