Re: [PATCH] Dynamic SSL Certificate Generation

On 16/11/10 20:27, Tsantilas Christos wrote:
> This version of the patch addresses the Amos comments and suggestions.
> Please see below
>
>
> On 11/11/2010 03:11 AM, Amos Jeffries wrote:
>> On Wed, 10 Nov 2010 17:05:16 +0200, Tsantilas Christos
>> <chtsanti_at_users.sourceforge.net> wrote:
>>> Hi all,
>>>
>>> This patch implements dynamic SSL certificate generartion in
>>> Squid.When used with SSL Bump, the feature allows Squid to dynamically
>>> generate (using a configurable CA certificate) and cache SSL
>>> certificates for the proxied hosts.
>>>
>>> A description for this feature can be found at:
>>> http://wiki.squid-cache.org/Features/DynamicSslCert
>>>
>>> A first version of the patch posted by Alex, some months before:
>>> http://www.squid-cache.org/mail-archive/squid-dev/201003/0201.html
>>>
>>> Some words about the patch:
>>>
>>> * ssl related source files moved under the src/ssl directory
>>>
>>> * Introduce the TidyPointer class similar to std::auto_ptr, which
>>> implements a pointer that deletes the object it points to when the
>>> pointer's owner or context is gone. It is designed to avoid memory
>>> leaks in the presence of exceptions and processing short cuts.
>>
>> So whats different about this new pointer type that the existing
>> std:auto_ptr, RefCount::Pointer and CbcPointer are all insufficient?
>>
>>> From what I've heard of the SSL problems they are all cause by it not
>> using RefCount on the contexts themselves.
>>
>>>
>>> * Implements ssl context cache to use with generated ssl contexts.
>>> The Ssl::LocalContextStorage class stores the hostname/ssl context pairs
>>
>>> for a local listening address/port. The Ssl::GlobalContextStorage class
>>
>>> used to store Ssl::LocalContextStorages per local listening address and
>>> handles squid shutdown/configure/reconfigure
>>>
>>> * Ssl::Helper class implements the squid part of the ssl_crtd
>> helpers.
>>>
>>> * The ssl_crtd helper implemented in ssl_crtd.cc and
>>> certificate_db.* files
>>>
>>> * The Ssl::CertificateDb class (certificate_db.* files) implements
>>> a database of certificates on disk files. It is used by ssl_crtd
>>> helper to manipulate generated certificates.
>>>
>>> * The ssl related files included in the libraries libsslutil.a which
>>
>>> contains common classes and functions and the libsquidssl.a which has
>>> squid related ssl objects and functions
>>>
>>> * Use the Ssl namespace for new ssl code
>>>
>>>
>>> Authors: Alex Rousskov, Andrew Balabohin, Christos Tsantilas
>>> This is a Measurement Factory Project.
>>
>> Thanks Christos,
<snip>
>
>> src/Makefile.am:
>> * why is WIN32_ALL_SOURCE being altered by this patch?
> It is not altered.
>

In EXTRA_squid_SOURCES you had/have:

- $(WIN32_ALL_SOURCE) \

Seems like it may be useful at some point but not related to this patch.

>> * Instead of adding a new /var/ssl_db which the distros are guaranteed to
>> patch elsewhere...
>>
>> Possibly $(DEFAULT_SWAP_DIR)/ssl_crtd/ (aka
>> $(localstatedir)/cache/squid/ssl_crtd/)
>> ** data may be erased at any time
>> or $(localstatedir)/lib/ssl_crtd/
>> ** data may be erased but must persist while process running
>> please see http://www.pathname.com/fhs/pub/fhs-2.3.pdf for the options
>> and
>> criteria.
>
> I put it to $(localstatedir)/lib/.
> I am not sure it is correct. Maybe the best is under the
> $(localstatedir)/cache/ but here exist the main squid cache.
>

Okay.

>> ** possibly also missing<climits> include for other g++ versions and
>> alternative compilers.
>> same in other .cc files.
>
> Is it needed? We are using "limits.h"
>

I think not when <limits.h> is used, but when the C++ <limits> is used
like ProtoPort.cc there have been problems.

Okay. Looks good. +1

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.9
   Beta testers wanted for 3.2.0.3