Hi Kaspar, thanks for the review.
On Sunday 10 June 2012, Kaspar Brand wrote: > On 08.06.2012 11:38, [email protected] 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
