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/httpd-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? > 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=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 ssl_init_ctx_tls_extensions( > modssl_init_stapling(s, p, ptemp, mctx); > } > #endif > + > +#ifndef OPENSSL_NO_SRP > + /* > + * TLS-SRP support > + */ > + if (mctx->srp_vfile != NULL) { > + int rv; > + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(02308) > + "Using SRP verifier file [%s]", mctx->srp_vfile); > + > + if (!(mctx->srp_vbase = SRP_VBASE_new(mctx->srp_unknown_user_seed))) > { > + ap_log_error(APLOG_MARK, APLOG_EMERG, 0, s, APLOGNO(02309) > + "Unable to initialize SRP verifier structure " > + "[%s seed]", > + mctx->srp_unknown_user_seed ? "with" : "without"); > + ssl_log_ssl_error(SSLLOG_MARK, APLOG_EMERG, s); > + ssl_die(); > + } > + > + rv = SRP_VBASE_init(mctx->srp_vbase, mctx->srp_vfile); > + if (rv != SRP_NO_ERROR) { > + ap_log_error(APLOG_MARK, APLOG_EMERG, 0, s, APLOGNO(02310) > + "Unable to load SRP verifier file [error %d]", rv); > + ssl_log_ssl_error(SSLLOG_MARK, APLOG_EMERG, s); > + ssl_die(); > + } > + > + SSL_CTX_set_srp_username_callback(mctx->ssl_ctx, > + ssl_callback_SRPServerParams); > + SSL_CTX_set_srp_cb_arg(mctx->ssl_ctx, mctx); > + } > +#endif > } > #endif 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.) > 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=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 > @@ -2226,4 +2243,30 @@ int ssl_callback_AdvertiseNextProtos(SSL > *size_out = size; > return SSL_TLSEXT_ERR_OK; > } > -#endif > + > +#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. > Modified: httpd/httpd/trunk/modules/ssl/ssl_private.h > URL: > http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_private.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 Kaspar
