Re: [PATCH] importinfo/admininfo

[Mark D. Baushke]

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

Ralf S. Engelschall <address@hidden> writes:

> On Sat, Oct 04, 2003, Mark D. Baushke wrote:
> 
> > Ralf S. Engelschall <address@hidden> writes:
> >
> > > The patch adds two still missing and very important auditing hook
> > > facilities to CVS: "CVSROOT/importinfo" for auditing "cvs import"
> > > operations and "CVSROOT/admininfo" for auditing "cvs admin" operations.
> > > I was prompted to implement these some years ago when I wrote OSSP
> > > Shiela (see http://www.ossp.org/pkg/tool/shiela/ for details) and had
> > > to recognize that it still could be circumvented by the two remaining
> > > repository-destructive operations "cvs import" and "cvs admin".
> >
> > I believe it is still possible for a 'cvs import' to destroy important
> > cvs branch and version tags. If the intent is to help lock down the
> > 'cvs import' command, then more work is probably needed...
> 
> Can you give more details, please? Why can "cvs import" still
> destroy any tags if an "importinfo" filter would stop processing? My
> "importinfo" hook is very early in the "cvs import" processing and AFAIK
> there is no repository write access before it.

I was under the impression that the importinfo_runproc was running the
filter and passing the importinfo_vtag, the repository and the files
being imported. Is the vendor branch getting to the script thru some
other means?

I suppose this is not a big deal as it should only really matter for
tags that are for real RCS branches as opposed to CVS magic branches,
but I wasn't 100% sure about failure modes...

Another fairly minor matter is that the -k options is not known at
import time and might be fairly critical to folks that might want
the -kb default for 'binary' files to be checked... I find it less
important to worry about the -k option on a cvs commit than on an
import. However, I suppose that both of them really should be treated
the same, so I am just noting in passing that a server on a Windows box
may be at more of a disadvantage in understanding how the imported files
are to be opened for verification than their non-windows counterparts.

> > [...]
> > I will note that the man pages are not the primary reference
> > documentation for cvs, so it would be desirable for you to extend the
> > docuemntation in the doc/cvs.texinfo file as well.
> 
> I've looked at the doc/cvs.texinfo file and have not found any chapter
> which really deals with the xxxxinfo hooks. There are just a few
> references and small examples at various places for some hooks, but
> no general documentation on them. So I decided to document them in
> cvs(5), because there at least mostly all xxxxinfo hooks _are_ already
> documented.

I understand the documentation is light, but just as there is an '@node
commitinfo' in doc/cvs.textinfo, there should be one for each of the two
new files you are adding.

> > It may not be argued that it is more complex to use multiple scripts and
> > possibly accidentally introduce different standards for entry into the
> > source base. I have a slight bias toward moving to a new importinfo
> > script to augment what already exists rather than overloading the
> > existing commitinfo with the added state caused by knowing that this is
> > an import over multiple directories and files with two new tags being
> > added to the repository... so, I have a slight bias in favor of your new
> > feature in principle over reworking the commitinfo script.
> 
> Especially, if you change the existing commitinfo script you too
> easily could break lots of existing tools.

Yes, I find this to be a good argument. It remains to be seen if the
consensus of the others matches it.

> > One thing that does concern me is that the tags for a cvsimport do
> > not appear to get passed to either importinfo nor taginfo for checking.
> > If the importinfo feature is to really check what the user is doing, is
> > it not possible that the user is about to destroy a very important
> > release tag that already exists as a side-effect of their import?
> 
> Why are the tags not passed? At least for my "importinfo", the
> vendor-tag is passed as an argument. Without this my OSSP Shiela
> facility would not be able to control imports to different branches.

Hmmm... I didn't notice this and it did not appear in the documentation
for how the file is used...

> > With regard to the admininfo trigger, I presume that users will either
> > need to be a part of the CVS_ADMIN_GROUP or there will need to be no
> > CVS_ADMIN_GROUP at all in order for the admininfo script to be run. In
> > addition, only those options permitted by UserAdminOptions will get
> > thru. Is that correct? If so, it probably deserves a mention in the
> > doc/cvs.texinfo file.
> 
> Correct. My current "admininfo" is processed _after_ the CVS_ADMIN_GROUP
> stuff is processed.

Good. I thought this was the case, but it should be documented for
folks in the man page and cvs.texinfo node on the file.

> > Also, there are many discrete operations possible with 'cvs admin'
> > and they are very different. It is not clear to me how the script
> > would know that a user might be deleting a revision in the file
> > versus changing from -kkv to -kk keyword expansion mode or trying
> > to unlock a revision that had somehow been locked.
> 
> That's a good point. I think we should pass the arguments
> to "cvs admin" through to the script. I'll see...

Excellent. That would make a lot of sense to me.

> > For example, might it be desirable to ensure that a replacement
> > log message (-mrev:msg) conformed to the same standards as one
> > that was given during a 'cvs commit' operation?
> 
> Hmmm... my personal opinion here is that "cvs admin" is a low-level
> operation and hence (once passed the "admininfo" filters) should be
> able to do whatever is wished and does not have to conform to the same
> standards as higher-level operations. But that's just my point of
> view...

Sure, it is one point of view.

Right now the administrator is able to allow various of the switches to
be used by anyone regardless of membership in the CVS_ADMIN_GROUP. However,
if the administrator wanted to make some of the more powerful features
available, they still might want to verify basic uses of the features.

> > It might be that the cvs administrator would need/want the list
> > of switches being passed to the admininfo script. A good design
> > now might really give a cvs administrator more flexibility in
> > what they allow their users to do for themselves.
> 
> Yes, we should pass the options on the "cvs admin" command line to the
> filter script.

Agreed.

        Thanks!
        -- Mark
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.3 (FreeBSD)

iD8DBQE/f0eR3x41pRYZE/gRAikfAJ9q1tMpYjuI08KRBTW5Sb3qQ0pcygCeIX/A
AcJ1R/0hfuILqktScfLoN/4=
=ehTR
-----END PGP SIGNATURE-----