Bug 447368 - (schroot) Review Request: schroot - Execute commands in a chroot environment
Review Request: schroot - Execute commands in a chroot environment
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-05-19 13:49 EDT by Zach Carter
Modified: 2011-03-16 14:00 EDT (History)
9 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-11-05 17:56:08 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Zach Carter 2008-05-19 13:49:39 EDT
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 13:50:53 EDT
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-19 20:22:14 EDT
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 13:16:28 EDT
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 13:34:21 EDT
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 13:17:40 EST
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 13:47:50 EST
(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 14:07:53 EST
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 14:43:04 EST
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 15:06:01 EDT
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 19:14:27 EDT
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 10:25:47 EDT
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 13:40:35 EDT
(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 19:06:24 EDT
(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 03:41:59 EDT
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 03:07:45 EDT
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 13:18:08 EDT
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 14:38:05 EDT
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 17:26:51 EDT
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 18:20:50 EDT
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 18:40:30 EDT
CVS done.  You should be able to import your package in a few minutes.
Comment 21 Zach Carter 2009-07-30 11:49:02 EDT
Awesome! Thanks Jason.

I'll see what I can find out about the test suite.
Comment 22 Jason Tibbitts 2009-11-05 17:47:28 EST
Is there any reason for this ticket to remain open?
Comment 23 Zach Carter 2009-11-05 17:56:08 EST
nope.  CLOSED
Comment 24 Zach Carter 2011-03-15 12:50:32 EDT
Package Change Request
======================
Package Name: schroot
New Branches: el6
Comment 25 Jason Tibbitts 2011-03-16 08:24:55 EDT
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 11:42:28 EDT
Package Change Request
======================
Package Name: zachcarter
New Branches: el6
Owners: zachcarter
Comment 27 Zach Carter 2011-03-16 11:42:52 EDT
crap, sorry
Comment 28 Zach Carter 2011-03-16 11:43:11 EDT
Package Change Request
======================
Package Name: schroot
New Branches: el6
Owners: zachcarter
Comment 29 Jason Tibbitts 2011-03-16 14:00:20 EDT
Git done (by process-git-requests).

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