[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] importinfo/admininfo
Mark D. Baushke
Re: [PATCH] importinfo/admininfo
Sat, 04 Oct 2003 11:13:55 -0700
-----BEGIN PGP SIGNED MESSAGE-----
[Note: I'll send Ralf the picky C-style comments in a separate private
Ralf S. Engelschall <address@hidden> writes:
> The first result is a completely worked-off "importinfo" and "admininfo"
> patch against the latest CVS HEAD version of CVS which you can find
> under http://www.engelschall.com/~rse/cvs/cvs.patch-info.diff and in the
> attachment to this email.
Thank you very much for your time.
> 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...
> The patch was adjusted in style (including C89) to now hopefully fully
> conform to the CVS coding style, and extended with documentation and
> full test suite additions. It also includes some of the feedback you
> already gave to Wu Yongwei. It should be now ready for final inclusion
> into the CVSHome.org CVS sources and become part of the next major CVS
Well, there are a few minor places where you use closely nested braces
rather than putting them on a new line by themselves and a few other
lines that should probably be reformatted as being too long.
However, the majority of the code does match the coding standards and I
really appreciate the work you did to adapt your patch and write the
documentation and test cases.
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.
> Please review the patch in detail and give me feedback. If everything is
> fine, please commit it to the CVS HEAD of CVS and let us proceed to the
> next possible contributions derived from my OpenPKG CVS patch set.
[I believe that Larry has suggested that a separate importinfo may not
be needed and that the existing commitinfo and taginfo scripts should be
extended. I will let Larry make his own comments concerning your patch.]
My own opinion is that some administrators may wish to impose controls
that are orthogonal to normal commits and having a different script
allows for more flexibility to the cvs administrator.
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.
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?
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
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.
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?
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.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.3 (FreeBSD)
-----END PGP SIGNATURE-----