[squid-dev] [PATCH] received_encrypted ACL

[Alex Rousskov]

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.


> * 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].


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


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


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

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


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.