Re: PATCH - one time password/always prompt for password

[Derek Robert Price]

Brian Murphy wrote:

> Derek Robert Price wrote:
>
> > Second, it occurs to me that the -p option itself might be overkill - 
> > just like a standard client tries an empty password if it can't find 
> > one in ~/.cvspass for this server, it should be able to always prompt 
> > the user for a password if the empty pass fails _or_ if it recieves 
> > the OTP-needed response from the server.  Unless you want to argue 
> > about this, please disregard my comments regarding no_password_prompt 
> > and documenting -p below as the option and variable should be removed 
> > entirely anyhow.
>
>
> The idea with the -p option is to send a "NULL" password, not just a 
> blank one. A blank password can be
> a valid one. The design idea is that -p sends blank in the password 
> field of the authentication message of
> the pserver protocol, not the scrambled version of "". The change in 
> the descramble function is to descramble
> this blank to a NULL because it doesn't start with 'A'. This means no 
> change in present behaviour and less
> upset people ;-). This leads naturally to all the checking for 
> nullness of password also. 


I still think I like prompting after a failed attempt to use an empty 
password better.  Is there any reason this would be incompatible with 
PAM?  It is only adding functionality - I don't see why anyone would get 
upset about the change.  It simplifies the client side code a bit too, 
in addition to not requiring a new command-line option.

> > > @@ -3254,7 +3258,8 @@
> > >     send_to_server_via(to_server, "\012", 1);
> > >
> > >         /* Paranoia. */
> > > -        memset (password, 0, strlen (password));
> > > +    if (!no_password_prompt)
> > > +        memset (password, 0, strlen (password));
> >
> > It is not necessary to skip the memset() here - memset() can handle a 
> > string length of 0 just fine.
>
>
> The problem with it is that if I set password = " ";, a space, then I 
> would be setting a constant string " "
> to zero and this "constant" string will be reused  at other places in 
> the code leading to very mysterious
> failures. I have learnt this lesson from experience and it is an awful 
> bug to find. 


You didn't.  You set it to "", which has a strlen of 0 and should cause 
memset to do nothing.  Comment the password = "" assignment if you like, 
if we end up settling on this method.

> > > # else /* ! AUTH_CLIENT_SUPPORT */
> > >     error (1, 0, "INTERNAL ERROR: This client does not support 
> > > pserver authentication");
> > > # endif /* AUTH_CLIENT_SUPPORT */
> > > @@ -3304,6 +3309,12 @@
> > >         fprintf (stderr, "%s\n", read_buf + 2);
> > >
> > >         /* Continue with the authentication protocol.  */
> > > +        }
> > > +        else if (strncmp (read_buf, "S ", 2) == 0)
> > > +        {
> >
> > The "S" response is going to need to be documented in 
> > doc/cvsclient.texi.  I think I might like another response name, like 
> > "OTP-needed", or perhaps send-password, given that the server should 
> > have just tried an empty password, as noted below.
>
>
> It doesn't have anything to do with OTP as such. "S" stands for 
> secret. The server has asked for something
> which should not be echoed at the terminal - which could be just as 
> useful for other things. Even if you
> don't use OTP the procedure works fine with PAM prompting if the unix 
> password module is used for
> instance. 


Okay, lets make it "prompt-secret" or the like, then.  I don't like the 
single-character addition to the protocol.  All the previous 
single-character portions of the protocol are basically only printing 
commands: "M", "MT", "E", & "F".  None of these responses require the 
client to send data.  I think I prefer something more human-readable.

> > > Index: src/scramble.c
> > > ===================================================================
> > > RCS file: /cvs/ccvs/src/scramble.c,v
> > > retrieving revision 1.17
> > > diff -u -r1.17 scramble.c
> > > --- src/scramble.c    23 Jul 2003 20:42:26 -0000    1.17
> > > +++ src/scramble.c    30 Jul 2003 20:08:19 -0000
> > > @@ -114,15 +114,7 @@
> > >     /* For now we can only handle one kind of scrambling.  In the 
> > > future
> > >        there may be other kinds, and this `if' will become a 
> > > `switch'.  */
> > >     if (str[0] != 'A')
> > > -#ifndef DIAGNOSTIC
> > > -    error (1, 0, "descramble: unknown scrambling method");
> > > -#else  /* DIAGNOSTIC */
> > > -    {
> > > -    fprintf (stderr, "descramble: unknown scrambling method\n", str);
> > > -    fflush (stderr);
> > > -    exit (EXIT_FAILURE);
> > > -    }
> > > -#endif  /* DIAGNOSTIC */
> > > +    return NULL;
> >
> > What's all this, then?  At the least it looks like this deserves to 
> > be in a separate patch?
>
>
> See above under NULLness ;-). Probably there should be a "B" option 
> instead to indicate
> the nullness of the password so this doesn't get triggered by mistake 
> and so that other
> errors in the protocol dont get hidden by them getting caught by the 
> generality of this test. 


If we don't settle on trying the empty password, then how about "N", for 
NULL?

Derek

--
               *8^)

Email: derek@ximbiot.com

Get CVS support at <http://ximbiot.com>!
--
Welcome to the Church of the Holy Cabbage.  Lettuce pray