You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2009/03/28 21:00:10 UTC
Re: SNI in 2.2.x (Re: Time for 2.2.10?)
On 08/19/2008 08:16 AM, Kaspar Brand wrote:
> Ruediger Pluem wrote:
>> At the moment we have 9 entries in the CHANGES file for 2.2.10 and
>> there are 5 more proposals in the STATUS file that are missing only
>> one vote. I think if get these done we also have enough stuff from
>> pure httpd point of view that warrants a release. WDYT?
>
> May I use this occasion to ask if there's still a chance of getting a
> backport of SNI accepted for 2.2.x? Following suggestions from Joe, I
> went through all relevant SSL* config directives and posted my findings
> in two parts on 9th/18th June:
>
> http://mail-archives.apache.org/mod_mbox/httpd-dev/200806.mbox/%3c484CBAA6.7050702@velox.ch%3e
> http://mail-archives.apache.org/mod_mbox/httpd-dev/200806.mbox/%3c48592955.2090303@velox.ch%3e
>
> The patch I posted on 18 June introduced a regression, however, so I
> posted an updated version on 26th June:
>
> http://mail-archives.apache.org/mod_mbox/httpd-dev/200806.mbox/%3c4863C04C.2010502@velox.ch%3e
>
> That's the version I still consider suitable for check-in to trunk
> (attached again for convenience). A backport to 2.2.x is available at
I know that quite a lot of time has passed since, but as you may have noticed we
finally found some time to hack things up at the Hackathon this week in Amsterdam.
Nevertheless the patch mentioned in this mail seems to be still missing in trunk.
Going through the archive I noticed several attachments with the same basename and
and a version string attached. Are these patches cumulative so that I only need
to review the latest one?
Regards
Rüdiger
Re: SNI in 2.2.x (Re: Time for 2.2.10?)
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Ursprüngliche Nachricht-----
> Von: Kaspar Brand
> Gesendet: Donnerstag, 2. April 2009 18:21
> An: dev@httpd.apache.org
> Betreff: Re: SNI in 2.2.x (Re: Time for 2.2.10?)
>
> Plüm, Rüdiger, VF-Group wrote:
> > Going through the follow ups the following question remains for me:
> >
> > Where did you address to adjust the
> >
> > SSLCARevocation{File,Path} and
> > SSLOCSP{Enable,DefaultResponder,OverrideResponder}
> >
> > settings in the case of an non SNI client connecting to the
> non default vhost?
>
> By modifying ssl_callback_SSLVerify and ssl_callback_SSLVerify_CRL to
> use r->server as the server_rec (instead of conn->base_server), which
> makes sure that the correct mctx gets selected. These
> callbacks will be
> used during a renegotiation, which is triggered by ssl_hook_Access if
> the non-default vhost has more restrictive SSLVerify{Client,Depth}
> settings compared to the default vhost.
>
Thanks for the pointer.
Regards
Rüdiger
Re: SNI in 2.2.x (Re: Time for 2.2.10?)
Posted by Kaspar Brand <ht...@velox.ch>.
Plüm, Rüdiger, VF-Group wrote:
> Going through the follow ups the following question remains for me:
>
> Where did you address to adjust the
>
> SSLCARevocation{File,Path} and
> SSLOCSP{Enable,DefaultResponder,OverrideResponder}
>
> settings in the case of an non SNI client connecting to the non default vhost?
By modifying ssl_callback_SSLVerify and ssl_callback_SSLVerify_CRL to
use r->server as the server_rec (instead of conn->base_server), which
makes sure that the correct mctx gets selected. These callbacks will be
used during a renegotiation, which is triggered by ssl_hook_Access if
the non-default vhost has more restrictive SSLVerify{Client,Depth}
settings compared to the default vhost.
Kaspar
Re: SNI in 2.2.x (Re: Time for 2.2.10?)
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Ursprüngliche Nachricht-----
> Von: Kaspar Brand
> Gesendet: Donnerstag, 2. April 2009 07:15
> An: dev@httpd.apache.org
> Betreff: Re: SNI in 2.2.x (Re: Time for 2.2.10?)
>
> Plüm, Rüdiger, VF-Group wrote:
> > A question regarding your patch:
> >
> > @@ -427,29 +435,26 @@ int ssl_hook_Access(request_rec *r)
> > * function and not by OpenSSL internally (and our
> function is aware of
> > * both the per-server and per-directory contexts). So
> we cannot ask
> > * OpenSSL about the currently verify depth. Instead
> we remember it in our
> > * ap_ctx attached to the SSL* of OpenSSL. We've to force the
> > * renegotiation if the reconfigured/new verify depth
> is less than the
> > * currently active/remembered verify depth (because
> this means more
> > * restriction on the certificate chain).
> > */
> > - if ((sc->server->auth.verify_depth != UNSET) &&
> > - (dc->nVerifyDepth == UNSET)) {
> > - /* apply per-vhost setting, if per-directory
> config is not set */
> > - dc->nVerifyDepth = sc->server->auth.verify_depth;
> > - }
> >
> > Why don't you stick with the old approach of updating
> dc->nVerifyDepth and using
> > this later on consistently
>
> Because it was called "ugly" by Joe (and not threadsafe, possibly[?]):
>
> http://mail-archives.apache.org/mod_mbox/httpd-dev/200806.mbox
> /%3c20080604140111.GA12050@redhat.com%3e
>
Thanks for the pointer.
Going through the follow ups the following question remains for me:
Where did you address to adjust the
SSLCARevocation{File,Path} and
SSLOCSP{Enable,DefaultResponder,OverrideResponder}
settings in the case of an non SNI client connecting to the non default vhost?
> > (the same happens with other fields in the same way later on)?
>
> I don't think any of my changes to ssl_hook_Access adds an assignment
> to any dc->something parameter (or it would be an
> oversight/bug if it did).
This was a misunderstanding. I wanted to say that you followed this
design pattern throughout your patch and changed the logic from
assigning to dc->something to not assigning but to check the
server config separately. So your patch is consistent regarding this.
Regards
Rüdiger
Re: SNI in 2.2.x (Re: Time for 2.2.10?)
Posted by Kaspar Brand <ht...@velox.ch>.
Plüm, Rüdiger, VF-Group wrote:
> A question regarding your patch:
>
> @@ -427,29 +435,26 @@ int ssl_hook_Access(request_rec *r)
> * function and not by OpenSSL internally (and our function is aware of
> * both the per-server and per-directory contexts). So we cannot ask
> * OpenSSL about the currently verify depth. Instead we remember it in our
> * ap_ctx attached to the SSL* of OpenSSL. We've to force the
> * renegotiation if the reconfigured/new verify depth is less than the
> * currently active/remembered verify depth (because this means more
> * restriction on the certificate chain).
> */
> - if ((sc->server->auth.verify_depth != UNSET) &&
> - (dc->nVerifyDepth == UNSET)) {
> - /* apply per-vhost setting, if per-directory config is not set */
> - dc->nVerifyDepth = sc->server->auth.verify_depth;
> - }
>
> Why don't you stick with the old approach of updating dc->nVerifyDepth and using
> this later on consistently
Because it was called "ugly" by Joe (and not threadsafe, possibly[?]):
http://mail-archives.apache.org/mod_mbox/httpd-dev/200806.mbox/%3c20080604140111.GA12050@redhat.com%3e
> (the same happens with other fields in the same way later on)?
I don't think any of my changes to ssl_hook_Access adds an assignment
to any dc->something parameter (or it would be an oversight/bug if it did).
Kaspar
Re: SNI in 2.2.x (Re: Time for 2.2.10?)
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Ursprüngliche Nachricht-----
> Von: Kaspar Brand
> Gesendet: Montag, 30. März 2009 18:15
> An: dev@httpd.apache.org
> Betreff: Re: SNI in 2.2.x (Re: Time for 2.2.10?)
>
> Ruediger Pluem wrote:
> > Going through the archive I noticed several attachments
> with the same
> > basename and and a version string attached. Are these patches
> > cumulative so that I only need to review the latest one?
>
> sni_sslverifyclient-v5.diff includes all improvements to
> ssl_hook_Access/ssl_callback_SSLVerify/ssl_callback_SSLVerify_CRL
> which I did in June 2008, yes. Then I stopped updating the
> trunk version
> (due to lack of responses) and only worked on further
> improvements on to
> the 2.2.x patch (latest version lives at
> http://sni.velox.ch/httpd-2.2.x-sni.20080928.patch).
A question regarding your patch:
@@ -427,29 +435,26 @@ int ssl_hook_Access(request_rec *r)
* function and not by OpenSSL internally (and our function is aware of
* both the per-server and per-directory contexts). So we cannot ask
* OpenSSL about the currently verify depth. Instead we remember it in our
* ap_ctx attached to the SSL* of OpenSSL. We've to force the
* renegotiation if the reconfigured/new verify depth is less than the
* currently active/remembered verify depth (because this means more
* restriction on the certificate chain).
*/
- if ((sc->server->auth.verify_depth != UNSET) &&
- (dc->nVerifyDepth == UNSET)) {
- /* apply per-vhost setting, if per-directory config is not set */
- dc->nVerifyDepth = sc->server->auth.verify_depth;
- }
Why don't you stick with the old approach of updating dc->nVerifyDepth and using
this later on consistently (the same happens with other fields in the same
way later on)?
- if (dc->nVerifyDepth != UNSET) {
+ if ((dc->nVerifyDepth != UNSET) ||
+ (sc->server->auth.verify_depth != UNSET)) {
/* XXX: doesnt look like sslconn->verify_depth is actually used */
if (!(n = sslconn->verify_depth)) {
sslconn->verify_depth = n = sc->server->auth.verify_depth;
}
/* determine whether a renegotiation has to be forced */
- if (dc->nVerifyDepth < n) {
+ if ((dc->nVerifyDepth < n) ||
+ (sc->server->auth.verify_depth < n)) {
renegotiate = TRUE;
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
"Reduced client verification depth will force "
"renegotiation");
}
}
/*
Regards
Rüdiger
Re: SNI in 2.2.x (Re: Time for 2.2.10?)
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Ursprüngliche Nachricht-----
> Von: Kaspar Brand
> Gesendet: Samstag, 25. April 2009 09:37
> An: dev@httpd.apache.org
> Betreff: Re: SNI in 2.2.x (Re: Time for 2.2.10?)
>
> >> Mind to sent a version v9 of your patch such that I can review the
> >> complete one again? Thanks for your efforts.
>
> Sorry, please disregard v9 - it makes SSL_VERIFY_CLIENT
> report GENEROUS
> even in cases where it could/should be SUCCESS, actually (if
> the CA list
> stays the same; i.e., v9 doesn't weaken things, security-wise, but
> possibly locks out legitimate [non-SNI] clients).
Sounds reasonable.
>
> I have attached v10. As far as ssl_var_lookup_ssl_cert_verify()
> is concerned, a tweak could look like:
>
> --- modules/ssl/ssl_engine_vars.c (revision 765079)
> +++ modules/ssl/ssl_engine_vars.c (working copy)
> @@ -607,7 +607,7 @@ static char *ssl_var_lookup_ssl_cert_verify(apr_po
> result = "SUCCESS";
> else if (vrc == X509_V_OK && vinfo != NULL &&
> strEQ(vinfo, "GENEROUS"))
> /* client verification done in generous way */
> - result = "GENEROUS";
> + result = xs ? "GENEROUS" : "NONE";
> else
> /* client verification failed */
> result = apr_psprintf(p, "FAILED:%s", verr);
>
>
> [Not included in v10. If it's added, we should probably
> update the comment
> to explain why we're doing it like this, exactly.]
I guess the following one is the better patch
Index: modules/ssl/ssl_engine_vars.c
===================================================================
--- modules/ssl/ssl_engine_vars.c (revision 768231)
+++ modules/ssl/ssl_engine_vars.c (working copy)
@@ -599,7 +599,7 @@
vrc = SSL_get_verify_result(ssl);
xs = SSL_get_peer_certificate(ssl);
- if (vrc == X509_V_OK && verr == NULL && vinfo == NULL && xs == NULL)
+ if (vrc == X509_V_OK && verr == NULL && xs == NULL)
/* no client verification done at all */
result = "NONE";
else if (vrc == X509_V_OK && verr == NULL && vinfo == NULL && xs != NULL)
IMHO we can report NONE whenever there was no error and the client cert is empty.
Opinions by the SSL Gurus?
Regards
Rüdiger
Re: SNI in 2.2.x (Re: Time for 2.2.10?)
Posted by Kaspar Brand <ht...@velox.ch>.
Plüm, Rüdiger, VF-Group wrote:
> Committed v10 with some smaller tweaks as r768499.
Thanks - I'm fine with them!
Also, I'm glad to see that the proposed default for
SSLStrictSNIVHostCheck is off (and hope that it will stay like this :-).
A default of "On" would imply that users have to explicitly change their
configurations to retain backward compatibility with versions <= 2.2.
Kaspar
Re: SNI in 2.2.x (Re: Time for 2.2.10?)
Posted by Ruediger Pluem <rp...@apache.org>.
On 04/25/2009 11:20 AM, Plüm, Rüdiger, VF-Group wrote:
> Committed v10 with some smaller tweaks as r768499. Especially I removed
>
> @@ -186,16 +186,6 @@ int ssl_hook_ReadReq(request_rec *r)
> return HTTP_BAD_REQUEST;
> }
> }
> - else if (r->connection->vhost_lookup_data) {
> - /*
> - * We are using a name based configuration here, but no hostname was
> - * provided via SNI. Don't allow that.
> - */
> - ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
> - "No hostname was provided via SNI for a name based"
> - " virtual host");
> - return HTTP_FORBIDDEN;
> - }
> #endif
> SSL_set_app_data2(ssl, r);
>
> as I want to make this configurable and this is easier to do when it remains in the code.
Since r768596 this is now configurable via SSLStrictSNIVHostCheck (with a default to off,
which is effectively the same as your original patch).
The default value might change depending on the results of peer review here.
So SSL gurus please some review here.
Regards
Rüdiger
Re: SNI in 2.2.x (Re: Time for 2.2.10?)
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
Committed v10 with some smaller tweaks as r768499. Especially I removed
@@ -186,16 +186,6 @@ int ssl_hook_ReadReq(request_rec *r)
return HTTP_BAD_REQUEST;
}
}
- else if (r->connection->vhost_lookup_data) {
- /*
- * We are using a name based configuration here, but no hostname was
- * provided via SNI. Don't allow that.
- */
- ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
- "No hostname was provided via SNI for a name based"
- " virtual host");
- return HTTP_FORBIDDEN;
- }
#endif
SSL_set_app_data2(ssl, r);
as I want to make this configurable and this is easier to do when it remains in the code.
Furthermore I tweaked
+#define MODSSL_CFG_CA_NE(f, sc1, sc2) \
+ (sc1->server->auth.f && \
+ (!sc2->server->auth.f || \
+ sc2->server->auth.f && \
+ strNE(sc1->server->auth.f, sc2->server->auth.f)))
+
So please have a quick check at
http://svn.apache.org/viewvc?view=rev&revision=768499
Regards
Rüdiger
> -----Ursprüngliche Nachricht-----
> Von: Kaspar Brand
> Gesendet: Samstag, 25. April 2009 09:37
> An: dev@httpd.apache.org
> Betreff: Re: SNI in 2.2.x (Re: Time for 2.2.10?)
>
> >> Mind to sent a version v9 of your patch such that I can review the
> >> complete one again? Thanks for your efforts.
>
> Sorry, please disregard v9 - it makes SSL_VERIFY_CLIENT
> report GENEROUS
> even in cases where it could/should be SUCCESS, actually (if
> the CA list
> stays the same; i.e., v9 doesn't weaken things, security-wise, but
> possibly locks out legitimate [non-SNI] clients).
>
> I have attached v10. As far as ssl_var_lookup_ssl_cert_verify()
> is concerned, a tweak could look like:
>
> --- modules/ssl/ssl_engine_vars.c (revision 765079)
> +++ modules/ssl/ssl_engine_vars.c (working copy)
> @@ -607,7 +607,7 @@ static char *ssl_var_lookup_ssl_cert_verify(apr_po
> result = "SUCCESS";
> else if (vrc == X509_V_OK && vinfo != NULL &&
> strEQ(vinfo, "GENEROUS"))
> /* client verification done in generous way */
> - result = "GENEROUS";
> + result = xs ? "GENEROUS" : "NONE";
> else
> /* client verification failed */
> result = apr_psprintf(p, "FAILED:%s", verr);
>
>
> [Not included in v10. If it's added, we should probably
> update the comment
> to explain why we're doing it like this, exactly.]
>
> Kaspar
>
>
Re: SNI in 2.2.x (Re: Time for 2.2.10?)
Posted by Kaspar Brand <ht...@velox.ch>.
>> Mind to sent a version v9 of your patch such that I can review the
>> complete one again? Thanks for your efforts.
Sorry, please disregard v9 - it makes SSL_VERIFY_CLIENT report GENEROUS
even in cases where it could/should be SUCCESS, actually (if the CA list
stays the same; i.e., v9 doesn't weaken things, security-wise, but
possibly locks out legitimate [non-SNI] clients).
I have attached v10. As far as ssl_var_lookup_ssl_cert_verify()
is concerned, a tweak could look like:
--- modules/ssl/ssl_engine_vars.c (revision 765079)
+++ modules/ssl/ssl_engine_vars.c (working copy)
@@ -607,7 +607,7 @@ static char *ssl_var_lookup_ssl_cert_verify(apr_po
result = "SUCCESS";
else if (vrc == X509_V_OK && vinfo != NULL && strEQ(vinfo, "GENEROUS"))
/* client verification done in generous way */
- result = "GENEROUS";
+ result = xs ? "GENEROUS" : "NONE";
else
/* client verification failed */
result = apr_psprintf(p, "FAILED:%s", verr);
[Not included in v10. If it's added, we should probably update the comment
to explain why we're doing it like this, exactly.]
Kaspar
Re: SNI in 2.2.x (Re: Time for 2.2.10?)
Posted by Kaspar Brand <ht...@velox.ch>.
Plüm, Rüdiger, VF-Group wrote:
> Comparing against anything else but 'SUCCESS' is IMHO a flaw in the
> configuration. 'GENEROUS' IMHO only says that some kind of
> certificate was sent at all.
That's also my interpretation. Depending on what the exact meaning of
'GENEROUS' should be in our particular situation, we might have to
slightly tweak ssl_engine_vars.c:ssl_var_lookup_ssl_cert_verify(). (With
the current code, you can also end up with GENEROUS if no certificate
was presented.)
> Mind to sent a version v9 of your patch such that I can review the
> complete one again? Thanks for your efforts.
Sure, no problem. I decided to not include any
ssl_var_lookup_ssl_cert_verify related modifications, for the time being.
Kaspar
Re: SNI in 2.2.x (Re: Time for 2.2.10?)
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Ursprüngliche Nachricht-----
> Von: Kaspar Brand
> Gesendet: Freitag, 24. April 2009 07:15
> An: dev@httpd.apache.org
> Betreff: Re: SNI in 2.2.x (Re: Time for 2.2.10?)
>
> Plüm, Rüdiger, VF-Group wrote:
> > As I said further down below I see also good and valid use
> cases for the
> > combination
> > SSLVerifyClient optional
> > and
> > %{SSL_CLIENT_VERIFY}
> > And this combination should be safe even if this comes at
> the price that
> > some configuration are not possible without SNI. But yes,
> we may disagree
> > on this :-).
>
> If that's the only remaining issue which needs to be sorted out, then
> I feel quite confident that we'll be able to agree on a solution
> within very reasonable time :-)
>
> > I would only love to see that the parts in your patch were you
> > used r->connection->aborted are adjusted accordingly.
>
> Using modssl_set_verify to restore the setting to "verify_old" seems
> fine, AFAICT (the client is free to retry the request over the same
> connection, but we'll send him a 403 again, anyway... that even saves
> us additional handshakes, in case of stubborn clients repeating
> their requests).
>
> Here's another idea for trying to cut that Gordian knot:
>
> if ((r->server != handshakeserver)
> && renegotiate
> && ((verify & SSL_VERIFY_PEER) ||
> (verify & SSL_VERIFY_FAIL_IF_NO_PEER_CERT))) {
> SSLSrvConfigRec *hssc = mySrvConfig(handshakeserver);
>
> #define MODSSL_CFG_CA_NE(f, sc1, sc2) \
> (sc1->server->auth.f && \
> (!sc2->server->auth.f || \
> sc2->server->auth.f && \
> strNE(sc1->server->auth.f, sc2->server->auth.f)))
>
> if ((MODSSL_CFG_CA_NE(ca_cert_file, sc, hssc) ||
> MODSSL_CFG_CA_NE(ca_cert_path, sc, hssc)) &&
> (verify & SSL_VERIFY_FAIL_IF_NO_PEER_CERT)) {
> ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
> "Non-default virtual host with
> SSLVerify set to "
> "'require' and VirtualHost-specific
> CA certificate "
> "list is only available to clients
> with TLS server "
> "name indication (SNI) support");
> modssl_set_verify(ssl, verify_old, NULL);
> return HTTP_FORBIDDEN;
> } else
> /* let it pass, possibly with an "incorrect"
> peer cert,
> * so make sure the SSL_CLIENT_VERIFY
> environment variable
> * will indicate partial success only, later on.
> */
> sslconn->verify_info = "GENEROUS";
> }
>
> I.e., if someone configures a non-default vhost with
> "SSLVerifyClient optional",
> and checks for %{SSL_CLIENT_VERIFY} in an SSLRequire
> expression (hopefully
> with 'eq "SUCCESS"'), then non-SNI clients will still be banned.
That should work. Comparing against anything else but 'SUCCESS' is
IMHO a flaw in the configuration. 'GENEROUS' IMHO only says that some
kind of certificate was sent at all.
Mind to sent a version v9 of your patch such that I can review the complete
one again?
Thanks for your efforts.
Regards
Rüdiger
Re: SNI in 2.2.x (Re: Time for 2.2.10?)
Posted by Kaspar Brand <ht...@velox.ch>.
Plüm, Rüdiger, VF-Group wrote:
> As I said further down below I see also good and valid use cases for the
> combination
> SSLVerifyClient optional
> and
> %{SSL_CLIENT_VERIFY}
> And this combination should be safe even if this comes at the price that
> some configuration are not possible without SNI. But yes, we may disagree
> on this :-).
If that's the only remaining issue which needs to be sorted out, then
I feel quite confident that we'll be able to agree on a solution
within very reasonable time :-)
> I would only love to see that the parts in your patch were you
> used r->connection->aborted are adjusted accordingly.
Using modssl_set_verify to restore the setting to "verify_old" seems
fine, AFAICT (the client is free to retry the request over the same
connection, but we'll send him a 403 again, anyway... that even saves
us additional handshakes, in case of stubborn clients repeating
their requests).
Here's another idea for trying to cut that Gordian knot:
if ((r->server != handshakeserver)
&& renegotiate
&& ((verify & SSL_VERIFY_PEER) ||
(verify & SSL_VERIFY_FAIL_IF_NO_PEER_CERT))) {
SSLSrvConfigRec *hssc = mySrvConfig(handshakeserver);
#define MODSSL_CFG_CA_NE(f, sc1, sc2) \
(sc1->server->auth.f && \
(!sc2->server->auth.f || \
sc2->server->auth.f && \
strNE(sc1->server->auth.f, sc2->server->auth.f)))
if ((MODSSL_CFG_CA_NE(ca_cert_file, sc, hssc) ||
MODSSL_CFG_CA_NE(ca_cert_path, sc, hssc)) &&
(verify & SSL_VERIFY_FAIL_IF_NO_PEER_CERT)) {
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
"Non-default virtual host with SSLVerify set to "
"'require' and VirtualHost-specific CA certificate "
"list is only available to clients with TLS server "
"name indication (SNI) support");
modssl_set_verify(ssl, verify_old, NULL);
return HTTP_FORBIDDEN;
} else
/* let it pass, possibly with an "incorrect" peer cert,
* so make sure the SSL_CLIENT_VERIFY environment variable
* will indicate partial success only, later on.
*/
sslconn->verify_info = "GENEROUS";
}
I.e., if someone configures a non-default vhost with "SSLVerifyClient optional",
and checks for %{SSL_CLIENT_VERIFY} in an SSLRequire expression (hopefully
with 'eq "SUCCESS"'), then non-SNI clients will still be banned.
Kaspar
Re: SNI in 2.2.x (Re: Time for 2.2.10?)
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Ursprüngliche Nachricht-----
> Von: Kaspar Brand
> Gesendet: Mittwoch, 22. April 2009 09:12
> An: dev@httpd.apache.org
> Betreff: Re: SNI in 2.2.x (Re: Time for 2.2.10?)
>
> Ruediger Pluem wrote:
> > the next configuration *can* do security harm:
> >
> > <VirtualHost foo.example.com:443>
> > SSLVerifyClient optional
> > SSLCACertificateFile foo-clientauth-bundle.pem
> > </VirtualHost>
> >
> > <VirtualHost bar.example.com:443>
> > SSLVerifyClient optional
> > SSLCACertificateFile bar-clientauth-bundle.pem
> > </VirtualHost>
> >
> > This would cause client certificates signed by foo-clientauth-bundle
> > accepted by the virtual host bar.example.com.
>
> That's true, but I think it's where we disagree about the meaning of
> "SSLVerifyClient optional", actually: if it's optional, then
> a client is
> free to not present any client certificate - which, IMO, is not really
> different from presenting a client certificate issued by a CA not
> included in the respective CA bundle.
IMHO this is a *huge* difference. If its optional I can decide whether to
present one or not, but if I present one it should be ensured that
the feedback whether it is valid or not is based on the correct CA.
>
> > Even if SSLVerifyClient is optional later processing steps such as
> > RewriteRules or applications may evaluate the environment
> variables set
> > by SSL and think that a successful authentication took place where
> > in fact it has not.
>
> In my opinion, depending on %{SSL_CLIENT_VERIFY} only makes sense in
> combination with "SSLVerifyClient require" (either clientauth is
> required, or it's not - this is also what the last paragraph in the
> documentation abouut SSLVerifyClient says, effectively).
As I said further down below I see also good and valid use cases for the
combination
SSLVerifyClient optional
and
%{SSL_CLIENT_VERIFY}
And this combination should be safe even if this comes at the price that
some configuration are not possible without SNI. But yes, we may disagree
on this :-).
IMHO the largest consumers of name based virtual hosts without SNI would be
people that simply want to setup a SSL based side without client certs.
> What's still possible, though (as some sort of "workaround"): checking
> both %{SSL_CLIENT_VERIFY} and %{SSL_CLIENT_I_DN*} variables in an
> SSLRequire statement, to make sure that even if a non-SNI client
> connecting to a non-default vhost with "SSLVerifyClient optional"
> presents a certificate of one of the CAs we "require" for this vhost.
>
> > The Fakeauth setting above is a perfect example why an admin might
> > set optional:
> >
> > Either the user has a cert or it has to authenticate via
> username and password.
>
> Basically correct, though browsers sometimes show
> "surprising" behavior
> when encountering a configuration like this. (The FakeBasicAuth option
> is of no use when the client does not present a cert, though
> - you would
> need to combine "SSLVerifyClient optional" with one of the Auth*
> directives to handle this case.)
This is exactly the intended configuration here:
Have a bunch of users that auth via cert and thus have the dummy password
setup and have other users with a real password that use basic auth and simply
do a "normal" authn and authz configuration
>
> > It took me some time, but I think I got it. So we either need to
> > end the keepalive request or we need to restore the old
> verify setting
> > via
> >
> > modssl_set_verify(ssl, verify_old, ssl_callback_SSLVerify);
> >
> > correct?
>
> Yes, actually I had a similar solution in place, when fixing
> this issue
> first. But then I compared to what is done when one of the
> renegotiation
> steps failed, and changed to setting r->connection->aborted ;-)
>
> Instead of setting ssl_callback_SSLVerify again, you can also
> use NULL,
> btw (this will leave the current callback unchanged).
>
> > r->connection->aborted should *only* be set if the client
> did a network disconnect
> > *never* if the server decides to shutdown the connection.
>
> I also found it pretty rude, but as it was used in other
> places in that
> same function later on, I was assuming that it would be the
> right thing
> to do.
>
> > In this case we should set
> >
> > r->connection->keepalive = AP_CONN_CLOSE
> >
> > But as said this IMHO needs fixing in the code further down
> below as well.
>
> May I suggest that this is dealt with in a separate patch (not the one
> for enabling access to the non-default vhost[s] for non-SNI clients)?
Agreed. I would only love to see that the parts in your patch were you
used r->connection->aborted are adjusted accordingly.
The other locations of r->connection->aborted will be fixed in a separate
patch.
Hope to get to your patch until after the weekend.
Regards
Rüdiger
Re: SNI in 2.2.x (Re: Time for 2.2.10?)
Posted by Kaspar Brand <ht...@velox.ch>.
Ruediger Pluem wrote:
> the next configuration *can* do security harm:
>
> <VirtualHost foo.example.com:443>
> SSLVerifyClient optional
> SSLCACertificateFile foo-clientauth-bundle.pem
> </VirtualHost>
>
> <VirtualHost bar.example.com:443>
> SSLVerifyClient optional
> SSLCACertificateFile bar-clientauth-bundle.pem
> </VirtualHost>
>
> This would cause client certificates signed by foo-clientauth-bundle
> accepted by the virtual host bar.example.com.
That's true, but I think it's where we disagree about the meaning of
"SSLVerifyClient optional", actually: if it's optional, then a client is
free to not present any client certificate - which, IMO, is not really
different from presenting a client certificate issued by a CA not
included in the respective CA bundle.
> Even if SSLVerifyClient is optional later processing steps such as
> RewriteRules or applications may evaluate the environment variables set
> by SSL and think that a successful authentication took place where
> in fact it has not.
In my opinion, depending on %{SSL_CLIENT_VERIFY} only makes sense in
combination with "SSLVerifyClient require" (either clientauth is
required, or it's not - this is also what the last paragraph in the
documentation abouut SSLVerifyClient says, effectively).
What's still possible, though (as some sort of "workaround"): checking
both %{SSL_CLIENT_VERIFY} and %{SSL_CLIENT_I_DN*} variables in an
SSLRequire statement, to make sure that even if a non-SNI client
connecting to a non-default vhost with "SSLVerifyClient optional"
presents a certificate of one of the CAs we "require" for this vhost.
> The Fakeauth setting above is a perfect example why an admin might
> set optional:
>
> Either the user has a cert or it has to authenticate via username and password.
Basically correct, though browsers sometimes show "surprising" behavior
when encountering a configuration like this. (The FakeBasicAuth option
is of no use when the client does not present a cert, though - you would
need to combine "SSLVerifyClient optional" with one of the Auth*
directives to handle this case.)
> It took me some time, but I think I got it. So we either need to
> end the keepalive request or we need to restore the old verify setting
> via
>
> modssl_set_verify(ssl, verify_old, ssl_callback_SSLVerify);
>
> correct?
Yes, actually I had a similar solution in place, when fixing this issue
first. But then I compared to what is done when one of the renegotiation
steps failed, and changed to setting r->connection->aborted ;-)
Instead of setting ssl_callback_SSLVerify again, you can also use NULL,
btw (this will leave the current callback unchanged).
> r->connection->aborted should *only* be set if the client did a network disconnect
> *never* if the server decides to shutdown the connection.
I also found it pretty rude, but as it was used in other places in that
same function later on, I was assuming that it would be the right thing
to do.
> In this case we should set
>
> r->connection->keepalive = AP_CONN_CLOSE
>
> But as said this IMHO needs fixing in the code further down below as well.
May I suggest that this is dealt with in a separate patch (not the one
for enabling access to the non-default vhost[s] for non-SNI clients)?
> Further comments might follow when I have time to review v8. Stay tuned :-).
Thanks for your time, again!
Kaspar
Re: SNI in 2.2.x (Re: Time for 2.2.10?)
Posted by Ruediger Pluem <rp...@apache.org>.
On 04/21/2009 07:10 AM, Kaspar Brand wrote:
> Ruediger Pluem wrote:
>> Looks good. Some minor nitpicks:
>
>> Furthermore I think that we need to check for CA list change in any case
>> that we need to renegotiate as even if we do not fail on SSL level due
>> to SSL_VERIFY_FAIL_IF_NO_PEER_CERT there could be other conditions later
>> on in the configuration (sslrequire / rewriterules) that check if we had been
>> successful with our check on SSL level to e.g. provide a nice error page
>> if we were not. So if we have changed the CA list we might signal success
>> to these downstream configuration options although there wasn't one because
>> we used the wrong CA list.
>
> Here I'm not sure I'm really following you / understanding your point.
> The case I was primarily thinking of is something like
>
> <VirtualHost foo.example.com:443>
> SSLVerifyClient none # (the default, anyway)
> </VirtualHost>
>
> <VirtualHost bar.example.com:443>
> SSLVerifyClient optional
> SSLCACertificateFile bar-clientauth-bundle.pem
> </VirtualHost>
>
> In this situation, if a non-SNI client requests content from
> bar.example.com, the "renegotiate" variable will get set, but since
> "verify & SSL_VERIFY_FAIL_IF_NO_PEER_CERT" is not true, we will
> currently let it proceed. Are you proposing to return HTTP_FORBIDDEN
> immediately after the MODSSL_CFG_CA_NE checks fail, instead (i.e., even
> if SSLVerifyClient is optional)? My idea when writing that code was that
Yes, this is my proposal. The configuration you mentioned above is a good
example for a non working configuration with name based virtual Hosts and
SSL *without* SNI.
As we have no SSLCACertificateFile set in the the default host we effectively
have no CA against which we can check client certs so we will never have
a successful client verification here.
While the above does no real security harm (it just doesn't work as expected)
the next configuration *can* do security harm:
<VirtualHost foo.example.com:443>
SSLVerifyClient optional
SSLCACertificateFile foo-clientauth-bundle.pem
</VirtualHost>
<VirtualHost bar.example.com:443>
SSLVerifyClient optional
SSLCACertificateFile bar-clientauth-bundle.pem
</VirtualHost>
This would cause client certificates signed by foo-clientauth-bundle
accepted by the virtual host bar.example.com.
Even if SSLVerifyClient is optional later processing steps such as
RewriteRules or applications may evaluate the environment variables set
by SSL and think that a successful authentication took place where
in fact it has not.
Or think about adding
SSLOptions +FakeBasicAuth
to the configuration of the virtual host bar.example.com
> unless SSLVerifyClient is set to "require", we should not stop non-SNI
> clients here - the evaluation of a possible SSLRequire configuration
> directive at the end of ssl_hook_Access can still return HTTP_FORBIDDEN,
> if really needed (OTOH, why exactly would the admin choose "optional",
> then?). But maybe I'm simply missing your real point.
The Fakeauth setting above is a perfect example why an admin might
set optional:
Either the user has a cert or it has to authenticate via username and password.
Another one is: SSLVerifyClient require causes a Forbidden. While you can customize
an error page for that you might want to have a specific one for this
very special failure e.g. with links on it who to contact for a client cert.
This can be done very nicely with rewriterules and the environment variable
SSL_CLIENT_VERIFY.
> What I had to do anyway, however, is setting r->connection->aborted
> before returning HTTP_FORBIDDEN... otherwise we run into a problem with
> keep-alive requests, as additional testing has shown: if we don't drop
> the connection at the same time (by setting r->connection->aborted),
> then verify_old keeps its value from the previous request, and
> "renegotiate" may therefore not be set again when processing the next
> request (if verify == verify_old). Using r->connection->aborted for
> closing the connection immediately is also used in code further down in
> ssl_hook_Access (when a renegotiation doesn't have the expected outcome)
> - i.e., we're not introducing new behavior by using this "technique".
It took me some time, but I think I got it. So we either need to
end the keepalive request or we need to restore the old verify setting
via
modssl_set_verify(ssl, verify_old, ssl_callback_SSLVerify);
correct? Dropping the connection is not so efficient but might be more secure.
Anyway the solution is bad. This is not your fault as the existing code is bad
here as well :-).
r->connection->aborted should *only* be set if the client did a network disconnect
*never* if the server decides to shutdown the connection.
In this case we should set
r->connection->keepalive = AP_CONN_CLOSE
But as said this IMHO needs fixing in the code further down below as well.
Further comments might follow when I have time to review v8. Stay tuned :-).
Regards
Rüdiger
Re: SNI in 2.2.x (Re: Time for 2.2.10?)
Posted by Kaspar Brand <ht...@velox.ch>.
Ruediger Pluem wrote:
> Looks good. Some minor nitpicks:
>
> Reviewing the code again I don't think we need to have
>
> +#ifndef OPENSSL_NO_TLSEXT
> + && !(SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name))
> +#endif
>
> this condition at all.
Agreed. I have removed this part from the if expression.
> 2. The whole
>
> + if ((r->server != sslconn->server)
> + && renegotiate
> + && (verify & SSL_VERIFY_FAIL_IF_NO_PEER_CERT)
> +#ifndef OPENSSL_NO_TLSEXT
> + && !(SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name))
> +#endif
> + )
>
> can move inside the if block above (
>
> + if ((dc->nVerifyClient != SSL_CVERIFY_UNSET) ||
> + (sc->server->auth.verify_mode != SSL_CVERIFY_UNSET)) {
>
> )
> because only if get inside this block it can happen that
> verify & SSL_VERIFY_FAIL_IF_NO_PEER_CERT
> gets true. So we safe some checks in the case that
> ((dc->nVerifyClient != SSL_CVERIFY_UNSET) ||
> (sc->server->auth.verify_mode != SSL_CVERIFY_UNSET))
>
> is not true that will always fail and thus waste cycles even more
> so as there are many SSL configurations outside that do not use
> client certs at all.
> This also means we can move the initialization of verify back
> into the if block.
Correct, changed accordingly.
> Furthermore I think that we need to check for CA list change in any case
> that we need to renegotiate as even if we do not fail on SSL level due
> to SSL_VERIFY_FAIL_IF_NO_PEER_CERT there could be other conditions later
> on in the configuration (sslrequire / rewriterules) that check if we had been
> successful with our check on SSL level to e.g. provide a nice error page
> if we were not. So if we have changed the CA list we might signal success
> to these downstream configuration options although there wasn't one because
> we used the wrong CA list.
Here I'm not sure I'm really following you / understanding your point.
The case I was primarily thinking of is something like
<VirtualHost foo.example.com:443>
SSLVerifyClient none # (the default, anyway)
</VirtualHost>
<VirtualHost bar.example.com:443>
SSLVerifyClient optional
SSLCACertificateFile bar-clientauth-bundle.pem
</VirtualHost>
In this situation, if a non-SNI client requests content from
bar.example.com, the "renegotiate" variable will get set, but since
"verify & SSL_VERIFY_FAIL_IF_NO_PEER_CERT" is not true, we will
currently let it proceed. Are you proposing to return HTTP_FORBIDDEN
immediately after the MODSSL_CFG_CA_NE checks fail, instead (i.e., even
if SSLVerifyClient is optional)? My idea when writing that code was that
unless SSLVerifyClient is set to "require", we should not stop non-SNI
clients here - the evaluation of a possible SSLRequire configuration
directive at the end of ssl_hook_Access can still return HTTP_FORBIDDEN,
if really needed (OTOH, why exactly would the admin choose "optional",
then?). But maybe I'm simply missing your real point.
What I had to do anyway, however, is setting r->connection->aborted
before returning HTTP_FORBIDDEN... otherwise we run into a problem with
keep-alive requests, as additional testing has shown: if we don't drop
the connection at the same time (by setting r->connection->aborted),
then verify_old keeps its value from the previous request, and
"renegotiate" may therefore not be set again when processing the next
request (if verify == verify_old). Using r->connection->aborted for
closing the connection immediately is also used in code further down in
ssl_hook_Access (when a renegotiation doesn't have the expected outcome)
- i.e., we're not introducing new behavior by using this "technique".
> 3. I created a var handshakeserver to avoid the dereferencing of sslconn->server
> over and over again but this might be only a minor issue.
Fine with me - let me know if v8 includes the changes you had in mind.
Kaspar
Re: SNI in 2.2.x (Re: Time for 2.2.10?)
Posted by Ruediger Pluem <rp...@apache.org>.
On 04/15/2009 10:31 AM, Kaspar Brand wrote:
>
> Correct, again. Changed accordingly in v7 of the patch, which I'm
> attaching (together with an interdiff from v6). I have tested this
> version with quite many different vhosts, per-vhost and per-dir
> settings, and with both SNI and non-SNI clients... but of course
> it's still possible that I missed something, so your review
> and additional testing is again very valuable and welcome.
Looks good. Some minor nitpicks:
1.
+ if ((r->server != sslconn->server)
+ && renegotiate
+ && (verify & SSL_VERIFY_FAIL_IF_NO_PEER_CERT)
+#ifndef OPENSSL_NO_TLSEXT
+ && !(SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name))
+#endif
Reviewing the code again I don't think we need to have
+#ifndef OPENSSL_NO_TLSEXT
+ && !(SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name))
+#endif
this condition at all. Lets look at the cases (always Apache *with* SNI support)
a. SNI enabled client, supplied SNI hostname, HTTP hostname header different from SNI hostname.
b. SNI enabled client, supplied SNI hostname, HTTP hostname header the same as SNI hostname.
c. Non SNI enabled client.
a. This results in a Bad Request in the Post Read Request hook (ssl_hook_ReadReq). So
we do not get here anymore.
b. In this case r->server == sslconn->server so we don't reach this condition.
c. In this case !(SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name)) is always true.
2. The whole
+ if ((r->server != sslconn->server)
+ && renegotiate
+ && (verify & SSL_VERIFY_FAIL_IF_NO_PEER_CERT)
+#ifndef OPENSSL_NO_TLSEXT
+ && !(SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name))
+#endif
+ )
can move inside the if block above (
+ if ((dc->nVerifyClient != SSL_CVERIFY_UNSET) ||
+ (sc->server->auth.verify_mode != SSL_CVERIFY_UNSET)) {
)
because only if get inside this block it can happen that
verify & SSL_VERIFY_FAIL_IF_NO_PEER_CERT
gets true. So we safe some checks in the case that
((dc->nVerifyClient != SSL_CVERIFY_UNSET) ||
(sc->server->auth.verify_mode != SSL_CVERIFY_UNSET))
is not true that will always fail and thus waste cycles even more
so as there are many SSL configurations outside that do not use
client certs at all.
This also means we can move the initialization of verify back
into the if block.
Furthermore I think that we need to check for CA list change in any case
that we need to renegotiate as even if we do not fail on SSL level due
to SSL_VERIFY_FAIL_IF_NO_PEER_CERT there could be other conditions later
on in the configuration (sslrequire / rewriterules) that check if we had been
successful with our check on SSL level to e.g. provide a nice error page
if we were not. So if we have changed the CA list we might signal success
to these downstream configuration options although there wasn't one because
we used the wrong CA list.
3. I created a var handshakeserver to avoid the dereferencing of sslconn->server
over and over again but this might be only a minor issue.
Regards
Rüdiger
Re: SNI in 2.2.x (Re: Time for 2.2.10?)
Posted by Kaspar Brand <ht...@velox.ch>.
Thanks for your time and review - much appreciated.
> + n = sslconn->verify_depth;
> + sslconn->verify_depth = (dc->nVerifyDepth != UNSET) ?
> + dc->nVerifyDepth : sc->server->auth.verify_depth;
> + if ((sslconn->verify_depth < n) ||
> + ((n == 0) && (sc->server->auth.verify_depth == 0))) {
> + renegotiate = TRUE;
> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
> + "Reduced client verification depth will force "
> + "renegotiation");
[...]
> I don't really understand this part of the patch. If the default host has
> a verify depth of lets say 10 and the virtual host with the correct name has 1
> then IMHO a renegotiation should happen but the code above does not seem to do this.
Good catch. When I was modifying the SSLVerifyDepth related code (only
for 2.2.x, at that time), I concentrated on doing "the right thing" for
persistent connections (keep-alive requests). Apparently I missed the
case you're bringing up in my tests then, but thanks to your
improvements with r757463, determining the depth of the current request
can now be done like this:
n = sslconn->verify_depth ?
sslconn->verify_depth :
(mySrvConfig(sslconn->server))->server->auth.verify_depth;
I.e., if it's a persistent connection, then verify_depth has been set
(by the previous run of ssl_hook_Access), and we use that one. If it's
the first connection, OTOH, then we have to figure out the default for
the respective vhost (where sslconn->server comes in handy).
> I don't get why the above code is only needed in the case that Openssl is SNI
> capable. IMHO this check is always needed or better regardless of SNI or not SNI
> only needed
>
> + if ((dc->nVerifyClient != SSL_CVERIFY_UNSET) ||
> + (sc->server->auth.verify_mode != SSL_CVERIFY_UNSET)) {
>
> Otherwise a compiler warning about verify might be used uninitialized tells you
> that something is wrong with the code anyway.
> The only thing that needs an ifndef above is
>
> +#ifndef OPENSSL_NO_TLSEXT
> + && !(SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name))
> +#endif
>
> which is always true in the non SNI case.
You're right, it doesn't hurt to add this check even if SNI support
isn't compiled in. When I added that code, I was mainly having the
backport to 2.2.x in mind, and therefore tried to avoid changes which
would also have an effect if SNI support wasn't compiled (probably not
completely consistent in all cases, though).
As far as the initialization of verify is concerned, that's another good
point you raise. In the httpd-2.2.x-sni.20080928.patch (from last
September), I removed the if clause around the code determining the new
verify mode, so verify was always initialized. Your change in r757373
obviated the need for always calculating the new verify mode (for SNI
clients, mostly), and when I ported the latest 2.2.x changes to trunk
again (in sni_sslverifyclient-v6.diff), I wasn't careful enough to check
all the consequences of keeping that if clause, obviously.
I propose we just initialize verify when we declare it - that's the most
straightforward solution, IMO.
> One other thing: r->connection->base_server is IMHO always wrong in the patch
> and should be replaced with sslconn->server
Correct, again. Changed accordingly in v7 of the patch, which I'm
attaching (together with an interdiff from v6). I have tested this
version with quite many different vhosts, per-vhost and per-dir
settings, and with both SNI and non-SNI clients... but of course
it's still possible that I missed something, so your review
and additional testing is again very valuable and welcome.
Kaspar
Re: SNI in 2.2.x (Re: Time for 2.2.10?)
Posted by Ruediger Pluem <rp...@apache.org>.
On 04/08/2009 10:17 AM, Kaspar Brand wrote:
> Plüm, Rüdiger, VF-Group wrote:
>> I reviewed your patch and found some issues with it.
>
> Thanks for your review and testing, Rüdiger. I assume you used and
> adapted version of the sni_sslverifyclient-v5.diff patch, is that correct?
>
>> (All cases below use Name based virtual hosting and a non SNI client):
>>
>> 1. Renegotiation due to more restrictive cipher requirements:
>>
>> Lets say the first virtual host allows cipher A and B.
>> The handshake with the client decided to use A.
>> The virtual host the client switches to later also allows A and B.
>> But /restricted in this host only allows B.
>> In this case a request to /restricted does not cause a renegotiation
>> but it should.
>
> Right. It also applies to SNI clients, actually, and the problem is that
> the logic of this code (added in sni_sslverifyclient-v4.diff) is flawed:
>
> if ((dc->szCipherSuite &&
> !modssl_set_cipher_list(ssl, dc->szCipherSuite)) ||
> (sc->server->auth.cipher_suite &&
> !modssl_set_cipher_list(ssl, sc->server->auth.cipher_suite))) {
>
> - it will override the per-dir setting for the cipher suite with that
> from the vhost level, if the latter is also set. Changing these lines to
>
> if ((dc->szCipherSuite || sc->server->auth.cipher_suite) &&
> !modssl_set_cipher_list(ssl, dc->szCipherSuite ?
> dc->szCipherSuite :
> sc->server->auth.cipher_suite)) {
>
> resolves this issue for me.
>
>> 2. The verification depth check causes unneeded renegotiations which
>> break the ssl v2 tests in the perl framework (No dicussion here please
>> whether we should still support SSL v2 :-))
>
> This is an issue I already addressed in the patch for 2.2.x
> (http://sni.velox.ch/httpd-2.2.x-sni.20080928.patch), but I guess you
> were testing a trunk version without these changes, is that correct?
Correct.
@@ -462,26 +460,17 @@ int ssl_hook_Access(request_rec *r)
* currently active/remembered verify depth (because this means more
* restriction on the certificate chain).
*/
- if ((sc->server->auth.verify_depth != UNSET) &&
- (dc->nVerifyDepth == UNSET)) {
- /* apply per-vhost setting, if per-directory config is not set */
- dc->nVerifyDepth = sc->server->auth.verify_depth;
+ n = sslconn->verify_depth;
+ sslconn->verify_depth = (dc->nVerifyDepth != UNSET) ?
+ dc->nVerifyDepth : sc->server->auth.verify_depth;
+ if ((sslconn->verify_depth < n) ||
+ ((n == 0) && (sc->server->auth.verify_depth == 0))) {
+ renegotiate = TRUE;
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
+ "Reduced client verification depth will force "
+ "renegotiation");
}
- if (dc->nVerifyDepth != UNSET) {
- /* XXX: doesnt look like sslconn->verify_depth is actually used */
- if (!(n = sslconn->verify_depth)) {
- sslconn->verify_depth = n = sc->server->auth.verify_depth;
- }
·
- /* determine whether a renegotiation has to be forced */
- if (dc->nVerifyDepth < n) {
- renegotiate = TRUE;
- ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
- "Reduced client verification depth will force "
- "renegotiation");
- }
- }
-
/*
* override of SSLVerifyClient
*
I don't really understand this part of the patch. If the default host has
a verify depth of lets say 10 and the virtual host with the correct name has 1
then IMHO a renegotiation should happen but the code above does not seem to do this.
·
+#ifndef OPENSSL_NO_TLSEXT
+ /* If we're handling a request for a vhost other than the default one,
+ * then we need to make sure that client authentication is properly
+ * enforced. For clients supplying an SNI extension, the peer certificate
+ * verification has happened in the handshake already. For non-SNI requests,
+ * an additional check is needed here. If client authentication is
+ * configured as mandatory, then we can only proceed if the CA list
+ * doesn't have to be changed (SSL_set_cert_store() would be required
+ * for this).
+ */
+#define MODSSL_CFG_CA_NE(f, sc1, sc2) \
+ (sc1->server->auth.f && \
+ (!sc2->server->auth.f || \
+ sc2->server->auth.f && strNE(sc1->server->auth.f, sc2->server->auth.f)))
+
+ if ((r->server != r->connection->base_server) &&
+ (verify & SSL_VERIFY_FAIL_IF_NO_PEER_CERT) &&
+ renegotiate &&
+ !(SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name))) {
+ SSLSrvConfigRec *bssc = mySrvConfig(r->connection->base_server);
+
+ if (MODSSL_CFG_CA_NE(ca_cert_file, sc, bssc) ||
+ MODSSL_CFG_CA_NE(ca_cert_path, sc, bssc)) {
+ ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
+ "Non-default virtual host with SSLVerify set to 'require' "
+ "and VirtualHost-specific CA certificate list is only "
+ "supported for clients with TLS server name indication "
+ "(SNI) support");
+ return HTTP_FORBIDDEN;
+ }
+ }
+#endif /* OPENSSL_NO_TLSEXT */
+
I don't get why the above code is only needed in the case that Openssl is SNI
capable. IMHO this check is always needed or better regardless of SNI or not SNI
only needed
+ if ((dc->nVerifyClient != SSL_CVERIFY_UNSET) ||
+ (sc->server->auth.verify_mode != SSL_CVERIFY_UNSET)) {
Otherwise a compiler warning about verify might be used uninitialized tells you
that something is wrong with the code anyway.
The only thing that needs an ifndef above is
+#ifndef OPENSSL_NO_TLSEXT
+ && !(SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name))
+#endif
which is always true in the non SNI case.
One other thing: r->connection->base_server is IMHO always wrong in the patch
and should be replaced with sslconn->server
Regards
Rüdiger
Re: SNI in 2.2.x (Re: Time for 2.2.10?)
Posted by Kaspar Brand <ht...@velox.ch>.
Plüm, Rüdiger, VF-Group wrote:
> I reviewed your patch and found some issues with it.
Thanks for your review and testing, Rüdiger. I assume you used and
adapted version of the sni_sslverifyclient-v5.diff patch, is that correct?
> (All cases below use Name based virtual hosting and a non SNI client):
>
> 1. Renegotiation due to more restrictive cipher requirements:
>
> Lets say the first virtual host allows cipher A and B.
> The handshake with the client decided to use A.
> The virtual host the client switches to later also allows A and B.
> But /restricted in this host only allows B.
> In this case a request to /restricted does not cause a renegotiation
> but it should.
Right. It also applies to SNI clients, actually, and the problem is that
the logic of this code (added in sni_sslverifyclient-v4.diff) is flawed:
if ((dc->szCipherSuite &&
!modssl_set_cipher_list(ssl, dc->szCipherSuite)) ||
(sc->server->auth.cipher_suite &&
!modssl_set_cipher_list(ssl, sc->server->auth.cipher_suite))) {
- it will override the per-dir setting for the cipher suite with that
from the vhost level, if the latter is also set. Changing these lines to
if ((dc->szCipherSuite || sc->server->auth.cipher_suite) &&
!modssl_set_cipher_list(ssl, dc->szCipherSuite ?
dc->szCipherSuite :
sc->server->auth.cipher_suite)) {
resolves this issue for me.
> 2. The verification depth check causes unneeded renegotiations which
> break the ssl v2 tests in the perl framework (No dicussion here please
> whether we should still support SSL v2 :-))
This is an issue I already addressed in the patch for 2.2.x
(http://sni.velox.ch/httpd-2.2.x-sni.20080928.patch), but I guess you
were testing a trunk version without these changes, is that correct?
> There might be further issues but I currently have no time to check.
>
> I think we both agree that without this patch from you name based virtual
> hosting with SSL is definitely unsafe.
> I haven't analyzed any further if the above issues are fixable or not
> and I admit that I currently have no resources to do so.
I'm attaching a new patch (against r763127, i.e. current trunk), which
addresses both issues. Would very much appreciate if you could have a
look at it / give it a try, as it would definitely improve the situation
regarding SNI support in mod_ssl.
Kaspar
Re: SNI in 2.2.x (Re: Time for 2.2.10?)
Posted by Henri Gomez <he...@gmail.com>.
I'm working on securing massive NameVirtualHost sites using SSL.
The SNI support should be avoided since we needed a stock Apache 2.x /
mod_ssl solution, so it prevent us to take a look at
mod_gnutls/gnutls.
Question : How hard will it be to have SNI support conditional and
activated/disabled by an Apache directive ?
Leaving this as disabled by default will preserve the current
situation but will allow advanced configuration to use SNI.
Just a suggestion.
Regards
2009/4/7 "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>:
>
>
>> -----Ursprüngliche Nachricht-----
>> Von: Kaspar Brand
>> Gesendet: Montag, 30. März 2009 18:15
>> An: dev@httpd.apache.org
>> Betreff: Re: SNI in 2.2.x (Re: Time for 2.2.10?)
>>
>> Ruediger Pluem wrote:
>> > Going through the archive I noticed several attachments
>> with the same
>> > basename and and a version string attached. Are these patches
>> > cumulative so that I only need to review the latest one?
>>
>> sni_sslverifyclient-v5.diff includes all improvements to
>> ssl_hook_Access/ssl_callback_SSLVerify/ssl_callback_SSLVerify_CRL
>> which I did in June 2008, yes. Then I stopped updating the
>> trunk version
>> (due to lack of responses) and only worked on further
>> improvements on to
>> the 2.2.x patch (latest version lives at
>> http://sni.velox.ch/httpd-2.2.x-sni.20080928.patch).
>>
>> > As said several times we consider name based virtual
>> hosting for SSL
>> > *without* SNI as not recommended and insecure. This has not changed
>> > with the SNI patches applied to trunk.
>>
>> Other than being the ceterum censeo, what's the rationale here? "Not
>> recommended and insecure" sounds like FUD to me, and apparently no one
>> has spent at least a few hours to verify (or disprove) the analysis I
>> undertook last June.
>
> I reviewed your patch and found some issues with it.
>
> (All cases below use Name based virtual hosting and a non SNI client):
>
> 1. Renegotiation due to more restrictive cipher requirements:
>
> Lets say the first virtual host allows cipher A and B.
> The handshake with the client decided to use A.
> The virtual host the client switches to later also allows A and B.
> But /restricted in this host only allows B.
> In this case a request to /restricted does not cause a renegotiation
> but it should.
>
> 2. The verification depth check causes unneeded renegotiations which
> break the ssl v2 tests in the perl framework (No dicussion here please
> whether we should still support SSL v2 :-))
>
> There might be further issues but I currently have no time to check.
>
> I think we both agree that without this patch from you name based virtual
> hosting with SSL is definitely unsafe.
> I haven't analyzed any further if the above issues are fixable or not
> and I admit that I currently have no resources to do so.
>
> Regards
>
> Rüdiger
>
Re: SNI in 2.2.x (Re: Time for 2.2.10?)
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Ursprüngliche Nachricht-----
> Von: Kaspar Brand
> Gesendet: Montag, 30. März 2009 18:15
> An: dev@httpd.apache.org
> Betreff: Re: SNI in 2.2.x (Re: Time for 2.2.10?)
>
> Ruediger Pluem wrote:
> > Going through the archive I noticed several attachments
> with the same
> > basename and and a version string attached. Are these patches
> > cumulative so that I only need to review the latest one?
>
> sni_sslverifyclient-v5.diff includes all improvements to
> ssl_hook_Access/ssl_callback_SSLVerify/ssl_callback_SSLVerify_CRL
> which I did in June 2008, yes. Then I stopped updating the
> trunk version
> (due to lack of responses) and only worked on further
> improvements on to
> the 2.2.x patch (latest version lives at
> http://sni.velox.ch/httpd-2.2.x-sni.20080928.patch).
>
> > As said several times we consider name based virtual
> hosting for SSL
> > *without* SNI as not recommended and insecure. This has not changed
> > with the SNI patches applied to trunk.
>
> Other than being the ceterum censeo, what's the rationale here? "Not
> recommended and insecure" sounds like FUD to me, and apparently no one
> has spent at least a few hours to verify (or disprove) the analysis I
> undertook last June.
I reviewed your patch and found some issues with it.
(All cases below use Name based virtual hosting and a non SNI client):
1. Renegotiation due to more restrictive cipher requirements:
Lets say the first virtual host allows cipher A and B.
The handshake with the client decided to use A.
The virtual host the client switches to later also allows A and B.
But /restricted in this host only allows B.
In this case a request to /restricted does not cause a renegotiation
but it should.
2. The verification depth check causes unneeded renegotiations which
break the ssl v2 tests in the perl framework (No dicussion here please
whether we should still support SSL v2 :-))
There might be further issues but I currently have no time to check.
I think we both agree that without this patch from you name based virtual
hosting with SSL is definitely unsafe.
I haven't analyzed any further if the above issues are fixable or not
and I admit that I currently have no resources to do so.
Regards
Rüdiger
Re: SNI in 2.2.x (Re: Time for 2.2.10?)
Posted by Kaspar Brand <ht...@velox.ch>.
Ruediger Pluem wrote:
> Going through the archive I noticed several attachments with the same
> basename and and a version string attached. Are these patches
> cumulative so that I only need to review the latest one?
sni_sslverifyclient-v5.diff includes all improvements to
ssl_hook_Access/ssl_callback_SSLVerify/ssl_callback_SSLVerify_CRL
which I did in June 2008, yes. Then I stopped updating the trunk version
(due to lack of responses) and only worked on further improvements on to
the 2.2.x patch (latest version lives at
http://sni.velox.ch/httpd-2.2.x-sni.20080928.patch).
> As said several times we consider name based virtual hosting for SSL
> *without* SNI as not recommended and insecure. This has not changed
> with the SNI patches applied to trunk.
Other than being the ceterum censeo, what's the rationale here? "Not
recommended and insecure" sounds like FUD to me, and apparently no one
has spent at least a few hours to verify (or disprove) the analysis I
undertook last June.
Locking out non-SNI capable clients from all but the first vhost is
overkill for many configurations (e.g. when no SSL access restrictions
are in place), and will break configurations which used to work with
2.2. People *are* doing things like those shown at
http://wiki.cacert.org/wiki/CSRGenerator to address their needs, and
this would break with your change suggested for 2.4 ("because we always
said..." - yes, I know, that will be your [compelling?] argument).
> Not SNI enabled clients receive a 403 when contacting any of
> the name based virtual hosts (which enables you to set a nice error
> page to explain to the user what happened and which browser to use).
Why hardcode HTTP_FORBIDDEN? Leave it to the user to configure it
as needed, as I outlined an earlier posting:
> * 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} =""'
Kaspar