[patch #5155] acl (access control list) extension feature patch

[Mark D. Baushke]

Follow-up Comment #1, patch #5155 (project cvs):

This patch appears to remove some recent changes to the latest
sources even as it adds in your new feature. This is undesirable.

The indentation for the lines added via patch.diff does not
follow the HACKING guidelines. Indent by 4 spaces rather than
by a tabstop.

Additions to the client/server protocol need to be discussed
and ratified on both the cvs-dev@nongnu.org and the
cvsnt-dev@march-hare.com. Please note that CVSNT already
has some ACL based commands (chacl2, rchacl2, lsacl, rlsacl).

In general, it would be nice to understand if there is any
functional overlap between CVSNT and your new proposed feature
addition to CVS.

It is highly desirable that you include doc/cvs.texi
(to add information on the new CVS commands and CVSROOT/config
options) and doc/cvsclient.texi (The CVS client/server protocol)

I am concerned that this FEATURE seems to make use of a
set-uid or set-gid cvs executable on the server. I believe
that the NetBSD folks have a number of SETXID_SUPPORT patches
to make this safer, but the use of the SETXID_SUPPORT setup
has not been well tested recently and may introduce security
flaws. Could you address the setups where ownership and groups
are being checked and modified? Is that only for :pserver: use
or is it also available under :ext: (just looking at the patch
it is not immediately obvious).

I do understand that this is an ongoing
  http://cvsacl.sourceforge.net
project and that it is desirable to get this code adopted
into the CVS FEATURE release. I just want to make sure
that everything needful is in place.


Please replace code constructs like this:

    accessfile = xmalloc (strlen (current_parsed_root->directory)
                          + sizeof (CVSROOTADM)
                          + sizeof (CVSROOTADM_ACCESS)
                          + 3);

    strcpy (accessfile, current_parsed_root->directory);
    strcat (accessfile, "/");
    strcat (accessfile, CVSROOTADM);
    strcat (accessfile, "/");
    strcat (accessfile, CVSROOTADM_ACCESS);

with Xasprintf () calls like this:

    accessfile = Xasprintf ("%s/%s/%s",
                            current_parsed_root->directory,
                            CVSROOTADM, CVSROOTADM_ACCESS);

If I understand the code correctly, there are times when a
file called 'access' could be sprinkled throughout the
repository. I suspect that CVSup might not do the right thing
with such files (I have not looked closely). Do you know
if access rights are properly copied in such situations?

I suspect that the contrib/validate_repo.pl will need some
updates to deal with such files.

There may be more things that I missed, but that is my first
pass feedback on the patch so far.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?func=detailitem&item_id=5155>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/