Bug 165689 - Review Request: SquidGuard: filter, redirector and access controller plugin for squid
Review Request: SquidGuard: filter, redirector and access controller plugin f...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Aurelien Bompard
Fedora Package Reviews List
http://filelister.linux-kernel.at/mod...
: Reopened
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-08-11 08:11 EDT by Oliver Falk
Modified: 2009-10-27 03:47 EDT (History)
5 users (show)

See Also:
Fixed In Version: 5
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-08-29 21:56:31 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Oliver Falk 2005-08-11 08:11:04 EDT
Spec Name or Url: http://filelister.linux-kernel.at/downloads/packages/FC_EXTRAS_APPROVAL/squidGuard/squidGuard.spec
SRPM Name or Url: http://filelister.linux-kernel.at/downloads/packages/FC_EXTRAS_APPROVAL/squidGuard/squidGuard-1.2.0-7.src.rpm
Description: 
squidGuard can be used to 
- limit the web access for some users to a list of accepted/well known
  web servers and/or URLs only.
- block access to some listed or blacklisted web servers and/or URLs
  for some users. **)
- block access to URLs matching a list of regular expressions or words
  for some users. **)
- enforce the use of domainnames/prohibit the use of IP address in
  URLs. **)
- redirect blocked URLs to an "intelligent" CGI based info page. **)
- redirect unregistered user to a registration form.
- redirect popular downloads like Netscape, MSIE etc. to local copies.
- redirect banners to an empty GIF. **)
- have different access rules based on time of day, day of the week,
  date etc.
- have different rules for different user groups.
- and much more.. 

Neither squidGuard nor Squid can be used to
- filter/censor/edit text inside documents 
- filter/censor/edit embeded scripting languages like JavaScript or
  VBscript inside HTML
Comment 1 Aurelien Bompard 2005-09-04 12:27:36 EDT
* the logrotate file contains "olddir /var/log/old/squid". This is not standard,
please remove it.
* this file should also be tagged as %config(noreplace) in case the user makes
changes.
* in the package's description, there are "**)" symbols. What does that mean ?
Maybe a bad copy/paste ?
* Source3 is guard-distrib.tar.gz: where does it come from ? It is copied to
samples/, but not included in the rpm (not in the %files section). If it is not
useful, please remove it.
* SMP flags are not used. If it does not build with it, please write a small
comment above the make command
* the checks "RPM_BUILD_ROOT != /" are useless in %install and %clean, please
remove them.
* use install -p to preserve timestamps
* the "mkdir -p $RPM_BUILD_ROOT%{_dbhomedir}" line in %install is useless, the
dir was created with install above
* "%dir %{_dbhomedir}" and "%{_dbhomedir}/*" can be summed up in simply
"%{_dbhomedir}/"
* don't create $RPM_BUILD_ROOT%{_datadir}/%{name} in %install, since there are
no files in it.
* Source 2 is not available anymore. In the same directory, there is a
blacklists.tar.gz file which seems to be the same as the versionned one. Maybe
you could use that.
* BuildRoot: should be
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
* The package should contain the text of the license. Please include the COPYING
file.
* The default squidGuard.conf file does not work since your blacklists are in
%dbhomedir/blacklists and the file refers to %dbhomedir/dest. It would be nice
to make it work by default, and a simple "s,dest/adult/,blacklists/porn/,g" on
the file should fix it. There is still the problem of the invalid redirection,
but I don't think we can do much about that, can we ?

* a possible improvement would be to add a script to update the blacklists. Many
are available on the web, you could for example try this one:
http://cuda.port-aransas.k12.tx.us/squid-getlist.html or base yours on it. This
is not a blocker, but could be a nice improvement if you are interested in
squidGuard.
Comment 2 Oliver Falk 2005-09-05 04:24:00 EDT
Thanks for your review!

(In reply to comment #1)
> * the logrotate file contains "olddir /var/log/old/squid". This is not standard,
> please remove it.

Removed.

> * this file should also be tagged as %config(noreplace) in case the user makes
> changes.

Done.

> * in the package's description, there are "**)" symbols. What does that mean ?
> Maybe a bad copy/paste ?

Of course - removed.

> * Source3 is guard-distrib.tar.gz: where does it come from ? It is copied to
> samples/, but not included in the rpm (not in the %files section). If it is not
> useful, please remove it.

Removed.

> * SMP flags are not used. If it does not build with it, please write a small
> comment above the make command

Added smpmflags

> * the checks "RPM_BUILD_ROOT != /" are useless in %install and %clean, please
> remove them.

Remoevd.

> * use install -p to preserve timestamps

OK - Done.

> * the "mkdir -p $RPM_BUILD_ROOT%{_dbhomedir}" line in %install is useless, the
> dir was created with install above

Removed.

> * "%dir %{_dbhomedir}" and "%{_dbhomedir}/*" can be summed up in simply
> "%{_dbhomedir}/"

Changed.

> * don't create $RPM_BUILD_ROOT%{_datadir}/%{name} in %install, since there are
> no files in it.

Removed.

> * Source 2 is not available anymore. In the same directory, there is a
> blacklists.tar.gz file which seems to be the same as the versionned one. Maybe
> you could use that.

Changed. There should be always a timestamped one and one without timestamp,
which is the same. I removed the _bldate macro and use the non-timestamped one now.

> * BuildRoot: should be
> %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

Yes, it should... 

> * The package should contain the text of the license. Please include the COPYING
> file.

Done. Shall I also include the GPL file from the tarball?

> * The default squidGuard.conf file does not work since your blacklists are in
> %dbhomedir/blacklists and the file refers to %dbhomedir/dest. It would be nice
> to make it work by default, and a simple "s,dest/adult/,blacklists/porn/,g" on
> the file should fix it.

Fixed with the provided regex. I didn't fix it, because I thought, that the
admin who installs it, will have a look at the config...

> There is still the problem of the invalid redirection,
> but I don't think we can do much about that, can we ?

I would have to install the guard.cgi and guard.conf into some place and create
an Apache config. But we'll never know the hostname/FQDN of the host running
squidGuard. It could also be possible, that the admin has no webserver on the
proxy-machine and want's to install the cgi on another server. So I think we
_can_ do something about that, but it doesn't make much sense... :-(

> * a possible improvement would be to add a script to update the blacklists. Many
> are available on the web, you could for example try this one:
> http://cuda.port-aransas.k12.tx.us/squid-getlist.html or base yours on it. This
> is not a blocker, but could be a nice improvement if you are interested in
> squidGuard.

Added this and slightly modified it (paths). I also added a var called ENABLED,
which is 0 (therfor disabled) by default...

Build 8. Please have a look.
Comment 3 Aurelien Bompard 2005-09-05 11:30:49 EDT
> Shall I also include the GPL file from the tarball?

Discussion is going on on fedora-maintainers about this. Red Hat Legal Dpt.
suggest to always ship the full license text, but on the other hand it is
ridiculous to ship 5000 copies of the GPL...
For the moment, COPYING is enough, you can always add it later when SquidGuard
is in CVS.

> I would have to install the guard.cgi and guard.conf into some place and
> create an Apache config. It could also be possible, that the admin has no 
> webserver on the proxy-machine and want's to install the cgi on another 
> server.

Exactly. It's nice to customize the default config to have it Just Work(tm), but
there are limits...

> Added this and slightly modified it (paths). I also added a var called 
> ENABLED, which is 0 (therfor disabled) by default...

Good idea, but we should not modify sources from upstream except with patches or
in the spec file. When I wget' the URL for Source3, I should always have the
exact same file as the one inside the SRPM. Therefore, please make a patch. (see 
the 10th "MUST" in http://fedoraproject.org/wiki/PackageReviewGuidelines)
Comment 4 Oliver Falk 2005-09-05 11:51:48 EDT
(In reply to comment #3)
> > Shall I also include the GPL file from the tarball?
> 
> Discussion is going on on fedora-maintainers about this. Red Hat Legal Dpt.
> suggest to always ship the full license text, but on the other hand it is
> ridiculous to ship 5000 copies of the GPL...
> For the moment, COPYING is enough, you can always add it later when SquidGuard
> is in CVS.

I also think, it better to not included it, it really makes no sense to included
the GPL everywhere...

> > I would have to install the guard.cgi and guard.conf into some place and
> > create an Apache config. It could also be possible, that the admin has no 
> > webserver on the proxy-machine and want's to install the cgi on another 
> > server.
> 
> Exactly. It's nice to customize the default config to have it Just Work(tm), but
> there are limits...

Agree!

> > Added this and slightly modified it (paths). I also added a var called 
> > ENABLED, which is 0 (therfor disabled) by default...
> 
> Good idea, but we should not modify sources from upstream except with patches or
> in the spec file. When I wget' the URL for Source3, I should always have the
> exact same file as the one inside the SRPM. Therefore, please make a patch. (see 
> the 10th "MUST" in http://fedoraproject.org/wiki/PackageReviewGuidelines)

Oh yes... I thought about makin' a patch, but then I was to lazy/busy. But I did
that now and uploaded the new spec && srpm.

Thanks for your comments!
Comment 5 Paul Howarth 2005-09-05 11:56:40 EDT
(In reply to comment #3)
> > Shall I also include the GPL file from the tarball?
> 
> Discussion is going on on fedora-maintainers about this. Red Hat Legal Dpt.
> suggest to always ship the full license text, but on the other hand it is
> ridiculous to ship 5000 copies of the GPL...
> For the moment, COPYING is enough, you can always add it later when SquidGuard
> is in CVS.

If the license text is in the upstream tarball, you should include it. The
discussion on fedora-maintainers is about what to do when upstream does *not*
include the text of the license.
Comment 6 Oliver Falk 2005-09-05 12:05:43 EDT
OK, build 10 includes GPL.
Comment 7 Aurelien Bompard 2005-09-05 13:01:05 EDT
* You install the update script with perms 750, why ? All other scripts in
/etc/cron.daily are 755.
* When you install it you still use %{SOURCE3}, thus installing the original
unpatched one. Just replace it by squid-getlist.html.
Comment 8 Oliver Falk 2005-09-06 03:39:38 EDT
(In reply to comment #7)
> * You install the update script with perms 750, why ? All other scripts in
> /etc/cron.daily are 755.

Typo. Fixed.

> * When you install it you still use %{SOURCE3}, thus installing the original
> unpatched one. Just replace it by squid-getlist.html.

Fixed.

Build 11.
Comment 9 Aurelien Bompard 2005-09-06 04:53:42 EDT
Passed:
* RPM name is OK
* Source squidGuard-1.2.0.tar.gz is the same as upstream
* Source blacklists.tar.gz is the same as upstream
* Source squid-getlist.html is the same as upstream
* Builds fine in mock
* rpmlint of squidGuard looks OK
* File list of squidGuard looks OK
* Seems to work fine
Comment 10 Oliver Falk 2005-09-06 05:33:30 EDT
So, shall I understand this as an approval? 
Comment 11 Aurelien Bompard 2005-09-06 05:48:16 EDT
Yes it is (I switched the blocked bug to FE-ACCEPT)
Comment 12 Warren Togami 2005-09-08 07:19:05 EDT
https://www.redhat.com/archives/k12osn/2005-September/msg00175.html
I apologize for not realizing this sooner, but *please* verify that this package
will allow a smooth upgrade from the K12LTSP squidGuard package without breaking
those existing servers.  Thousands of schools around the world are using K12LTSP
so we really should try to avoid breaking them in a package upgrade if possible.

This is only a precaution, maybe there really is no problem.  I am removing the
currently built squidGuard packages before this Extras push for now.  Please
rebuild after this has been verified as safe.
Comment 13 Oliver Falk 2005-09-08 08:55:32 EDT
Thanks Warren!
And where is the K12LTSP package? I'll try to do my best to check if there are
differences, that may break somethin'.
Comment 14 Paul Howarth 2005-09-08 09:03:10 EDT
(In reply to comment #13)
> Thanks Warren!
> And where is the K12LTSP package? I'll try to do my best to check if there are
> differences, that may break somethin'.

Try here:

ftp://k12linux.mesd.k12.or.us/pub/K12LTSP/4.2.1/SRPMS/squidGuard-1.2.0-8.k12ltsp.2.4.2.src.rpm
Comment 15 Oliver Falk 2005-09-09 05:27:40 EDT
Warren, Paul, I tried to merge K12 version with mine... I can commit the changes
today, but I'd like to have someone who re-reviews the changes and maybe check
compatibility with K12.... Let me know, then I commit.
Comment 16 Warren Togami 2005-09-09 08:10:59 EDT
Go ahead and checkin changes to CVS but don't request any build yet.  Let's give
the K12LTSP developers and users a chance to try the SRPM as they are the people
who would be effected by a possible auto-upgrade into this package.
Comment 17 Oliver Falk 2005-09-09 08:16:15 EDT
Committed for devel. Could you inform the K12 developers that I commited a new
version that is meant for testing in K12... Merci!
Comment 18 Warren Togami 2005-09-13 09:54:09 EDT
I gave them warning on k12osn list and no response there or here.  Let's give
K12LTSP until Thursday before pushing this into Extras.
Comment 19 Warren Togami 2005-09-14 20:14:46 EDT
eharrison wrote back that he is testing this.  Cancel timeout.
Comment 20 Eric Harrison 2005-09-14 23:13:25 EDT
Thanks for the heads up! This package will indeed require some changes to the
K12LTSP packaging.


There are a number of differences between the FC package and the one included in
K12LTSP. Pretty much all of these differences, except the first one, could be
moved into a supplementary package for K12LTSP. You may or may not want to
include them in the Fedora package.


* K12LTSP includes a patch
(ftp://k12linux.mesd.k12.or.us/pub/squidGuard/source/squidguard-sed.patch) that
enables full sed-compatible regular expressions in the config file. The primary
use of this is to force Google's "Safe Search" feature to always be on (see
ftp://k12linux.mesd.k12.or.us/pub/squidGuard/source/squidGuard.conf). VERY
USEFUL feature in my environment!

* I have not had good experiences with the "official" blacklists at
http://ftp.teledanmark.no/pub/www/proxy/squidguard/contrib/, I no longer
included these in the K12LTSP update cron job. The blacklists at
ftp://ftp.univ-tlse1.fr/pub/reseau/cache/squidguard_contrib/ are included in the
K12LTSP update cron job. K12LTSP also generates blacklists that are available
via either http or rsync (see
ftp://k12linux.mesd.k12.or.us/pub/squidGuard/source/update_squidguard_blacklists)

* the K12LTSP package includes an initscript and squid.conf file so that users
don't have to edit /etc/squid.conf to enable squidGuard (see
ftp://k12linux.mesd.k12.or.us/pub/squidGuard/source/squid-squidGuard.conf and 
ftp://k12linux.mesd.k12.or.us/pub/squidGuard/source/squidguard). "chkconfig
squidguard on ; service squidguard start" works with the K12LTSP package.

* the K12LTSP package includes a script
(ftp://k12linux.mesd.k12.or.us/pub/squidGuard/source/transparent-proxying) that
redirects all port 80 traffic to squid/squidGuard. This is a "CIPA-compliance"
feature for schools, allowing them to easily force filtering if the box is
installed as a gateway.



General comments on the FC package:

* the FC package requires direct editing of /etc/cron.daily/squidGuard for it to
be enabled. The "ENABLED=0" setting should be moved to /etc/sysconfig/squidGuard
& that file sourced in the cron script.

* You may want to consider turning on logging in squidGuard.conf and add the
relevant entries in the logrotate config

Comment 21 Warren Togami 2005-09-14 23:23:35 EDT
I personally think that our Extras package should merge and enable all of the
K12LTSP behaviors by default.  I have used K12LTSP's squidGuard and found it be
surprisingly reliable.  The target audience of K12LTSP is a good default for
squidGuard users in general, and it is easy to modify the default config if you
don't want K12LTSP defaults.  K12LTSP's blacklist choices are also good.

It is problematic to split stuff into a K12LTSP sub-package, because we want
K12LTSP users to upgrade to the Extras package and be certain that everything
continues to work as expected without manual intervention.
Comment 22 Aurelien Bompard 2005-09-15 00:22:01 EDT
> I personally think that our Extras package should merge and enable all of the
> K12LTSP behaviors by default.

Agreed.
Comment 23 Oliver Falk 2005-09-15 07:33:05 EDT
Will have a look at this... I'm currently a bit busy, so it might take 'til next
week.
Comment 24 Warren Togami 2005-10-18 10:39:54 EDT
ping?
Comment 25 Ralf Corsepius 2006-03-07 10:04:47 EST
What is the status of this package?

It seems to be in FE (listed in owners.list), despite the fact this PR is still
tagged FE-REVIEW (i.e. tagged NOT APPROVED)
Comment 26 Christian Iseli 2006-03-07 10:20:05 EST
(In reply to comment #25)
> It seems to be in FE (listed in owners.list)

Strange.  It's not listed in my copy of owners.list...
Comment 27 Christian Iseli 2006-03-07 10:21:54 EST
(In reply to comment #26)
> Strange.  It's not listed in my copy of owners.list...

Remove foot from mouth...

argh.  scratch that.  it's listed as squidGuard...
Comment 28 Warren Togami 2006-03-07 10:38:25 EST
Comment #20 and #21 still require changes to the proposed squidGuard package.

Anything new to report, Eric Harrison?
Comment 29 Oliver Falk 2006-03-07 11:47:22 EST
If anyone is willing to takeover... Just do it. I'm currently in too much
projects... :-(
Comment 30 Christian Iseli 2006-04-08 17:10:08 EDT
So, given this ticket is now closed and a squidGuard packages is in FE 5 and
devel, are we all agreed that the blocker should now be FE-ACCEPT ?
Comment 31 Warren Togami 2006-04-09 15:33:22 EDT
The changes requested by Eric Harrison on September 14th, 2005 were never done.
 The package built in FE5 has been done in error.  This package has never been
approved.  I am removing the package from FE5 because it will cause upgrade
problems for K12LTSP.

REOPENING

Oliver has given up on this package, so we need a new owner.  K12LTSP is now
working on officially merging with the Fedora Project, so hopefully we will
figure something out for this package.
Comment 32 Michael J Knox 2006-05-08 00:49:03 EDT
In the future, can packages that have been "given up on" by someone please be
added to the orphan page? 

http://fedoraproject.org/wiki/Extras/OrphanedPackages

This will hopefully allow for a swifter take over. 

Has been added now. 
Comment 33 John Berninger 2006-05-16 14:13:04 EDT
I'm looking through orphaned packages, and this package seems to have had all
the requested changes from 14-Sep-2005 made to it.  I'm willing to take over
packaging of squidguard, but would like someone to verify the state of the
package before I request a build in plague or update the owners list.  The
package is still listed as owned by oliver@linux-kernel.at.

If requested, I can generate a src.rpm and make it available for review as
opposed to a CVS checkout.
Comment 34 Warren Togami 2006-05-16 15:03:52 EDT
jwb, all required changes were not yet merged.  The Fedora squidguard RPM must
maintain upgrade and functionality compatibility with the K12LTSP version.
Comment 35 John Berninger 2006-05-16 16:15:03 EDT
I've gone back through the list of changes mentioned on 14-Sep, and can find
nothing that hasn't been merged.  I'm probably not seeing something, but I'll
need someone else to point it out to me.  Can one of the K12LTSP people review
the srpm at http://www.berningeronline.net/squidGuard-1.2.0-12.src.rpm and let
me know what hasn't been merged yet?

SPEC: http://www.berningeronline.net/squidGuard.spec
Comment 36 Warren Togami 2006-05-16 16:49:35 EDT
Oh, I didn't see an updated .src.rpm posted after that.  Let's get existing
K12LTSP users to test your RPM before we push it to Extras.
Comment 37 John Berninger 2006-05-16 17:14:37 EDT
There wasn't an actual updated .src.rpm posted; this was the .src.rpm I build
from a 'cvs co squidguard; cd squidguard/devel; make srpm'.  So I think things
were merged, just nothing was ever tested / acked / built.
Comment 38 John Berninger 2006-05-22 19:58:34 EDT
Any comments form the K12LTSP folks in other forums than this bug?  
Comment 39 Jason Tibbitts 2006-06-26 18:24:29 EDT
If the original submitter is not interested in continuing with this package,
this ticket should be closed.  If someone else wants to submit SquidGuard then
they can open a new ticket.

I'll go ahead and close this now.
Comment 40 Warren Togami 2006-06-26 23:57:12 EDT
K12LTSP is slowly being merged to become an official part of the Fedora Project,
so we eventually need this to be added.  Reopening.

Anybody else want to be maintainer?  If not then I'll have to do it.  I at least
used this before...
Comment 41 John Berninger 2006-06-27 08:25:54 EDT
This was stalled waiting on feedback from K12LTSP after my last package post -
see comment 38...
Comment 42 Brent 2006-08-28 12:01:16 EDT
This says fixed in 5, but I don't see this package either in Core or Extras.  Am
I not looking in the correct location or did it never get built?
Comment 43 John Berninger 2006-08-28 19:18:27 EDT
This package has *not* yet been approved for FE.  It is still stalled waiting on
a review of the package at the location posted in <a
href="https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=165689#c35">comment
35</a> to be done by K12LTSP folks.  I've just about given up on anyone actually
reviewing it, and if noone responds by the next time I think to check this bug,
I'll just close it since there doesn't seem to be any interest in
reviewing/approving it.
Comment 44 Warren Togami 2006-08-28 22:03:14 EDT
I had hoped that someone actually using this in K12LTSP would confirm that it
works exactly as they are used to.  The upstream K12LTSP is not a drop-in
replacement for the K12LTSP version.  Unfortunately, it seems that K12LTSP lacks
a development community and relies too heavily on Eric Harrison to just hand
them a solution.

I cannot prioritize analyzing this myself, and holding it indefinitely doesn't
seem to be a good solution either, so let's just let it publish and see if
anybody screams.  It will need to be fixed after the fact then.
Comment 45 John Berninger 2006-08-28 22:24:02 EDT
ACK - will build in mock to reconfirm that it builds, then push it to FE 5/devel
normally.
Comment 46 Oliver Falk 2006-08-29 02:33:15 EDT
fine. thanks.
Comment 47 John Berninger 2006-08-29 21:56:31 EDT
Updated in devel/, FC-[345] branches removed from CVS.  Closing this bug /
review request.
Comment 48 Christian Iseli 2006-12-31 06:21:33 EST
This should now block FE-ACCEPT.
Comment 49 Jon Ciesla 2009-10-23 12:22:43 EDT
Package Change Request
======================
Package Name: squidGuard
New Branches: EL-5
Owners: limb
Comment 50 Kevin Fenzi 2009-10-26 16:19:58 EDT
cvs done.
Comment 51 Oliver Falk 2009-10-27 03:47:21 EDT
It finally made it. Great to see :-)

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