Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 197740
Summary: | Review Request: dircproxy - IRC proxy server | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Jarod Wilson <jarod> |
Component: | Package Review | Assignee: | Chris Weyl <cweyl> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | cweyl, panemade |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-09-14 17:15:26 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Jarod Wilson
2006-07-05 21:40:10 UTC
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. Blocker: * runs as root although daemon supports non-root operation ("switch_user" feature) 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. Anyone want to pick up this review?... Please?... 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. 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. :) 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. No pressure, just want to note the state :) (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... (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 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 Erm, in full that should have read "APPROVED, provided that the above is addressed." (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... Imported and built, closing ticket. |