httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kaspar Brand <httpd-dev.2...@velox.ch>
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 08:13:13 GMT
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/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

Mime
View raw message