Re: [PATCH] Add support for SASL security layers to ra_svn

Vlad Georgescu wrote:
> Hi,
>
> Some SASL mechanisms (e.g. DIGEST-MD5, GSSAPI, etc.) support security
> layers (i.e. encryption). This patch adds support for such layers,
> using the new svn_ra_svn__stream_t interface introduced in r22238. The
> presence and level of encryption are negotiated during the
> authentication exchange. On the server side, two new svnserve.conf
> options (min-encryption and max-encryption) can be used in this
> negotiation. For example, setting both to 0 would disable encryption,
> setting min-encryption to 128 would require at least 128-bit
> encryption etc.

Ping...

Has any of the committers looked at Vlad's patch? If nothing happens
with it in the next few days, I'll go ahead and file an issue.

-Hyrum

> [[[
> Add support for SASL security layers to ra_svn.
>
> * subversion/libsvn_ra_svn/ra_svn.h
> (svn_ra_svn_conn_st): New member 'encrypted'.
>
> * subvesion/libsvn_ra_svn/marshal.c
> (svn_ra_svn_create_conn): Initialize conn->encrypted.
>
> * subversion/libsvn_ra_svn/sasl_auth.c
> (sasl_baton_t): New typedef.
> (sasl_read_cb, sasl_write_cb, sasl_timeout_cb, sasl_pending_cb): Implement
> the svn_ra_svn__stream_t interface.
> (svn_ra_svn__enable_sasl_encryption): New function. Wraps the existing
> stream with a SASL encrypted stream.
> (svn_ra_svn__do_auth): Make sure sasl_ctx has the same lifetime as the
> connection. Call svn_ra_svn__enable_sasl_encryption.
>
> * subversion/include/private/ra_svn_sasl.h
> (svn_ra_svn__enable_sasl_encryption): New declaration.
>
> * subversion/include/svn_config.h
> (SVN_CONFIG_OPTION_MIN_SSF, SVN_CONFIG_OPTION_MAX_SSF): New options for
> svnserve.conf.
>
> * subversion/libsvn_repos/repos.c
> (create_conf): Add the new options to the default svnserve.conf.
>
> * subversion/svnserve/sasl_auth.c
> (sasl_auth_request): Make sure sasl_ctx has the same lifetime as the
> connection. Read the new svnserve.conf options. Call
> svn_ra_svn__enable_sasl_encryption after the authentication exchange.
> ]]]
>
>
> ------------------------------------------------------------------------
>
> Index: subversion/include/private/ra_svn_sasl.h
> ===================================================================
> --- subversion/include/private/ra_svn_sasl.h (revision 22383)
> +++ subversion/include/private/ra_svn_sasl.h (working copy)
> @@ -50,6 +50,13 @@
> const char **remote_addrport,
> svn_ra_svn_conn_t *conn,
> apr_pool_t *pool);
> +
> +/* If a security layer was negotiated during the authentication exchange,
> + create an encrypted stream for conn. */
> +svn_error_t *svn_ra_svn__enable_sasl_encryption(svn_ra_svn_conn_t *conn,
> + sasl_conn_t *sasl_ctx,
> + apr_pool_t *pool);
> +
> #ifdef __cplusplus
> }
> #endif /* __cplusplus */
> Index: subversion/include/svn_config.h
> ===================================================================
> --- subversion/include/svn_config.h (revision 22383)
> +++ subversion/include/svn_config.h (working copy)
> @@ -107,6 +107,8 @@
> #define SVN_CONFIG_OPTION_AUTHZ_DB "authz-db"
> #define SVN_CONFIG_SECTION_SASL "sasl"
> #define SVN_CONFIG_OPTION_USE_SASL "use-sasl"
> +#define SVN_CONFIG_OPTION_MIN_SSF "min-encryption"
> +#define SVN_CONFIG_OPTION_MAX_SSF "max-encryption"
>
> /* For repository password database */
> #define SVN_CONFIG_SECTION_USERS "users"
> Index: subversion/libsvn_ra_svn/marshal.c
> ===================================================================
> --- subversion/libsvn_ra_svn/marshal.c (revision 22383)
> +++ subversion/libsvn_ra_svn/marshal.c (working copy)
> @@ -52,6 +52,7 @@
> assert((sock && !in_file && !out_file) || (!sock && in_file && out_file));
> #ifdef SVN_HAVE_SASL
> conn->sock = sock;
> + conn->encrypted = FALSE;
> #endif
> conn->session = NULL;
> conn->read_ptr = conn->read_buf;
> Index: subversion/libsvn_ra_svn/ra_svn.h
> ===================================================================
> --- subversion/libsvn_ra_svn/ra_svn.h (revision 22383)
> +++ subversion/libsvn_ra_svn/ra_svn.h (working copy)
> @@ -68,6 +68,7 @@
> interface, SASL still needs direct access to the underlying socket
> for stuff like IP addresses and port numbers. */
> apr_socket_t *sock;
> + svn_boolean_t encrypted;
> #endif
> char read_buf[SVN_RA_SVN__READBUF_SIZE];
> char *read_ptr;
> Index: subversion/libsvn_ra_svn/sasl_auth.c
> ===================================================================
> --- subversion/libsvn_ra_svn/sasl_auth.c (revision 22383)
> +++ subversion/libsvn_ra_svn/sasl_auth.c (working copy)
> @@ -443,6 +443,186 @@
> return SVN_NO_ERROR;
> }
>
> +/* Baton for a SASL encrypted svn_ra_svn__stream_t. */
> +typedef struct sasl_baton {
> + svn_ra_svn__stream_t *stream; /* Inherited stream. */
> + sasl_conn_t *ctx; /* The SASL context for this connection. */
> + unsigned int maxsize; /* The maximum amount of data we can encode. */
> + const char *read_buf; /* The buffer returned by sasl_decode. */
> + unsigned int read_len; /* Its current length. */
> + const char *write_buf; /* The buffer returned by sasl_encode. */
> + unsigned int write_len; /* Its length. */
> +} sasl_baton_t;
> +
> +/* Functions to implement a SASL encrypted svn_ra_svn__stream_t. */
> +
> +/* Implements svn_read_fn_t. */
> +static svn_error_t *sasl_read_cb(void *baton, char *buffer, apr_size_t *len)
> +{
> + sasl_baton_t *sasl_baton = baton;
> + int result;
> + /* A copy of *len, used by the wrapped stream. */
> + unsigned int len2 = *len;
> +
> + /* sasl_decode might need more data than a single read can provide,
> + hence the need to put a loop around the decoding. */
> + while (! sasl_baton->read_buf || sasl_baton->read_len == 0)
> + {
> + SVN_ERR(svn_ra_svn__stream_read(sasl_baton->stream, buffer, &len2));
> + if (len2 == 0)
> + {
> + *len = 0;
> + return SVN_NO_ERROR;
> + }
> + result = sasl_decode(sasl_baton->ctx, buffer, len2,
> + &sasl_baton->read_buf,
> + &sasl_baton->read_len);
> + if (result != SASL_OK)
> + return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
> + sasl_errdetail(sasl_baton->ctx));
> + }
> +
> + /* The buffer returned by sasl_decode might be larger than what the
> + caller wants. If this is the case, we only copy back *len bytes now
> + (the rest will be returned by subsequent calls to this function).
> + If not, we just copy back the whole thing. */
> + if (*len >= sasl_baton->read_len)
> + {
> + memcpy(buffer, sasl_baton->read_buf, sasl_baton->read_len);
> + *len = sasl_baton->read_len;
> + sasl_baton->read_buf = NULL;
> + sasl_baton->read_len = 0;
> + }
> + else
> + {
> + memcpy(buffer, sasl_baton->read_buf, *len);
> + sasl_baton->read_len -= *len;
> + sasl_baton->read_buf += *len;
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> +/* Implements svn_write_fn_t. */
> +static svn_error_t *
> +sasl_write_cb(void *baton, const char *buffer, apr_size_t *len)
> +{
> + sasl_baton_t *sasl_baton = baton;
> + int result;
> +
> + if (! sasl_baton->write_buf || sasl_baton->write_len == 0)
> + {
> + /* Make sure we don't write too much. */
> + *len = (*len > sasl_baton->maxsize) ? sasl_baton->maxsize : *len;
> + result = sasl_encode(sasl_baton->ctx, buffer, *len,
> + &sasl_baton->write_buf,
> + &sasl_baton->write_len);
> +
> + if (result != SASL_OK)
> + return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
> + sasl_errdetail(sasl_baton->ctx));
> + }
> +
> + do
> + {
> + unsigned int tmplen = sasl_baton->write_len;
> + SVN_ERR(svn_ra_svn__stream_write(sasl_baton->stream,
> + sasl_baton->write_buf,
> + &tmplen));
> + if (tmplen == 0)
> + {
> + /* The output buffer and its length will be preserved in sasl_baton
> + and will be written out during the next call to this function
> + (which will have the same arguments). */
> + *len = 0;
> + return SVN_NO_ERROR;
> + }
> + sasl_baton->write_len -= tmplen;
> + sasl_baton->write_buf += tmplen;
> + }
> + while (sasl_baton->write_len > 0);
> +
> + sasl_baton->write_buf = NULL;
> + sasl_baton->write_len = 0;
> +
> + return SVN_NO_ERROR;
> +}
> +
> +/* Implements ra_svn_timeout_fn_t. */
> +static void sasl_timeout_cb(void *baton, apr_interval_time_t interval)
> +{
> + sasl_baton_t *sasl_baton = baton;
> + svn_ra_svn__stream_timeout(sasl_baton->stream, interval);
> +}
> +
> +/* Implements ra_svn_pending_fn_t. */
> +static svn_boolean_t sasl_pending_cb(void *baton)
> +{
> + sasl_baton_t *sasl_baton = baton;
> + return svn_ra_svn__stream_pending(sasl_baton->stream);
> +}
> +
> +svn_error_t *svn_ra_svn__enable_sasl_encryption(svn_ra_svn_conn_t *conn,
> + sasl_conn_t *sasl_ctx,
> + apr_pool_t *pool)
> +{
> + sasl_baton_t *sasl_baton;
> + const sasl_ssf_t *ssfp;
> + int result;
> + const void *maxsize;
> +
> + if (! conn->encrypted)
> + {
> + /* Get the strength of the security layer. */
> + result = sasl_getprop(sasl_ctx, SASL_SSF, (void*) &ssfp);
> + if (result != SASL_OK)
> + return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
> + sasl_errdetail(sasl_ctx));
> +
> + if (*ssfp > 0)
> + {
> + /* Flush the connection, as we're about to replace its stream. */
> + SVN_ERR(svn_ra_svn_flush(conn, pool));
> +
> + /* Create and initialize the stream baton. */
> + sasl_baton = apr_pcalloc(conn->pool, sizeof(*sasl_baton));
> + sasl_baton->ctx = sasl_ctx;
> +
> + /* Find out the maximum input size for sasl_encode. */
> + result = sasl_getprop(sasl_ctx, SASL_MAXOUTBUF, &maxsize);
> + if (result != SASL_OK)
> + return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
> + sasl_errdetail(sasl_ctx));
> + sasl_baton->maxsize = *((unsigned int *) maxsize);
> +
> + /* If there is any data left in the read buffer at this point,
> + we need to decrypt it. */
> + if (conn->read_end > conn->read_ptr)
> + {
> + result = sasl_decode(sasl_ctx, conn->read_ptr,
> + conn->read_end - conn->read_ptr,
> + &sasl_baton->read_buf,
> + &sasl_baton->read_len);
> + if (result != SASL_OK)
> + return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
> + sasl_errdetail(sasl_ctx));
> + conn->read_end = conn->read_ptr;
> + }
> +
> + /* Wrap the existing stream. */
> + sasl_baton->stream = conn->stream;
> +
> + conn->stream = svn_ra_svn__stream_create(sasl_baton, sasl_read_cb,
> + sasl_write_cb,
> + sasl_timeout_cb,
> + sasl_pending_cb, conn->pool);
> + /* Yay, we have a security layer! */
> + conn->encrypted = TRUE;
> + }
> + }
> + return SVN_NO_ERROR;
> +}
> +
> svn_error_t *svn_ra_svn__get_addresses(const char **local_addrport,
> const char **remote_addrport,
> svn_ra_svn_conn_t *conn,
> @@ -556,7 +736,7 @@
>
> SVN_ERR(new_sasl_ctx(&sasl_ctx, sess->is_tunneled,
> hostname, local_addrport, remote_addrport,
> - pool));
> + sess->conn->pool));
> SVN_ERR(try_auth(sess,
> sasl_ctx,
> creds,
> @@ -578,6 +758,8 @@
> while (!success);
> svn_pool_destroy(subpool);
>
> + SVN_ERR(svn_ra_svn__enable_sasl_encryption(sess->conn, sasl_ctx, pool));
> +
> SVN_ERR(svn_auth_save_credentials(iterstate, pool));
>
> return SVN_NO_ERROR;
> Index: subversion/libsvn_repos/repos.c
> ===================================================================
> --- subversion/libsvn_repos/repos.c (revision 22383)
> +++ subversion/libsvn_repos/repos.c (working copy)
> @@ -1460,6 +1460,20 @@
> "### library for authentication. Default is false."
> APR_EOL_STR
> "# use-sasl = true"
> + APR_EOL_STR
> + "### These options specify the desired strength of the security layer"
> + APR_EOL_STR
> + "### that you want SASL to provide. 0 means no encryption, 1 means"
> + APR_EOL_STR
> + "### integrity-checking only, values larger than 1 are correlated"
> + APR_EOL_STR
> + "### to the effective key length for encryption (e.g. 128 means 128-bit"
> + APR_EOL_STR
> + "### encryption). The values below are the defaults."
> + APR_EOL_STR
> + "# min-encryption = 0"
> + APR_EOL_STR
> + "# max-encryption = 256"
> #endif
> APR_EOL_STR;
>
> Index: subversion/svnserve/sasl_auth.c
> ===================================================================
> --- subversion/svnserve/sasl_auth.c (revision 22383)
> +++ subversion/svnserve/sasl_auth.c (working copy)
> @@ -229,7 +229,7 @@
> apr_pool_t *subpool;
> apr_status_t apr_err;
> const char *localaddrport = NULL, *remoteaddrport = NULL;
> - const char *mechlist;
> + const char *mechlist, *val;
> char hostname[APRMAXHOSTLEN + 1];
> sasl_security_properties_t secprops;
> svn_boolean_t success, no_anonymous;
> @@ -261,7 +261,7 @@
> }
>
> /* Make sure the context is always destroyed. */
> - apr_pool_cleanup_register(pool, sasl_ctx, sasl_dispose_cb,
> + apr_pool_cleanup_register(b->pool, sasl_ctx, sasl_dispose_cb,
> apr_pool_cleanup_null);
>
> /* Initialize security properties. */
> @@ -275,6 +275,18 @@
> if (no_anonymous)
> secprops.security_flags |= SASL_SEC_NOANONYMOUS;
>
> + svn_config_get(b->cfg, &val,
> + SVN_CONFIG_SECTION_SASL,
> + SVN_CONFIG_OPTION_MIN_SSF,
> + "0");
> + secprops.min_ssf = atoi(val);
> +
> + svn_config_get(b->cfg, &val,
> + SVN_CONFIG_SECTION_SASL,
> + SVN_CONFIG_OPTION_MAX_SSF,
> + "256");
> + secprops.max_ssf = atoi(val);
> +
> /* Set security properties. */
> result = sasl_setprop(sasl_ctx, SASL_SEC_PROPS, &secprops);
> if (result != SASL_OK)
> @@ -316,6 +328,8 @@
> while (!success);
> svn_pool_destroy(subpool);
>
> + SVN_ERR(svn_ra_svn__enable_sasl_encryption(conn, sasl_ctx, pool));
> +
> if (no_anonymous)
> {
> char *p;