Re: [PATCH] Server PAC file from Squid

On 06/29/2010 05:04 PM, Amos Jeffries wrote:
> On Tue, 29 Jun 2010 14:29:43 -0600, Alex Rousskov
> <rousskov_at_measurement-factory.com> wrote:
>> 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.

>> 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.
>
> I wasn't wanting to go quite that far yet.

I do not think there is a big difference in terms of code between what
you have done and what should be, IMO, done. However, if we commit your
feature, we would have to support it or go through feature withdrawal pains.

In fact, I can even simplify it further by initially allowing only one
internally stored object. In other words, only one serve_internally
squid.conf option would be honored. This will further minimize the
amount of extra work needed to go from serving PAC-only to serving any
arbitrary file.

Later, somebody will add support for multiple serve_internally options,
with no negative effect on code or users.

>> 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.
>>
>
> The PAC file needs to still be a special case, either way due to its
> unusual content-type.

Content-type and other custom HTTP headers should be a part of the
stored file, IMO. Squid can also guess content type for popular
extensions, including *.pac. Nothing special about PAC here.

>> * 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.
>
> As far as error responses can be cached yes.
> The default for errors is non-cacheable, but we can add/update any header
> in the forward.cc to the StoreEntry after the body has been altered/added
> (the PAC file content-type is currently the only alteration).

By "cachable", I meant cachable by the Squid instance producing the
response. It looks like the producing Squid does not cache these
responses, so there is no problem.

Thank you,

Alex.

>> Lower-level comment:
>>
>> * Please s/ERR_BLANK/ERR_INTERNAL_NOT_FOUND/ (or similar) and add a
>> comment describing the new ERR_ constant.
>
> Okay. Adding ERR_INTERNAL_NOT_FOUND too.
>
>> * Should errors/templates/ERR_BLANK file contain some basic "not found"
>> text? Seems wrong to serve an empty error response by default.
>
> ERR_BLANK is still just as useful on its own for "deny_info ERR_BLANK
> foo" to allow admin to send back HTTP status and headers but not error
> texts. As an alternative to TCP_RESET when that is a bit too much eliding
> for some.
>
> Amos
Received on Wed Jun 30 2010 - 22:42:42 MDT