Spec URL: http://www.zachcarter.com/rpms/schroot.spec SRPM URL: http://www.zachcarter.com/rpms/schroot-1.2.0-1.fc9.src.rpm Description: schroot allows users to execute commands or interactive shells in different chroots. Any number of named chroots may be created, and access permissions given to each, including root access for normal users, on a per-user or per-group basis. Additionally, schroot can switch to a different user in the chroot, using PAM for authentication and authorisation. All operations are logged for security. Several different types of chroot are supported, including normal directories in the filesystem, and also block devices. Sessions, persistent chroots created on the fly from files (tar with optional compression and zip) and LVM snapshots are also supported. schroot supports kernel personalities, allowing the programs run inside the chroot to have a different personality. For example, running 32-bit chroots on 64-bit systems, or even running binaries from alternative operating systems such as SVR4 or Xenix. schroot also integrates with sbuild, to allow building packages with all supported chroot types, including session-managed chroot types such as LVM snapshots. schroot shares most of its options with dchroot, but offers vastly more functionality.
Given the pam and suid components of the package, it would probably be wise for a security SME to review the package.
Some comments based on an older schroot rpm I had written some time ago, but haven't gotten around to finish: MUSTFIX: - dchroot.1.gz belongs into the dchroot-package - These "Requires" are superfluous, please remove them: Requires: pam Requires: boost Requires: lockdev - The i18n files (aka %{_localedir]/*) belong into the packages containing libsbuild.so.* CONSIDER: - Splitting out libsbuild.so.* into a separate libsbuild package doesn't make much sense. On Debian such kind of split is technically necessary, but it isn't on rpm-based systems. - Add a *-devel package to take the devel-files. Points I am not sure about: - AFAICT, schroot needs an additional directory: /var/lib/schroot/mount My old schroot rpm didn't work without it. - I am having doubts on your schroot-remove-tmpfs-shm.patch. Why are you removing tmpfs? In my old rpm, I also patch mount-defaults, but unlike you, I keep tmpfs and remove /home and /tmp.
Thanks for the feedback Ralf. I made these changes: * Tue May 20 2008 Zach Carter <z.carter> - 1.2.0-1 - move dchroot.1.gz to correct subpackage - removed superfluous Requires: statements - moved i18n files into libsbuild subpackage - removed tmpfs patch Some comments: - The reason I split the libsbuild.so into a separate package was to conform to the Fedora packaging guidelines. If I can get this approved without that subpackage, I'd be happy to change it. - I'm willing to add a *-devel package, but I'm not sure anyone would find it useful. Do you think anyone would actually want it? - The latest version of schroot automagically creates the /var/lib/schroot/mount directory as soon as it is needed. I tested that to be sure. (the session dir still needs to be created ahead of time) - /home is very useful in my environment, but I can remove it if needed. As it is right now, the mount-defaults file comes unchanged from the upstream version.
Updated spec and SRPM with new 1.2.1 version: http://www.zachcarter.com/rpms/schroot.spec http://www.zachcarter.com/rpms/schroot-1.2.1-1.fc9.src.rpm
I don't feel competent to fully review this package but I can make a few comments: You shouldn't duplicate all of those %doc files between the various subpackages. I'm curious as to which guideline you believe mandates that you split off the libsbuild package. Generally library splits are only required to prevent multilib conflicts, but I don't believe this is a multilib package. (For one thing, it has no -devel subpackage.) There are a few rpmlint complaints: dchroot.x86_64: E: setuid-binary /usr/bin/dchroot root 04755 dchroot.x86_64: E: non-standard-executable-perm /usr/bin/dchroot 04755 schroot.x86_64: E: setuid-binary /usr/bin/schroot root 04755 schroot.x86_64: E: non-standard-executable-perm /usr/bin/schroot 04755 Obviously these are intended. dchroot.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/dchroot ['/usr/lib64'] schroot.x86_64: E: binary-or-shlib-defines-rpath /usr/libexec/schroot/schroot-releaselock ['/usr/lib64'] schroot.x86_64: E: binary-or-shlib-defines-rpath /usr/libexec/schroot/schroot-mount ['/usr/lib64'] schroot.x86_64: E: binary-or-shlib-defines-rpath /usr/libexec/schroot/schroot-listmounts ['/usr/lib64'] schroot.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/schroot ['/usr/lib64'] These are problematic. The tarball seems to include a large amount of doxygen-generated documentation. Is that of any use to end-users? If so it should probably be packaged, although a subpackage might be useful. I agree that a security review would be useful, but I'm certainly not the one to do it.
(In reply to comment #5) > I don't feel competent to fully review this package but I can make a few > comments: thanks :) > You shouldn't duplicate all of those %doc files between the various > subpackages. will fix. > > I'm curious as to which guideline you believe mandates that you split off the > libsbuild package. Generally library splits are only required to prevent > multilib conflicts, but I don't believe this is a multilib package. (For one > thing, it has no -devel subpackage.) I can't find it anymore, but I was pretty sure it existed. I'll change it back. > schroot.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/schroot > ['/usr/lib64'] > These are problematic. I'll fix these. > The tarball seems to include a large amount of doxygen-generated documentation. > Is that of any use to end-users? If so it should probably be packaged, > although a subpackage might be useful. It looks mostly like developer docs, indexed source code, etc. I would think developers would probably have downloaded the source code anyway, so I'm not sure how useful it would be to individual users.
Updated spec and SRPM: http://www.zachcarter.com/rpms/schroot.spec http://www.zachcarter.com/rpms/schroot-1.2.1-2.fc10.src.rpm %changelog * Wed Nov 5 2008 Zach Carter <z.carter> - 1.2.1-2 - move libsbuild subpackage into main package - remove duplicate doc entries - disable rpath - defattr for dchroot files
Note that I've asked on fedora-security-list for someone to have a look at this package. I've no idea of what response I might receive, but there's at least a chance that we can move forward.
New upstream version, built for f11: http://www.zachcarter.com/rpms/schroot.spec http://www.zachcarter.com/rpms/schroot-1.2.2-1.fc11.src.rpm
The pam security patch seems fine to me. It's a straight over move from a non-fedora pam to our system-auth model. Looks pretty good. I'd say a cursory review from someone a bit more experienced than I is a good idea, but it sure seems like a nice applcation.
You may want to have a look at Debian bug: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=526788 I have not checked if this may affect Fedora packages, probably worth checking before this enters repositories.
(In reply to comment #11) > You may want to have a look at Debian bug: > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=526788 > > I have not checked if this may affect Fedora packages, probably worth checking > before this enters repositories. Thanks for the heads up. Here is an update that includes the patch: http://www.zachcarter.com/rpms/schroot.spec http://www.zachcarter.com/rpms/schroot-1.2.2-2.fc11.src.rpm
(In reply to comment #11) > You may want to have a look at Debian bug: > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=526788 > > I have not checked if this may affect Fedora packages, probably worth checking > before this enters repositories. Tomas, I noticed you are somewhat active on the fedora-security-list Would you be willing to take a look at this package from a setuid and pam perspective? I think it's ready to go, but no one wants to push it thru without a security-oriented review. thanks!
Given my actions backlog, I can't really promise you to have any serious look at the moment, sorry. ;(
Well, that was a bust. At this point I'm of the opinion that yes, this is potentially problematic, but we aren't installing on everyone's system by default and if there's a problem we can deal with it. Honestly a review ticket shouldn't have to sit around for fourteen months anyway. So, I have some time so I'm going to go ahead and work on this. Here's the latest rpmlint output: dchroot.x86_64: E: non-standard-executable-perm /usr/bin/dchroot 04755 schroot.x86_64: E: non-standard-executable-perm /usr/bin/schroot 04755 dchroot.x86_64: E: setuid-binary /usr/bin/dchroot root 04755 schroot.x86_64: E: setuid-binary /usr/bin/schroot root 04755 These are OK; being setuid is kind of necessary. schroot.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libsbuild.so.1.0.0 /lib64/libm.so.6 schroot.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libsbuild.so.1.0.0 /usr/lib64/libboost_filesystem.so.5 schroot.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libsbuild.so.1.0.0 /usr/lib64/libboost_program_options.so.5 The indicated library is linked against those three libraries without actually calling anything in them. This is common with libtool-using packages. It's not an especially big deal, but you can find some info on making it go away at http://fedoraproject.org/wiki/Common_Rpmlint_issues#unused-direct-shlib-dependency schroot.x86_64: E: library-without-ldconfig-postin /usr/lib64/libsbuild.so.1.0.0 schroot.x86_64: E: library-without-ldconfig-postun /usr/lib64/libsbuild.so.1.0.0 You must call ldconfig when installing shared libraries into system locations. http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Shared_libraries dchroot.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/dchroot ['/usr/lib64'] schroot.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/schroot ['/usr/lib64'] schroot.x86_64: E: binary-or-shlib-defines-rpath /usr/libexec/schroot/schroot-listmounts ['/usr/lib64'] schroot.x86_64: E: binary-or-shlib-defines-rpath /usr/libexec/schroot/schroot-mount ['/usr/lib64'] schroot.x86_64: E: binary-or-shlib-defines-rpath /usr/libexec/schroot/schroot-releaselock ['/usr/lib64'] These need fixing, and unfortunately passing --disable-rpath to %configure didn't help. There's some info at http://fedoraproject.org/wiki/Packaging:Guidelines#Beware_of_Rpath Some other random comments: The description for the dchroot package could use a bit more elaboration. I think the license is GPLv3+, not GPLv3 only. Do you see some place where the version is limited to v3? Nothing owns /etc/schroot, /etc/schroot/exec.d, /etc/schroot/setup.d, /usr/libexec/schroot or /var/lib/schroot. That's enough for now. If you get those fixed up I'll do a complete review.
Thanks Jason, Per feedback from the upstream maintainer, I've elected to compile this statically, which should avoid the rpath/ldconfig/shlib issues. I've uploaded a new upstream version with this changelog: %changelog * Tue Jul 14 2009 Zach Carter <z.carter> - 1.2.3-1 - new upstream version - compile with --enable-static --disable-shared - improve dchroot description - define directory ownership - add + to GPLv3 license definition http://www.zachcarter.com/rpms/schroot.spec http://www.zachcarter.com/rpms/schroot-1.2.3-1.fc11.src.rpm
New build, fixed "file listed twice" warnings. http://www.zachcarter.com/rpms/schroot.spec http://www.zachcarter.com/rpms/schroot-1.2.3-2.fc11.src.rpm %changelog * Tue Jul 14 2009 Zach Carter <z.carter> - 1.2.3-2 - fix "file listed twice" warnings
I ran out of time there for a bit, but I have some time now. This builds fine; rpmlint is down to just complaints about setuid binaries, which we've already established is OK. Generally I wouldn't advocate static linking, but I don't really see how it makes a difference as the library is only intended for use within this package. If it's what upstream recommends then that's fine, although those issues were trivial to fix in the usual manner. There does seem to be a test suite; did you try running it in a %check section? I tried and I get "All 0 tests passed" but it's possible that something other than a plain "make check" is needed. I don't think this is a significant issue, though, and it's the only issue I see, so I'll go ahead and approve this package and if your investigations show that it is reasonable to call the test suite then you can set that up. I've sponsored you; it should take perhaps an hour for the ACLs to propagate and then you can make your CVS request. * source files match upstream. sha256sum: c3bca449abdf28b66f6aede8892ce61967b5c1d758ba567e8648ccfb0cf914ec schroot_1.2.3.orig.tar.gz * package meets naming and versioning guidelines. * specfile is properly named, is cleanly written and uses macros consistently. * summary is OK. * description is OK. * dist tag is present. * build root is OK. * license field matches the actual license. * license is open source-compatible. * license text included in package. * latest version is being packaged. * BuildRequires are proper. * compiler flags are appropriate. * %clean is present. * package builds in mock (rawhide, x86_64). * package installs properly. * debuginfo package looks complete. * rpmlint has acceptable complaints. * final provides and requires are sane: dchroot-1.2.3-2.fc12.x86_64.rpm dchroot = 1.2.3-2.fc12 dchroot(x86-64) = 1.2.3-2.fc12 = libboost_filesystem-mt.so.5()(64bit) libboost_program_options-mt.so.5()(64bit) libboost_regex-mt.so.5()(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) liblockdev.so.1()(64bit) libpam.so.0()(64bit) libpam.so.0(LIBPAM_1.0)(64bit) libstdc++.so.6()(64bit) libstdc++.so.6(CXXABI_1.3)(64bit) libstdc++.so.6(GLIBCXX_3.4)(64bit) libstdc++.so.6(GLIBCXX_3.4.11)(64bit) libstdc++.so.6(GLIBCXX_3.4.9)(64bit) schroot-1.2.3-2.fc12.x86_64.rpm config(schroot) = 1.2.3-2.fc12 schroot = 1.2.3-2.fc12 schroot(x86-64) = 1.2.3-2.fc12 = /bin/sh config(schroot) = 1.2.3-2.fc12 libboost_filesystem-mt.so.5()(64bit) libboost_program_options-mt.so.5()(64bit) libboost_regex-mt.so.5()(64bit) libboost_system-mt.so.5()(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) liblockdev.so.1()(64bit) libpam.so.0()(64bit) libpam.so.0(LIBPAM_1.0)(64bit) libstdc++.so.6()(64bit) libstdc++.so.6(CXXABI_1.3)(64bit) libstdc++.so.6(GLIBCXX_3.4)(64bit) libstdc++.so.6(GLIBCXX_3.4.11)(64bit) libstdc++.so.6(GLIBCXX_3.4.9)(64bit) ? There's a test suite, but no %check section. Not sure it makes sense to run it. * no shared libraries are added to the regular linker search paths. * owns the directories it creates. * doesn't own any directories it shouldn't. * no duplicates in %files. * file permissions are appropriate. * no generically named files * code, not content. * documentation is small, so no -doc subpackage is necessary. * %docs are not necessary for the proper functioning of the package. * no headers. * no pkgconfig files. * no static libraries. * no libtool .la files. APPROVED
New Package CVS Request ======================= Package Name: schroot Short Description: Execute commands in a chroot environment Owners: zachcarter Branches: F-10 F-11 InitialCC:
CVS done. You should be able to import your package in a few minutes.
Awesome! Thanks Jason. I'll see what I can find out about the test suite.
Is there any reason for this ticket to remain open?
nope. CLOSED
Package Change Request ====================== Package Name: schroot New Branches: el6
This SCM request is not valid; it lists no owners. These are processed by a script, so please do not deviate from the formats given in https://fedoraproject.org/wiki/Package_SCM_admin_requests
Package Change Request ====================== Package Name: zachcarter New Branches: el6 Owners: zachcarter
crap, sorry
Package Change Request ====================== Package Name: schroot New Branches: el6 Owners: zachcarter
Git done (by process-git-requests).