Re: [PATCH] Dynamic SSL Certificate Generation

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,
>
> configure.in:
> * why is the ./configure option disabled by default?
> * please polish up to new configure.in style:
> ** use SQUID_YESNO, SQUID_DEFINE_BOOL helper macros
> ** use ENABLE_* for the automake conditional
> (the ident-lookups block just above the new lines should serve as a
> demo.
> If a warning is needed the internal-dns block demos that)
>
done

> src/Makefile.am:
> * why is WIN32_ALL_SOURCE being altered by this patch?
It is not altered.

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

>
> src/ProtoPort.cc:
> * missing required wrapping around<> includes.
done

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

>
> src/cf.data.pre:
> (http_port?)
> - grammar: "If this option is enable[d], cert and key options [are]
> use[d] to sign generated certificate."
> "When enabled, the cert and key options are used to sign generated
> certificates."
> - "This option is enabled by default". hopefully that is not true.
> missing qualifier "when SslBump is used"?
>
> (sslcrtd_program)
> - grammar: "SHOULD receive -s and -m options to correct run"
> "requires the -s and -M parameters."
>
> - grammar: "The maximum number of processes spawn to service ssl server"
> "The maximum number of processes to spawn for SSL certificate
> generation"
>

Done.

> src/ssl/Makefile.am:
> * are you able to wrap ALL of the ssl_crtd lines inside the conditional
> ENABLE_SSL_CRTD ?
OK.

> with just the source files added to EXTRA_DIST in the else case?
Should not required....

> * please use $(COMPAT_LIB) instead of linking explicitly. This will build
> fail since libcompat-squid has sub-dependencies.

done

>
>
> NP: all of the new .cc and .h files lack<> include wrappers.

done

>
> src/ssl/certificate_db.cc and src/ssl/certificate_db.h:
> * sys/types.h is included by config.h maybe some of the others as well.
done
> * use "#if _SQUID_MSWIN_" not #ifdef.
done
> * Ssl::CertificateDb:: size(), readSize() and getSNString() seem to be
> missing const;
fixed

> * please refer to SSL (uppercase) in the documentation.
done

>
> src/ssl/context_storage.cc and src/ssl/helper.cc:
> * missing config.h first include

done

>
> src/ssl/crtd_message.h:
> * CrtdMessage::parse() - please use \retval for doxygen syntax with
> multiple return values.

done
> * no need to use \brief if there is only one line of paragraph
> documentation. Just stick the text on the /** line.

done

>
> src/ssl/gadgets.h and src/ssl/gadgets.cc:
> * missing #endif \n #if HAVE_OPENSSL_TXT_DB_H between openssl includes.
> (and around<string>)

done

> * please try and move config.h include from .h to .cc

done

>
>
> src/ssl/ssl_crtd.cc:
> * please add -d option for debug tracing.
> * please use helpers/defines.h macros for communication back to Squid,
> at least for the debug tracing messages and the IO buffer sizes.

Well, I add the -d option but currently does not actually used.
Also I adjust the input buffer.

>
> src/structs.h:
> * can the new config options at least be placed in a new src/ssl/Config.*
> location?
done

>
>
> Amos
>