httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Fritsch ...@sfritsch.de>
Subject Re: svn commit: r1347980 - in /httpd/httpd/trunk: ./ docs/conf/extra/ docs/log-message-tags/ docs/manual/mod/ docs/manual/ssl/ modules/ssl/
Date Sun, 10 Jun 2012 19:58:54 GMT
Hi Kaspar,

thanks for the review.

On Sunday 10 June 2012, Kaspar Brand wrote:
> On 08.06.2012 11:38, sf@apache.org wrote:
> > Author: sf
> > Date: Fri Jun  8 09:38:44 2012
> > New Revision: 1347980
> > 
> > URL: http://svn.apache.org/viewvc?rev=1347980&view=rev
> > Log:
> > Add support for TLS-SRP (Secure Remote Password key exchange
> > for TLS, RFC 5054).
> > 
> > PR: 51075
> > Submitted by: Quinn Slack <sqs cs stanford edu>, Christophe
> > Renou,
> > 
> >               Peter Sylvester
> 
> [...]
> 
> > Modified: httpd/httpd/trunk/docs/conf/extra/httpd-ssl.conf.in
> > URL:
> > http://svn.apache.org/viewvc/httpd/httpd/trunk/docs/conf/extra/h
> > ttpd-ssl.conf.in?rev=1347980&r1=1347979&r2=1347980&view=diff
> > ================================================================
> > ============== ---
> > httpd/httpd/trunk/docs/conf/extra/httpd-ssl.conf.in (original)
> > +++ httpd/httpd/trunk/docs/conf/extra/httpd-ssl.conf.in Fri Jun 
> > 8 09:38:44 2012 @@ -157,6 +157,12 @@ SSLCertificateKeyFile
> > "@exp_sysconfdir@/
> > 
> >  #SSLVerifyClient require
> >  #SSLVerifyDepth  10
> > 
> > +#   TLS-SRP mutual authentication:
> > +#   Enable TLS-SRP and set the path to the OpenSSL SRP verifier
> > +#   file (containing login information for SRP user accounts).
> > See +#   the mod_ssl FAQ for instructions on creating this file.
> > +#SSLSRPVerifierFile "@exp_sysconfdir@/passwd.srpv"
> > +
> 
> As a matter of style / documentation "policy", I would prefer if
> the setup instructions in the reference documentation
> (mod_ssl.xml) are self-contained, i.e. people should not have to
> look at the FAQ to get this kind of information.
> 
> Maybe we should also add a notice about SRP support only being
> available if compiled against OpenSSL 1.0.1 or later?

done in r1348653

> 
> > Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
> > URL:
> > http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_e
> > ngine_init.c?rev=1347980&r1=1347979&r2=1347980&view=diff
> > ================================================================
> > ============== ---
> > httpd/httpd/trunk/modules/ssl/ssl_engine_init.c (original) +++
> > httpd/httpd/trunk/modules/ssl/ssl_engine_init.c Fri Jun  8
> > 09:38:44 2012 @@ -526,6 +526,38 @@ static void

> 
> For the sake of consistent style, I suggest removing the "rv"
> variable and use
> 
>   if (SRP_VBASE_init(mctx->srp_vbase, mctx->srp_vfile) !=
> SRP_NO_ERROR)
> 
> instead. (In other places in mod_ssl, rv is typically declared as
> apr_status_t and used for APR calls. Furthermore, we shouldn't have
> to specifically log the SRP_ERR_* return code - OpenSSL's SRP code
> should stuff this information into OpenSSL's error queue, which we
> pick up in ssl_log_ssl_error.)

Unfortunately, ssl_log_ssl_error() doesn't log any error. Instead 
openssl logs to stderr (newlines doubled by me for clarity):

Sun Jun 10 21:21:46.051674 2012] [ssl:info] [pid 6734:tid 4148467456] 
AH01914: Configuring server localhost:443 for SSL protocol

wrong number of fields on line 1 (looking for field 6, got 1, '' left)

[Sun Jun 10 21:21:46.051806 2012] [ssl:emerg] [pid 6734:tid 
4148467456] AH02308: Unable to load SRP verifier file [error 1]


I have renamed rv to err, but I don't want to change the rest for now.
I think the "if ((var = foo(...)) != bar) " style is not very readable 
and try to avoid it if possible without adding too many lines.

> > Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c
> > URL:
> > http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_e
> > ngine_kernel.c?rev=1347980&r1=1347979&r2=1347980&view=diff
> > ================================================================
> > ============== ---
> > httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c (original) +++
> > httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c Fri Jun  8
> > 09:38:44 2012

> > +
> > +#endif /* HAVE_TLS_NPN */
> > +
> > +#ifndef OPENSSL_NO_SRP
> > +
> > +int ssl_callback_SRPServerParams(SSL *ssl, int *ad, void *arg)
> > +{
> > +    modssl_ctx_t *mctx = (modssl_ctx_t *)arg;
> > +    char *username = SSL_get_srp_username(ssl);
> > +    SRP_user_pwd *u;
> > +
> > +    if ((u = SRP_VBASE_get_by_user(mctx->srp_vbase, username))
> > == NULL) {
> 
> We should add a check for a non-NULL "username".
> SRP_VBASE_get_by_user() uses it for a strcmp, without explicitly
> checking for a non-NULL value before.

Good catch. Added.

> > Modified: httpd/httpd/trunk/modules/ssl/ssl_private.h
> > URL:
> > http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_p
> > rivate.h?rev=1347980&r1=1347979&r2=1347980&view=diff
> > ================================================================
> > ============== --- httpd/httpd/trunk/modules/ssl/ssl_private.h
> > (original) +++ httpd/httpd/trunk/modules/ssl/ssl_private.h Fri
> > Jun  8 09:38:44 2012 @@ -185,6 +185,13 @@
> > 
> >  #define HAVE_TLSV1_X
> >  #endif
> > 
> > +/* SRP support came in OpenSSL 1.0.1 */
> > +#if (OPENSSL_VERSION_NUMBER < 0x10001000)
> > +#define OPENSSL_NO_SRP
> > +#else
> > +#include <openssl/srp.h>
> > +#endif
> > +
> 
> If possible I prefer "feature-based" detection, i.e. in this case
> something like:
> 
> #ifdef SSL_CTRL_SET_TLS_EXT_SRP_USERNAME_CB &&
> !defined(OPENSSL_NO_SRP) #include <openssl/srp.h>
> #else
> #define OPENSSL_NO_SRP
> #endif

I will trust you that SSL_CTRL_SET_TLS_EXT_SRP_USERNAME_CB will not 
appear in 1.0.0 without full SRP support. The defines seem to be 
rather haphazard: Most are named *_TLSEXT_* but some new ones are 
*_TLS_EXT_*.

Cheers,
Stefan

Mime
View raw message