You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Geoff Thorpe <ge...@geoffthorpe.net> on 2003/03/05 00:43:03 UTC
[PATCH] openssl configuration (v2)
Hi all,
Thanks to those who replied to my earlier post on this subject,
especially Madhu. Here's the next incarnation of the patch based on that
feedback and some ferretting of my own.
Patch notes;
- This now probes for sslc.h to distinguish between OpenSSL and SSL-C.
- Existing version checking only made sense for OpenSSL so is now only
done for OpenSSL.
- The version "error" is now just a warning.
- HAVE_OPENSSL and HAVE_SSLC are set up from these tests so that
modules/ssl/ code can be clearer and less ambiguous.
- Libraries are now checked by AC_CHECK_LIB instead of AC_TRY_LINK as in
my previous patch. This lets autoconf/automake deal with adding the
appropriate "-lssl -lcrypto" linker flags, so this should be a little
more portable.
- inclusion of openssl or ssl-c headers now takes place in
ssl_toolkit_compat.h instead of mod_ssl.h. This results in cleaner
handling of the [HAVE|NO]_SSL_X509V3_H stuff for SSL-C (and which
isn't relevant for OpenSSL at all). Also, paths are now correct for
headers, so an installed version of OpenSSL or SSL-C will usable as-is
without needing any -I flags generated from the configure script.
Questions for apache gurus/code-reviewers;
- AC_CHECK_HEADERS() appears difficult to coax into accepting additional
include paths, so if "--with-ssl=<path>" is specified there appears no
obvious way to have AC_CHECK_HEADERS() pick up those headers in
(particularly if versions exist in system locations too and we want
autoconf's tests to find the <path> versions in preference to any
auto-detectable ones). I've left some comments in the acinclude.m4
changes about this. For now, I've made do with adding "-I<...>" to
CFLAGS prior to AC_TRY_COMPILE, but I'm sure autoconf intended some
other way of handling this. For one thing, is "-I" actually portable
anyway? The existing code depends utterly on it but it would be nice
to do away with it altogether.
- My changes use autoconf tests for openssl/ssl-c headers and libraries
(existing code just looks for files but doesn't actually try to use
them). As a result, linker flags like -ldl, -lsocket, -lnsl, -ldld,
etc are needed in advance of these tests. I've added the obvious ones
I know about so that this patch can be tested as-is, but ideally
Apache's builtin tests (which are obviously OK because Apache links
fine) should occur before the AC_CHECK_LIB()s for "ssl" and "crypto".
See "Step 3" of my changes to acinclude.m4.
- The adjustments made to LDFLAGS at the end of the testing has been
written to try and match the existing stuff, but I don't confess to
know what the significance of $ap_platform_runtime_link_flag is so I'm
working blind there.
- I'm tagging "-DHAVE_OPENSSL" or "-DHAVE_SSLC" directly onto CFLAGS
rather than using anything like AC_DEFINE because the latter
possibility would require HAVE_OPENSSL and HAVE_SSLC to be stubbed
into an appropriate "something.h.in" file. If you prefer not to have
such stuff polluting CFLAGS then please suggest an appropriate ".in"
file for me to hook into.
Note that (I think) the problems of relying on syntax like "-I", "-L",
etc all apply to the existing code anyway, so they're more questions of
style than whether my changes will work. The only things I can think of
that my changes my break are that I've dumped the builtin paths for
searching for headers and libraries and instead let autoconf probes
(combined with --with-ssl=<path>) find whatever they find. In other
words, perhaps some people might have been relying on the unconventional
anti-autoconf nature of the existing checks that my changes remove?
Any/all feedback most welcome.
Cheers,
Geoff
--
Geoff Thorpe
geoff@geoffthorpe.net
http://www.geoffthorpe.net/
Re: [PATCH] openssl configuration (v2)
Posted by Geoff Thorpe <ge...@geoffthorpe.net>.
Hi there,
* Justin Erenkrantz (justin@erenkrantz.com) wrote:
>
> Yes, the former is what I would expect as well. I would add a third clause
> to your AC_CHECK_LIB which appends -lLIBRARY to saved_LIBS. Hmm. I wonder
> where saved_LIBS will end up in relationship to AP_LIBS. Ah, it seems that
> EXTRA_LIBS will be before AP_LIBS - Solaris and other one-pass linkers
> should be happy.
Yeah I've changed this to manually add "-lssl -lcrypto" to LIBS after
it's restored from saved_LIBS and that makes things work. I also added a
missing AC_MSG_ERROR() if the libs weren't found, as this is clearly a
fatal SSL/TLS configuration error I should have been catching already.
So I guess we make do with that then? I know this is what is currently
done so I presume this is OK, though I admit to being less than pleased
with manually added linker flags given that we're using AC_CHECK_LIB()
which is supposed to hide all that (and handle whatever the correct
syntax is on the host).
NB: If it's later decided that AC_CHECK_LIB() should take care of adding
the linker flags internally, the save/restore trick with LIBS would have
to go, which in turn means that the other linker flags would have to be
configured in advance using AC_CHECK_LIB macros (rather than using
$apr_config --libs).
> >in portability problems. Unless someone tells me the Apache-Approved(tm)
> >way to do this, I'd rather just leave the comment there to guide someone
> >else if they feel so moved afterwards.
>
> ap_ssltk_version="`$p/openssl version`"
>
> I kinda like that approach (i.e. what we're currently doing...).
I'm not sure I like it but I'll go with the flow if you insist. The
thing is, it requires the openssl executable be installed which on
package-management systems will be the openssl package containing
programs, documents, scripts, certificates, etc. rather than just a
"libopenssl" package contains libs and headers. This also requires us to
test for the "openssl" program and work around the case that it's not
found (the current code seems to consider this fatal but that seems
dramatic under the circumstances). At the least, can we leave that as a
second step once the configuration is tidied up? Right now the version
check in configure is OK even if there's no obvious way to propogate a
text form of the version to the console. I can't see the implications of
requiring the presence of the openssl binary to be worth the effort for
a purely aesthetic issue.
> If you want to be cute, you could do something with AC_EGREP_HEADER, but
> I'm not totally clear what the syntax would be.
I took a look because this idea seemed promising at first look, but the
code generated by AC_EGREP_HEADER sends both stdout and stderr to
/dev/null, so the regexp seems useful only for a yes/no pattern-match -
I can't see any obvious way to capture what comes out of the egrep.
Perhaps there's another macro lurking somewhere that would solve all
this?
> >- Seeing as CPPFLAGS seems ideal for header *and* compilation checks, is
> > INCLUDES still the appropriate place to APR_ADDTO() any required
> > include path once the tests are done?
>
> Yeah, I thought about that after I sent my original message. We really
> should be adding the -I's to INCLUDES. But, autoconf will only temporarily
> use CPPFLAGS, so we should add to CPPFLAGS for the header checks, then if
> it works, add it to INCLUDES (which gets morphed to EXTRA_INCLUDES later
> on). Our build system should use INCLUDES properly. -- justin
OK, I'll stick with INCLUDES then. So AFAICS, if we accept the
following;
(1) explicitly adding "-lssl -lcrypto" into LIBS if the library checks
succeed (using APR_ADDTO),
(2) don't try to grep the version text yet for output from "configure"
nor assume the "openssl" binary to generate it,
(3) put include paths into INCLUDES (using APR_ADDTO)
then the patch attached to this mail should be OK? Does this seem
reasonable? (Note, I've still left the old version stubbed in - I prefer
to leave the diff readable until someone's ready to start using the word
"commit").
Cheers,
Geoff
--
Geoff Thorpe
geoff@geoffthorpe.net
http://www.geoffthorpe.net/
Re: [PATCH] openssl configuration (v2)
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Thursday, March 6, 2003 5:16 PM -0500 Geoff Thorpe
<ge...@geoffthorpe.net> wrote:
> then strangely the -lcrypto and -lssl checks report success *yet* they
> do not find their way into the linker flags (so apache fails to link
> after compiling). I can only imagine 2 reasons for this; either my
> restoring of LIBS to its original form obliterates the result of the
> AC_CHECK_LIB() macros, or the fact the linker flags that -lcrypto and
> -lssl depend have disappeared (and didn't occur in AC_CHECK_LIB() form)
> mean that autoconf does away with -lcrypto and -lssl some point later. I
> suspect the former as the latter seems just too weird a possibility.
Yes, the former is what I would expect as well. I would add a third clause to
your AC_CHECK_LIB which appends -lLIBRARY to saved_LIBS. Hmm. I wonder where
saved_LIBS will end up in relationship to AP_LIBS. Ah, it seems that
EXTRA_LIBS will be before AP_LIBS - Solaris and other one-pass linkers should
be happy.
> weren't being used. Can I assume then that I'm right to parrot the
> existing code's production of the additional LDFLAGS entry if that flag
> is set?
Yup.
> selective familiarity with. :-) Anyway, following your comments, I
> noticed that declaring the symbols in the top-level acconfig.h and
> running "./buildconf" eventually propogating those values into
> ap_config_auto.h.in and everything works. I'm happy with that, is it
> what you had in mind?
Yup.
> Yeah, but I'm leaving it like this while hashing out the patch details
> because it makes the diff readable (if you remove the old version and
> produce a diff, you'll see what I mean).
True. But, when it gets time to submit a final version of your patch, we need
the real version. =)
> I'm all for it, but would rather not guess as to the preferred mechanism
> for slurping that text out of the header file (especially as we're not
> supposed to assume anything about its location). I imagine the proper
> way is to have a compile test spit the string out and then capture it
> somehow. No doubt an autoconf-guru knows the relevant macro trick, but
> on my own I'm more likely to wildly overcomplicate this code and bring
> in portability problems. Unless someone tells me the Apache-Approved(tm)
> way to do this, I'd rather just leave the comment there to guide someone
> else if they feel so moved afterwards.
ap_ssltk_version="`$p/openssl version`"
I kinda like that approach (i.e. what we're currently doing...).
If you want to be cute, you could do something with AC_EGREP_HEADER, but I'm
not totally clear what the syntax would be.
> - Seeing as CPPFLAGS seems ideal for header *and* compilation checks, is
> INCLUDES still the appropriate place to APR_ADDTO() any required
> include path once the tests are done?
Yeah, I thought about that after I sent my original message. We really should
be adding the -I's to INCLUDES. But, autoconf will only temporarily use
CPPFLAGS, so we should add to CPPFLAGS for the header checks, then if it
works, add it to INCLUDES (which gets morphed to EXTRA_INCLUDES later on).
Our build system should use INCLUDES properly. -- justin
Re: [PATCH] openssl configuration (v2)
Posted by Geoff Thorpe <ge...@geoffthorpe.net>.
Hi Justin,
* Justin Erenkrantz (justin@erenkrantz.com) wrote:
>
> I think you mean adding -I<...> to CPPFLAGS not to CFLAGS. That should be
> portable and supported everywhere. It's a C preprocessor flag not a flag
> for the compiler. Autoconf will evaluate ac_compile which includes
> $CPPFLAGS. You should be able to use APR_ADDTO on CPPFLAGS for the explicit
> path case. (Since the user specified it, we always need to add it to
> CPPFLAGS.)
I was just getting ready to launch into a spiel about how none of the
documented flags seemed to be honoured by AC_CHECK_HEADERS and how I'd
looked at the generated configure script to see that cpp invocation
looked very different to compiler invocation, blah blah blah. Anyway, I
will spare you that spiel because having double checked I see that
you're absolutely right and I'm now wondering how I ended up so derailed
in the first place. I guess something else must have been broken whilst
I was trying that out and I never twigged. Anyway yes, putting the
include paths into CPPFLAGS makes header checking work. Thanks :-)
[snip]
> > Apache's builtin tests (which are obviously OK because Apache links
> > fine) should occur before the AC_CHECK_LIB()s for "ssl" and "crypto".
> > See "Step 3" of my changes to acinclude.m4.
[snip]
>
> A much easier route would be to include `$apr_config --libs` in that
> section. Those libraries would be in there. When we go to actually link,
> the libtool dependencies will have them there. But, you could add those to
> LIBS temporarily - just remove them when you are done trying to link.
Well, this is exactly what I was looking for. Unfortunately it seems not
to work as-is. What happens is that if I remove my explicit
AC_CHECK_LIB() calls for dl, socket, nsl, etc and do the following;
...
saved_LIBS=$LIBS
LIBS="$LIBS `$apr_config --libs`"
AC_CHECK_LIB(crypto, SSLeay_version)
AC_CHECK_LIB(ssl, SSL_CTX_new)
LIBS=$saved_LIBS
...
then strangely the -lcrypto and -lssl checks report success *yet* they
do not find their way into the linker flags (so apache fails to link
after compiling). I can only imagine 2 reasons for this; either my
restoring of LIBS to its original form obliterates the result of the
AC_CHECK_LIB() macros, or the fact the linker flags that -lcrypto and
-lssl depend have disappeared (and didn't occur in AC_CHECK_LIB() form)
mean that autoconf does away with -lcrypto and -lssl some point later. I
suspect the former as the latter seems just too weird a possibility.
> ap_platform_runtime_link_flag is '-R' on those platforms that need a
> special flag to indicate where to look at run-time for libraries. (Solaris
> is the predominate case.) Some platforms use '-rpath' as well. -L is not
> enough.
I figured it must be something of that sort from the existing code's
comments, thanks for clarifying. I mainly wanted to be sure it wasn't
producing something to compensate for the fact autoconf's own mechanisms
weren't being used. Can I assume then that I'm right to parrot the
existing code's production of the additional LDFLAGS entry if that flag
is set?
> >- I'm tagging "-DHAVE_OPENSSL" or "-DHAVE_SSLC" directly onto CFLAGS
> > rather than using anything like AC_DEFINE because the latter
> > possibility would require HAVE_OPENSSL and HAVE_SSLC to be stubbed
> > into an appropriate "something.h.in" file. If you prefer not to have
> > such stuff polluting CFLAGS then please suggest an appropriate ".in"
> > file for me to hook into.
>
> Why is having HAVE_OPENSSL or HAVE_SSLC in ap_config_auto.h a bad thing? I
> don't understand.
I didn't say it was a bad thing, on the contrary - I think it's a good
thing, I just wasn't sure which (if any) .in file would have been the
appropriate one. It's a big-ass tree of code that I have only very
selective familiarity with. :-) Anyway, following your comments, I
noticed that declaring the symbols in the top-level acconfig.h and
running "./buildconf" eventually propogating those values into
ap_config_auto.h.in and everything works. I'm happy with that, is it
what you had in mind?
> >-AC_DEFUN(APACHE_CHECK_SSL_TOOLKIT,[
> >+AC_DEFUN(APACHE_CHECK_SSL_TOOLKIT_OLD,[
>
> Just toss the old APACHE_CHECK_SSL_TOOLKIT. No need to keep the old
> version around.
Yeah, but I'm leaving it like this while hashing out the patch details
because it makes the diff readable (if you remove the old version and
produce a diff, you'll see what I mean).
> >+ AC_MSG_CHECKING(for OpenSSL version)
> >+ AC_TRY_COMPILE([#include <openssl/opensslv.h>],
> >+[#if !defined(OPENSSL_VERSION_NUMBER) || OPENSSL_VERSION_NUMBER <
> >0x0090609f
> >+#error "invalid openssl version"
> >+#endif],
> >+ [dnl Replace this with OPENSSL_VERSION_TEXT from opensslv.h?
> >+ AC_MSG_RESULT(OK)],
>
> Yes, it should indicate the version found somehow. I believe we did that
> before. I think it's worthwhile to have.
I'm all for it, but would rather not guess as to the preferred mechanism
for slurping that text out of the header file (especially as we're not
supposed to assume anything about its location). I imagine the proper
way is to have a compile test spit the string out and then capture it
somehow. No doubt an autoconf-guru knows the relevant macro trick, but
on my own I'm more likely to wildly overcomplicate this code and bring
in portability problems. Unless someone tells me the Apache-Approved(tm)
way to do this, I'd rather just leave the comment there to guide someone
else if they feel so moved afterwards.
> > dnl # hook module into the Autoconf mechanism (--enable-ssl option)
> > APACHE_MODULE(ssl, [SSL/TLS support (mod_ssl)], $ssl_objs, , no, [
> > APACHE_CHECK_SSL_TOOLKIT
> >- AC_CHECK_FUNCS(SSL_set_state)
> >- AC_CHECK_FUNCS(SSL_set_cert_store)
> >+ dnl These checks aren't really useful and could fail for silly
> >reasons
>
> Just remove them altogether.
Agreed.
> The rest of it looks good. -- justin
Thanks for the feedback. I've incorporated all the above and attached a
new patch. Does this seem OK? There are a couple of notes I want to
leave you with;
- I've incorporated the use of `$apr_config --libs` as you suggested and
so my patch is currently broken, but I want to head in the right
direction. Any ideas why the successfully reported -lssl -lcrypto
flags disappear from the generated Makefile's?
- I've moved the version checks into the header check steps so I don't
have to save and restore CPPFLAGS twice.
- Seeing as CPPFLAGS seems ideal for header *and* compilation checks, is
INCLUDES still the appropriate place to APR_ADDTO() any required
include path once the tests are done?
Cheers,
Geoff
--
Geoff Thorpe
geoff@geoffthorpe.net
http://www.geoffthorpe.net/
Re: [PATCH] openssl configuration (v2)
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Tuesday, March 4, 2003 6:43 PM -0500 Geoff Thorpe <ge...@geoffthorpe.net>
wrote:
> Questions for apache gurus/code-reviewers;
>
> - AC_CHECK_HEADERS() appears difficult to coax into accepting additional
> include paths, so if "--with-ssl=<path>" is specified there appears no
> obvious way to have AC_CHECK_HEADERS() pick up those headers in
> (particularly if versions exist in system locations too and we want
> autoconf's tests to find the <path> versions in preference to any
> auto-detectable ones). I've left some comments in the acinclude.m4
> changes about this. For now, I've made do with adding "-I<...>" to
> CFLAGS prior to AC_TRY_COMPILE, but I'm sure autoconf intended some
> other way of handling this. For one thing, is "-I" actually portable
> anyway? The existing code depends utterly on it but it would be nice
> to do away with it altogether.
I think you mean adding -I<...> to CPPFLAGS not to CFLAGS. That should be
portable and supported everywhere. It's a C preprocessor flag not a flag for
the compiler. Autoconf will evaluate ac_compile which includes $CPPFLAGS.
You should be able to use APR_ADDTO on CPPFLAGS for the explicit path case.
(Since the user specified it, we always need to add it to CPPFLAGS.)
> - My changes use autoconf tests for openssl/ssl-c headers and libraries
> (existing code just looks for files but doesn't actually try to use
> them). As a result, linker flags like -ldl, -lsocket, -lnsl, -ldld,
> etc are needed in advance of these tests. I've added the obvious ones
> I know about so that this patch can be tested as-is, but ideally
> Apache's builtin tests (which are obviously OK because Apache links
> fine) should occur before the AC_CHECK_LIB()s for "ssl" and "crypto".
> See "Step 3" of my changes to acinclude.m4.
A much easier route would be to include `$apr_config --libs` in that section.
Those libraries would be in there. When we go to actually link, the libtool
dependencies will have them there. But, you could add those to LIBS
temporarily - just remove them when you are done trying to link.
> - The adjustments made to LDFLAGS at the end of the testing has been
> written to try and match the existing stuff, but I don't confess to
> know what the significance of $ap_platform_runtime_link_flag is so I'm
> working blind there.
ap_platform_runtime_link_flag is '-R' on those platforms that need a special
flag to indicate where to look at run-time for libraries. (Solaris is the
predominate case.) Some platforms use '-rpath' as well. -L is not enough.
> - I'm tagging "-DHAVE_OPENSSL" or "-DHAVE_SSLC" directly onto CFLAGS
> rather than using anything like AC_DEFINE because the latter
> possibility would require HAVE_OPENSSL and HAVE_SSLC to be stubbed
> into an appropriate "something.h.in" file. If you prefer not to have
> such stuff polluting CFLAGS then please suggest an appropriate ".in"
> file for me to hook into.
Why is having HAVE_OPENSSL or HAVE_SSLC in ap_config_auto.h a bad thing? I
don't understand.
> Any/all feedback most welcome.
Comments inline.
> Index: acinclude.m4
> ===================================================================
> RCS file: /home/cvspublic/httpd-2.0/acinclude.m4,v
> retrieving revision 1.136
> diff -u -r1.136 acinclude.m4
> --- acinclude.m4 17 Feb 2003 02:32:19 -0000 1.136
> +++ acinclude.m4 4 Mar 2003 23:00:03 -0000
> @@ -320,7 +320,7 @@
> dnl and then AC_TRY_LINK to test the libraries directly for the version,
> dnl but that will require someone who knows how to program openssl.
> dnl
> -AC_DEFUN(APACHE_CHECK_SSL_TOOLKIT,[
> +AC_DEFUN(APACHE_CHECK_SSL_TOOLKIT_OLD,[
> if test "x$ap_ssltk_base" = "x"; then
> AC_MSG_CHECKING(for SSL/TLS toolkit base)
> ap_ssltk_base=""
Just toss the old APACHE_CHECK_SSL_TOOLKIT. No need to keep the old version
around.
> + dnl Step 5: run version checks
> + if test "$ap_ssltk_type" = "openssl"; then
> + saved_CFLAGS=$CFLAGS
> + if test "x$ap_ssltk_inc" != "x"; then
> + CFLAGS="$CFLAGS $ap_ssltk_inc"
> + fi
> + AC_MSG_CHECKING(for OpenSSL version)
> + AC_TRY_COMPILE([#include <openssl/opensslv.h>],
> +[#if !defined(OPENSSL_VERSION_NUMBER) || OPENSSL_VERSION_NUMBER < 0x0090609f
> +#error "invalid openssl version"
> +#endif],
> + [dnl Replace this with OPENSSL_VERSION_TEXT from opensslv.h?
> + AC_MSG_RESULT(OK)],
Yes, it should indicate the version found somehow. I believe we did that
before. I think it's worthwhile to have.
> Index: modules/ssl/config.m4
> ===================================================================
> RCS file: /home/cvspublic/httpd-2.0/modules/ssl/config.m4,v
> retrieving revision 1.11
> diff -u -r1.11 config.m4
> --- modules/ssl/config.m4 29 Mar 2002 07:36:01 -0000 1.11
> +++ modules/ssl/config.m4 4 Mar 2003 23:00:05 -0000
> @@ -77,8 +77,13 @@
> dnl # hook module into the Autoconf mechanism (--enable-ssl option)
> APACHE_MODULE(ssl, [SSL/TLS support (mod_ssl)], $ssl_objs, , no, [
> APACHE_CHECK_SSL_TOOLKIT
> - AC_CHECK_FUNCS(SSL_set_state)
> - AC_CHECK_FUNCS(SSL_set_cert_store)
> + dnl These checks aren't really useful and could fail for silly reasons
if
> + dnl ever the flags configured by APACHE_CHECK_SSL_TOOLKIT aren't in
> + dnl effect when these checks run (but are in effect during apache
> + dnl compilation). The version checks on openssl already make sure the
> + dnl below functions exist anyway.
> + dnl AC_CHECK_FUNCS(SSL_set_state)
> + dnl AC_CHECK_FUNCS(SSL_set_cert_store)
> ])
Just remove them altogether.
The rest of it looks good. -- justin