patchConcurrent Versions System - Patches: patch #5155, acl (access control list)...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #5155: acl (access control list) extension feature patch

Submitter:  Baris Sahin <sbaris>
Submitted:  Fri 02 Jun 2006 03:04:57 PM UTC
   
 
Category:  Feature (patch attached) Priority:  5 - Normal
Status:  None Privacy:  Public
Assigned to:  None Open/Closed:  Open
Fixed Release:  None Fixed Feature Release:  None

Wed 07 Jun 2006 08:18:20 AM UTC, comment #3: 


>> Additions to the client/server protocol need to be discussed....
> actually there is not much changes to client/server protocol,


I believe your patch provides two new keywords with options be
sent from the client to the server. These appear to be

  1) "racl\012" with options -R, -l, -r <tag> and -- <files>
  2) "acl\012"  with options -R, -l, -r <tag> and -- <files>

The names need to be 'reserved' for use in both CVS and CVSNT
so that there are no-interoprability problems.

The presence of new REQ_LINE entries in the server are what
lets a CVS client know it is talking to a server which is
able to support the commands the client wishes to use.

Your client side code may need to use

    if (is_racl && !(supported_request ("racl"))
        error (1, 0, "server does not suport racl");

Or, you may wish to also check to see if the similar
CVSNT feature is supported using something like

        supported_request ("rlsacl")
or
        supported_request ("chacl2")

and adjust what you send to the server from the client to
be useful for the other implementation.

> it will just setuid when CVSServerRunAsUser keyword is set.


This is not really relevant to the client/server discussion,
but your are correct that setuid is also a potential issue.

>> It is highly desirable that you include doc/cvs.texi...
> which tool i should use to modify these documentation files?


I use either Emacs, XEmacs, or vim. They are simple text files,
so you should use your favorite text editor for the purpose.
You will probably want teTeX 3.0 or texinfo 4.8 if you wish
to convert the cvs.texi file into cvs.info and cvs.pdf files.

> Then i will edit them.


Okay.

>> I am concerned that this FEATURE seems to make use of a
>> set-uid or set-gid cvs executable ...


> I only use setuid when CVSServerRunAsUser keyword is set,


How safe is a set-uid or set-gid cvs executable? Running
in :pserver: mode, it switches out of root as quickly as
possible.

There are some folks that are using a set-gid cvs for some
things, but they typically do not make that cvs executable
available on an unprotected subnet. The NetBSD folks have
some additional SETXID_SUPPORT tests in their CVS code base,
but I have not had time to audit those changes for cvs 1.11.x
or look at the implications for cvs 1.12.x usage.

As such, it may be that you need your documentation to
recommend against using the CVSServerRunAsUser keyword
on unprotected networks... unless you are trying to say
that you have done a full audit of all of the set-uid and/or
set-gid code and know this is not a problem for anyone?

> it switch to specified user in server.c file,


This can potentially be a security hole, which is my concern.

> then other controls apply in server.c.
> So, i think it will not be a problem, ?


I do not actually trust CVS to run on the internet other than
using :ext: with an SSHv2 transport unless the server is a
read-only mirror of a safely protected repository.

> access file is normally under $CVSROOT. But when
> UseSeparateACLFileForEachDir keyword is set to
> yes, one access file is created for each directory
> in repository.


Okay. This is what I thought might be happening.

> I am not sure about this that is it needed or not.


It may indeed be needed, but control files for CVS
probably belong in a subdirectory called CVS
rather than at the same level as the ,v files.

This is one of the kinds of discussion that should
probably be thrashed out on the -email is unavailable-
mailing list.

> I thought about big repositories (thousands of
> files and directories and lots of acl definitions
> so a big access file...).


Yes, consistency locking of the file for read
could make a single CVSROOT/access file a hot-spot
and that is probably not desirable.

> When access file is very big, it may slow down cvs
> actions. Dont sure about it. ?


It seems likely that there will be a cost
associated with it.

> Actually, if access files under each directory
> are copied, acl definitions will also be copied,
> but i dont know cvsup will copy access files,
> will check it.


I suspect it is configurable, but I am not sure.
Thank you for looking into it. Your findings are
probably also reasonable to add to your
documentation.

Mark D. Baushke <mdb>
Group administrator
Wed 07 Jun 2006 07:25:07 AM UTC, comment #2: 


> This patch appears to remove some recent changes to the .....

my checkedout copy of ccvs wat not the latest, will just remove those lines.

> The indentation for the lines added via patch.diff....

i will replace tabs with spaces, editor will do it easily.

> Additions to the client/server protocol need to be discussed....

actually there is not much changes to client/server protocol, it will just setuid when CVSServerRunAsUser keyword is set.

> In general, it would be nice to understand if there is any
> functional overlap between CVSNT...

it does almost same thing with cvsnt, but the logic is different. Also permissions and usage is very different.

> It is highly desirable that you include doc/cvs.texi...

which tool i should use to modify these documentation files? Then i will edit them.

> I am concerned that this FEATURE seems to make use of a set-uid or set-gid cvs executable ...

I only use setuid when CVSServerRunAsUser keyword is set, it switch to specified user in server.c file, then other controls apply in server.c. So, i think it will not be a problem, ?

> Please replace code constructs like ...

Ok, will replace all these lines, didn't see new Xasprintf().


> If I understand the code correctly, there are times when a
> file called 'access' could be sprinkled throughout the
> repository...

access file is normally under $CVSROOT. But when UseSeparateACLFileForEachDir keyword is set to yes, one access file is created for each directory in repository. I am not sure about this that is it needed or not. I thought about big repositories (thousands of files and directories and lots of acl definitions so a big access file...). When access file is very big, it may slow down cvs actions. Dont sure about it. ?
Actually, if access files under each directory are copied, acl definitions will also be copied, but i dont know cvsup will copy access files, will check it.

> I suspect that the contrib/validate_repo.pl will need some
> updates...

I may do changes....


Baris Sahin <sbaris>
Fri 02 Jun 2006 05:25:45 PM UTC, comment #1: 

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 -email is unavailable- 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.

Mark D. Baushke <mdb>
Group administrator
Fri 02 Jun 2006 03:04:57 PM UTC, original submission:  

This is the patch for access control list extension feature.
tgz file includes diff files, acl.c file and documentation file.


No need to say but, below are steps to apply patch...
cd to ccvs folder
patch -p0 < patch.diff
patch -p0 < sanity.sh.diff
cp acl.c under src folder
add acl.c to src/Makefile.am
automake-1.9

Regards,

Baris Sahin
-email is unavailable-

Baris Sahin <sbaris>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #10109:  acl-feature-patch.tgz added by sbaris (25KiB - application/x-gzip-compressed - acl feature patch file)

 

Depends on the following items: None found

Items that depend on this one: None found

 

CC list is empty

 

There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

 

Follow 2 latest changes.

Date Changed by Updated Field Previous Value => Replaced by
2006-06-02 sbaris Attached File- Added acl-feature-patch.tgz, #10109
    Carbon-Copy- Added -email is unavailable-

Back to the top

Powered by Savane 3.13-f8d8.
Corresponding source code