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

["Mike Kaufman" <kaufman@xxxxxxxxxxxxxxxxxxxxxx>]

On Thu, May 22, 2003 at 05:22:49AM -0700, Raimar Falke wrote:
> On Sat, May 17, 2003 at 08:51:27PM -0700, Mike Kaufman wrote:
> > my bad, shoulda known that a capability was going to be added in the mean
> > time, here's an updated patch. sorry for the bandwidth.
> 
> This is only a patch review. I made no actual testing.
> 
> > +static enum {
> > +  LOGIN_TYPE,
> > +  NEW_PASSWORD_TYPE,
> > +  VERIFY_PASSWORD_TYPE,
> > +  ENTER_PASSWORD_TYPE
> > +} dialog_config;
> 
> These are duplicated in the GUI code.

and...? we don't have a connectdlg_common.c yet, and I'm not going to make
one for this triviality. Maybe when the new connectdlg shows up.
 
> > +  char password[MAX_LEN_NAME];
> 
> IMHO should a connection have a pointer to a struct user. This field
> should then go.

no. I thought about this. This would require that user.h be in common and
that is not right as the client has absolutely nothing to do with that
struct. The client should not have access to it.

> > +    /* Holds number of tries for authentication from client. */
> > +    int authentication_tries;
> 
> If you limit this you also have to think about a real rate
> limiting. So what stops an attacker issuing another connection after
> his 3 tries are over? IMHO we should rate limit the connection per
> time from one IP. To one connection per 2 sec for example. This
> prevents bture force attacks against the servers.

No. This problem is orthogonal to this patch. You have the same problem
in the current code (with probably even more overhead).

> > +    enum conn_status status;
> 
> IMHO should the field have an "auth" in its name -> "auth_status".

hmm. possibly.

> > +enum authentication_type {
> > +  AUTH_NORMAL,     /* request a password for a returning user */
> > +  AUTH_NEW,        /* request a password for a new user */
> 
> > +  AUTH_BAD,     /* inform the client that the new password was bad */
> > +  AUTH_WRONG,   /* inform the client that the entered password was wrong */
> 
> Why can't we use normal messages for these?

because the client is expected to take some kind of configuration action in
response to this which can't be handled my a text message.

> > +struct packet_authentication_request {
> 
> > +  int type;
> 
> You should use the enum from above here.

sure.

> > diff -Nur -Xsnap/diff_ignore snap/server/connecthand.c 
> > snap-a6/server/connecthand.c
> > --- snap/server/connecthand.c       2003-05-03 22:20:18.000000000 -0500
> > +++ snap-a6/server/connecthand.c    2003-05-17 22:38:29.000000000 -0500
> 
> > +#define NEW_USERS_ALLOWED  /* if defined, new users may log in. */
> > +#define GUESTS_ALLOWED     /* if defined, users can log in as guests */
> 
> These settings are hidden quite nice in this file. I think that these
> two settings and the settings below should be server variables.

sure, but there is a perpetual argument over new server options are you are
advocating five (or at least 3) new ones in a single stroke. Until a better
system in stdinhand is worked out, the answer will be: not in this patch.

> > +#define GUEST_NAME "guest"
> 
> > +#define GUEST_NAME_LEN 5
> 
> What is wrong with strlen("guest")?

nothing really.
 
> > +    switch(user_db_load(&user)) {
> > +    case USER_DB_ERROR:
> > +#ifdef GUESTS_ALLOWED
> > +      sz_strlcpy(tmpname, pconn->username);
> > +      get_unique_guest_name(tmpname); /* do not pass pconn->username here! 
> > */
> > +      sz_strlcpy(pconn->username, tmpname);
> > +
> > +      freelog(LOG_ERROR, "Error reading database; connection -> guest");
> > +      notify_conn(&pconn->self, _("There was an error reading the user "
> > +                                  "database, logging in as guest 
> > connection "
> > +                                  "'%s'."), pconn->username);
> > +      establish_new_connection(pconn);
> > +#else
> > +      reject_new_connection(_("There was an error reading the user 
> > database "
> > +                            "and guest logins are not allowed. Sorry"), 
> > pconn);
> > +#endif /* GUESTS_ALLOWED */
> > +      break;
> 
> I would just let the server answer "error, please try guest".

uh, this won't work if guest logins aren't allowed...

> > +/**************************************************************************
> > +  Receives a password from a client and verifies it.
> > +**************************************************************************/
> > +bool handle_authentication_reply(struct connection *pconn,
> 
> Since the check for
>   if (pconn->server.authentication_tries >= MAX_AUTHENTICATION_TRIES) {
> is quite universal I would move it at the top.

Are you actually reading the patch Raimar? There are five or six paths
through this function are only two of them require that check...

> > --- snap/server/userdb/lockfile.c   1969-12-31 18:00:00.000000000 -0600
> > +++ snap-a6/server/userdb/lockfile.c        2003-05-17 22:38:29.000000000 
> > -0500
> 
> > +#define HAS_FILE_LOCKING /* FIXME: use configure here! */
> > +#define HAS_FLOCK        /* FIXME: use configure here! */
> > +#ifdef HAS_FLOCK /* FIXME: need a check for sys/file.h */
> 
> No comment ;)

I'm still waiting on someone like Jason or Per with enough knowhow to 
volunteer to write the appropriate config stuff for this.

> > +static int fd;
> 
> This variable is set but never used.

are you kidding?! Oh, you're not kidding. Look at the definitions for
LOCK_CMD and UNLOCK_CMD.

There is a bug though and that is that it should be instead:

#ifdef HAS_FILE_LOCKING
static int fd;
#endif

thx to Chris for finding this.
 
> > +  /* open the file */
> 
> > +  if ((fd = open(FC_LOCK_FILE, O_RDWR|O_CREAT, S_IRUSR|S_IWUSR)) == -1) {
> 
> If you don't use O_EXCL here you can use fopen IMHO.

you know how flock and/or fnctl works right? They kinda require a file
descriptor...

> This can be better writted using goto:

barely.

-mike