Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Date: Sun, 25 May 2003 15:46:16 -0400
From: Geoff Thorpe <geoff@geoffthorpe.net>
Subject: Re: cvs commit: httpd-2.0/modules/ssl ...
In-reply-to: <5.2.0.9.2.20030524075137.01e53128@pop3.rowe-clan.net>
To: dev@httpd.apache.org
Cc: "William A. Rowe, Jr." <wrowe@rowe-clan.net>
Message-id: <200305251546.16597.geoff@geoffthorpe.net>
MIME-version: 1.0
Content-type: text/plain; charset=iso-8859-1
Content-transfer-encoding: 7BIT
Content-disposition: inline
User-Agent: KMail/1.5
References: <5.2.0.9.2.20030519113825.03e3dea8@pop3.rowe-clan.net>
 <5.2.0.9.2.20030524075137.01e53128@pop3.rowe-clan.net>

Hello,

On May 24, 2003 09:38 am, William A. Rowe, Jr. wrote:
> At 02:31 PM 5/19/2003, Geoff Thorpe wrote:
> >The support I coded up used autoconfisms for testing the various
> > things, AC_CHECK_HEADER(S), AC_CHECK_LIB, etc. The previous support
> > simply scanned hard-coded paths and did various shell-based
> > examinations to *find* header and lib paths. My reading at the time
> > of those
> >AC_CHECK_FUNCS macros in modules/ssl/config.m4 was that they existed
> >merely to provide some verification that what had been "found"
> > actually worked. If there was any other significance to them, it was
> > too subtle for me. :-)
>
> Well, in the SSLC case, HAVE_SSL_SET_STATE was required because
> you can't even renegotiate without toggling the state.  I tried a
> pretty simple hack, replacing the set state code (which on OpenSSL is
> simply a bit tweak into what should be opaque data) with the
> SSL_renegotate() function across toolkits (with the patch applied to
> 2_0_BRANCH).
>
> That didn't work, as several of the perl-framework ssl/ tests started
> failing. Considering that SSL_set_state a source patch on top of the
> SSLC toolkit, and most SSLC users work from binary distributions, this
> was never clean in the first place.  Since ApacheSSL uses the
> SSL_renegotiate() function, and it's support in both toolkits, this is
> the right direction, I just missed something important (which I will
> revisit as I find some cycles again.)

OK, I can't say anything of much usefulness about SSL-C as I have no 
experience with it. What I would suggest is that clean SSL-C support in 
Apache2 may be useful to many people, but there are none who benefit from 
it quite so much as RSA themselves. IIRC, it was from RSA that the 
original mod_ssl tweaks for SSL-C originated - so it begs the question; 
are RSA involved in maintaining this code at all? It seems they should be 
doing as much as possible to avoid bitrot in the SSL-C hooks, and they'd 
be far better placed than anyone to make it happen (and test it)?

> In the HAVE_SSL_CERT_STORE, that's an add-in patch on any toolkit.
> because we have only a single SSL_CTX*, one can't toggle the CA within
> the per-dir context within a threaded server.  With SSL_cert_store, one
> can then toggle the CA within each SSL*.  So this too was not a 'noop.'

OK, yep - I see this now.

> The choice of checking HAVE_ENGINE_INIT over forcing the user to
> toggle the experminental-foo should be pretty obvious.

Sure, I only question how the presence of ENGINE support should be 
detected - it's pretty clear that having experimental flags all over the 
place defeats both autoconf and common sense.

> >Because that's what was happening for me when I was writing the
> > autoconf code to begin with. I had to start saving and restoring
> > variables so that I could mangle them while testing (eg. pulling in
> > the apr linker flags temporarily). Running further ssl tests outside
> > that save/restore macro block before the rest of configuration
> > process is complete seems more fragile to my eye [...]
>
> Yup - I can see the benefit.  But it's fundamentally broken autoconf
> style if we leave invalid flags sitting around.  Sure, we need to add
> -l references to libraries that don't yet exist.  But I don't know that
> we want to do that before the autodetection is finished?

I totally agree here - as I said way back when I was hacking on this, the 
ideal thing would be that those "low level" flags already be configured 
before we go bumbling into the ssl configuration. However, my 
self-imposed terms of reference were confined to the ssl macro as I don't 
pretend to understand the whole configuration system (particularly w.r.t. 
apr). You're right that this workaround may be ok inside the save/restore 
block but that it results in incomplete flags being configured - and this 
is what subsequent AC_CHECK_FUNC(SSL_**) checks can stub their toes on. 
That this problem was already present in the previous ssl configuration 
code made me feel less bad about not correcting it :-)

> In any case, we won't 'HAVE' ENGINE_init if we don't inject ssl libs,
> so I don't have any problem moving those detection macros into the
> 'protected' reconfiguration.  That still leaves me with the question of
> how we can test on a platform like solaris, where OpenSSL needs
> libraries like -lsocket just to perform a test compile of
> ENGINE_init().

Well this works inside the protected configuration for the reason that it 
pulls in the apr linker flags, whatever they may be (eg. -ldl -lsocket 
-lnsl [etc]). But the better solution is to re-arrange the configuration 
order so the dependencies are already satisfied.

> >I merely wanted to draw your attention to the changes I'd made before,
> >as the revert and changes you made did not show whether or not the
> >previous changes were on your radar.
>
> I was aware of them (I'm working in both trees) but no, I hadn't
> studied the earlier discussion/history.  I'm proceeding to do so.  But
> I'm not really keen on the operating philisophy 'the commit broke
> something, back it out!' I'm trying to work forward from the effort you
> and Madhu invested.
>
> So the simple answer is yes - we must support autoconf checks (but we
> can likely eliminate one of the existing ones) and your patch broke it.
> What is most disturbing is the fact that the committed code ignored
> the last paragraph of the following earlier thread;
>
> >Date: Wed, 12 Mar 2003 23:03:15 -0600
> >To: dev@httpd.apache.org
>
> From: "William A. Rowe, Jr." <wrowe@rowe-clan.net>
>
> >Subject: RE: [PATCH] openssl configuration (v2)
> >
> >Madhu, I really like the gist of this patch.
> >
> >I'm trying to get rolling 2.0.45 out the door, so I haven't had time
> > to watch your conversation.  I have my own toolkit patch for SSL-C
> > 2.3 that probably breaks earlier SSL-C flavors.  This is why I
> > haven't had time to investigate/respond - spending too much time in
> > firefighting mode :-/ [...]
> >But we cannot quit testing for the SSL_set_state sort of functions (I
> > don't know if you did or not) because I for one enjoy forcing SSL-C
> > to do what it should have done in the first place :-)  Please don't
> > clobber function detection in your final patch, please!

Well this certainly slipped through the cracks - apologies for my part, I 
don't have SSL-C so I was largely flying blind in that respect.

> >> >I'd suggest that the ENGIEN test be inserted into
> >> >APACHE_CHECK_SSL_TOOLKIT macro,
> >
> >Cool. If nothing else, that'd probably keep things more self-contained
> > and with less potential for surprises.
>
> We can move the existing SSL-C tests that were removed back into the
> SSLC path of the config section, and the ENGINE_init into the OpenSSL
> branch of the code.  (SSLC has an entirely different engine schema.)
>
> This solution still gives me grief though, when I try to build for
> static -lssl and -lcrypto on OpenSSL 0.9.7 on Solaris.  The installed
> lib/pkgconfing/ file openssl.pc tells us exactly what libraries we
> should be adding, and even goes as far as to spell out the other
> goodies we are 'autodetecting'.
>
> Perhaps we should go ahead and use the pkgconfig if it exists, or fall
> back on our own autoconfig if it doesn't.  Would that be a win?

Maybe, but I think elegance dictates that the first priority would be to 
get the existing configuration steps ordered by dependency. The fact that 
apache-2 compiles and links indicates that there are no missing linker 
flags missing once the configuration has completed - it's only during 
configuration itself that there are one or two moments when the state of 
the flags can be left inconsistent. Later, if you then want to detect and 
add some logic to pull flags out of openssl.pc (and/or any equivalent for 
SSL-C), you could presumably use some macro to merge them in and catch 
any necessary flags you might otherwise be missing.

> >Well the previous --with-ssl=... logic was broken which is why I ended
> > up hacking in this code to begin with. I'm fully aware of the need to
> > support building against arbitrary SSL installations (and/or
> > uninstalled build-trees). I'm not sure that AC_CHECK_FUNC() is more
> > resistant against inadvertently linking with unintended libraries
> > than AC_CHECK_HEADER() is with unintended headers.
>
> Huh?  Of course it does, silly... consider the following, where ENGINE
> only exists in the system path, but we request to use the myssl flavor
> that does not support ENGINE...
>
> /usr/myssl/openssl/include/openssl/openssl.h
> /usr/myssl/openssl/lib/libssl.a  {missing ENGINE_init}
>
> /usr/include/openssl/openssl.h
> /usr/include/openssl/engine.h
> /usr/lib/libssl.a  {containing ENGINE_init}
>
> Now if you test HAVE_ENGINE_H, of course it will find engine.h. 
> However, if you test compile for ENGINE_init, against -lssl, it will
> link against only the FIRST libssl and then try compiling for that
> function.  It won't try binding to more than one of any given library!

Well yes, but as I pointed out - if a future version redefines ENGINE_init 
as a backward compatible macro (or simply removes it, heaven forbid) then 
you're still going to hurt, whether the autoconf test fails or not. In 
this case, it seems you want AC_TRY_LINK. But that's perhaps a bit 
pedantic of me. I'll drop this one, your call :-)

> Again, I think we are setting up some -l's just a little too early in
> our config scripts, but that's a bigger issue.  For today I agree.

Yes, I think this is the basic problem. The saving/restoring tricks used 
in the ssl config macro (and in loads of other typical autoconf usage) is 
a valid approach for trying certain things out with the facitilty to 
rollback. What one should *not* do is use such trickery to temporarily 
correct a problem in the way the configuration process is organised, and 
unfortunately that's the case here.

> >Well, if you want to stick with that, go ahead. You'll still need to
> > check for the ENGINE header anyway because you need to know whether
> > to include it or not. As I say, API compatibility doesn't require
> > that ENGINE_init() remains a function, only that it remains defined
> > in the headers. For the short term, you're safe one way or the other.
>
> As I say, on linux this worked.  On solaris, we still need the extra
> libs. This is why I'm like to attack the pkgconfig method instead, when
> that openssl.pc datum file is available to us.

Well I agree openssl.pc may be useful down the line to make the whole 
configuration more robust for the future. However the fact apache links 
on solaris today (and on oodles of other platforms as well) means you're 
not missing any linker flags, they're just not being assembled in the 
right order. Pulling in openssl's linker dependencies in-place using the 
current apr trickery, or doing the same thing via some external mechanism 
like openssl.pc, is a band-aid on top of the real problem - namely that 
openssl/ssl-c support should be configured after its dependencies rather 
than before. The question is, is it actually *possible* to re-arrange 
this given the current configuration system?

Cheers,
Geoff

-- 
Geoff Thorpe
geoff@geoffthorpe.net
http://www.geoffthorpe.net/