Bug 197740 - Review Request: dircproxy - IRC proxy server
Review Request: dircproxy - IRC proxy server
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Chris Weyl
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-07-05 17:40 EDT by Jarod Wilson
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-09-14 13:15:26 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Jarod Wilson 2006-07-05 17:40:10 EDT
Spec URL: http://wilsonet.com/packages/dircproxy/dircproxy.spec
SRPM URL: http://wilsonet.com/packages/dircproxy/dircproxy-1.2.0-0.beta.fc6.src.rpm
Description:
--
dircproxy is an IRC proxy server ("bouncer") designed for people
who use IRC from lots of different workstations or clients, but
wish to remain connected and see what they missed while they
were away. You connect to IRC through dircproxy, and it keeps
you connected to the server, even after you detach your client
from it. While you're detached, it logs channel and private
messages as well as important events,
--
Comment 1 Parag AN(पराग) 2006-07-06 00:03:47 EDT
Not an official review as I'm not yet sponsored
Mock build for development i386 is sucessfull
MUST Items:
     - MUST: rpmlint shows no error. 
     - MUST: dist tag is present.
     - MUST: The package is named according to the Package Naming Guidelines.
     - MUST: The spec file name matching the base package dircproxy, in the
format dircproxy.spec.
      - MUST: This package meets the Packaging Guidelines.
      - MUST: The package is licensed with an open-source compatible license GPL.
      - MUST: The sources used to build the package matches the upstream source,
as provided in the spec URL. md5sum is correct (bd6abe933f90d80fbc71a00563f9c7de)
      - MUST: This package owns all directories that it creates. 
      - MUST: This package did not contain any duplicate files in the %files
listing.
      - MUST: This package  have a %clean section, which contains rm -rf
$RPM_BUILD_ROOT.
      - MUST: This package used macros.
      - MUST: Document files are included like INSTALL README, RFCs, SPEC, PROTOCOL.
      - MUST: Package did NOT contained any .la libtool archives.
      * Source URL is present and working.
      * BuildRoot is correct BuildRoot:       
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
      * BuildRequires is correct
      * preun and postun installing service correctly.
Comment 2 Enrico Scholz 2006-07-10 13:46:19 EDT
Blocker:
* runs as root although daemon supports non-root operation ("switch_user" feature)
Comment 3 Jarod Wilson 2006-07-10 17:56:23 EDT
The daemon can be run as any user, but it won't work for multiple users unless
run as root. The "switch_user" feature does not run the daemon as non-root, it
only briefly seteuid's to display another name:

 6.1  How do I change what username is presented on IRC?

     The obvious answer is to run dircproxy under that username,
     but that doesn't help if you're proxying for multiple people.
     Another option is to use one of the many fake ident daemons
     to return a false answer for you.

     There's also a third option, which is available to those
     running dircproxy as root (either as a daemon or from inetd).
     You can use the 'switch_user' configuration file directive.
     This ensures that the connection to the server appears as from
     whatever local username you give it (by seteuid()ing to that
     briefly) while the dircproxy process remains as root.

Exactly what should be done here, I dunno.
Comment 4 Jarod Wilson 2006-07-31 14:18:59 EDT
Anyone want to pick up this review?... Please?...
Comment 5 Chris Weyl 2006-08-02 00:56:08 EDT
Jarod --

As I start going through the review, any reason to not build this with ssl
support?  It would seem that enabling it is as easy as appending --enable-ssl to
%configure, and add openssl-devel as a br.
Comment 6 Jarod Wilson 2006-08-02 10:01:13 EDT
Huh, not sure why I didn't have ssl turned on... Oh yeah, from the dircproxy
wiki (http://dircproxy.securiweb.net/):

New feature in 1.2.0 (currently beta)
[..]
    * SSL Support (client mode and server mode, --enable-ssl ) (merge not
finished yet) 

However, now that I look at it more, the README.ssl file (added to the svn trunk
post-beta tarball) seems to indicate it actually works reasonably well (just a
few caveats), so I'll turn it on and include the README.ssl file...

Okay, just pushed a new spec and srpm out.

And thank you for picking up the review. :)
Comment 7 Chris Weyl 2006-08-09 23:17:59 EDT
Whew!  ok :)

1. It appears to me that this proxy can be run as a non-root user and still be
able to do everything needed except use the "switch_user" command.  (Read
another way, it looks like you only need to run dircproxy as root if you want
to use "switch_user".)  Let's find a way to have this service start up as a
non-root user by default (perhaps just "nobody" as README.identd suggests).

It would seem to make sense to patch /etc/init.d/dircproxy to read values from
/etc/sysconf/dircproxy (as other init scripts do) to determine what user to
run under, etc.

2. The release tag here, as this is a beta package, should be something like:

    0.x.beta%{?dist}

Where x is the actual release.  (On coming out of beta, it should start being
the normal "x%{?dist}".)  See wiki - Packaging/NamingGuidelines.

3. rpmlint emits two warnings -- both of these are easy enough to fix.

+ package meets naming and packaging guidelines.
+ specfile is properly named, is cleanly written and uses macros consistently.
+ dist tag is present.
X release tag doesn't meet naming guidelines.
+ build root is correct.
+ license field matches the actual license.
+ license is open source-compatible.  License text included in package.
+ source files match upstream:
bd6abe933f90d80fbc71a00563f9c7de  dircproxy-1.2.0-beta.tar.bz
bd6abe933f90d80fbc71a00563f9c7de  dircproxy-1.2.0-beta.tar.bz.srpm
+ latest version is being packaged.
+ BuildRequires are proper.
+ package builds in mock (fc5/x86_64).
X rpmlint is silent.
[build@zeus dircproxy]$ rpmlint dircproxy-1.2.0-0.beta.1.fc5.src.rpm
W: dircproxy mixed-use-of-spaces-and-tabs
[build@zeus x86_64]$ rpmlint dircproxy-1.2.0-0.beta.1.fc5.x86_64.rpm
W: dircproxy wrong-file-end-of-line-encoding
/usr/share/doc/dircproxy-1.2.0/README.ssl
+ final provides and requires are sane:
 ** dircproxy-1.2.0-0.beta.1.fc5.x86_64.rpm
 == provides
 config(dircproxy) = 1.2.0-0.beta.1.fc5
 dircproxy = 1.2.0-0.beta.1.fc5
 == requires
 /bin/sh
 /usr/bin/perl
 config(dircproxy) = 1.2.0-0.beta.1.fc5
 perl(strict)
 perl(vars)
+ no shared libraries are present.
+ package is not relocatable.
+ owns the directories it creates.
+ doesn't own any directories it shouldn't.
+ no duplicates in %files.
+ file permissions are appropriate.
+ %clean is present.
O %check is not present, but there are no tests defined
O scriptlets look sane, and conform to ScriptletSnippets
+ code, not content.
+ documentation is small, so no -docs subpackage is necessary.
+ %docs are not necessary for the proper functioning of the package.
+ no headers.
+ no pkgconfig files.
+ no libtool .la droppings.
+ not a GUI app.
+ not a web app.
Comment 8 Chris Weyl 2006-08-20 14:51:09 EDT
No pressure, just want to note the state :)
Comment 9 Jarod Wilson 2006-09-06 00:40:01 EDT
(In reply to comment #7)
> Whew!  ok :)

I know that feeling... Been a bit overloaded with other stuff, finally getting back to this...

> 1. It appears to me that this proxy can be run as a non-root user and still be
> able to do everything needed except use the "switch_user" command.  (Read
> another way, it looks like you only need to run dircproxy as root if you want
> to use "switch_user".)  Let's find a way to have this service start up as a
> non-root user by default (perhaps just "nobody" as README.identd suggests).

Won't work as nobody:

# su nobody -c "/usr/bin/dircproxy -f /etc/dircproxyrc"
This account is currently not available.

I'll have to add an account w/a valid login shell, but that's easy enough.

> It would seem to make sense to patch /etc/init.d/dircproxy to read values from
> /etc/sysconf/dircproxy (as other init scripts do) to determine what user to
> run under, etc.

Will do.

> 2. The release tag here, as this is a beta package, should be something like:
> 
>     0.x.beta%{?dist}

Fixed locally.

> 3. rpmlint emits two warnings -- both of these are easy enough to fix.
[...]
> [build@zeus dircproxy]$ rpmlint dircproxy-1.2.0-0.beta.1.fc5.src.rpm
> W: dircproxy mixed-use-of-spaces-and-tabs
> [build@zeus x86_64]$ rpmlint dircproxy-1.2.0-0.beta.1.fc5.x86_64.rpm
> W: dircproxy wrong-file-end-of-line-encoding

Both fixed locally.

Just need to fix up the bits to run as non-root, which I'll have to save for tomorrow...
Comment 10 Jarod Wilson 2006-09-06 00:56:55 EDT
(In reply to comment #9)
[...]
> Just need to fix up the bits to run as non-root, which I'll have to save for tomorrow...

I lied. Can't sleep at night if I leave things unfinished. :)

http://wilsonet.com/packages/dircproxy/dircproxy-1.2.0-0.3.beta.fc6.src.rpm
http://wilsonet.com/packages/dircproxy/dircproxy.spec
Comment 11 Chris Weyl 2006-09-14 01:22:15 EDT
rpmlint, as it is wont to do, bestows upon us two errors:

E: dircproxy executable-marked-as-config-file /etc/sysconfig/dircproxy
E: dircproxy script-without-shellbang /etc/sysconfig/dircproxy

A chmod -x /etc/sysconfig/dircproxy should do the trick.

Aside from that, it works and looks good to me!

APPROVED
Comment 12 Chris Weyl 2006-09-14 01:43:24 EDT
Erm, in full that should have read "APPROVED, provided that the above is addressed."
Comment 13 Jarod Wilson 2006-09-14 11:09:34 EDT
(In reply to comment #11)
> rpmlint, as it is wont to do, bestows upon us two errors:
> 
> E: dircproxy executable-marked-as-config-file /etc/sysconfig/dircproxy
> E: dircproxy script-without-shellbang /etc/sysconfig/dircproxy
> 
> A chmod -x /etc/sysconfig/dircproxy should do the trick.

Even better, fix a stupid copy-n-paste error, use install -m 0644 instead of
install -m 755. :) (done in rel 0.4.beta)

> Aside from that, it works and looks good to me!
> 
> APPROVED

Cool, importing rel 0.4.beta shortly...
Comment 14 Jarod Wilson 2006-09-14 13:15:26 EDT
Imported and built, closing ticket.

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