[squid-dev] [PATCH] received_encrypted ACL

[Amos Jeffries]

On 24/07/2015 7:41 a.m., Tsantilas Christos wrote:
> On 07/23/2015 07:21 PM, Alex Rousskov wrote:
>> On 07/23/2015 07:41 AM, Amos Jeffries wrote:
>>> On 23/07/2015 3:32 a.m., Alex Rousskov wrote:
>>>> On 07/21/2015 04:25 AM, Amos Jeffries wrote:
>>>>> On 21/07/2015 9:42 a.m., Alex Rousskov wrote:
>>>>>>>> adaptation_access icapS aclIcap
>>>>>>>> adaptation_access icapN !aclIcap
>>>>>>>>
>>>>>>>>
>>>>>>>> aclIcap can be a received_encrypted ACL. What ACL expression
>>>>>>>> would you
>>>>>>>> suggest for aclIcap if received_encrypted is not available?
>>>>
>>>>
>>>>>>>    # top 5 criteria - TLS transactions
>>>>>>>    acl aclIcap allof HTTPS
>>>>
>>>>>> What is "HTTPS" ACL? If it is based on the URI scheme, then AFAIK,
>>>>>> it is
>>>>>> not going to work because it will not match bumped http://
>>>>>> requests (at
>>>>>> least).
>>>>
>>>>> Good you have seen those. That is what the HTTPbis WG call
>>>>> "opportunistic encryption" traffic.
>>>>
>>>>
>>>> I bet many (most?) of them are actually regular HTTP requests sent over
>>>> TLS/SSL connections. There is nothing opportunistic about them.
>>>
>>> That is the very definition of "opportunistic" in the related HTTP
>>> documentation.
>>
>>
>> Hi Amos,
>>
>>      The opportunistic security (OS) documents you site talk about
>> optionally applying encryption to traffic X, to arrive at traffic XE. A
>> general-purpose intermediary cannot use those documents to go backwards
>> and say that any applied encryption was optional if we see traffic XE!
>> This backward direction is possible only when X or XE includes some sort
>> of an "it is OK to decrypt at any time" flag. Until such a flag is
>> defined for HTTP and/or TLS, and Squid learns to interpret it, the
>> discussion about opportunistic encryption is pretty much not relevant to
>> the received_encrypted ACL.
>>
>> Moreover, the received_encrypted ACL itself and the related use cases do
>> not become wrong or obsolete when/if "it is OK to decrypt at any time"
>> flag is supported.
>>
>> Is "http://" scheme such a flag? Not for Squid. That URI scheme existed
>> long before any of the HTTP WG proposals you site were written, so Squid
>> should not assume that an encrypted http:// request is the result of OS.
>> Either a more reliable flag or an admin decision is required to enable
>> such behavior. All of this is orthogonal to received_encrypted.
>>

I think you still misunderstand the OppSec RFC meanings.

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.
 As I hope you are aware, this has zero meaning in the end-2-end
security of the messages themselves. Only that the link between the two
proxy is somewhat private/secure against surveillance.
For example; its quite popular for Enterprise networks with remote
office to use as a pseudo-VPN to HQ, but does not mean the traffic
delivered over it can avoid the company firewall when branch office
staff browse the Internet.


 The browser guys have finally been half-convinced that its a good
thing, and "opportunistic security" is the new name being applied for
what Squid has been doing all along. Since the WG has to deal with many
more real-world scenarios than Squid peering there are other mechanisms
added on top for negotiation and capability advertisement,
generalizations to handle the non-HTTP web protocols, etc.

 But dont let that extra fanciness detract from the fact that the RFC
core is documenting what "cache_peer ... 80 0 ssl" has been doing
(manually) to the HTTP protocol all along.


Anyhow. My objections resolved below ...

>>> Look closely:
>>
>>> +    if (!(filled->request->sources & HttpMsg::srcUnsafe)  ||
>>> +        (filled->reply && !(filled->reply->sources &
>>> HttpMsg::srcUnsafe)))
>>> +        return 1;
>>> +
>>> +    return 0;
> 
> 
> This is a stupid bug, probably caused while I was trying to convert a
> more complex expression to a simpler....
> 
>>
>> That code does not make sense to me. I think it should be written like
>> this instead:
>>
>>      const bool safeRequest =
>>          !(filled->request->sources & HttpMsg::srcUnsafe);
>>      const bool safeReply = !filled->reply ||
>>          !(filled->reply->sources & HttpMsg::srcUnsafe);
>>      return (safeRequest && safeReply) ? 1 : 0;
> 
> Yes!

Aha! that rather dramatically changes the ACL behaviour matrix in about
half the "opportunistic security" cases.


> 
>>
>> Furthermore, the values of "unsafe" srcX enum constants should be
>> increased to actually match the srcUnsafe mask (16 is still smaller than
>> 0xFFFF).
> 
> This is should be OK.
> The safe flags are from:
>  (1 << (16 + 0)) = (1 << (16 + 0)) & 0xFFFF0000
> to:
>   (1 << (16 + 15)) = (1 << (16 + 15)) & 0xFFFF0000
> 
> Am I loosing something?
> (It is late here, this is not a time for playing with bits and shifts,
> my mind is broken ...)
> 
>>
>>
>>> While I personally like the outcome (more security). Its clearly not
>>> matching the use case presented as *sole* reason for the ACLs existence.
>>> Nor the apparent admin policy in adaptation_access.
>>
>> To me, it just looks like an implementation bug. Christos, do you think
>> the code should be rewritten as in my sketch above?
>>
>>
>>> So I ask; why are we bothering with this ACL?
>>>   Just use an https:// scheme match, which will only be true in the top
>>> line of above if()-matrix. Admin *wanting* "opportunistic security"
>>> should not be the ones trying to avoid using icapS in the first place.
>>
>> I have answered that question several times already, including providing
>> specific examples requested by Kinkie: The URI scheme alone does not
>> match what the ACL is defined to match.

It seems our difference of opinion about what was happening was caused
by that implementation bug that made received_encrypted overlap
significantly with "proto HTTPS". With that fix the overlap is now
absent which removes my objection about that.


There are some small bits still to work out.

So finally the full audit...


in cf.data.pre:

* does not say clearly whether any value is used for this ACL.
 - I imagine not, but its unclear.


* 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 am minded that there are now also such fun things as having
received Content-Encoding encrypted messages in plain text, which may be
as confusing as "opportunistic security" to some.


* 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".
 - there is no code relating to cache in the patch anyway.


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.


* can you explain the rational of doing this with a bitmask instead of a
default-true 'tlsEncrypted' boolean flag in class HttpMsg ?
 - 32-bit mask is one more thing to remember and update when adding any
new Server or Client child class.
 - we could save 24 bits of memory now (later 56 bits) on every
HttpRequest/HttpReply object with a bool.


in src/acl/*:

* please dont use "\ingroup" in new code.
 - for this patch where possible put ACL things inside namespace "Acl"
instead.
 - I'm okay if you dont want to, but avoiding more SourceLayout
shuffling is always beneficial.


* in ACLReceivedEncrypted::dump() please make the empty SBufList a
static (const?), or just "return SBufList();"


* in match() of course the bug fix Alex mentioned.


in gopher.cc:

* please use HttpReply::Pointer instead of HTTPMSG*() macros.
 - or HttpReplyPointer from http/forward.h
 - this goes for all newly added HttpRequest/HttpReply pointers.


Cheers
Amos