[squid-dev] [PATCH] Secure ICAP

[Tsantilas Christos]

patch applied as rev.14055 to trunk

On 05/04/2015 11:58 AM, Tsantilas Christos wrote:
> If there are not any objections  I will apply the latest patch to
> squid-trunk.
>
> On 04/23/2015 08:58 PM, Tsantilas Christos wrote:
>> A new patch  for Secure ICAP.
>> This is synced with the latest trunk.
>>
>> Also handles Amos requests.
>>
>> Regards,
>>     Christos
>>
>> On 04/10/2015 04:51 AM, Amos Jeffries wrote:
>>> On 10/04/2015 2:43 a.m., Tsantilas Christos wrote:
>>>>
>>>> This patch adds support for ICAP services that require SSL/TLS
>>>> transport
>>>> connections.
>>>>
>>>> To mark an ICAP service as "secure", use an "icaps://" service URI
>>>> scheme when listing your service via an icap_service directive.
>>>
>>> The squid.conf parsing code contradicts that - it expects "... ssl
>>> icap://...".
>>
>> Inside Adaptation::Icap::ServiceRep::finalize() method if the protocol
>> is the "icaps" then sets the secure.encryptTransport flag to true. IT
>> can be used to enable secure icap without any ssl option.
>>
>>>
>>>
>>>>
>>>> Squid uses port 11344 for Secure ICAP by default, following another
>>>> popular proxy convention. The old 1344 default for plain ICAP ports has
>>>> not changed.
>>>>
>>>> This patch should applied after the "server_name" and "splicing resumed
>>>> sessions" patches applied to trunk, and after re-merged with the trunk.
>>>> However we can start the discussion if you agree.
>>>>
>>>>
>>>> Technical Details
>>>> ==================
>>>>
>>>> This patch:
>>>>    - Splits Ssl::PeerConnector class into Ssl::PeerConnector parent and
>>>> two kids: Ssl::BlindPeerConnector, a basic SSL connector for
>>>> cache_peers, and Ssl::PeekingPeerConnector, a peek-and-splice SSL
>>>> connector for HTTP servers.
>>>>
>>>>    - Adds a third Ssl::IcapPeerConnector kid to connect to Secure ICAP
>>>> servers.
>>>>
>>>>    - Fixes ErrorState class to avoid crashes on nil ErrorState::request
>>>> member. (Ssl::IcapPeerConnector may generate an ErrorState with a nil
>>>> request).
>>>>
>>>>    - Modifies the ACL peername to use the Secure ICAP server name as
>>>> value while connecting to an ICAP server. This is useful to make SSL
>>>> certificate  policies based on ICAP server name. However, this
>>>> change is
>>>> undocumented until we decide whether a dedicated ACL would be better.
>>>>
>>>>
>>>> This is a Measurement Factory project.
>>>>
>>>
>>> Initial audit:
>>>
>>> * Watch for HERE macros in the new, changed or moved code. I see at
>>> least one being added in PeerConnector.cc
>>
>> fixed
>>
>>
>>
>>>
>>>
>>> in src/acl/FilledChecklist.h:
>>>
>>> * please use an SBuf for dst_peer_name
>>>   - that will also prevent memory leaks in
>>> Ssl::IcapPeerConnector::initializeSsl().
>>
>> ok.
>>
>>
>>>
>>> in src/adaptation/ServiceConfig.cc:
>>>
>>> * the new "ssl" option needs some polishing.
>>>   - for the dependency WARNING it should have:
>>> debugs(3, DBG_PARSE_NOTE(DBG_IMPORTANT), "WARNING: ... ICAP service
>>> option ignored.");
>>>   - since this is all new code please use "tls-" as the TLS/SSL options
>>> prefix.
>>
>> OK
>> I am not documenting "tls-" prefix of the options in cf.data.pre.
>> But it works.
>>
>>>
>>>
>>> in src/cf.data.pre:
>>> * the new securtiy options section title should have s/HTTPS/ICAPS/
>>
>> OK.
>>
>>
>>>
>>>
>>> in src/adaptation/icap/ServiceRep.cc:
>>>
>>> * please create/use #define DEFAULT_ICAP_PORT and DEFAULT_ICAPS_PORT
>>> instead of magic port numbers.
>>>
>>>
>>
>> ok.
>>
>>> in src/adaptation/icap/ServiceRep.h:
>>>
>>> * please use Security::ContextPointer instead of SSL_CTX* for
>>> sslContext.
>>>   - and it can go outside the USE_OPENSSL wrapper
>>
>> ok
>>
>>>
>>>
>>> in src/adaptation/icap/Xaction.cc:
>>>
>>> * Adaptation::Icap::Xaction::noteCommClosed() does not need to wrap
>>> securer use with USE_OPENSSL.
>>
>> fixed.
>>
>>
>>>
>>>
>>> in src/err_detail_type.h:
>>> * whats with the change to ERR_DETAIL_REDIRECTOR_TIMEDOUT ?
>>>    - if thats a bug-fix you found along the way it should go in separate
>>> for backporting.
>>
>> Yes it is a bug.
>> I committed as separate patch. It is a trunk only bug.
>>
>>
>>>
>>>
>>> NOTE: I've not done much audit on the PeerConnector changes, my brain is
>>> too fuzzy right now. Though I am liking the class split.
>>>
>>>
>>> Amos
>>> _______________________________________________
>>> squid-dev mailing list
>>> squid-dev at lists.squid-cache.org
>>> http://lists.squid-cache.org/listinfo/squid-dev
>>>
>>
>>
>>
>> _______________________________________________
>> squid-dev mailing list
>> squid-dev at lists.squid-cache.org
>> http://lists.squid-cache.org/listinfo/squid-dev
>>
>
> _______________________________________________
> squid-dev mailing list
> squid-dev at lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev