Re: [PATCH] certificate_db/ssl_crtd fixes

On 14.11.2012 04:43, Tsantilas Christos wrote:
> On 11/13/2012 09:48 AM, Amos Jeffries wrote:
>> On 11/11/2012 10:41 p.m., Tsantilas Christos wrote:
>>> On 11/10/2012 06:25 AM, Amos Jeffries wrote:
>>>> Christos,
>>>> which of these bugs (if either) are closed by this patch?
>>>> http://bugs.squid-cache.org/show_bug.cgi?id=3405
>>>> http://bugs.squid-cache.org/show_bug.cgi?id=3436
>>> Looks that it fixes the 3405 bug. But maybe we still need to make
>>> some
>>> improvements.
>>
>> Great. Ported back to 3.3 citing bug 3405.
>
> OK.
>
>>
>> There is at least one clear conflict which makes a semantic change
>> I'm
>> not confident enough about to resolve properly for the port back to
>> 3.2:
>> Ssl::CertificateDb::addCertAndPrivateKey() condition "if (rrow !=
>> NULL)"
>> return result changed from false in 3.2 to true in 3.3 as a result
>> of
>> the stable cert feature update. I am quite uncomfortable just back
>> porting this false->true change and not sure that the false is
>> correct
>> for this patch either. If you wish to supply a patch against 3.2 I
>> would be happy to have it applied but IMO it is not urgent.
>
> Amos I posted a patch for squid3.2 too with the trunk/3.3 patch...
> It is the file certificate_db_fixes-squid-32-v2.patch included in my
> first mail in this mail thread.
>
> About the condition "if (rrow != NULL)" changed with the "SslBump:
> Support bump-ssl-server-first and mimic SSL server certificates"
> patch.
> This check examines if a certificate with a same serial number is
> already stored in database. If the rrow is NULL then there is not
> any.
>
> In squid-3.2 the latest serial number stored in a file, and any new
> generated certificate has as serial this number increment by one (and
> updates the serial file). It is a fatal error for the certificates
> db
> to have a certificate in database with a serial number same as a new
> generated certificate. This is why it returns false in squid-3.2.
>
> After the bump-ssl-server-first/mimic patch the serial number is a
> hash
> of the generated certificate (or something like that), and if we
> found a
> certificate with the same serial number in db means 1) hash conflict
> (not possible) or 2) the certificate is already stored (because an
> other
> squid kid add it for example). On both cases there is not any
> problem,
> we just do not store the already stored certificate.
>
> So the return value for "if (rrow != NULL)" condition must not
> change.
> If you have any problem with the
> certificate_db_fixes-squid-32-v2.patch
> tell me, I will build a new one...
>
>
>>>
>>> We need to make more work to fix the 3436 bug. Currently the
>>> ssl_crtd
>>> helper will crash for every simple failure. For example if it fails
>>> to
>>> add to certificate db a certificate, because the TXT_DB_insert
>>> openSSL
>>> function fails.
>>> We should not let ssl_crtd daemon crash on every single error, but
>>> just
>>> trying to not leave garbage on DB on failures.
>>> This patch make a step, because try to clean up the database after
>>> failures. But the ssl_crtd still crashes on failures....
>>>
>>
>> BTW, I am starting out on adding GnuTLS and other SSL library
>> support by
>> Squid.
>
> Are you planning to use GnuTLS openssl wrappers?

Not the official ones no. They still have known issues when linking
both OpenSSL and GnuTLS against the same binary - which we will have to
allow for a while. I am going for implementing Squid custom wrappers as
appropriate. I was playing with your SSL_CTX_Pointer and related
typedefs last night and I think we should move to using them exclusively
and only have to change them with #if..#else'd definitions for each
library. there will be some functional operations to re-write from
scratch too I'm fairly sure.

> Which are the other SSL libraries?

GnuTLS (Debian, Gentoo, Ubuntu), libNSS (Fedora/RHEL), CryptoAPI
(MacOS) are the ones I'm getting pressure about. There are others on a
"nice to have" list if they are easy to do - but not worth any extra
work.

>
>> That db_TXT_DB_insert function was a good step. If there are any
>> other
>> pieces of SSL code you find which can be shuffled into
>> operational-step
>> pieces like that it would be a great help if it were done. I am
>> having
>> to identify what the core operation for each SSL call and usage is
>> about
>> and find the matching feature usage in each library. The more
>> modular we
>> can make Squid code usages the better.
>
> OK. But I do not believe that it is enough just replacing openSSL
> functions with GnuTLS equivalent. Whether we like it or not the
> architecture affected by the SDKs and libraries we are using.

Understood and I agree. Which is why I'm requesting modularity along
the lines of operations rather than OpenSSL API calls.

>
> I am worrying because we are still continue adding new SSL features
> in
> squid. For example we are ready to submit patches to squid-dev which
> add
> support for shared SSL sessions and other SSL related features.
> It is not easy to stop the development, there are still thinks to do
> in
> SSL subsystem...

I'm not asking you to stop or even to alter what you have lined up.
Just to keep an eye out on the layering for me as you go forward from
today - you have the most visible experience with current SSL
infrastructure in Squid-3 so seem to me a good person to help decide
when something is, or not, appropriate to be abstracted into a unit
behind one function/class/typedef/#define name.

>
> However with a very quick research looks that there are equivalent
> interfaces in GnuTLS for the openSSL interfaces we are using for
> shared
> sessions ...

I am fully expecting that there will be features we cannot support in
all SSL libraries. That is fine, user demand will drive any re-working
that needs to be done on the more difficult features. For now the demand
is simply to get the basic old HTTPS connectivity working without
OpenSSl dependency.

Amos
Received on Tue Nov 13 2012 - 22:32:45 MST