Re: [squid-dev] [PATCH] initial GnuTLS support for encrypted server connections

[Amos Jeffries]

On 2/02/2017 2:28 p.m., Alex Rousskov wrote:
> On 01/19/2017 12:11 PM, Alex Rousskov wrote:
>> On 01/19/2017 12:16 AM, Amos Jeffries wrote:
>>> Well, there is no such thing as a "SSL connection" - it is security
>>> added onto some *other* Transport Protocol's layer.
> 
>> There is. The "security added onto some other Transport Protocol's
>> layer" is called SSL connection. It is not a TCP connection, of course.
>> It is an SSL connection. See RFC 5246 for numerous examples of this
>> usage. Calling that connection a "session" in Squid sources is abomination.
> 
> I probably should have been more explicit here. Please rename the new
> Security::CreateClientSession() and friends to
> Security::CreateClientConnection() and such. If the patch adds other
> sessions that are actually SSL connections, please fix them as well.

Just as we were getting so close to agreeing on the names.


Can we agree on this being a fundamental design in Squid:

 * all connections have an associated socket ID.

 * all _open_ connections are stored in fd_table. Indexed by the
connections socket ID. If not that is a bug.

Do you agree on that?



Now a Question, and please answer carefully:

 Does the PeerConnector or the new() operator 'connect' the "SSL
connection" ?


Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] initial GnuTLS support for encrypted server connections

[Alex Rousskov]

On 01/19/2017 12:11 PM, Alex Rousskov wrote:
> On 01/19/2017 12:16 AM, Amos Jeffries wrote:
>> Well, there is no such thing as a "SSL connection" - it is security
>> added onto some *other* Transport Protocol's layer.

> There is. The "security added onto some other Transport Protocol's
> layer" is called SSL connection. It is not a TCP connection, of course.
> It is an SSL connection. See RFC 5246 for numerous examples of this
> usage. Calling that connection a "session" in Squid sources is abomination.

I probably should have been more explicit here. Please rename the new
Security::CreateClientSession() and friends to
Security::CreateClientConnection() and such. If the patch adds other
sessions that are actually SSL connections, please fix them as well.

There is also misnamed Security::SessionPointer but this patch does not
introduce that bug so I cannot ask you to fix it in this patch. It
should be renamed to Security::Connection in a separate commit.


FWIW, I am not married to the word "Connection" in this context. It is
possible that another (less overloaded in other contexts) word would
work here, but

* "Connection" is technically correct
* I cannot think of a better word (and we use it in other projects)
* "Session" is very wrong because it already names another SSL concept.


Thank you,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] initial GnuTLS support for encrypted server connections

[Alex Rousskov]

On 02/01/2017 01:42 PM, Christos Tsantilas wrote:
> must take in account that some openSSL calls
> returns locket objects, and some other unlocked objects.

Does the patch start using shared pointers for any objects in the
second, "returned unlocked" category? AFAICT, only the SSL connection
object (shared_ptr) is currently affected. That object is always
given to Squid locked by OpenSSL, right?

We would have to remember to ask ourselves these questions for every new
OpenSSL-lockable type that we start using inside a shared_ptr, of
course, but if OpenSSL always returns locked SSL objects, those should
be "safe to share" AFAICT.

Thank you,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] initial GnuTLS support for encrypted server connections

[Christos Tsantilas]

On 19/01/2017 09:11 μμ, Alex Rousskov wrote:

Does the patched code continue to work well with OpenSSL?


You have not answered this question. Please do not commit these changes
until the OpenSSL build is tested.



Amos, asks me to make some tests if I have time. I make some simple 
tests with openSSL  and looks that builds and runs.
However I did not check if OpenSSL objects locking system works as 
expected. Just someone must take in account that some openSSL calls 
returns locket objects, and some other unlocked objects.





___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Bug 4662 adding --with-libressl build option

[Alex Rousskov]

On 02/01/2017 08:20 AM, Marcus Kool wrote:
>> Do you think we can compromise and call it USE_OPENSSL_OR_LIBRESSL ?

> or call it USE_OPENSSL_API
> 
> and then the code will eventually have none or few occurrences of
> USE_OPENSSL and USE_LIBRESSL to deal with OpenSSL and LibreSSL specifics.

Yes, and I have suggested this natural solution before (and again a few
minutes ago). The correct decision making algorithm is really simple if
you start from the right end:

1. Agree to continue to use a single primary SSL guard macro. Ignore its
exact spelling of that macro for now. Agree that it _means_ that Squid
is configured to use an OpenSSL API (which has many
implementations/flavors).

2. Discuss weather --with-libressl is necessary or there is a better way
to detect LibreSSL presence.

3a. If --with-libressl is necessary, rename USE_OPENSSL to
USE_OPENSSL_API and add code to set that renamed macro. ./configure will
set USE_OPENSSL and USE_LIBRESSL, of course.

3b. Otherwise, do not rename USE_OPENSSL. ./configure will continue to
set it when building with OpenSSL or LibreSSL libraries, of course.

4. Add code to fix bug 4662.


Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Bug 4662 adding --with-libressl build option

[Alex Rousskov]

Executive summary:

* Still no agreement on whether or how to rename the primary SSL guard.
* Possibly an agreement to continue using a single primary SSL guard??
* Clarification that --with-libressl itself is a relatively minor issue.
* A firm veto on adding support for the 3rd SSL API.
  Whether that veto applies to bug 4662 depends on the fix,
  but a correct fix will not add support for the 3rd API IMO.

Details below.

On 02/01/2017 06:51 AM, Amos Jeffries wrote:

> Do you think we can compromise and call it USE_OPENSSL_OR_LIBRESSL ?

I would not view that name as a compromise: It still enumerates specific
libraries instead of naming the API used by the code it protects. You
can try to argue that it avoids code duplication but the value of this
literal/meaningless avoidance is very small. In other words, this
solution would preserve 95% of the bad properties of the original
proposal while giving me a superficial 5% satisfaction of removing
spaces from (a || b).

[rant]Compromises in design and development are rarely a good idea. Two
correct designs are rarely on the same curve. Adding them up and
dividing by two usually gives a solution that is worse than either of
the alternatives. Please do not misinterpret this general statement as
an implication that there are two correct designs here; I have not seen
them (or I would not be objecting).[/rant]


I would like to capitalize on your willingness to use a single macro.

This macro should name the associated API. I think it is a bad idea to
call the associated API "OpenSSL or LibreSSL" when we know that

* there are other libraries that implement the same API (leading to
USE_OPENSSL_OR_LIBRESSL_OR_FOO_OR_BAR)

* LibreSSL provides more than one API
(leading to USE_OPENSSL_OR_LIBRESSL_PART_PROVIDING_OPENSSL_API)

If we do not add --with-libressl, then USE_OPENSSL would be a natural
choice given the current code. In case we decide to add --with-libressl,
I can offer the following alternatives and remain open to other suggestions:

  * USING_OPENSSL
  * USING_OPENSSL_API
  * USE_OPENSSL_API
  * USE_OPENSSL_FLAVORS
  * USE_ANY_OPENSSL
  * HAVE_OPENSSL_API


>> Repeating (a || b) condition many times is bad. Using a single (c)
>> condition many times is not. We should not be arguing about that basic
>> programming principle!

> So you wish de define a 3rd macro in addition to the other two

I wish we add no new macros until they become necessary. You are adding
macros. I am content with what we already have for this high-level
context: A single USE_OPENSSL macro.

The existing USE_OPENSSL macro is not the problem. The lack of an
explicit --with-libressl option is not the problem. The problem is that
Squid relies on the OPENSSL_VERSION_NUMBER (which LibreSSL has changed
in an incompatible way). This is a minor compatibility problem that has
several correct solutions, none of which require changing most Squid
code dealing with the OpenSSL API. Here are a few sketches:

A. Define SQUID_OPENSSL_VERSION to match available API version (in
OpenSSL terms) and use that instead of raw OPENSSL_VERSION_NUMBER.

B. Replace raw OPENSSL_VERSION_NUMBER uses with macros that guard the
corresponding features (without adding feature tests).

C. Replace OPENSSL_VERSION_NUMBER with feature tests.

Solution C is the best/cleanest but also requires more work. I am not
sure, but I suspect that all solutions can be implemented without adding
a new --with-* option for every OpenSSL API implementation (LibreSSL,
BoringSSL, etc.).


>> I believe we can simply continue to use USE_OPENSSL almost everywhere it
>> is used today. No "fixing" is needed in this context except changing
>> what you think that macro should stand for.

> So a hypothetical patch is submitted to use, lets say a function
> parameter is made const, in the LibreSSL but breaks when building with
> OpenSSL. Is required to be wrapped in USE_OPENSSL.
> 
>  Can you spot the contradiction?

There is no contradiction if you interpret USE_OPENSSL correctly.

Yes, there could be other macros/guards inside USE_OPENSSL code,
including, if really necessary USE_LIBRESSL set by --with-libressl. In
the latter case, USE_OPENSSL itself (i.e., the primary guard for the
code using OpenSSL API) would have to be renamed to USING_OPENSSL_API or
some such. Needless to say, those other macros/guards have to be
confined to a few exceptional cases.


>> In other words, you seem to abstract at the level of file name spelling.
>> I abstract at the level of APIs. Naturally, it is difficult to agree on
>> a single scheme that accommodates both approaches!

> I am following the autoconf guidelines

I rest my case! We should be following autoconf guidelines when
implementing a high-level design. We should not follow autoconf
guidelines to come up with or understand a high-level design. Again and
again, you are obsessed with letters such as those used to spell the
USE_OPENSSL macro. We should be obsessed with that macro meaning instead.


> The 

Re: [squid-dev] [PATCH] Bug 4662 adding --with-libressl build option

[Marcus Kool]

Do you think we can compromise and call it USE_OPENSSL_OR_LIBRESSL ?


or call it USE_OPENSSL_API

and then the code will eventually have none or few occurrences of
USE_OPENSSL and USE_LIBRESSL to deal with OpenSSL and LibreSSL specifics.

Marcus
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Bug 4662 adding --with-libressl build option

[Alex Rousskov]

On 02/01/2017 05:00 AM, Eliezer  Croitoru wrote:

> I do believe that for the latest hardware with beefy CPU, code
> repetition in C++ might not be much of a regression but not everybody
> can replace their systems hardware every year. (If my assumption
> about code repetition affecting older systems is wrong let me know)

In general, code duplication itself has negligible impact on performance
(which can go either way, depending on the circumstances).

Code duplication leads to bugs and increases the cost of development.
Its long-term effect is often exponential. I speculate that at least 20%
of Squid bugs are caused by code duplication, and an average Squid
development project is at least 20% more expensive because of it. It is
a big deal!

Fortunately, it is usually easy to avoid when writing new code.
Unfortunately, I have to keep begging and begging to avoid it in new
code, as if it is some obscure or unimportant problem and not a basic
development principle.

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Case-insensitive URI schemes

[Eduard Bagdasaryan]

This is a bit improved version of previous patch:
fill static schemes array at configuration phase.


Eduard.


On 31.01.2017 16:18, Eduard Bagdasaryan wrote:

Optimized with static array as you suggested and
re-attached the patch.


Eduard.


On 30.01.2017 19:24, Alex Rousskov wrote:

On 01/29/2017 07:10 AM, Amos Jeffries wrote:


I'm thinking the quick-and-dirty way is to just lowercase the 'proto'
variable in url.cc urlParse() function. Doing that in the for-loop 
where

it is copied from 'src' would be easiest.
  - it breaks the case preservation on unknown schemes a litte bit. But
since they are supposed to be insensitive anyway the harm is minimal.


The UriScheme constructor is broken. No quick-and-dirty fix in some
constructor caller will address that problem. Eduard's patch fixes that
problem the right way. Unfortunately, the fixed constructor is
expensive. Fortunately, it is easy to optimize it, addressing another
old problem (that the patch has documented but did not address). With
that optimization, there will be no motivation for quick-and-dirty
workarounds.

Eduard, please create and use a UriScheme::SchemeImages or similar
static array with lowercase versions of ProtocolType_str.


Thank you,

Alex.




Fix URI scheme case-sensitivity treatment broken since r14802.

A parsed value for the AnyP::UriScheme image constructor parameter was
stored without toLower() canonicalization for known protocols (e.g.,
after successful "HTTP://EXAMPLE.COM/" parsing in urlParseFinish).
Without that canonicalization step, Squid violated various HTTP caching
rules related to URI comparison (and served fewer hits) when dealing
with absolute URLs containing non-lowercase HTTP scheme.

According to my limited tests, URL-based ACLs are not affected by this
bug, but I have not investigated how URL-based ACL code differs from
caching code when it comes to stored URL access and whether some ACLs
are actually affected in some environments.

=== modified file 'src/anyp/UriScheme.cc'
--- src/anyp/UriScheme.cc	2017-01-01 00:12:22 +
+++ src/anyp/UriScheme.cc	2017-02-01 14:26:36 +
@@ -1,61 +1,81 @@
 /*
  * Copyright (C) 1996-2017 The Squid Software Foundation and contributors
  *
  * Squid software is distributed under GPLv2+ license and includes
  * contributions from numerous individuals and organizations.
  * Please see the COPYING and CONTRIBUTORS files for details.
  */
 
 /* DEBUG: section 23URL Scheme parsing */
 
 #include "squid.h"
 #include "anyp/UriScheme.h"
 
+std::vector AnyP::UriScheme::LowercaseSchemeNames;
+
 AnyP::UriScheme::UriScheme(AnyP::ProtocolType const aScheme, const char *img) :
 theScheme_(aScheme)
 {
-if (img)
-// image could be provided explicitly (case-sensitive)
-image_ = img;
-
+// RFC 3986 section 3.1: schemes are case-insensitive.
+
+// To improve diagnostic, remember exactly how an unsupported scheme looks like.
+// XXX: Object users may rely on toLower() canonicalization that we refuse to provide.
+if (img && theScheme_ == AnyP::PROTO_UNKNOWN)
+ image_ = img;
+
+// XXX: A broken caller supplies an image of an absent scheme?
+// XXX: We assume that the caller is using a lower-case image.
+else if (img && theScheme_ == AnyP::PROTO_NONE)
+ image_ = img;
+
+// the caller knows nothing about the scheme
+// TODO: Avoid special casing by storing this string in the generated ProtocolType_str?
 else if (theScheme_ == AnyP::PROTO_UNKNOWN)
-// image could be actually unknown and not provided
 image_ = "(unknown)";
 
-else if (theScheme_ > AnyP::PROTO_NONE && theScheme_ < AnyP::PROTO_MAX) {
-// image could be implied by a registered transfer protocol
-// which use upper-case labels, so down-case for scheme image
-image_ = AnyP::ProtocolType_str[theScheme_];
-image_.toLower();
+else if (theScheme_ > AnyP::PROTO_NONE && theScheme_ < AnyP::PROTO_MAX)
+image_ = LowercaseSchemeNames.at(theScheme_);
+// else, the image remains empty (e.g., "://example.com/")
+// hopefully, theScheme_ is PROTO_NONE here
+}
+
+void
+AnyP::UriScheme::Init()
+{
+if (LowercaseSchemeNames.empty()) {
+for (int i = AnyP::PROTO_NONE; i < AnyP::PROTO_MAX; ++i) {
+SBuf image(ProtocolType_str[i]);
+image.toLower();
+LowercaseSchemeNames.push_back(image);
+}
 }
-// else, image is an empty string ("://example.com/")
 }
 
 unsigned short
 AnyP::UriScheme::defaultPort() const
 {
 switch (theScheme_) {
 
 case AnyP::PROTO_HTTP:
 return 80;
 
 case AnyP::PROTO_HTTPS:
 return 443;
 
 case AnyP::PROTO_FTP:
 return 21;
 
 case AnyP::PROTO_COAP:
 case AnyP::PROTO_COAPS:
 // coaps:// default is TBA as of draft-ietf-core-coap-08.
 // Assuming IANA policy of allocating same port for base and TLS protocol versions will occur.
 return 5683;
 
 case 

Re: [squid-dev] [PATCH] Bug 4662 adding --with-libressl build option

[Eliezer Croitoru]

I want to add my opinion on the matter as a system administrator compared to 
some developers about LibreSSL.
The systems I am working are mostly stable or attempts to be stable.
I mentioned in the past that we should consider sticking on releases with 
something stable compared to non-stable as an approach.
Squid 4.0.X is trying to be Stable "Compatible" and what I consider stable in 
the Linux systems I work are:
- Debian
- CentOS
- Ubuntu 14.04 (16.04 is exactly on the point between stable enough for use to 
stable for enterprise).
- FreeBSD 10.X
- Solaris

Debian is far behind in libreSSL support and CentOS is not trying to be 
compatible in the near future.
So it leaves me\us with something that tries to be stable such as Ubuntu 16.04.
They do not offer LibreSSL officially(as far as I know) and it's the same for 
FreeBSD 10.X. and Solaris(as far as I know)

I believe that 4.0.X should not try to be compatible with LibreSSL and this is 
due to the fact that the developers of the SSL related code needs a stable 
ground.
BUG 4662[http://bugs.squid-cache.org/show_bug.cgi?id=4662] is a result of a 
testing experiment with a not enough production or non-tested enough by many 
software.
I believe that at this stage we should define the exact point in the future 
that we would try to support LibreSSL instead of supporting it.
The developers of the SSL related code should define if the goal is 5.0 or 
another more advanced version.

Also I do believe that for the latest hardware with beefy CPU, code repetition 
in C++ might not be much of a regression but not everybody can replace their 
systems hardware every year.
(If my assumption about code repetition affecting older systems is wrong let me 
know)

I hope that my words helped and contributed to the subject in hands.

Regards,
Eliezer


Eliezer Croitoru
Linux System Administrator
Mobile: +972-5-28704261
Email: elie...@ngtech.co.il


-Original Message-
From: squid-dev [mailto:squid-dev-boun...@lists.squid-cache.org] On Behalf Of 
Alex Rousskov
Sent: Tuesday, January 31, 2017 6:50 PM
To: Squid Developers 
Subject: Re: [squid-dev] [PATCH] Bug 4662 adding --with-libressl build option

On 01/31/2017 08:20 AM, Amos Jeffries wrote:
> On 31/01/2017 7:04 a.m., Alex Rousskov wrote:
>> On 01/29/2017 04:26 AM, Amos Jeffries wrote:
>>> This is I think all we need to do code-wise to resolve the Bug 4662 
>>> issues with LibreSSL being incompatible with OpenSSL 1.1.

>> I do not think these changes should be committed. As you probably 
>> know from earlier communication, I think we should avoid using both 
>> USE_OPENSSL and USE_LIBRESSL in the code if LibreSSL is [treated as] 
>> a replacement for OpenSSL. I have suggested several ways to avoid the 
>> dangerous and needless repetition of (USE_OPENSSL || USE_LIBRESSL) 
>> conditions, and we even seemed to agree on one of those solutions.


> IMO we only agreed on the HAVE_* macro usage for determining whether 
> the
> 1.1 API was in use. Which is included in this patch.

I do not limit "feature tests" to "1.1 API". Any feature is eligible. In fact, 
it may be a good idea to test individual features rather than bundle many of 
them into a single API version test (that will become obsolete as parts of that 
API are going to change). However, those details are probably not very 
important when resolving the core disagreement here.


> I don't think the repetition of (USE_OPENSSL || USE_LIBRESSL) is 
> either needless or dangerous.

The needless part is not a matter of opinion. It is a fact -- you do not need 
to repeat the (USE_OPENSSL || USE_LIBRESSL) test. You may use a single 
USE_FOOBAR macro to avoid this code repetition. The exact spelling of FOOBAR is 
a separate question.

Whether code duplication is bad, dangerous, unwanted, etc. is certainly a 
matter of opinion and subject to context, but I doubt you can convince me that 
it is not in this case, especially as we have started using USE_OPENSSL macro 
in complex expressions involving GnuTLS.


> No more than USE_OPENSSL was in the same spot.

Repeating (a || b) condition many times is bad. Using a single (c) condition 
many times is not. We should not be arguing about that basic programming 
principle!


> Fixing the sheer number of uses is not in scope.
> 
> Keep it simple. 

I believe we can simply continue to use USE_OPENSSL almost everywhere it is 
used today. No "fixing" is needed in this context except changing what you 
think that macro should stand for.


> Only one macro per library. --with-foo sets USE_FOO.

For you, the libraries provided by OpenSSL and LibreSSL project are two 
different "libraries". For me, they are two slightly different flavors of the 
same "library". IMO, the developers should not be forced to think which flavor 
is in use now or which flavors are supported today, except in very few 
low-level cases where that information might be needed. With feature tests, the 
number of such cases