[squid-dev] [PATCH] received_encrypted ACL

[Amos Jeffries]

On 25/07/2015 3:28 a.m., Alex Rousskov wrote:
> On 07/24/2015 05:26 AM, Amos Jeffries wrote:
>> I think you still misunderstand the OppSec RFC meanings.
> 
> Ditto.
> 
> 
>> Since SSL support first went into Squid back in 1998 we have allowed
>> cache_peer to connect to a remote https_port and sent http:// traffic
>> over it.
> 
> Using the above as a sub-case of the use case we are considering, a
> third independent proxy intercepting that peer-to-peer link (among other
> traffic) should send bumped http:// messages from that link to a remote
> ICAP service using Secure ICAP. To configure that desirable behavior,
> the intercepting proxy admin may use received_encrypted.
> 
> In short, what is "optional" or "opportunistic" for some is often
> "required" or "highly desirable" for others. Independent intercepting
> intermediaries should honor others' decision to use encryption, even
> when that use was "optional" or "opportunistic". There are many
> exceptions and wrinkles to these simple rules of thumb, and the admin
> has the power to configure them.
> 

Good point. I can agree with that.

> 
>> * the "received" part of the AC type name seems a bit odd.
>>  - perhapse "connections_encrypted" makes more sense with the
>> description for it, and focus on TLS connections.
> 
> I disagree that connections_encrypted is better than received_encrypted,
> especially in the presence of connectionless eCAP and
> half-connectionless hits. A more general name may make this ACL more
> forward-compatible.
> 
> However, if you insist, we will use your "connections_encrypted" name to
> avoid discussing this further [unless somebody else suggests a better
> name that you agree to, of course].

I think insist. With that [] caveat.

Regards the ecaps relationship. Its already been mentiond that ecap is
secure by default being "just" a library attachment. AFAIK that normally
a local machine memory-only thing. What would make eCAP services worthy
of the in-secure tag would usually be fetching/sending the data
externally over some type of unsecured connections. Just like ICAP vs
ICAPS. Which brings in the "connection" meaning to eCAP as well in an
only slightly twisted way.

Regards the "half-connectionless hits". If the prior transactions
involved with the cache data matter that much they should be marked as
secure for https:// and insecure for http://.
 BUT, you convinced me earlier that the scope of the ACL was deciding if
the current transactions connections all used TLS (or not). Which means
that pulling details about past transactions into the current one is a
bit out of scope.
 The half-connection they do have is already marked in the sources data.
So that fits "connections" as well if one squints sideways.


> 
>> * you mentionend cache HITs earlier.
>>  - IMO you can drop all mention of cache HIT in the commit message on
>> grounds that no reply connection was involved with HIT at the scope
>> level of "current transaction".
> 
> The new ACL is evaluated against the master transaction (a set of
> messages), not reply connection, so we have to document what happens to
> cache hits IMO. I hope that you will not object to more explicit
> documentation as long as it is correct, matches the code, etc.
> 

No I dont think I object to correct documentation :-)


What I meant was that the code was not actually doing any reply markings
in regards to cache HITs that I could see. So it seems irrelevant to
mention their lack of interaction with a non-existent reply connection
when it is not affecting the outcome.


> 
>> in src/HttpMsg.h:
>>
>> * Unless you are going to add secure eCAP flag functionality as part of
>> ths patch scope I dont see why we should have the extra code or enum for
>> srcEcaps existing yet.
>>  - please remove srcEcaps, or implement the ecaps flag feature.
>>  - TODO in places where the ecaps flag feature would need to hook in
>> seems sufficient to me at present.
>>  - what remains is plain eCAP which does not naturally involve network
>> connections, TLS or otherwise.
>>  - marking eCAP involvement does not seem useful either until that ecaps
>> feature is added. Please consider turning those into TODO as well,
>> though I will accept the current code for srcEcap.
> 
> I disagree that a TODO comment is better than correct, small, and
> symmetric code, but we will replace eCAP-related code with TODO comments
> to avoid prolonging this discussion.
> 

Thank you.

I just realized after writing the above about eCAP that with a loaded
library being effectively secure by default and only certain uncommon
external activities making it non-secure. Then marking the traffic as
srcEcap == Unsafe before the ecaps flag exists is decreasing the amount
of visibly secure transactions. Erring on the side of insecurity rather
than security. Which is much of a sameness, but less desirable in common
cases.


> 
>> * can you explain the rational of doing this with a bitmask instead of a
>> default-true 'tlsEncrypted' boolean flag in class HttpMsg ?
> 
> IIRC, there were two reasons for using distinct, named flags:
> 
> 1. They simplify adding received_encrypted parameters that adjust how
> the ACL is applied/computed.
> 
> 2. "tlsEncrypted" or similar does not work well for connection-less
> things like eCAP and hits. By using protocol/module names, we were able
> to avoid this naming problem.
> 
> Distinct flags also serve as an excellent way of finding various message
> sources in Squid source code, but we probably did not realize that when
> designing the code.

Okay. Fair enough.

> 
> If you insist, we can use boolean storage OR reduce the storage member
> size to 16 bits (let us know what would you prefer).

At this point I'm okay with the current design. It can be easily changed
later if we get on a serious minimalist rampage.


> 
> Your other comments not discussed above should be addressed in the next
> patch version. Please comment on what changes discussed above you insist on.
> 
> 
> Thank you,
> 
> Alex.
> 

HTH
Amos