You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by fu...@apache.org on 2007/12/21 14:16:21 UTC
svn commit: r606190 - in /httpd/httpd/trunk: CHANGES
modules/ssl/ssl_engine_init.c modules/ssl/ssl_engine_kernel.c
modules/ssl/ssl_toolkit_compat.h
Author: fuankg
Date: Fri Dec 21 05:16:21 2007
New Revision: 606190
URL: http://svn.apache.org/viewvc?rev=606190&view=rev
Log:
Added server name indication (RFC 4366) support (PR 34607).
Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c
httpd/httpd/trunk/modules/ssl/ssl_toolkit_compat.h
Modified: httpd/httpd/trunk/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=606190&r1=606189&r2=606190&view=diff
==============================================================================
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Fri Dec 21 05:16:21 2007
@@ -2,6 +2,9 @@
Changes with Apache 2.3.0
[ When backported to 2.2.x, remove entry from this file ]
+ *) mod_ssl: Added server name indication support (RFC 4366).
+ PR 34607. [Kaspar Brand <asfbugz velox.ch>]
+
*) ApacheMonitor.exe: Introduce --kill argument for use by the
installer. This will permit the installation tool to remove
all running instances before attempting to remove the .exe.
Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_init.c?rev=606190&r1=606189&r2=606190&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/ssl/ssl_engine_init.c (original)
+++ httpd/httpd/trunk/modules/ssl/ssl_engine_init.c Fri Dec 21 05:16:21 2007
@@ -135,6 +135,87 @@
return OK;
}
+#ifndef OPENSSL_NO_TLSEXT
+static int set_ssl_vhost(void *servername, conn_rec *c, server_rec *s)
+{
+ SSLSrvConfigRec *sc;
+ SSL *ssl;
+ BOOL found = FALSE;
+ apr_array_header_t *names;
+ int i;
+
+ /* check ServerName */
+ if (!strcasecmp(servername, s->server_hostname))
+ found = TRUE;
+
+ /* if not matched yet, check ServerAlias entries */
+ if (!found) {
+ names = s->names;
+ if (names) {
+ char **name = (char **) names->elts;
+ for (i = 0; i < names->nelts; ++i) {
+ if(!name[i]) continue;
+ if (!strcasecmp(servername, name[i])) {
+ found = TRUE;
+ break;
+ }
+ }
+ }
+ }
+
+ /* if still no match, check ServerAlias entries with wildcards */
+ if (!found) {
+ names = s->wild_names;
+ if (names) {
+ char **name = (char **) names->elts;
+ for (i = 0; i < names->nelts; ++i) {
+ if(!name[i]) continue;
+ if (!ap_strcasecmp_match(servername, name[i])) {
+ found = TRUE;
+ break;
+ }
+ }
+ }
+ }
+
+ /* set SSL_CTX (if matched) */
+ if (found) {
+ if ((ssl = ((SSLConnRec *)myConnConfig(c))->ssl) == NULL)
+ return 0;
+ if (!(sc = mySrvConfig(s)))
+ return 0;
+ SSL_set_SSL_CTX(ssl,sc->server->ssl_ctx);
+ return 1;
+ }
+ return 0;
+}
+
+int ssl_set_vhost_ctx(SSL *ssl, const char *servername)
+{
+ conn_rec *c;
+
+ if (servername == NULL) /* should not occur. */
+ return 0;
+
+ SSL_set_SSL_CTX(ssl,NULL);
+
+ if (!(c = (conn_rec *)SSL_get_app_data(ssl)))
+ return 0;
+
+ return ap_vhost_iterate_given_conn(c,set_ssl_vhost,servername);
+}
+
+int ssl_servername_cb(SSL *s, int *al, modssl_ctx_t *mctx)
+{
+ const char *servername = SSL_get_servername(s,TLSEXT_NAMETYPE_host_name);
+
+ if (servername) {
+ return ssl_set_vhost_ctx(s,servername)?SSL_TLSEXT_ERR_OK:SSL_TLSEXT_ERR_ALERT_FATAL;
+ }
+ return SSL_TLSEXT_ERR_NOACK;
+}
+#endif
+
/*
* Per-module initialization
*/
@@ -355,6 +436,29 @@
}
}
+static void ssl_init_server_extensions(server_rec *s,
+ apr_pool_t *p,
+ apr_pool_t *ptemp,
+ modssl_ctx_t *mctx)
+{
+ /*
+ * Configure TLS extensions support
+ */
+
+#ifndef OPENSSL_NO_TLSEXT
+ ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
+ "Configuring TLS extensions facility");
+
+ if (!SSL_CTX_set_tlsext_servername_callback(mctx->ssl_ctx, ssl_servername_cb) ||
+ !SSL_CTX_set_tlsext_servername_arg(mctx->ssl_ctx, mctx)) {
+ ap_log_error(APLOG_MARK, APLOG_ERR, 0, s,
+ "Unable to initialize servername callback, bad openssl version.");
+ ssl_log_ssl_error(APLOG_MARK, APLOG_ERR, s);
+ ssl_die();
+ }
+#endif
+}
+
static void ssl_init_ctx_protocol(server_rec *s,
apr_pool_t *p,
apr_pool_t *ptemp,
@@ -712,6 +816,8 @@
/* XXX: proxy support? */
ssl_init_ctx_cert_chain(s, p, ptemp, mctx);
}
+
+ ssl_init_server_extensions(s, p, ptemp, mctx);
}
static int ssl_server_import_cert(server_rec *s,
@@ -1038,6 +1144,7 @@
}
}
+#ifdef OPENSSL_NO_TLSEXT
/*
* Give out warnings when more than one SSL-aware virtual server uses the
* same IP:port. This doesn't work because mod_ssl then will always use
@@ -1082,6 +1189,7 @@
"Init: You should not use name-based "
"virtual hosts in conjunction with SSL!!");
}
+#endif
}
#ifdef SSLC_VERSION_NUMBER
Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c?rev=606190&r1=606189&r2=606190&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c (original)
+++ httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c Fri Dec 21 05:16:21 2007
@@ -297,6 +297,19 @@
* the currently active one.
*/
+#ifndef OPENSSL_NO_TLSEXT
+ /*
+ * We will switch to another virtualhost and to its ssl_ctx
+ * if changed, we will force a renegotiation.
+ */
+ if (r->hostname && !SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name)) {
+ SSL_CTX *ctx = SSL_get_SSL_CTX(ssl);
+ if (ssl_set_vhost_ctx(ssl,(char *)r->hostname) &&
+ ctx != SSL_get_SSL_CTX(ssl))
+ renegotiate = TRUE;
+ }
+#endif
+
/*
* Override of SSLCipherSuite
*
@@ -1063,6 +1076,9 @@
SSLDirConfigRec *dc = myDirConfig(r);
apr_table_t *env = r->subprocess_env;
char *var, *val = "";
+#ifndef OPENSSL_NO_TLSEXT
+ const char* servername;
+#endif
STACK_OF(X509) *peer_certs;
SSL *ssl;
int i;
@@ -1088,6 +1104,13 @@
*/
/* the always present HTTPS (=HTTP over SSL) flag! */
apr_table_setn(env, "HTTPS", "on");
+
+#ifndef OPENSSL_NO_TLSEXT
+ /* add content of SNI TLS extension (if supplied with ClientHello) */
+ if (servername = SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name)) {
+ apr_table_set(env, "SSL_TLS_SNI", servername);
+ }
+#endif
/* standard SSL environment variables */
if (dc->nOptions & SSL_OPT_STDENVVARS) {
Modified: httpd/httpd/trunk/modules/ssl/ssl_toolkit_compat.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_toolkit_compat.h?rev=606190&r1=606189&r2=606190&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/ssl/ssl_toolkit_compat.h (original)
+++ httpd/httpd/trunk/modules/ssl/ssl_toolkit_compat.h Fri Dec 21 05:16:21 2007
@@ -270,6 +270,12 @@
#define SSL_SESS_CACHE_NO_INTERNAL SSL_SESS_CACHE_NO_INTERNAL_LOOKUP
#endif
+#ifndef OPENSSL_NO_TLSEXT
+#ifndef SSL_CTRL_SET_TLSEXT_HOSTNAME
+#define OPENSSL_NO_TLSEXT
+#endif
+#endif
+
#endif /* SSL_TOOLKIT_COMPAT_H */
/** @} */
Re: svn commit: r606190 - in /httpd/httpd/trunk:
CHANGESmodules/ssl/ssl_engine_init.c
modules/ssl/ssl_engine_kernel.cmodules/ssl/ssl_toolkit_compat.h
Posted by Guenter Knauf <fu...@apache.org>.
Hi Joe,
thanks for your review and your detailed comments.
> Commits containing changes authored by someone other than the committer
> should have a "Submitted by: " line giving appropriate credit.
hmm, you mean with the SVN log? Sorry, forgot there - but it appears in CHANGES.
> This patch introduces the following warnings for a build with
> OPENSSL_NO_TLSEXT undefined.
I believe I've fixed these now - sorry but I didnt test on Linux yet;
and with the NetWare compiler its not possible to develop with all warnings enabled.
Please let me know if I dodnt catch them all.
> My main comment: it's not obvious to me how this actually does what it
> needs to. The issue with getting true SNI support is that the hostname
> sent in the ClientHello must cause a change to the selection of
> r->server early enough to ensure that any security policy specified in
> the vhost configuration is applied correctly (e.g. SSLVerifyClient) -
> not merely selection of the correct SSL_CTX (and hence server cert). I
> can't see how this patch achieves that. Any clues? Has a configuration
> with an SSLVerifyClient specified in the named vhost been tested?
I must admit that I didnt think of this;
I tested for the server certs only, and for this it works fine - the cert/name mismatch warning is gone.
> 1) There are numerous code style issues. Please ensure new code added
> meets the style guide: http://httpd.apache.org/dev/styleguide.html
I tried now to meet the style guide, but at some places I was unsure - let me know what I missed.
> 2) Please don't introduce new global functions named "ssl_". Use
> "modssl_".
hmmm, looking at all other functions in ssl_private.h the only two which are prefixed with "modssl_" are those two you recently introduced with OCSP support, all others are prefixed with "ssl_" only; so why you want this?
> 3) Calling an env var "SSL_TLS_..." is redundant;
I prefixed it with "SSL_" since all mod_ssl vars are refixed with that...
> why is an extra env var necessary here anyway?
probably not - do you think I should remove it?
> 4) Why is it desirable to send a fatal TLS alert if no vhost is found
> matching the hostname in the clienthello?
what would you suggest there?
>> + /*
>> + * We will switch to another virtualhost and to its ssl_ctx
>> + * if changed, we will force a renegotiation.
>> + */
> That sentence doesn't make sense.
changed - let me know if that'S now better.
>> + if (r->hostname && !SSL_get_servername(ssl,
>> TLSEXT_NAMETYPE_host_name)) {
>> + SSL_CTX *ctx = SSL_get_SSL_CTX(ssl);
> That shadows a variable of the same name at function scope.
removed - from what I see it was anyway obsolete: some lines above we do already set the ctx if ssl is not NULL,
and if ssl is NULL we return before we can reach this code, so I think at this place ctx should already been set, or?
>> + if (ssl_set_vhost_ctx(ssl,(char *)r->hostname) &&
> Why is the cast needed?
removed.
>> @@ -712,6 +816,8 @@
>> /* XXX: proxy support? */
>> ssl_init_ctx_cert_chain(s, p, ptemp, mctx);
>> }
>> +
>> + ssl_init_server_extensions(s, p, ptemp, mctx);
>> }
> This is being called for both a server context and a proxy context, it
> is not necessary for the latter.
moved up so that its only called for server now.
> This warning has no less weight given the presence of SNI support in the
> version of OpenSSL against which mod_ssl is built. Many (probably most)
> deployed clients do not support SNI and hence cannot support name-based
> vhosting with SSL. I don't think this should be omitted, merely
> reworded.
hmmmm, this is where I didnt change yet - what would you propose here?
thanks again for your comments.
Guen.
Re: svn commit: r606190 - in /httpd/httpd/trunk: CHANGES modules/ssl/ssl_engine_init.c
modules/ssl/ssl_engine_kernel.c modules/ssl/ssl_toolkit_compat.h
Posted by Kaspar Brand <ht...@velox.ch>.
Kaspar Brand wrote:
>> Has a configuration
>> with an SSLVerifyClient specified in the named vhost been tested?
>
> Yes, and one specific configuration actually made me tweak the code in
> the servername callback further: [...]
It turns out that this change was too radical, actually - it prevented
per-directory specific SSLVerifyClient settings [1] from working
correctly. I have updated the patch to fix this, as shown by the
attached interdiff (against the version attached to
http://mail-archives.apache.org/mod_mbox/httpd-dev/200801.mbox/%3c477BBD86.9020600@velox.ch%3e).
The complete patch against trunk is attached to PR 34607
(http://issues.apache.org/bugzilla/show_bug.cgi?id=34607):
http://issues.apache.org/bugzilla/attachment.cgi?id=21365
And a proposed backport for 2.2.x is available at:
http://sni.velox.ch/httpd-2.2.x-sni.diff
These versions should fix the remaining issues I'm aware of, so it would
be great if there's a chance to consider this for inclusion in 2.2.8.
Please let me know if there's anything else I can do to help with this
process.
Thanks,
Kaspar
[1] I've also verified that other per-directory mod_ssl settings, such
as SSLCipherSuite, will work as expected, e.g. with a configuration like
the following:
NameVirtualHost *:443
<VirtualHost *:443>
ServerName www1.example.net
SSLCertificateFile www1.crt
SSLCertificateKeyFile www1.key
SSLCipherSuite ALL
</VirtualHost>
<VirtualHost *:443>
ServerName www2.example.net
SSLCertificateFile www2.crt
SSLCertificateKeyFile www2.key
SSLCACertificateFile my_ca.crt
SSLVerifyClient optional_no_ca
SSLCipherSuite MEDIUM:HIGH
<Location /topsecret>
SSLVerifyClient require
SSLCipherSuite HIGH
</Location>
</VirtualHost>
In this case, more restrictive requirements are enforced for
www2.example.net (at least 128-bit ciphers, client auth optional), and
even more restrictive settings for www2.example.net/topsecret (stronger
ciphers, and mandatory client auth).
Re: svn commit: r606190 - in /httpd/httpd/trunk: CHANGES modules/ssl/ssl_engine_init.c
modules/ssl/ssl_engine_kernel.c modules/ssl/ssl_toolkit_compat.h
Posted by Kaspar Brand <ht...@velox.ch>.
> If I understand correctly, the current code is relying on
> ssl_hook_Access to perform a *second* handshake which will ensure that
> the named vhost's access control configuration is enforced, if
> necessary?
No, that's probably due to a misunderstanding. In its current form, the
patch will set the correct SSLVerify parameters for the named vhost very
early in the initial TLS handshake (in ssl_find_vhost, which is called
by ap_vhost_iterate_given_conn). If "SSLVerifyClient require" is
configured at the vhost level and there's no certificate from the peer,
then the initial handshake will already fail (as you would expect).
A second handshake (triggered by ssl_hook_Access) only takes place if
there are per-directory specific settings (SSLVerifyClient, but also
SSLCipher) which differ from the per-server setting. Maybe the following
- somewhat artificial - pseudo config helps to illustrate the difference:
<VirtualHost *:443>
# Server 1
SSLVerifyClient optional_no_ca
</VirtualHost>
<VirtualHost *:443>
# Server 2
SSLVerifyClient require
</VirtualHost>
<VirtualHost *:443>
# Server 3
<Location />
SSLVerifyClient require
</Location>
</VirtualHost>
For server 2, there will only be one handshake, and it will fail right
after the CertificateRequest if the peer doesn't supply a valid client
cert. For server 3 (only shown for illustration purposes, doesn't make
much sense in a real-word config), a second handshake is triggered by
ssl_hook_Access, as the SSLVerifyClient parameter is a per-directory
setting in this case (there's an implied "SSLVerifyClient none" default
setting at the vhost level, which is used during the initial handshake).
> it should be done in the initial handshake,
> since it theoretically can - and requiring a second handshake invokes
> the spectre of PR 12355 and associated problems.
Absolutely agreed. That's exactly what the SSL_set_verify call in
ssl_find_vhost is for (which I added after your previous comments, this
was indeed a problem with the old version of the patch).
I hope this helps to clear up some of the confusion - let me know if you
still see issues with the current implementation.
Thanks,
Kaspar
Re: svn commit: r606190 - in /httpd/httpd/trunk: CHANGES
modules/ssl/ssl_engine_init.c modules/ssl/ssl_engine_kernel.c
modules/ssl/ssl_toolkit_compat.h
Posted by Joe Orton <jo...@redhat.com>.
Thanks for the detailed response and sorry for the slow follow-up,
Kaspar. I'll just deal with the main issue for the time being:
On Wed, Jan 02, 2008 at 05:36:22PM +0100, Kaspar Brand wrote:
> > Has a configuration
> > with an SSLVerifyClient specified in the named vhost been tested?
>
> Yes, and one specific configuration actually made me tweak the code in
> the servername callback further: when modifying the SSL connection,
> OpenSSL's SSL_set_SSL_CTX() will only adjust the server cert from the
> context, but not additional settings like verify_mode and the verify
> callback. These are relevant when SSLVerifyClient is configured at the
> *per-server* context (i.e. at the vhost level), and the previous version
> of the patch failed to enforce such a configuration, at least in cases
> where SSLVerifyClient for the first (=default) VirtualHost was different
> than for any subsequent VirtualHosts.
This still seems like a big issue to me. There are other settings in
the SSL_CTX which are relevant to the initial handshake - and indeed,
later renegotiations - notably the CA used for client cert verification,
and the list of CA cert names which is sent to the client (to allow
selection of a pertinent client cert).
If I understand correctly, the current code is relying on
ssl_hook_Access to perform a *second* handshake which will ensure that
the named vhost's access control configuration is enforced, if
necessary?
This seems like a hack - it should be done in the initial handshake,
since it theoretically can - and requiring a second handshake invokes
the spectre of PR 12355 and associated problems.
I don't think it's even a sufficient hack: ssl_hook_Access will not
check for situations where per-vhost settings have changed from the
initial handshake (since without SNI, this cannot occur). e.g. for a
config where the initial vhost has a different SSLCACertificate* to the
named vhost, and 'SSLVerifyClient require' in both; ssl_hook_Access will
do nothing.
(It may seem pedantic to bring up pathological config examples like
this, but silently failing to implement the configured access control,
however crazy, is certainly a security issue.)
joe
Re: svn commit: r606190 - in /httpd/httpd/trunk: CHANGES modules/ssl/ssl_engine_init.c
modules/ssl/ssl_engine_kernel.c modules/ssl/ssl_toolkit_compat.h
Posted by Kaspar Brand <ht...@velox.ch>.
Joe, many thanks for your review and valuable comments, first off! As
the person who contributed to the original SNI patch (authored by Peter
Sylvester/EdelWeb), I'll try to address your questions to the best of my
knowledge/ability.
An updated version of the patch (based on Guenter's latest version), is
attached to this message, in two versions: first against r606189 (the
last w/o any SNI related code) and second, against the current trunk
version. I'm grateful for comments, hints about flaws in my reasoning /
incorrect assumptions / omissions, or even objections, if there are.
> My main comment: it's not obvious to me how this actually does what it
> needs to. The issue with getting true SNI support is that the hostname
> sent in the ClientHello must cause a change to the selection of
> r->server early enough to ensure that any security policy specified in
> the vhost configuration is applied correctly (e.g. SSLVerifyClient) -
> not merely selection of the correct SSL_CTX (and hence server cert).
That's correct. However, choosing the appropriate r->server is mainly
handled by ap_read_request() in protocol.c, fortunately: specifically,
ap_read_request() calls ap_update_vhost_from_headers() to select the
right virtual host.
> I can't see how this patch achieves that. Any clues?
If the request includes a Host: header (which results in r->hostname
being set), then everything is fine as soon as the headers have been
read (i.e. before ssl_hook_ReadReq is called, and of course before
ssl_hook_Access checks for directory-specific access restrictions). We
"only" need to make sure that the correct SSL context (and hence the
right server cert) is selected while the TLS handshake takes place.
> Has a configuration
> with an SSLVerifyClient specified in the named vhost been tested?
Yes, and one specific configuration actually made me tweak the code in
the servername callback further: when modifying the SSL connection,
OpenSSL's SSL_set_SSL_CTX() will only adjust the server cert from the
context, but not additional settings like verify_mode and the verify
callback. These are relevant when SSLVerifyClient is configured at the
*per-server* context (i.e. at the vhost level), and the previous version
of the patch failed to enforce such a configuration, at least in cases
where SSLVerifyClient for the first (=default) VirtualHost was different
than for any subsequent VirtualHosts.
Per-directory SSLVerifyClient settings don't suffer from this problem
(as the correct vhost is selected before ssl_hook_Access is called),
these are enforced by triggering a renegotiation, if needed.
When speaking of renegotiating (but not related to SSLVerifyClient
specifically), note that I removed this code from ssl_hook_Access:
> - * We will force a renegotiation if we switch to another virtualhost.
> - */
> - if (r->hostname && !SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name)) {
> - if (ssl_set_vhost_ctx(ssl, r->hostname) && ctx != SSL_get_SSL_CTX(ssl))
> - renegotiate = TRUE;
> - }
This applied to connections from clients which did not send an SNI
extension; renegotiating here doesn't make much sense to me since these
clients already received a (possibly) wrong certificate, so the mismatch
has already happened.
On the other hand, I've added some code to ssl_hook_ReadReq which deals
with the case where the client supplies an SNI extension but doesn't
send a Host: header (which is probably rare, but anyway):
> + if (!r->hostname &&
> + (servername = SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name))) {
> + /* Use the SNI extension as the hostname if no Host: header was sent */
> + r->hostname = apr_pstrdup(r->pool, servername);
> + ap_update_vhost_from_headers(r);
> + }
In reply to your further comments (unless addressed by Guenter already):
> 3) Calling an env var "SSL_TLS_..." is redundant; why is an extra env
> var necessary here anyway?
It adds a couple of useful options when dealing with SSL requests:
* in a CGI application, you can programmatically determine if the client
supplied an SNI extension (and possibly redirect the user, or point
him to further information)
* you can log this information (LogFormat and "%{SSL_TLS_SNI}e" as the
parameter), which allows to gather statistics about the percentage
of SNI capable clients your site has, e.g.
* you can use this information in mod_rewrite rules, e.g. for
rewriting requests based on the absence of an SNI extension (such as
'RewriteCond %{SSL:SSL_TLS_SNI} =""' - note that I had to add support
for this in ssl_engine_vars.c, this didn't work so far)
I agree that "SSL_TLS_" is somewhat redundant, but on the other hand
"SSL_" seems to be the proper prefix for all mod_ssl related variables,
and using "SSL_SNI" only might lead to the conclusion that SNI is also
relates to pure SSLv2/v3 connections.
> 4) Why is it desirable to send a fatal TLS alert if no vhost is found
> matching the hostname in the clienthello?
I've changed that to SSL_TLSEXT_ERR_ALERT_WARNING, which also makes it
less of a dead end for NSS based clients e.g. (such as Firefox).
> This warning has no less weight given the presence of SNI support in the
> version of OpenSSL against which mod_ssl is built. Many (probably most)
> deployed clients do not support SNI and hence cannot support name-based
> vhosting with SSL. I don't think this should be omitted, merely
> reworded.
Right, the previous approach was too crude. I'm adapting the wording of
these warnings based on the presence of support for the SNI extension.
Additional changes, compared to the latest version committed to the
trunk by Guenter:
* renamed most of the functions, to make them more consistent
with the general style for mod_ssl function names
* moved the callback function from ssl_engine_init.c to
ssl_engine_kernel.c (where all the other OpenSSL callbacks live)
* merged the (intermediate) ssl_set_vhost_ctx function into
ssl_callback_ServerNameIndication (was only needed by the code in
ssl_hook_Access, which is now gone)
* some style improvements (last part of ssl_find_vhost, notably)
* in ssl_callback_ServerNameIndication, add some diagnostics at
APLOG_DEBUG level
Thanks again,
Kaspar
Re: svn commit: r606190 - in /httpd/httpd/trunk: CHANGES
modules/ssl/ssl_engine_init.c modules/ssl/ssl_engine_kernel.c
modules/ssl/ssl_toolkit_compat.h
Posted by Joe Orton <jo...@redhat.com>.
On Fri, Dec 21, 2007 at 01:16:21PM -0000, fuankg@apache.org wrote:
> Author: fuankg
> Date: Fri Dec 21 05:16:21 2007
> New Revision: 606190
>
> URL: http://svn.apache.org/viewvc?rev=606190&view=rev
> Log:
> Added server name indication (RFC 4366) support (PR 34607).
Commits containing changes authored by someone other than the committer
should have a "Submitted by: " line giving appropriate credit.
This patch introduces the following warnings for a build with
OPENSSL_NO_TLSEXT undefined.
ssl_engine_init.c:194: warning: no previous prototype for 'ssl_set_vhost_ctx'
ssl_engine_init.c: In function 'ssl_set_vhost_ctx':
ssl_engine_init.c:205: warning: implicit declaration of function 'ap_vhost_iterate_given_conn'
ssl_engine_init.c: At top level:
ssl_engine_init.c:209: warning: no previous prototype for 'ssl_servername_cb'
ssl_engine_init.c: In function 'ssl_init_CheckServers':
ssl_engine_init.c:1119: warning: unused variable 'conflict'
ssl_engine_init.c:1117: warning: unused variable 'klen'
ssl_engine_init.c:1116: warning: unused variable 'key'
ssl_engine_init.c:1115: warning: unused variable 'table'
ssl_engine_init.c:1113: warning: unused variable 'ps'
ssl_engine_kernel.c: In function 'ssl_hook_Access':
ssl_engine_kernel.c:307: warning: implicit declaration of function 'ssl_set_vhost_ctx'
ssl_engine_kernel.c: In function 'ssl_hook_Fixup':
ssl_engine_kernel.c:1110: warning: suggest parentheses around assignment used as truth value
My main comment: it's not obvious to me how this actually does what it
needs to. The issue with getting true SNI support is that the hostname
sent in the ClientHello must cause a change to the selection of
r->server early enough to ensure that any security policy specified in
the vhost configuration is applied correctly (e.g. SSLVerifyClient) -
not merely selection of the correct SSL_CTX (and hence server cert). I
can't see how this patch achieves that. Any clues? Has a configuration
with an SSLVerifyClient specified in the named vhost been tested?
Further comments:
1) There are numerous code style issues. Please ensure new code added
meets the style guide: http://httpd.apache.org/dev/styleguide.html
2) Please don't introduce new global functions named "ssl_". Use
"modssl_".
3) Calling an env var "SSL_TLS_..." is redundant; why is an extra env
var necessary here anyway?
4) Why is it desirable to send a fatal TLS alert if no vhost is found
matching the hostname in the clienthello?
Some random nit-picks below:
> --- httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c (original)
> +++ httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c Fri Dec 21 05:16:21 2007
> @@ -297,6 +297,19 @@
> * the currently active one.
> */
>
> +#ifndef OPENSSL_NO_TLSEXT
> + /*
> + * We will switch to another virtualhost and to its ssl_ctx
> + * if changed, we will force a renegotiation.
> + */
That sentence doesn't make sense.
> + if (r->hostname && !SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name)) {
> + SSL_CTX *ctx = SSL_get_SSL_CTX(ssl);
That shadows a variable of the same name at function scope.
> + if (ssl_set_vhost_ctx(ssl,(char *)r->hostname) &&
Why is the cast needed?
> ==============================================================================
> --- httpd/httpd/trunk/modules/ssl/ssl_engine_init.c (original)
> +++ httpd/httpd/trunk/modules/ssl/ssl_engine_init.c Fri Dec 21 05:16:21 2007
> @@ -135,6 +135,87 @@
...
> + }
> +#endif
> +}
> +
> static void ssl_init_ctx_protocol(server_rec *s,
> apr_pool_t *p,
> apr_pool_t *ptemp,
> @@ -712,6 +816,8 @@
> /* XXX: proxy support? */
> ssl_init_ctx_cert_chain(s, p, ptemp, mctx);
> }
> +
> + ssl_init_server_extensions(s, p, ptemp, mctx);
> }
This is being called for both a server context and a proxy context, it
is not necessary for the latter.
> static int ssl_server_import_cert(server_rec *s,
> @@ -1038,6 +1144,7 @@
> }
> }
>
> +#ifdef OPENSSL_NO_TLSEXT
> /*
> * Give out warnings when more than one SSL-aware virtual server uses the
> * same IP:port. This doesn't work because mod_ssl then will always use
This warning has no less weight given the presence of SNI support in the
version of OpenSSL against which mod_ssl is built. Many (probably most)
deployed clients do not support SNI and hence cannot support name-based
vhosting with SSL. I don't think this should be omitted, merely
reworded.
joe