[Freeciv-Dev] Re: client/server authentication (PR#1767)

["Raimar Falke" <rf13@xxxxxxxxxxxxxxxxx>]

On Tue, May 06, 2003 at 04:31:36PM -0700, Mike Kaufman wrote:
> On Tue, May 06, 2003 at 01:36:00PM -0700, Raimar Falke wrote:
> > 
> > Just some points. I didn't checked the GUI code.
> 
> hmm.
> 
> > > +#define MAX_AUTHENTICATION_TRIES 3
> > 
> > When is this reseted? When the server restarts? IMHO this allows a
> > nice DOS attack for the given user. Better would be to allow unlimited
> > tries but slow them down. 3s for a failed login for example.
> 
> this is reset when a connection is rejected. 

> Did you compile and try this?

No. Not yet. First come all issues which are visible from the diff ;)

> I don't think unlimited tries is good. For one, a user will have to quit
> the client if he screws up his username for example, instead he can just
> press return three times and be rejected and be back. Or we can make the
> client more complicated in the first iteration. I don't really want to do
> that. Doing pauses and such just makes things more complex.

What if the user enters the password in the connect dialog? IMHO it
belongs there.

> > >    struct conn_list self; /* list with this connection as single element 
> > > */
> > >    char username[MAX_LEN_NAME];
> > 
> > > +  char password[MAX_LEN_NAME];
> > 
> > Do we really need to save this here? This just increases the chance
> > that it leaks in some way.
> 
> yes we do if the server code checks the password.
> alternatively we might do pconn->server->user->password, but that's kinda
> ugly and probably not worth it.

struct user already contains a password field. Another one for
connection doesn't seem to be needed.

> leaks? You were the one that wanted the server to check the password.

> I'll blank out password when we're done with it.

Thats good.

> > > +#undef NEW_USERS_DISALLOWED  /* if defined, new users may not log in. */
> > > +#undef GUESTS_DISALLOWED     /* if defined, users cannot log in as 
> > > guests */
> > 
> > Please don't use double negation.
> 
> this is not double negation, but would you feel better if they were
> NEW_USERS_NOT_ALLOWED and GUESTS_NOT_ALLOWED?

IMHO "undef NEW_USERS_DISALLOWED" is double negation because it is the
same as "define NEW_USERS_ALLOWED". It should be NEW_USERS_ALLOWED and
GUESTS_ALLOWED.

> > You may also want to count the chars which aren't in a..z.
> 
> this function is starting to get complicated. Without encryption, I don't
> see the point.

These are unrelated. Encryption is against an attacker which attacks
in a technical way (sniff network, ptrace to the server binary, get
access to the DB, ...). A non-trivial password is against
non-technical password guessing. Be it dictionary based or brute
force. Even with encryption you can guess the passwords. So you want
non-trivial passwords.

> > > -#define MAX_NUM_CONNECTIONS (MAX_NUM_PLAYERS+MAX_NUM_BARBARIANS)
> > > +#define MAX_NUM_CONNECTIONS (2 * (MAX_NUM_PLAYERS + MAX_NUM_BARBARIANS))
> > 
> > Reason?
> 
> People should be able to log in (to observe or multiconnect or whatever)
> even if all the players have been taken.

But isn't the same true for the current code with observers?! Except
the fact that it isn't used.

> > > +#endif /* FC__USER_H */
> > 
> > IMHO this file belongs to (core) freeciv. It should IMHO in server/.
>  
> hmm. I'm skeptical about this. I think it should belong where other user
> manipulation functions are going to be and that's in /userdb. However,
> server/ is where I originally had it, so I can be convinced. Another reason
> is that an external library is going to need access to [a copy of] user.h. 

Server and userdb implementation both need user.h and user_db.h. IMHO
both should be in server/. The user_db.c should be renamed to
user_db_file.c and be put in server/userdb. For more complex
implementation we can also add subdirs under server/userdb.

> > > +{
> > > +  FILE *lock;
> > > +  static int check;
> > > +
> > 
> > > +  lock = fopen(FC_LOCK_FILE, "r");
> > > +
> > > +  /* if we aren't locked, create one */
> > > +  if (!lock) {
> > > +    lock = fopen(FC_LOCK_FILE, "w");
> > 
> > This isn't atomic. In the time window between these two opens another
> > may got the lock.
> 
> what do you suggest? Me and issues like these don't mix well.

Sadly there is no easy answer.

Read http://www.washington.edu/imap/documentation/locking.txt.html

Look at 
http://216.239.39.104/search?q=cache:2oa8Q1g2DZYC:sigep.stanford.edu/sendmail/libsmutil/lockfile.c+lockfile.c&hl=en&ie=UTF-8

procmail comes with a lockfile program: 
http://www.mit.edu/afs/sipb/service/rtfm/src/procmail-3.11pre4/src/lockfile.c

> > > +/**************************************************
> > > + The user db statuses are:
> > > +
> > > + 1: an error occurred, possibly we couldn't access the database file.
> > > + 2: the password we checked for was ok, or we were able to successfully
> > > +    insert an entry.
> > > + 3: the user's password was wrong.
> > > + 4: the user's we were searching for was found.
> > > + 5: the user's we were searching for was not found.
> > > +***************************************************/
> > > +enum userdb_status {
> > > +  USER_DB_ERROR = 1,
> > 
> > > +  USER_DB_LOAD_SUCCESS,
> > > +  USER_DB_SAVE_SUCCESS,
> > 
> > Are the reason for these two two values handle_db_lookup? Please
> > remove this ugly callback function.
> 
> that's right. now let me tell you why we must have a callback for
> user_db_load and user_db_save. It is simply because in the industrial
> strength version of this, libuserdb.a is going to fork a process which does
> lookup into a real database. We cannot have the server hang while this is
> happening. For various reasons, the lookup might take a while to return.
> Should a running game freeze while this is happening? In my opinion, no it
> shouldn't. Hence the callback. Yes there's going to need to be some more
> apparatus to stop "waiting" for a lookup to return (some check in
> sniff_packets probably), but damned if I'm going to entirely rewrite the
> connecthand code in the meantime.

What is so bad about waiting till the db returns the data? Do you have
data how long a SQL-DB lookup take?

If we do what you suggest we would have to disable somehow all players
which have an outstanding DB query from all actions. Also we would
have listen to these extra fds in the main-loop.

I'm for waiting. It is easier to program. If there however are systems
which need 5s to retrieve the data and people want to use these as
user-db I will change my mind.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  This customer comes into the computer store. "I'm looking for a mystery
  Adventure Game with lots of graphics. You know, something realy
  challenging". "Well," replied the clerk, "have you tried Windows 98 ?"