On 14/05/2013 11:13 p.m., Tsantilas Christos wrote:
On 05/14/2013 12:52 PM, Amos Jeffries wrote:
On 14/05/2013 9:11 p.m., Tsantilas Christos wrote:
On 05/14/2013 06:55 AM, Amos Jeffries wrote:
On 14/05/2013 6:28 a.m., Tsantilas Christos wrote:
I am attaching a fix.
Still needs some discussion.
This patch does the following two checks:
1) Checks if the SSL_get_certificate is buggy
2) Checks it he workaround can be enabled.
Inside squid:
If the workaround can be used, enable it
else if the SSL_get_certificate is not buggy, use it
else hit an assertion
I select this approach:
1) because the workaround is significant faster than using the
SSL_get_certificate
2) to avoid the segfault if the SSL_get_certificate is buggy .
Problems:
I had problem with the LD_LIBRARY_PATH. For example if the user does
not want to use system libraries and use openSSL SDK installed under a
non standard directory, the test program will run with system
libraries.
To avoid this someone should use the LD_LIBRARY_PATH in configure
script:
./configure --with-openssl=/path/to/openssl/
LD_LIBRARY_PATH=/path/to/openssl/
I do not like this option, so in the test I am using the -wl,-rpath
compiler option to pass the openSSL libraries path.
But this option does not looks good too..
Also we may want to harden the workaround test to use a hardcoded
certificate instead of a NULL certificate. (I attached an example in a
previous mail)
Regards,
Christos
Looks like good progress.
Have you tried moving the m4_include statement after AC_SUBST(SSLLIB)?
The the m4_include will expand the file in-place inside configure.ac.
Have you tried passing the flags as an argument to the check macro? eg.
SQUID_CHECK_OPENSSL_GETCERTIFICATE_WORKS([$SSLLIB])
The $SSLLIB variable can be used without problem in the check. This is
not a problem. The problem is different. Imagine the following scenario:
1) The user has installed in your system a buggy version of openSSL. The
configure script finds the SSL_get_certificate buggy
2) Installs a new version under the /opt/openSSL/
3) Runs again the configure script:
./configure --with-openssl=/opt/openSSL/
The check will be compiled using:
g++ -o conftest test.cc -I/opt/openSSL/include -L/opt/openSSL/lib
-lssl
But it will run as
./conftest
This is means that the conftest will use the system ssl libraries
installed under the "/usr/lib/". The test for SSL_get_certificate will
fail again in this case.
And Squid built with the same options will also encounter this identical
library problem when its run as "/usr/bin/squid" or whatever.
Yes.
However squid binary builds with the correct options.
I'm not seeing which ones. But if you know, they need to be added to the
conftest.
We also need to ensure that the main squid binaries are also built with
the same extra options when --with-openssl is given parameters. So a
SSLFLAGS global may be needed creating.
With compiler flags we do not have such problems. We are not using an
SSLFLAGS but we are updating the CPPFLAGS directly, so we are sure that
the include dir is the correct one.
The CPPFLAGS used in generated makefiles too so the squid binaries build
correctly.
Thats all done before the new check macro is run though, right? so
CPPFLAGS setup by --enable-ssl and --with-openssl should be used for
both squid and conftest.
Currently, after squid build, the system admin have to update the ld.so
configuration or set LD_LIBRARY_PATH to load the correct openSSL libraries.
That is what I mean. If they have to do that then we are not setting
CPPFLAGS quite right. They should only have to build --with-openssl=X to
use the X library installation. Nothing else special.
This is also for Kerveros, LDAP libraries, XML libraries, and expat
libraries. Probably this is requires a separate patch fixing all of these...
Yes I think we are missing a while pile of potential -Wl,-rpath=X
settings which our ./configure.ac should be adding when relevant.
Like I said this is separate issue to the bug you were fixing. So if you
want to leave it out for now that is fine.
We can just add comment after configure script finishes to inform user
that he have to update the ld.so configuration too....
I dont think its worth doing anything that special for one library and
not all of them.
An alternate maybe is to set the LD_RUN_PATH or LD_LIBRARY_PATH
environment variable before run the check and unset it after the test
finishes:
OLD_LD_RUN_PATH="$LD_RUN_PATH"
if test "x$SSLLIBDIR" != "x"; then
LD_RUN_PATH="$LD_RUN_PATH:$SSLLIBDIR"
fi
SQUID_CHECK_OPENSSL_GETCERTIFICATE_WORKS
LD_RUN_PATH="$OLD_LD_RUN_PATH"
But still we may have problems. Are the LD_RUN_PATH or LD_LIBRARY_PATH
environment variables available in all systems?
Probably not, and the issue existing for bin/squid still needs it later
anyway.
It is time I suppose to ask, are you wanting to fix all of these known
OpenSSL hack bugs in one update, or do them separately?
If you want do them separately then you have IMHO fixed the bug 3816
issue already, this path problem is a second bug todo, and bug 3759 is
still waiting a fix.
NP: bug 3759 is hitting the entire RHEL hierarchy of distros now.
http://build.squid-cache.org/job/3.HEAD-amd64-centos-6/lastFailedBuild/console,
http://bugs.squid-cache.org/show_bug.cgi?id=3759
Aggr... The bug3759 it is related to bug3232 which force us to build a
patch which used the new openSSL interface for TXT_DB api and a
workaround because the new interface was buggy.
But I am not sure that just checking if new API exist or not is enough.
I will provide a fix for this on a separate patch.
Is there any know Fedore/redhat openSSL package which has these problem?
I need to look on their patch before fix this problem...
All of them from 0.9.8 (f or g IIRC) to the 1.0.0c as far as I'm aware.
1.0.0d was the officially changed OpenSSL version.
Our BuildFarm CentOS machine is using the stock package from CentOS 6 so
we can test the fix there. If you would like a login to the node to
trial things ask Kinkie.
Other checks we are doing using OpenSSL version are:
1) Use "SSL_METHOD *" or "const SSL_METHOD *" with SSL*_method()
functions (0.1.x requires const)
2) check if TLSv1_1_client_method() and TLSv1_2_client_method() exists.
These functions added on openSSL-1.0.1 release.
We may want to add them too in configure script.
Maybe. IIRC these have not been a problem with people back-porting or
mangling the OpenSSL APIs, its just been a matter of the official
releases not supporting them before a certain number.
So, going back to your last patch submission. Please move the AC_DEFUN
to acinclude/lib-checks.m4 where all our library hack tests are supposed
to be defined, and remove the new .m4 file creation. Once that is done I
am happy for you to commit with "--fixes squid:3816". Then lets continue
the thread for the rest of the mess.
Amos
