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

["Raimar Falke" <rf13@xxxxxxxxxxxxxxxxx>]

On Tue, May 27, 2003 at 09:04:09PM -0700, Mike Kaufman wrote:
> On Tue, May 27, 2003 at 01:36:56PM -0700, ChrisK@xxxxxxxx wrote:
> 
> heh, auth6q.diff at your service.

> diff -Nur -Xsnap/diff_ignore snap/client/options.c snap-a6/client/options.c
> --- snap/client/options.c     2003-05-18 11:26:20.000000000 -0500
> +++ snap-a6/client/options.c  2003-05-23 21:29:02.000000000 -0500
> @@ -441,6 +441,10 @@
>    if (!section_file_load(&sf, name))
>      return;  
>  
> +  /* a "secret" option for the lazy. TODO: make this saveable */
> +  sz_strlcpy(password, 
> +             secfile_lookup_str_default(&sf, "", "%s.password", prefix));

This means only one password for all servers. There should be multiple
password keys by "user@host".

> +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 */
> +};

Can be abstract this?

AUTH_LOGIN_FIRST,
AUTH_LOGIN_RETRY,
AUTH_NEWUSER_FIRST,
AUTH_NEWUSER_RETRY

The reason (wrong, bad) for the retry doesn't matter and may in the
worst case leak information.

> +#define MAX_AUTHENTICATION_TRIES 3
> +
> +/* after each wrong guess for a password, the server waits this
> + * many seconds to reply to the client */
> +const int throttle_period[] = { 1, 1, 2, 3 };

We should remove the tries and just use a delay of 2-5s. Maybe even
random *evil grin*

> +#ifdef AUTHENTICATION_ENABLED 
> +  /* assign the client a unique guest name */
> +  if (is_guest_name(req->username)) {

> +#ifdef GUESTS_DISALLOWED

This is now GUESTS_ALLOWED

> +/**************************************************************************
> +  return a guest name
> +**************************************************************************/
> +static const char *get_guest_name(void)
> +{
> +  return GUEST_NAME;
> +}

Doesn't provide much by itself since high-level code shouldn't use
this function anyway.

> +      pconn->server.authentication_stop = 2147483640; /* big enough */

You can use 0 as a flag value.

> +/**************************************************************************
> + attempt to create a lock, so that other servers aren't accessing the 
> + database when we are. return TRUE for success, FALSE for failure
> + N.B. This is a simple locking mechanism: it for example does not check for
> + stale lock files. This means that if the application crashes in between 
> + lock creation and removal, you'll get a dangling lock file (this could 
> + easily happen if you were to corrupt the database. The registry functions
> + will bomb and quit the server). Perhaps this warrants a FIXME.
> +**************************************************************************/

Here and also in other comments: please write sentences.

> +struct user {
> +  char name[MAX_LEN_NAME];
> +  char password[MAX_LEN_NAME];
> +
> +  /* add more fields here as warranted */
> +};

What is the current state for the idea that struct user is moved to
common and struct connection gets a pointer to the user? I like the
idea. It is the correct thing if you do OO-modeling.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "I was dead ... but I'm better now."
    -- Capitain Sheridan in Babylon 5