bug-cvs
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Derek Robert Price
Subject: Re: PATCH - one time password/always prompt for password
Date: Thu, 31 Jul 2003 15:06:59 -0400
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4) Gecko/20030624 Netscape/7.1

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: address@hidden

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





reply via email to

[Prev in Thread] Current Thread [Next in Thread]