Re: [PATCH] Dynamic SSL Certificate Generation

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)

src/Makefile.am:
 * why is WIN32_ALL_SOURCE being altered by 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.

src/ProtoPort.cc:
 * missing required wrapping around <> includes.
 ** possibly also missing <climits> include for other g++ versions and
alternative compilers.
  same in other .cc files.

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"

src/ssl/Makefile.am:
 * are you able to wrap ALL of the ssl_crtd lines inside the conditional
ENABLE_SSL_CRTD ?
   with just the source files added to EXTRA_DIST in the else case?
 * please use $(COMPAT_LIB) instead of linking explicitly. This will build
fail since libcompat-squid has sub-dependencies.

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

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.
 * use "#if _SQUID_MSWIN_" not #ifdef.
 * Ssl::CertificateDb:: size(), readSize() and getSNString() seem to be
missing const;
 * please refer to SSL (uppercase) in the documentation.

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

src/ssl/crtd_message.h:
 * CrtdMessage::parse() - please use \retval for doxygen syntax with
multiple return values.
 * no need to use \brief if there is only one line of paragraph
documentation. Just stick the text on the /** line.

src/ssl/gadgets.h and src/ssl/gadgets.cc:
 * missing #endif \n #if HAVE_OPENSSL_TXT_DB_H between openssl includes.
(and around <string>)
 * please try and move config.h include from .h to .cc

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.

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

Amos
Received on Thu Nov 11 2010 - 01:11:07 MST