[Cvs-dev] Re: cvs-passwd patch

[Mark D. Baushke]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Prasad J Pandit <address@hidden> writes:

> > In fact, there are a great many functions defined in passwd.h as globals
> > that should probably not be there at all and should instead be static
> > functions that appear only in passwd.c
> 
>    I'll really appreciate, if you could list them out.

All of them with the exception of the entry points needed by server or
client operations should be static. So, 

  * the passwd() should NOT be static (used by main.c)
  * the spasswd() should NOT be static (used by server.c)

Everything else should be static and contained only in passwd.c
I believe these are the functions and global variables

Global variables:

  * mode - This is a bad name to use as a global in any case. I would
           suggest something like pw_cmd_mode might be more appropriate.
  * do_encrypt
  * pass_file - Is this something that shoule be shared with login.c ?

Functions:

  * check_option
  * init_pass
  * exit_pass
  * get_md5_digest
  * encrypt_pass
  * generate_salt
  * read_new_passwd - NOTE: THis one may be combined with login.c functionality
  * change_pass
  * alias - This is also a bad name and should be changed.
  * gettok - This may not be the best name to use. pw_entry_split or
             some such may be a better name. I am not worried about the
             name too much as long as it is static. I do worry if it needs
             to be global.
  * is_admin - This is a bad name and/or not needed in 1.12.13.1+
  * read_pass
  * write_pass
  * getrec    - Linked list processing. Replace with hash.c stuff eventually?
  * l_insert  - Linked list processing. Replace with hash.c stuff eventually?
  * l_remove  - Linked list processing. Replace with hash.c stuff eventually?
  * l_search  - Linked list processing. Replace with hash.c stuff eventually?
  * l_free    - Linked list processing. Replace with hash.c stuff eventually?
  * show_list - Linked list processing. Replace with hash.c stuff eventually?

In addition, any of the structures or 'global' variables need to be
examined and probably made static.

Have you tried to configure --disable-client and had it work? If so, how
did the 

...
passwd (...)
{
...
#ifdef CLIENT_SUPPORT    
...
#endif    
    if (!mode) free (pass);
    free (current_parsed_root->password);

    return ret;
}
...

lines really work? (I was not aware that 'password' was a valid field
when unless either CLIENT_SUPPORT or SERVER_SUPPORT was defined.)

I suspect you have another bug to fix.

> > I do not understand why you need your own linked list package in
> > passwd.c ... Could you use the list processing in hash.c instead?
> > getlist(), getnode(), addnode(), delnode(), freenode(), insert_before(),
> > dellist(), et al? I won't worry too much about it right now as what you
> > have seems simple enough and might even be correct.
> 
>    Oh, come on Mark, this is really absurd. I mean, linked list is
> there, right from the begining. Why didn't you tell me this earlier,
> isn't this a bit too late, to point this out.

To be honest, there were many other problems with your initial offering
and I was more concerned about the protocol you were proposing than I
was about the implementation you were attempting. I also tried to give
you guidelines about coding style and tests.

I am not forcing you to use the hash.c package. 
(It is a shame that you did not use it to start.)

Looking at your patch file is very painful as it contains so may
regenerated and binary files that I need to prune to even look at it.
There are still lots of gratuitous white-space changes to the source
that should not be used. Hunks like these need to be revereted:

... main.c ...
@@ -829,7 +831,6 @@
        if (!strcmp (cvs_cmd_name, cm->fullname))
            break;
     }
- -
     if (!cm->fullname)
     {
        fprintf (stderr, "Unknown command: `%s'\n\n", cvs_cmd_name);


... sanity.sh ...
@@ -35507,7 +35562,6 @@
          ;;
 
 
- -
        *)
           echo $what is not the name of a test -- ignored
           ;;

... server.c ...
@@ -6312,7 +6319,7 @@
 
     if (argc == -1)
        usage (server_usage);
- -
+       
     /* Options were pre-parsed in main.c.  */
 
     /*

Hmmm... those might be the only ones left at present. Some of the other
stuff is due to auto-generated file changes.

> > You have a similar problem with read_pass().
> >
> > I did not look closely. How does gettok() differ from strtok() ?
> 
>    Yes, there is a difference. I had used strtok() first, but it
> doesn't work for a CVSROOT/passwd entry like 'username::' or
> 'username::password'.

Ahhh... Okay, I suggest a different name may be desirable,
but making it static would also probably be okay.

> > The sanity.sh stuff is still a mess. If you feel you MUST have tests that
> > rely upon $PWCVSROOT, you should not fail if that variable is not properly
> > set, instead print out the tests you are skipping. However, you really
> > should still exercise the server side protocol for doing the various
> > 'pw' 'pw -a username' 'pw -r username othername' 'pw username' 'pw -R user'
> > 'pw -x user' 'pw -X user' situations in EXACTLY the same manner as the
> > 'pserver' tests
> >
> >            cat >$CVSROOT_DIRNAME/CVSROOT/passwd <<EOF
> > testme:q6WV9d2t848B2:$username
> > dontroot:q6WV9d2t848B2:root
> > anonymous::$username
> > $username:
> > willfail:   :whocares
> > EOF
> >
> >        # NOTE THIS next test shows that the passwd command does not
> >        # properly deal with bogus arguments. This is a bug that should
> >        # be fixed... the problem appears that spasswd can send garbage
> >        # to the descramble() function if that is what the user
> >        # provides. This is a bad thing.
> >
> >         # Test how the passwd command works
> >            dotest_fail passwd-1 \
> > "$servercvs --allow-root=${CVSROOT_DIRNAME} pserver" \
> > "$DOTSTAR LOVE YOU
> > error  unrecognized request `'
> > E Terminated with fatal signal 11
> > # Core dumped; preserving $DOTSTAR on server\.
> > E CVS locks may need cleaning up\.
> > error" <<EOF
> > BEGIN AUTH REQUEST
> > $CVSROOT_DIRNAME
> > testme
> > Ay::'d
> > END AUTH REQUEST
> > Root $CVSROOT_DIRNAME
> > passwd
> >
> > EOF
> >
> > Yes, I know that there are supposed to be arguments to the passwd
> > command. However, your server needs to deal with all possibilities
> > of bogus input from a remote client.
> 
>    ummn okay, I'll do that.

Good. Validation of arguments is better than a security incident.

> Ones again, thanks for the information.

Note: I do not have a :pserver: configured and I have no plans to ever
use one (I believe they are inherently a bad idea). However, if you want
to capture the transaction for each of your tests using CVS_CLIENT_LOG
and playing the .in and .out files toward the pserver as in the example
test above, it would exercise the server side variations for all users
that do not have a $PWCVSROOT and still wish to validate that the server
should work properly. If $PWCVSROOT is not set, then the passwd* tests
should skip those tests which would have required it be defined.

If you can't figure out how to do the task (it can be tricky), then
including a tarball with the series of $CVS_CLIENT_LOG.in and
$CVS_CLIENT_LOG.out files might be helpful if someone else has time to
write those tests.

        Thanks,
        -- Mark

PS: Please note that I did not really look at the changes to doc/*texi
files. If anyone else feels up to doing that review, it would be good.

I will note that the actual wire-line protocol is not yet provided for
the passwd command itself. I expect you may need to provide an '@item
passwd' to the '@node Requests' '@section Requests' '@table code' in
that file.

I am also assuming that the changes to cvs.1 are not needed because they
would be regenerated by your changes to cvs.texi ...

For future reference, including your getdate.log~ file is probably not
desirable.

If you want another drop of the top-of-tree sources at some point, let
me know. It has drifted a bit from the 1.12.13 release you are using.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.4 (FreeBSD)

iD8DBQFFF3RZCg7APGsDnFERAtkBAKCaDItrsxivzJrdCypwvDveEelKCACfUQRb
O/fm3TD92DI+YWeq2oWAU0U=
=dECH
-----END PGP SIGNATURE-----