Re: [PATCH] Server PAC file from Squid

On 06/28/2010 10:01 PM, Amos Jeffries wrote:
> One of the old requests is to server the PAC files directly from Squid
> without needing a local HTTP server just for the one file.
>
> This patch makes Squid intercept requests which use it's visible_hostname
> (and hostname_aliases) for /wpad.dat and pass out a PAC file specified in
> the configuration.
>
> This is particularly useful for public hot-spot installs. Allowing tighter
> security since unauthorized and badly configured clients no longer need
> access to anywhere but the proxy itself; and less configuration since an
> additional web server is no longer required just to serve the PAC file.
>
> NP: An ERR_AGENT_PACFILE error template is added to display configuration
> details. For now, like the manual configuration template an assumption has
> to be made about the Squid listening ports since we don't yet have any to
> dynamically insert the listening port into error pages.

There is no argument that the feature is useful.

Big-picture comments:

* IMO, the proposed implementation is a missed opportunity to support
not just one specific internally stored object but any number of any
admin-configured internally stored objects. Instead of bloating code and
configuration options with each particular use case, we should add a
single configuration option to map internal URL paths to locally stored
[error] responses.

For example, the simplest configuration option could work like this:

    serve_internally "/wpad.dat" "errors/templates/ERR_WPAD"
    serve_internally "/logo.png" "icons/logo.png"
    ...

The code to handle the above would be almost as simple as the current
code, but will open up a lot more possibilities. The above can be
extended with more flexible ACL-driven options in the future, of course.

* Will placing the intercept in forward.cc allow caching of the
response? If yes, can the admin prevent caching somehow? If caching is
allowed and not preventable, it may be better to prohibit it, so that
dynamic substitutions inside the error page template continue to work.

Lower-level comment:

* Please s/ERR_BLANK/ERR_INTERNAL_NOT_FOUND/ (or similar) and add a
comment describing the new ERR_ constant.

* Should errors/templates/ERR_BLANK file contain some basic "not found"
text? Seems wrong to serve an empty error response by default.

Thank you,

Alex.
Received on Tue Jun 29 2010 - 20:30:33 MDT