[squid-dev] [PATCH] Secure ICAP

[Amos Jeffries]

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://...".


> 
> 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


in src/acl/FilledChecklist.h:

* please use an SBuf for dst_peer_name
 - that will also prevent memory leaks in
Ssl::IcapPeerConnector::initializeSsl().



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.


in src/cf.data.pre:
* the new securtiy options section title should have s/HTTPS/ICAPS/


in src/adaptation/icap/ServiceRep.cc:

* please create/use #define DEFAULT_ICAP_PORT and DEFAULT_ICAPS_PORT
instead of magic port numbers.


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


in src/adaptation/icap/Xaction.cc:

* Adaptation::Icap::Xaction::noteCommClosed() does not need to wrap
securer use with USE_OPENSSL.


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.


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