[Cvs-dev] Re: cvs-passwd patch

[Mark D. Baushke]

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

P J P <address@hidden> writes:

> On Mon, 16 Oct 2006, Mark D. Baushke wrote:
> > 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.
> 
>    Well, I believe, using two seperate functions makes it more clear &
> easy to understand. I know, what I went through, when I had to
> understand it. And as far as argument processing goes, it's still
> consistent, isn't it?

I'm not sure. I do know that a lot of the global state problems you had
could be attributed to needing to worry about rusr and dpass for the
argument processing not being in the 'main' program for the 'passwd'
function. It will also be much easier to have a :local: implementation
for 'cvs passwd' if you use a single function for it instead of assuming
that you will be running remotely on the server and the spasswd function
to do the heavy lifting.

> > now ask this next question... Is there any reason that the "passwd"
> > command is required to be used only by :pserver: client/server
> > connections?
> 
>    Oh certainly! See, I started using cvs with the :pserver: protocol;
> I mean, that is how I was introduced to it. Later, when all that
> password changing stuff freaked me out, I decided to make the
> (required)changes the cvs, and obviously, back then, I wanted to do it
> with the :pserver: only.

Hmmm... there are some significant architecture choices you have made
that will change if you make it possible to use the passwd command for
non-pserver client/server communications.

You really should look at the CVSNT passwd.cpp even if it is C++ code to
see how they handle running 'cvs passwd' properly from any CVSROOT
specification.

> > 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?
> 
>    Sure! In fact, I had thought of doing it, once the :pserver: part
> is over; I mean, once you accept these :pserver: changes; As that is
> the prime *itch* for *me*, to do all these things. And also the fact,
> that, I'll first have to do some :ext: work, to make the necessary
> changes. I hope you understand.

Well, I would rather not put in a half formulated feature.

I really do wish for reasonable things to happen when 'cvs passwd'
is given as the command regardless of method being used:

   :local:/path/to/repository
   :ext:host.name/path/to/repository
   :fork:/path/to/repository
   :pserver:host.name/path/to/repository

Among these will be the desire to avoid use of

    connect_to_pserver (current_parsed_root, NULL, NULL, 1, 0);

to validate the current password. Either they are connected to the
repository by having done a 'cvs login' or they are authenticated as a
regular user via the appropriate authentication method for their current
username.   

> > 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.
> 
>    Yes right!
> 
> > 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).
> 
>    Ummn, right, I know it.
> 
>    First, for the size limit of 128, I don't know, if that's possible
> with the 'getpass()' function, which I'm using to read the password.
> I'll do it if it's doable.

getline() which is used by getpass() will return an arbitrarily sized
password. All you would want to do is restrict it to an appropriate
length. I only suggested using the maximum length of a CVSNT password to
ensure interoperability with CVSNT.

So, you don't want to create a password on the server that a CVSNT
client is unable to specify when it talks to your server.

>    Second, Initially, I too, had thought of warning the administrator,
> who wish to 'delete' or 'disable' herself; As, 'add' is anyway not
> going to happen, because of duplicate username(error: 'name `%s' is
> not unique').
> But, then thought(still think), it's kind of unnecessary; I mean, the
> administrator knows, what she is up to, isn't it?

Really? I do not assume that anyone will always get things right via
a command-line interface.

> > 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.
> 
>    It was a bit confusing. I read the DJG, and couple of others; it
> says s/random are not portable enough, and s/rand are standard ANSI-C
> functions. But the manual of both rand & srand say that, random &
> srandom are better choices, and discourage the use of rand/srand in
> cryptographic application.
> 
>    But in the end(after some more reading), I thought, all I'm need is
> random array index between 0-63, and for that rand/srand seems to be a
> good choice. Anyway, the actual md5 encryption is going to be done by
> 'crypt(3)', isn't it?
> 
>   I'll look into the '/dev/urandom' stuff in main.c.

I have been informed by Larry that some of the stuff in main.c looks
non-optimal. I don't have time to do any hacking on CVS code myself this
month, so I am not going to look at it until next month when I hope to
have a bit more development time available... right now I am spending
most of my CVS development cycles trying to give you feedback. :-(

So, in the mean time, I suggest you just keep the srand/rand
implementation. We can look at using configure to provide for
determining if any of srand48/rand48 or srandom/random or srand/rand are
available at some later date.

Important things remaining:

  1) test cases which do not require a :pserver: to exercise the code
     [Hint: Being able to use :local: or :fork: would make testing a
     lot easier and make the administrator who might only be allowing
     :pserver: for read-only users an easier job of getting in via
     :ext: over SSH or using :local: on the box]

  2) Handling more error conditions. What happens when a user issues
     a 'cvs passwd' command and no CVSROOT/passwd file exists? Or,
     what happens if the file is read-only and the user is not able
     to modify it.

If you are not actually going to finish passwd to work on other
transports (I think this would require a vote of the other cvs-dev folks
to agree to that subset of work), then you need to send some
CVS_CLIENT_LOG examples for your existing test cases that we can see if
we can engineer them to work. It might also be possible to setup the
pserver process to listen on a non-2401 port and wait for our client to
connect to that port. This might require some kind of debugging output
that lets the parent process know which port the child pserver is
listening on to get things right.

        -- Mark
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.4 (FreeBSD)

iD8DBQFFNEnOCg7APGsDnFERAjlRAKDri9FqHFfAh6gk6MzgBNubQAWRmgCgxvOR
Qf7UjLz4KAUco6G6RW+b16U=
=3zp8
-----END PGP SIGNATURE-----