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

["Mike Kaufman" <kaufman@xxxxxxxxxxxxxxxxxxxxxx>]

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?
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.

> >    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. 

leaks? You were the one that wanted the server to check the password.
I'll blank out password when we're done with it.

> Why is the type needed for the reply? IMHO it isn't. Also "bool
> is_old_user" should be changed to "enum auth_state auth_state" where
> enum auth_state looks like
> 
> enum auth_state {
>   NONE,
>   JOINED,
>   GUEST,
>   REQUEST_SENT,
>   AUTHED
> };
> and maybe some others. So you go from NONE to JOINED. And from JOINED
> to GUEST or REQUEST_SENT. From REQUEST_SENT to AUTHED or you keep in
> REQUEST_SENT if the wrong password was sent.
> 

possibly. I'll think about this.
 
> > +#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?
 
> > @@ -189,7 +233,7 @@
> >    } conn_list_iterate_end;
> >  
> >    freelog(LOG_NORMAL, _("Connection request from %s from %s"),
> > -            req->username, pconn->addr);
> > +          req->username, pconn->addr);
> 
> Looks like noise.

hmm? was fixing a style mistake I made previously.

> 
> >    /* print server and client capabilities to console */
> >    freelog(LOG_NORMAL, _("%s has client version %d.%d.%d%s"),
> > @@ -229,6 +273,36 @@
> >      return FALSE;
> >    }
> >  
> > +#ifdef AUTHENTICATION_ENABLED 
> > +  /* if authentication is enabled, we need an extra check as to whether
> > +   * a connection can be established: the client must authenticate itself 
> > */
> > +
> > +  if (has_capability("auth", req->capability) 
> > +      && !is_guest_name(req->username)) {
> > +    struct user *puser = fc_malloc(sizeof(struct user)); /* must allocate 
> > */
> > +
> > +    sz_strlcpy(puser->name, req->username);
> > +    sz_strlcpy(pconn->username, req->username);
> > +    user_db_load(puser);
> > +    return TRUE;
> 
> ?? Who frees puser? You should check the result of user_db_load. What
> is the purpose of this code anyway?

handle_db_lookup() frees puser. see the end for a rationale for this
callback.
 
> > +static char *get_guest_name(void)
> 
> Add a const.

sure.

> 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.

> > -#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.

> > +#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. 

> The usage of handle_db_lookup here is the wrong concept.
> 
> > +static int check_lock(void)
> 
> bool

sure. a carryover from the days when this file didn't know about shared.h
 
> > +{
> > +  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.

> > +/**************************************************
> > + 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.

Now, if you can offer a better solution, by all means.

-mike