[Cvs-dev] Re: cvs-passwd patch

[Mark D. Baushke]

Hi Prasad,

I only have had time to look a form rather than function for your last
version. I will also mention a few compatiblity considerations with the
CVSNT implementation (to the best of my understanding of how that is
implemented).

I notice from the latest main.c file patch that you did not pick up my
hint ('* FYI. The CVSNT synonyms for "passwd" are "password" and "setpass"')
about the command aliases used by CVSNT. Let me be more explicit this time.

If the intent is to have a completely interoperable to CVS and CVSNT
"passwd" client/server command, then you should probably use their
command aliases:

    { "passwd",   "pw",       NULL,        passwd,    
CVS_CMD_MODIFIES_REPOSITORY },

should be re-written as this:

    { "passwd",   "password", "setpass",   passwd,    
CVS_CMD_MODIFIES_REPOSITORY },

Yes, this means that "pw" is not really a favorable synonym for the
"passwd" command for me. Other CVS developers may have a separate
opinion and you could poll them.

For reasons I have not investigated, the CVSNT main.cpp 1.123 revision
of this table entry also uses:

    CVS_CMD_USES_WORK_DIR | CVS_CMD_MODIFIES_REPOSITORY

which would seem to indicate the need for a temporary directory in the
repository. This may be due to the way that a new passwd file is being
generated and copied into place (I have not looked closely).

Also, like most CVS client/server functions, the CVSNT passwd.cpp file
seems to use passwd function for both the client and server processing
which does help to ensure that the processing of the command-line
arguments will be consistent.

After having let you attempt to create sanity.sh tests that actually
exercise the server (which you would have had to do in any case), I will
now ask this next question... Is there any reason that the "passwd"
command is required to be used only by :pserver: client/server
connections? That is, could you consider writing sanity.sh tests to
allow an administrator who might have :ext: connections available to the
repository to administer the CVSROOT/passwd file over that possibly more
secure channel? You could still validate the changes by looking at the
resulting CVSROOT/passwd file values chosen for many examples. The
biggest difficulty would validating be the passwd-e testing. So, you
will possibly still want your other tests that want a :pserver:
repository too, but most of the functionality should be able to be
tested.

I will also note that the CVSNT passwd function limits the typed
password to 128 bytes and further restricts that you cannot add or
delete yourself. You may wish to consider those operational
functionality changes in your implementation of the code for a
consistent API to what is done with CVSNT. (You are NOT obligated to
make exactly the same choices, but it would be good to understand WHEN
and WHY you are making alternative decisions).

I will also note that CVSNT does not quote the %s arguments as my
suggested change is doing. You may therefore consider those changes
below as advistory. Although, the changes to match with the other CVS
code we have been updating.

I see that you went to srand/rand from srandom/random. I think that
should help in portability. Of course, I still think that using
/dev/urandom like main.c (I believe I mentioned this once before) does
is a better first approximation to getting a good random number source
and falling back ro random/rand48/rand when those are not available. The
rand function is ANSI C, so it will always be available, but you need
not always use it. If you need us to add random and rand48 to the
AC_CHECK_FUNCS in configure.in so that HAVE_RAND48 and HAVE_RANDOM are
available macros, let me know.

I hope you continue to find my comments useful.
        
        -- Mark

ChangeLog Entry:

        * passwd.c (passwd): Quote unsupported cmdname in error message.
        (passwd, spasswd): Use a NULL assignment to update bookkeeping
        of non-local pointers dpass and rusr.
        (passwd): Remove trailing whitespace on comment.
        (change_pass): Remove trailing whitespace on assert statement.
        (gettok): Remove trailing whitespace on local variable assignment.

--- passwd.c    Sat Oct 14 10:54:10 2006
+++ passwd.c    Sat Oct 14 11:02:51 2006
@@ -160,7 +160,7 @@ passwd (int argc, char *argv[])
         start_server ();
 
         if (!supported_request ("passwd"))
-            error (1, 0, "server does not support %s", cmdname);
+            error (1, 0, "server does not support `%s'", cmdname);
         if (!usr && (mode & ADD || mode & DISABLE || mode & DELETE
                     || mode & ALIAS || mode & RALIAS))
             error (1, 0, "option requires [username]");
@@ -171,6 +171,7 @@ passwd (int argc, char *argv[])
         {
             option_with_arg ("-D", dpass);
             free (dpass);
+            dpass = NULL;
         }
         if (do_encrypt)
         {
@@ -182,6 +183,7 @@ passwd (int argc, char *argv[])
         {
             option_with_arg ("-r", rusr);   /* make alias */
             free (rusr);
+            rusr = NULL;
         }
         if (mode & RALIAS)
             send_arg ("-R");    /* remove alias */
@@ -192,7 +194,7 @@ passwd (int argc, char *argv[])
         if (!mode)
         {                       /* change password */
             usr = (usr == NULL) ? current_parsed_root->username : usr;
-            printf ("Changing password for %s\n", usr);
+            printf ("Changing password for `%s'\n", usr);
             key = getpass ("(current) CVS password: ");
             if (!key)
                 error (1, errno, "failed to read password");
@@ -231,7 +233,7 @@ passwd (int argc, char *argv[])
 /*
  * spasswd: This is called by cvs server, is supposed to receive username,
  * password, etc. from cvs-client and update appropriately the CVSROOT/passwd
- * file. It receives the user credentials as command line arguments in the 
+ * file. It receives the user credentials as command line arguments in the
  * following format
  * cvs server passwd [-a] [-D domain] [-e] [-r user] [-R] [-x] [-X]
  *                   [username] [passwd]
@@ -267,12 +269,14 @@ spasswd (int argc, char *argv[])
     if (dpass)
     {
         free (dpass);
+        dpass = NULL;
         error (1, 0, "option `-D domain' is not supported by server");
     }
     if (mode & ALIAS)
     {
         alias (usr, rusr);       /* -r make alias */
         free (rusr);
+        rusr = NULL;
     }
     if (mode & RALIAS)
         alias (usr, "");         /* -R remove alias */
@@ -309,12 +313,12 @@ init_pass (void)
     pass_list = getlist ();
     fname = Xasprintf ("%s%s", current_parsed_root->directory, pass_file);
     if ((fpass = fopen (fname, "r")) == NULL)
-        error (1, errno, "could not open: %s", fname);
+        error (1, errno, "could not open: `%s'", fname);
     flockfile (fpass); /* acquire exclussive lock on CVSROOT/passwd file */
     read_pass (fpass);
     funlockfile (fpass);
     if (fclose (fpass) != 0)
-        error (1, errno, "could not close: %s", fname);
+        error (1, errno, "could not close: `%s'", fname);
 
     free (fname);
 }
@@ -329,12 +333,12 @@ exit_pass (void)
 
     fname = Xasprintf ("%s%s", current_parsed_root->directory, pass_file);
     if ((fpass = fopen (fname, "w")) == NULL)
-        error (1, errno, "could not open: %s", fname);
+        error (1, errno, "could not open: `%s'", fname);
     flockfile (fpass); /* acquire exclussive lock on CVSROOT/passwd file */
     write_pass (fpass);
     funlockfile (fpass);
     if (fclose (fpass) != 0)
-        error (1, errno, "could not close: %s", fname);
+        error (1, errno, "could not close: `%s'", fname);
 
     dellist (&pass_list);
     free (fname);
@@ -436,7 +440,7 @@ change_pass (const char *usr, const char
     Node *tmp = NULL;
     char *lusr = NULL, *rec = NULL;
 
-    assert (usr != NULL); 
+    assert (usr != NULL);
     assert (pass != NULL);
 
     tmp = findnode (pass_list, usr);
@@ -511,7 +515,7 @@ alias (const char *usr, const char *als)
 static int
 gettok (const char *s, char delim, char **ret)
 {
-    int j = 0; 
+    int j = 0;
     char buf[BUFLEN];   /* BUFLEN = 512 */
     static int i = 0;