"Vlad Georgescu" <vgeorgescu@gmail.com> writes:
> --- subversion/libsvn_ra_svn/ra_svn_sasl.h (revision 0)
> +++ subversion/libsvn_ra_svn/ra_svn_sasl.h (revision 0)
> @@ -0,0 +1,48 @@
> +/*
> + * ra_svn_sasl.h : SASL-related declarations shared between the
> + * ra_svn and svnserve module
> + *
> + * ====================================================================
> + * Copyright (c) 2000-2004 CollabNet. All rights reserved.
Wrong date.
> + *
> + * This software is licensed as described in the file COPYING, which
> + * you should have received as part of this distribution. The terms
> + * are also available at http://subversion.tigris.org/license-1.html.
> + * If newer versions of this license are posted there, you may use a
> + * newer version instead, at your option.
> + *
> + * This software consists of voluntary contributions made by many
> + * individuals. For exact contribution history, see the revision
> + * history and logs, available at http://subversion.tigris.org/.
> + * ====================================================================
> + */
> +
> +
> +
> +#ifndef RA_SVN_SASL_H
> +#define RA_SVN_SASL_H
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif /* __cplusplus */
> +
> +#include <sasl/sasl.h>
> +
> +#define DEFAULT_SECPROPS {0, 256, 4096, 0, NULL, NULL}
If you are going to put "magic" numbers into a header file it's a good
idea to have a comment saying what they are and how you chose them.
> +
> +/* This function is called by the client and the server before
> + * calling sasl_{client, server}_init */
> +apr_status_t svn_ra_svn__sasl_common_init();
> +
> +/* Sets local_addrport and remote_addrport
> + * to a string containing the remote and local IP address and port,
> + * formatted like this: a.b.c.d;port */
> +void svn_ra_svn__get_addresses(apr_socket_t *sock,
> + char **local_addrport,
> + char **remote_addrport,
> + apr_pool_t *pool);
Is it really impossible for this function to fail?
> +#ifdef __cplusplus
> +}
> +#endif /* __cplusplus */
> +
> +#endif /* RA_SVN_SASL_H */
> Index: subversion/libsvn_ra_svn/sasl_auth.c
> ===================================================================
> --- subversion/libsvn_ra_svn/sasl_auth.c (revision 0)
> +++ subversion/libsvn_ra_svn/sasl_auth.c (revision 0)
> @@ -0,0 +1,540 @@
> +/*
> + * sasl_auth.c : Functions for SASL-based authentication
> + *
> + * ====================================================================
> + * Copyright (c) 2000-2006 CollabNet. All rights reserved.
Hmm, I think that should be 2006 without the 2000-.
> + *
> + * This software is licensed as described in the file COPYING, which
> + * you should have received as part of this distribution. The terms
> + * are also available at http://subversion.tigris.org/license-1.html.
> + * If newer versions of this license are posted there, you may use a
> + * newer version instead, at your option.
> + *
> + * This software consists of voluntary contributions made by many
> + * individuals. For exact contribution history, see the revision
> + * history and logs, available at http://subversion.tigris.org/.
> + * ====================================================================
> + */
> +
> +#define APR_WANT_STRFUNC
> +#include <apr_want.h>
> +#include <apr_general.h>
> +#include <apr_strings.h>
> +#include <apr_atomic.h>
> +#include <apr_thread_mutex.h>
> +#include <apr_version.h>
> +
> +#include "svn_types.h"
> +#include "svn_string.h"
> +#include "svn_error.h"
> +#include "svn_pools.h"
> +#include "svn_private_config.h"
> +#include "svn_ra.h"
> +#include "svn_ra_svn.h"
> +#include "svn_base64.h"
> +
> +#include "ra_svn.h"
> +#include "ra_svn_sasl.h"
> +
> +#ifdef SVN_HAVE_SASL
> +
> +#ifdef APR_HAS_THREADS
> +
> +/* Cyrus SASL is thread-safe only if we supply it with mutex functions
> + * (with sasl_set_mutex()). To make this work with APR, we need to use a
> + * global pool for the mutex allocations. Freeing a mutex actually
> + * returns it to a global array. We allocate mutexes from this
> + * array if it is non-empty, or directly from the pool otherwise.
> + * We also need a mutex to serialize accesses to the array itself.
> + */
> +
> +/* We allocate mutexes for SASL from this pool. */
> +static apr_pool_t *sasl_pool = NULL;
> +
> +/* An array of allocated, but unused, mutexes. */
Mention the type of the elements in the array.
> +static apr_array_header_t *free_mutexes = NULL;
> +
> +/* A mutex to serialize access to the array. */
> +static apr_thread_mutex_t *array_mutex;
= NULL for consistency with the two above.
> +
> +/* Callbacks we pass to sasl_set_mutex. */
> +
> +static void *sasl_mutex_alloc_cb(void)
> +{
> + apr_thread_mutex_t *mutex;
> + if (apr_is_empty_array(free_mutexes))
> + {
> + int r = apr_thread_mutex_create(&mutex,
> + APR_THREAD_MUTEX_DEFAULT,
> + sasl_pool);
> + if (r != APR_SUCCESS)
> + return NULL;
Use "apr_status_t status" not "int r".
> + }
> + else
> + {
> + apr_thread_mutex_lock(array_mutex);
A POSIX mutex won't usually fail, but ignoring errors is bad practice.
> + mutex = *((apr_thread_mutex_t**)apr_array_pop(free_mutexes));
> + apr_thread_mutex_unlock(array_mutex);
ditto
> + }
> + return mutex;
> +}
> +
> +static int sasl_mutex_lock_cb(void *mutex)
> +{
> + if (mutex)
> + return (apr_thread_mutex_lock(mutex) == APR_SUCCESS) ? 0 : -1;
> + else
> + return 0;
Is returning success the right thing to do if the mutex doesn't exist?
> +}
> +
> +static int sasl_mutex_unlock_cb(void *mutex)
> +{
> + if (mutex)
> + return (apr_thread_mutex_unlock(mutex) == APR_SUCCESS) ? 0 : -1;
> + else
> + return 0;
> +}
> +
> +static void sasl_mutex_free_cb(void *mutex)
> +{
> + apr_thread_mutex_lock(array_mutex);
Check for errors?
> + *((apr_thread_mutex_t**)apr_array_push(free_mutexes)) = mutex;
> + apr_thread_mutex_unlock(array_mutex);
> +}
> +#endif /* APR_HAS_THREADS */
> +
> +static sasl_callback_t interactions[] =
> +{
> + /* Use SASL interactions for username & password */
> + {SASL_CB_AUTHNAME, NULL, NULL},
> + {SASL_CB_PASS, NULL, NULL},
> + {SASL_CB_LIST_END, NULL, NULL}
> +};
> +
> +apr_status_t svn_ra_svn__sasl_common_init()
> +{
> + apr_status_t apr_err = APR_SUCCESS;
> +
> + atexit(sasl_done);
If the client calls this function more than once then it will register
more than one atexit handler. Is it safe to call sasl_done more than
once?
Is it safe to call sasl_done at all? What if some other library is
using SASL?
> +#ifdef APR_HAS_THREADS
> + sasl_set_mutex(sasl_mutex_alloc_cb,
> + sasl_mutex_lock_cb,
> + sasl_mutex_unlock_cb,
> + sasl_mutex_free_cb);
> + sasl_pool = svn_pool_create(NULL);
> + free_mutexes = apr_array_make(sasl_pool, 0, sizeof(apr_thread_mutex_t *));
> + apr_err = apr_thread_mutex_create(&array_mutex,
> + APR_THREAD_MUTEX_DEFAULT,
> + sasl_pool);
> +#endif /* APR_HAS_THREADS */
> + return apr_err;
It all looks a bit dodgy. Does it work if the application calls
apr_init/apr_terminate more than once? Does sasl_done work if
apr_terminate has deleted the pools?
> +}
> +
> +/* ### These defines were taken from libsvn_fs_base/bdb/env.c.
> + * Perhaps they should be moved to svn_private_config.h as suggested there? */
> +#if APR_MAJOR_VERSION > 0
> +# define svn__atomic_t apr_uint32_t
> +# define svn__atomic_cas(mem, with, cmp) \
> + apr_atomic_cas32((mem), (with), (cmp))
> +#else
> +# define svn__atomic_t apr_atomic_t
> +# define svn__atomic_cas(mem, with, cmp) \
> + apr_atomic_cas((mem), (with), (cmp))
> +#endif
> +
> +/* Magic values for atomic initialization of the SASL library. */
> +#define SASL_UNINITIALIZED 0
> +#define SASL_START_INIT 1
> +#define SASL_INIT_FAILED 2
> +#define SASL_INITIALIZED 3
> +
> +static volatile svn__atomic_t sasl_status = SASL_UNINITIALIZED;
> +
> +svn_error_t *svn_ra_svn__sasl_init()
> +{
> + /* We have to initialize the cache exactly once. Because APR
> + doesn't have statically-initialized mutexes, we implement a poor
> + man's spinlock using svn__atomic_cas. */
This is obviously based on the BDB code. It would be much better if
this code was shared somehow.
> +#ifdef APR_HAS_THREADS
> + svn__atomic_t status = svn__atomic_cas(&sasl_status,
> + SASL_START_INIT,
> + SASL_UNINITIALIZED);
> + if (status == SASL_UNINITIALIZED)
> +#else
> + if (!sasl_pool)
> +#endif /* APR_HAS_THREADS */
> + {
> + if (svn_ra_svn__sasl_common_init() != APR_SUCCESS
> + || sasl_client_init(interactions) != SASL_OK)
> + {
> +#ifdef APR_HAS_THREADS
> + svn__atomic_cas(&sasl_status, SASL_INIT_FAILED, SASL_START_INIT);
> +#endif /* APR_HAS_THREADS */
> + return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
> + _("Could not initialize the SASL library"));
> + }
> +#ifdef APR_HAS_THREADS
> + svn__atomic_cas(&sasl_status, SASL_INITIALIZED, SASL_START_INIT);
> +#endif /* APR_HAS_THREADS */
> + }
> +#ifdef APR_HAS_THREADS
> + /* Make sure that the other threads wait for
> + the initialization to finish */
> + else while (status != SASL_INITIALIZED)
> + {
> + if (status == SASL_INIT_FAILED)
> + return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
> + _("Could not initialize the SASL library"));
> + apr_sleep(APR_USEC_PER_SEC / 1000);
> + status = svn__atomic_cas(&sasl_status,
> + SASL_UNINITIALIZED,
> + SASL_UNINITIALIZED);
> + }
> +#endif /* APR_HAS_THREADS */
> + return SVN_NO_ERROR;
> +}
> +
> +/* Create a new SASL context. */
> +static svn_error_t *new_sasl_ctx(sasl_conn_t **sasl_ctx,
> + svn_boolean_t is_tunneled,
> + const char *hostname,
> + const char *local_addrport,
> + const char *remote_addrport,
> + apr_pool_t *pool)
> +{
> + sasl_security_properties_t secprops = DEFAULT_SECPROPS;
> + int result;
> +
> + result = sasl_client_new("svn", hostname, local_addrport, remote_addrport,
> + interactions, SASL_SUCCESS_DATA,
> + sasl_ctx);
> + if (result != SASL_OK)
> + return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
> + sasl_errstring(result, NULL, NULL));
> +
> +
> + if (is_tunneled)
> + {
> + /* We need to tell SASL that this connection is tunneled,
> + * otherwise it will ignore EXTERNAL. The third paramater
> + * should be the username, but since SASL doesn't seem
> + * to use it on the client side, any non-empty string will do. */
> + result = sasl_setprop(*sasl_ctx,
> + SASL_AUTH_EXTERNAL, " ");
> + if (result != SASL_OK)
> + return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
> + sasl_errdetail(*sasl_ctx));
> + }
> +
> + /* Set security properties.
> + * Don't allow PLAIN or LOGIN, since we don't support TLS yet. */
> + secprops.security_flags = SASL_SEC_NOPLAINTEXT;
> + sasl_setprop(*sasl_ctx, SASL_SEC_PROPS, &secprops);
> +
> + return SVN_NO_ERROR;
> +}
> +
> +/* Get the username and password from the client */
> +static svn_error_t* get_creds(svn_auth_iterstate_t **iterstate_p,
> + svn_ra_svn__session_baton_t *sess,
> + const char *realm,
> + const char *last_err,
> + const char **username, const char **password,
> + apr_pool_t *pool)
> +{
> + void *creds;
> +
> + if (*iterstate_p == NULL)
> + {
> + /* If iterstate_p is NULL (and thus unallocated), we need to call
> + svn_auth_first_credentials(). */
> + const char *realmstring;
> +
> + realmstring = realm ?
> + apr_psprintf(pool, "%s %s", sess->realm_prefix, realm)
> + : sess->realm_prefix;
> + SVN_ERR(svn_auth_first_credentials(&creds, iterstate_p,
> + SVN_AUTH_CRED_SIMPLE,
> + realmstring,
> + sess->auth_baton, pool));
> + }
> + else
> + SVN_ERR(svn_auth_next_credentials(&creds, *iterstate_p, pool));
> +
> + if (!creds)
> + return svn_error_createf(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
> + _("Authentication error from server: %s"),
> + last_err);
> +
> + *username = ((svn_auth_cred_simple_t *)creds)->username;
> + *password = ((svn_auth_cred_simple_t *)creds)->password;
> + return SVN_NO_ERROR;
> +}
> +
> +/* Fill in the information requested by client_interact */
> +static svn_error_t *handle_interact(svn_auth_iterstate_t **iterstate_p,
> + svn_ra_svn__session_baton_t *sess,
> + sasl_interact_t *client_interact,
> + const char* realm,
> + const char *last_err,
> + apr_pool_t *pool)
> +{
> + sasl_interact_t *prompt;
> + const char *username=NULL, *password=NULL;
> +
> + SVN_ERR(get_creds(iterstate_p, sess, realm, last_err,
> + &username, &password, pool));
> +
> + for (prompt = client_interact; prompt->id != SASL_CB_LIST_END; prompt++)
> + {
> + switch (prompt->id)
> + {
> + case SASL_CB_AUTHNAME:
> + prompt->result = username;
> + prompt->len = strlen(username);
> + break;
> + case SASL_CB_PASS:
> + prompt->result = password;
> + prompt->len = strlen(password);
> + break;
> + default:
> + /* This should never be reached */
> + return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
> + _("Unhandled SASL interaction"));
> + break;
> + }
> + }
> + return SVN_NO_ERROR;
> +}
> +
> +/* Perform an authentication exchange */
> +static svn_error_t *try_auth(svn_ra_svn__session_baton_t *sess,
> + sasl_conn_t *sasl_ctx,
> + svn_auth_iterstate_t **iterstate_p,
> + svn_boolean_t *success,
> + const char **last_err,
> + const char *realm,
> + const char *mechstring,
> + svn_boolean_t compat,
> + apr_pool_t *pool)
> +{
> + sasl_interact_t *client_interact = NULL;
> + const char *out, *mech, *status = NULL;
> + const svn_string_t *arg = NULL, *in;
> + int result;
> + unsigned int outlen;
> +
> + do
> + {
> + result = sasl_client_start(sasl_ctx,
> + mechstring,
> + &client_interact,
> + &out,
> + &outlen,
> + &mech);
> +
> + /* Fill in username and password, if required */
> + if (result == SASL_INTERACT)
> + SVN_ERR(handle_interact(iterstate_p, sess, client_interact,
> + realm, *last_err, pool));
> + }
> + while (result == SASL_INTERACT);
> +
> + if (result != SASL_OK && result != SASL_CONTINUE)
> + return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
> + sasl_errdetail(sasl_ctx));
> +
> + /* Prepare the initial authentication token. */
> + if (outlen > 0 || strcmp(mech, "EXTERNAL") == 0)
> + arg = svn_base64_encode_string(svn_string_ncreate(out, outlen, pool),
> + pool);
> +
> + /* Send the initial client response */
> + SVN_ERR(svn_ra_svn__auth_response(sess->conn, pool, mech,
> + arg ? arg->data : NULL, compat));
> +
> + while (result == SASL_CONTINUE)
> + {
> + /* Read the server response */
> + SVN_ERR(svn_ra_svn_read_tuple(sess->conn, pool, "w(?s)",
> + &status, &in));
> +
> + if (strcmp(status, "failure") == 0)
> + {
> + /* Authentication failed. Use the next set of credentials */
> + *success = FALSE;
> + /* Remember the message sent by the server because we'll want to
> + * return a meaningful error if we run out of auth providers. */
> + *last_err = in ? in->data : "";
> + return SVN_NO_ERROR;
> + }
> +
> + if ((strcmp(status, "success") != 0 && strcmp(status, "step") != 0)
> + || in == NULL)
> + return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
> + _("Unexpected server response"
> + " to authentication"));
> +
> + /* If the mech is CRAM-MD5 we don't base64-decode the server response. */
> + if (strcmp(mech, "CRAM-MD5") != 0)
> + in = svn_base64_decode_string(in, pool);
> +
> + do
> + {
> + result = sasl_client_step(sasl_ctx,
> + in->data,
> + in->len,
> + &client_interact,
> + &out, /* Filled in by SASL. */
> + &outlen);
> +
> + /* Fill in username and password, if required. */
> + if (result == SASL_INTERACT)
> + SVN_ERR(handle_interact(iterstate_p, sess, client_interact,
> + realm, *last_err, pool));
> + }
> + while (result == SASL_INTERACT);
> +
> + if (result != SASL_OK && result != SASL_CONTINUE)
> + return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
> + sasl_errdetail(sasl_ctx));
> +
> + if (outlen > 0)
> + {
> + arg = svn_string_ncreate(out, outlen, pool);
> + /* Write our response. */
> + /* For CRAM-MD5, we don't use base64-encoding. */
> + if (strcmp(mech, "CRAM-MD5") != 0)
> + arg = svn_base64_encode_string(arg, pool);
> + SVN_ERR(svn_ra_svn_write_cstring(sess->conn, pool, arg->data));
> + }
> + }
Lots of loops, do you need subpools?
> +
> + if (!status || strcmp(status, "step") == 0)
> + {
> + /* This is a client-send-last mech. Read the last server response. */
> + SVN_ERR(svn_ra_svn_read_tuple(sess->conn, pool, "w(?s)",
> + &status, &in));
> +
> + if (strcmp(status, "failure") == 0)
> + {
> + *success = FALSE;
> + *last_err = in ? in->data : "";
> + }
> + else if (strcmp(status, "success") == 0)
> + {
> + /* We're done */
> + *success = TRUE;
> + }
> + else
> + return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
> + _("Unexpected server response"
> + " to authentication"));
> + }
> + else
> + *success = TRUE;
> + return SVN_NO_ERROR;
> +}
> +
> +void svn_ra_svn__get_addresses(apr_socket_t *sock,
> + char **local_addrport,
> + char **remote_addrport,
> + apr_pool_t *pool)
> +{
> + apr_sockaddr_t *local_sa, *remote_sa;
> + char *local_addr, *remote_addr;
> +
> + apr_socket_addr_get(&local_sa, APR_LOCAL, sock);
> + apr_socket_addr_get(&remote_sa, APR_REMOTE, sock);
> +
> + apr_sockaddr_ip_get(&local_addr, local_sa);
> + apr_sockaddr_ip_get(&remote_addr, remote_sa);
Check for errors?
> +
> + /* Format the IP address and port number like this: a.b.c.d;port */
> + *local_addrport = apr_pstrcat(pool, local_addr, ";",
> + apr_itoa(pool, (int)local_sa->port), NULL);
> + *remote_addrport = apr_pstrcat(pool, remote_addr, ";",
> + apr_itoa(pool, (int)remote_sa->port), NULL);
> +}
> +
> +void get_remote_hostname(apr_socket_t *sock, char **hostname)
> +{
> + apr_sockaddr_t *sa;
> + apr_socket_addr_get(&sa, APR_REMOTE, sock);
> + apr_getnameinfo(hostname, sa, 0);
Check for errors?
> +}
> +
> +svn_error_t *svn_ra_svn__do_auth(svn_ra_svn__session_baton_t *sess,
> + apr_array_header_t *mechlist,
> + const char *realm, apr_pool_t *pool)
> +{
> + apr_pool_t *subpool;
> + sasl_conn_t *sasl_ctx;
> + const char *mechstring = "", *last_err = "";
> + svn_ra_svn_item_t *elt;
> + svn_auth_iterstate_t *iterstate = NULL;
> + svn_boolean_t success, compat = (realm == NULL);
> + char *hostname = NULL, *local_addrport = NULL, *remote_addrport = NULL;
> + int i;
> +
> + if (!sess->is_tunneled)
> + {
> + svn_ra_svn__get_addresses(sess->conn->sock,
> + &local_addrport, &remote_addrport,
> + pool);
> + get_remote_hostname(sess->conn->sock, &hostname);
> + }
> +
> + subpool = svn_pool_create(pool);
> +
> + /* Create a string containing the list of mechanisms, separated by spaces. */
> + for (i = 0; i < mechlist->nelts; i++)
> + {
> + elt = &APR_ARRAY_IDX(mechlist, i, svn_ra_svn_item_t);
> +
> + /* Force the client to use ANONYMOUS if it is available. */
> + if (strcmp(elt->u.word, "ANONYMOUS") == 0)
> + {
> + mechstring = "ANONYMOUS";
> + break;
> + }
> +
> + mechstring = apr_pstrcat(subpool,
> + mechstring,
> + i == 0 ? "" : " ",
> + elt->u.word, NULL);
> + }
> +
> + do
> + {
> + /* We shouldn't have to create a new SASL context for each
> + * authentication attempt, but it seems that sasl_client_start
> + * forgets to initialise some data structures. This means that
> + * the library will treat calls to sasl_client_step as if they were part
> + * of the previous auth exchange, which will obviously fail. */
> + SVN_ERR(new_sasl_ctx(&sasl_ctx, sess->is_tunneled,
> + hostname, local_addrport, remote_addrport,
> + pool));
> +
> + SVN_ERR(try_auth(sess,
> + sasl_ctx,
> + &iterstate,
> + &success,
> + &last_err,
> + realm,
> + mechstring,
> + compat,
> + subpool));
> +
> + /* Dispose of this SASL context before creating a new one. */
> + sasl_dispose(&sasl_ctx);
> + }
> + while (!success);
> +
> + SVN_ERR(svn_auth_save_credentials(iterstate, pool));
> +
> + svn_pool_destroy(subpool);
> + return SVN_NO_ERROR;
> +}
> +
> +#endif /* SVN_HAVE_SASL */
> Index: subversion/libsvn_ra_svn/simple_auth.c
> ===================================================================
> --- subversion/libsvn_ra_svn/simple_auth.c (revision 20716)
> +++ subversion/libsvn_ra_svn/simple_auth.c (working copy)
> @@ -31,6 +31,8 @@
>
> #include "ra_svn.h"
>
> +#ifndef SVN_HAVE_SASL
> +
> static svn_boolean_t find_mech(apr_array_header_t *mechlist, const char *mech)
> {
> int i;
> @@ -114,3 +116,5 @@
> return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
> _("Cannot negotiate authentication mechanism"));
> }
> +
> +#endif /* SVN_HAVE_SASL */
> Index: subversion/libsvn_ra_svn/ra_svn.h
> ===================================================================
> --- subversion/libsvn_ra_svn/ra_svn.h (revision 20716)
> +++ subversion/libsvn_ra_svn/ra_svn.h (working copy)
> @@ -122,6 +122,10 @@
> const char *mech, const char *mech_arg,
> svn_boolean_t compat);
>
> +/* Initialize the SASL library */
> +svn_error_t *svn_ra_svn__sasl_init();
> +
> +
> #ifdef __cplusplus
> }
> #endif /* __cplusplus */
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Jul 19 23:03:43 2006