Bug 447368 (schroot) - Review Request: schroot - Execute commands in a chroot environment
Summary: Review Request: schroot - Execute commands in a chroot environment
Keywords:
Status: CLOSED NOTABUG
Alias: schroot
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Jason Tibbitts
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-05-19 17:49 UTC by Zach Carter
Modified: 2011-03-16 18:00 UTC (History)
9 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-11-05 22:56:08 UTC
Type: ---
tibbs: fedora-review+
tibbs: fedora-cvs+


Attachments (Terms of Use)

Description Zach Carter 2008-05-19 17:49:39 UTC
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.

Comment 1 Zach Carter 2008-05-19 17:50:53 UTC
Given the pam and suid components of the package, it would probably be wise for
a security SME to review the package.

Comment 2 Ralf Corsepius 2008-05-20 00:22:14 UTC
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.


Comment 3 Zach Carter 2008-05-20 17:16:28 UTC
Thanks for the feedback Ralf.

I made these changes:

* Tue May 20 2008 Zach Carter <z.carter@f5.com> - 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.

Comment 4 Zach Carter 2008-09-15 17:34:21 UTC
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

Comment 5 Jason Tibbitts 2008-11-05 18:17:40 UTC
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.

Comment 6 Zach Carter 2008-11-06 18:47:50 UTC
(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.

Comment 7 Zach Carter 2008-11-06 19:07:53 UTC
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@f5.com> - 1.2.1-2
- move libsbuild subpackage into main package
- remove duplicate doc entries
- disable rpath
- defattr for dchroot files

Comment 8 Jason Tibbitts 2008-11-11 19:43:04 UTC
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.

Comment 9 Zach Carter 2009-04-25 19:06:01 UTC
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

Comment 10 Clint Savage 2009-04-25 23:14:27 UTC
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.

Comment 11 Tomas Hoger 2009-05-05 14:25:47 UTC
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.

Comment 12 Zach Carter 2009-05-05 17:40:35 UTC
(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

Comment 13 Zach Carter 2009-05-11 23:06:24 UTC
(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!

Comment 14 Tomas Hoger 2009-05-15 07:41:59 UTC
Given my actions backlog, I can't really promise you to have any serious look at the moment, sorry. ;(

Comment 15 Jason Tibbitts 2009-07-11 07:07:45 UTC
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.

Comment 16 Zach Carter 2009-07-14 17:18:08 UTC
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@f5.com> - 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

Comment 17 Zach Carter 2009-07-14 18:38:05 UTC
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@f5.com> - 1.2.3-2
- fix "file listed twice" warnings

Comment 18 Jason Tibbitts 2009-07-29 21:26:51 UTC
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

Comment 19 Zach Carter 2009-07-29 22:20:50 UTC
New Package CVS Request
=======================
Package Name: schroot
Short Description: Execute commands in a chroot environment
Owners: zachcarter
Branches: F-10 F-11
InitialCC:

Comment 20 Jason Tibbitts 2009-07-29 22:40:30 UTC
CVS done.  You should be able to import your package in a few minutes.

Comment 21 Zach Carter 2009-07-30 15:49:02 UTC
Awesome! Thanks Jason.

I'll see what I can find out about the test suite.

Comment 22 Jason Tibbitts 2009-11-05 22:47:28 UTC
Is there any reason for this ticket to remain open?

Comment 23 Zach Carter 2009-11-05 22:56:08 UTC
nope.  CLOSED

Comment 24 Zach Carter 2011-03-15 16:50:32 UTC
Package Change Request
======================
Package Name: schroot
New Branches: el6

Comment 25 Jason Tibbitts 2011-03-16 12:24:55 UTC
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

Comment 26 Zach Carter 2011-03-16 15:42:28 UTC
Package Change Request
======================
Package Name: zachcarter
New Branches: el6
Owners: zachcarter

Comment 27 Zach Carter 2011-03-16 15:42:52 UTC
crap, sorry

Comment 28 Zach Carter 2011-03-16 15:43:11 UTC
Package Change Request
======================
Package Name: schroot
New Branches: el6
Owners: zachcarter

Comment 29 Jason Tibbitts 2011-03-16 18:00:20 UTC
Git done (by process-git-requests).


Note You need to log in before you can comment on or make changes to this bug.