You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Vlad Georgescu <vg...@gmail.com> on 2006/08/21 09:39:38 UTC
[PATCH] Server-side Cyrus SASL support
This patch adds Cyrus SASL support to svnserve. Security layers are
not supported yet - I'll submit another patch for that when this one
gets the 'stamp of approval'.
[[[
* subversion/include/svn_config.h
(SVN_CONFIG_SECTION_SASL): New section.
(SVN_CONFIG_OPTION_USE_SASL): New option.
* subversion/libsvn_repos/repos.c
(create_conf): Document the new option.
* subversion/svnserve/sasl_auth.c: New file.
* subversion/svnserve/main.c
(main): Call sasl_init().
* subversion/svnserve/serve.c:
(server_baton_t, authn_type, authz_type, get_access): Moved to server.h.
(simple_auth_request): Contains the code that was prieviously in auth_request.
(auth_request): Call sasl_auth_request() or simple_auth_request().
(must_have_access): Consider the value of b->use_sasl when
determining whether authentication should be performed.
(find_repos): Read the value of the use-sasl option into b->use_sasl.
Use that value when determining whether access is allowed to the repository.
* subversion/svnserve/server.h
(server_baton_t): Moved here from serve.c. Has a new member 'use_sasl'
(authn_type, authz_type, get_access): Moved here from serve.c.
(sasl_init, sasl_auth_request): New declarations.
]]]
SASL supports many password-checking methods. It can use an external
daemon saslauthd that can authenticate against /etc/shadow, PAM, LDAP,
etc. It only works with plaintext passwords, though, so we can't use
it until we have SSL support in for ra_svn. Until then, we have to use
the other method, auxprop, which fetches passwords from a database
(which is usually stored in /etc/sasldb2, but could also be an
external SQL server, or a LDAP database)
To test this patch, create a configuration file Subversion.conf in
/usr/lib/sasl2. This file will be read by Cyrus SASL at runtime to
determine how authentication should be performed.
See the SASL docs for more details. A simple example could be:
pwcheck_method: auxprop
mech_list: DIGEST-MD5 ANONYMOUS
Then add users and passwords to Cyrus SASL's password database:
saslpasswd2 -c user -u realm -a Subversion
where 'realm' is the actual realm of your repository.
Just like Subversion, Cyrus SASL supports the concept of
"authentication realms". In fact, svnserve will treat the repository
realm as a SASL realm when authenticating the user. This is necessary
because the password database is global instead of per-repository, so
it's the only way for Subversion to tell which users belong to which
repository.
Comments and criticism are welcome, as always.
--
Vlad
Re: [PATCH] Server-side Cyrus SASL support
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 8/25/06, Vlad Georgescu <vg...@gmail.com> wrote:
> Ok, here goes..
Applied in r21266. Thanks!
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Server-side Cyrus SASL support
Posted by Vlad Georgescu <vg...@gmail.com>.
On 8/25/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> On 8/25/06, Vlad Georgescu <vg...@gmail.com> wrote:
> > On 8/25/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> > > I'm thinking that we may be able to get away with sticking if
> > > (sasl_done) return; in various places, since this will only ever
> > > happen at global shutdown time, at which point we've already assumed
> > > that we're in single threaded mode, so locking isn't needed...
> >
> > I'll add those checks. Should I submit a separate patch, or
> > incorporate them in the other one?
>
> One patch that does both should be fine.
>
Ok, here goes..
[[[
Handle the case when sasl_pool is destroyed before the connection pool,
by making sure that:
a) nothing allocated in sasl_pool is accessed after that pool is destroyed.
b) sasl_done() is always called _after_ the last call to sasl_dispose().
* subversion/include/private/svn_atomic.h
(svn_atomic_inc, svn_atomic_dec): New macro definitions.
* subversion/libsvn_ra_svn/sasl_auth.c
(sasl_ctx_count): New static variable.
(svn_ra_svn__sasl_common_init): Initialize sasl_ctx_count.
(sasl_done_cb, sasl_dispose_cb): Decrement sasl_ctx_count. If it
is 0, call sasl_done().
(new_sasl_ctx): Increment sasl_ctx_count..
(sasl_mutex_alloc_cb, sasl_mutex_lock_cb,
sasl_mutex_unlock_cb, sasl_mutex_free_cb): Check the value of sasl_status
before doing anything.
]]]
--
Vlad
Re: [PATCH] Server-side Cyrus SASL support
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 8/25/06, Vlad Georgescu <vg...@gmail.com> wrote:
> On 8/25/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> > On 8/25/06, Vlad Georgescu <vg...@gmail.com> wrote:
> >
> > > Hi Garrett,
> > >
> > > Could you test the following patch?
> > >
> > > It solves the problem by maintaining a count of how many sasl contexts
> > > were created, and only calling sasl_done() when all of them are
> > > destroyed (the count starts from 1, because we also decrement it in
> > > sasl_done_cb). It seems to work for me (i.e. the test runs
> > > successfully).
> >
> > Unfortunately, that's not going to work. The problem isn't that we
> > call sasl_done(), then later on call sasl_dispose() on some existing
> > sasl context (well, that's probably A problem, but it's not THE
> > problem). The problem is that if we do any sasl stuff after sasl_pool
> > is destroyed then the mutex locking/unlocking that happens in the
> > subsequent calls to sasl_done() or sasl_dispose() will try to
> > lock/unlock mutexes allocated in memory that's potentially been reused
> > because it was allocated out of sasl_pool.
>
> Ok, but this patch by itself is still valid, right? After all, it
> somehow manages to stop svn from crashing on my machine, despite the
> other issues :)
You might want to try with an APR that was built with
--enable-pool-debug, that tends to trip up such things...
> > I'm thinking that we may be able to get away with sticking if
> > (sasl_done) return; in various places, since this will only ever
> > happen at global shutdown time, at which point we've already assumed
> > that we're in single threaded mode, so locking isn't needed...
>
> I'll add those checks. Should I submit a separate patch, or
> incorporate them in the other one?
One patch that does both should be fine.
Thanks,
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Server-side Cyrus SASL support
Posted by Vlad Georgescu <vg...@gmail.com>.
On 8/25/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> On 8/25/06, Vlad Georgescu <vg...@gmail.com> wrote:
>
> > Hi Garrett,
> >
> > Could you test the following patch?
> >
> > It solves the problem by maintaining a count of how many sasl contexts
> > were created, and only calling sasl_done() when all of them are
> > destroyed (the count starts from 1, because we also decrement it in
> > sasl_done_cb). It seems to work for me (i.e. the test runs
> > successfully).
>
> Unfortunately, that's not going to work. The problem isn't that we
> call sasl_done(), then later on call sasl_dispose() on some existing
> sasl context (well, that's probably A problem, but it's not THE
> problem). The problem is that if we do any sasl stuff after sasl_pool
> is destroyed then the mutex locking/unlocking that happens in the
> subsequent calls to sasl_done() or sasl_dispose() will try to
> lock/unlock mutexes allocated in memory that's potentially been reused
> because it was allocated out of sasl_pool.
Ok, but this patch by itself is still valid, right? After all, it
somehow manages to stop svn from crashing on my machine, despite the
other issues :)
> I'm thinking that we may be able to get away with sticking if
> (sasl_done) return; in various places, since this will only ever
> happen at global shutdown time, at which point we've already assumed
> that we're in single threaded mode, so locking isn't needed...
I'll add those checks. Should I submit a separate patch, or
incorporate them in the other one?
--
Vlad
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Server-side Cyrus SASL support
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 8/25/06, Vlad Georgescu <vg...@gmail.com> wrote:
> Hi Garrett,
>
> Could you test the following patch?
>
> It solves the problem by maintaining a count of how many sasl contexts
> were created, and only calling sasl_done() when all of them are
> destroyed (the count starts from 1, because we also decrement it in
> sasl_done_cb). It seems to work for me (i.e. the test runs
> successfully).
Unfortunately, that's not going to work. The problem isn't that we
call sasl_done(), then later on call sasl_dispose() on some existing
sasl context (well, that's probably A problem, but it's not THE
problem). The problem is that if we do any sasl stuff after sasl_pool
is destroyed then the mutex locking/unlocking that happens in the
subsequent calls to sasl_done() or sasl_dispose() will try to
lock/unlock mutexes allocated in memory that's potentially been reused
because it was allocated out of sasl_pool.
I'm thinking that we may be able to get away with sticking if
(sasl_done) return; in various places, since this will only ever
happen at global shutdown time, at which point we've already assumed
that we're in single threaded mode, so locking isn't needed...
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Server-side Cyrus SASL support
Posted by Vlad Georgescu <vg...@gmail.com>.
On 8/24/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
>
> Hey Vlad, I spent a fair amount of time looking at this today, and
> it's looking pretty good, but I think I found a problem in the
> existing sasl client code. I'm getting some strage deadlocks while
> running the svnsync tests, specifically test number 13, "detect
> non-svnsync commits in destination". The problem appears to be one of
> pool destruction ordering. We end up calling sasl_done before we
> destroy some of the pools that hold sasl contexts, so when sasl tries
> to use the mutex wrappers the pool has already been destroyed, so it
> tries to lock/unlock mutexes that are allocated in memory that's been
> reused. Oops.
>
> The following patch shows the problem:
> Index: subversion/libsvn_ra_svn/sasl_auth.c
> ===================================================================
> --- subversion/libsvn_ra_svn/sasl_auth.c (revision 21225)
> +++ subversion/libsvn_ra_svn/sasl_auth.c (working copy)
> @@ -40,6 +40,21 @@
> #include "ra_svn.h"
> #include "ra_svn_sasl.h"
>
> +static void
> +debug(const char *fmt, ...)
> +{
> + FILE *f = fopen("/tmp/debug.out", "a+");
> + va_list ap;
> +
> + fprintf(f, "%d ", getpid());
> +
> + va_start(ap, fmt);
> + vfprintf(f, fmt, ap);
> + va_end(ap);
> +
> + fclose(f);
> +}
> +
> static volatile svn_atomic_t sasl_status;
>
> static apr_pool_t *sasl_pool = NULL;
> @@ -47,6 +62,7 @@
> /* Pool cleanup called when sasl_pool is destroyed. */
> static apr_status_t sasl_done_cb(void *data)
> {
> + debug("sasl_done\n");
> /* Reset sasl_status, in case the client calls
> apr_initialize()/apr_terminate() more than once. */
> sasl_status = 0;
> @@ -76,6 +92,9 @@
> apr_thread_mutex_t *mutex;
> apr_status_t apr_err;
>
> + if (! sasl_status)
> + return NULL;
> +
> apr_err = apr_thread_mutex_lock(array_mutex);
> if (apr_err != APR_SUCCESS)
> return NULL;
> @@ -164,6 +183,7 @@
>
> static apr_status_t sasl_dispose_cb(void *data)
> {
> + debug("sasl_dispose_cb: %d\n", (int) data);
> sasl_conn_t *sasl_ctx = data;
> sasl_dispose(&sasl_ctx);
> return APR_SUCCESS;
>
> Run it with svnsync_tests.py --url=svn://localhost/ 13
>
> Then look in /tmp/debug.log, and you'll see the ordering problem. The
> if (! sasl_status) bit is a hack so that we don't do the bogus stuff,
> which avoids the deadlock, but obviously isn't the "right" solution.
> Good enough to see the debug trace though...
>
> I'm not entirely sure what the solution here is, need to think about
> it some more.
>
> -garrett
>
Hi Garrett,
Could you test the following patch?
It solves the problem by maintaining a count of how many sasl contexts
were created, and only calling sasl_done() when all of them are
destroyed (the count starts from 1, because we also decrement it in
sasl_done_cb). It seems to work for me (i.e. the test runs
successfully).
[[[
Ensure that sasl_done() is always called _after_ all the SASL contexts
are disposed of, regardless of the order in which sasl_pool and the
connection pool are destroyed.
* subversion/include/private/svn_atomic.h
(svn_atomic_inc, svn_atomic_dec): New macro definitions.
* subversion/libsvn_ra_svn/sasl_auth.c
(sasl_ctx_count): New static variable.
(svn_ra_svn__sasl_common_init): Initialize sasl_ctx_count.
(sasl_done_cb, sasl_dispose_cb): Decrement sasl_ctx_count. If it is
0, call sasl_done().
(new_sasl_ctx): Increment sasl_ctx_count..
]]]
Index: subversion/include/private/svn_atomic.h
===================================================================
--- subversion/include/private/svn_atomic.h (revision 21257)
+++ subversion/include/private/svn_atomic.h (working copy)
@@ -60,6 +60,20 @@
#define svn_atomic_set(mem, val) apr_atomic_set((mem), (val))
#endif /* APR_MAJOR_VERSION */
+/** Atomically increment an #svn_atomic_t. */
+#if APR_MAJOR_VERSION > 0
+#define svn_atomic_inc(mem) apr_atomic_inc32(mem)
+#else
+#define svn_atomic_inc(mem) apr_atomic_inc(mem)
+#endif /* APR_MAJOR_VERSION */
+
+/** Atomically decrement an #svn_atomic_t. */
+#if APR_MAJOR_VERSION > 0
+#define svn_atomic_dec(mem) apr_atomic_dec32(mem)
+#else
+#define svn_atomic_dec(mem) apr_atomic_dec(mem)
+#endif /* APR_MAJOR_VERSION */
+
/**
* Atomic compare-and-swap.
*
Index: subversion/libsvn_ra_svn/sasl_auth.c
===================================================================
--- subversion/libsvn_ra_svn/sasl_auth.c (revision 21257)
+++ subversion/libsvn_ra_svn/sasl_auth.c (working copy)
@@ -42,6 +42,8 @@
static volatile svn_atomic_t sasl_status;
+static volatile svn_atomic_t sasl_ctx_count;
+
static apr_pool_t *sasl_pool = NULL;
/* Pool cleanup called when sasl_pool is destroyed. */
@@ -50,7 +52,8 @@
/* Reset sasl_status, in case the client calls
apr_initialize()/apr_terminate() more than once. */
sasl_status = 0;
- sasl_done();
+ if (svn_atomic_dec(&sasl_ctx_count) == 0)
+ sasl_done();
return APR_SUCCESS;
}
@@ -124,6 +127,7 @@
apr_status_t apr_err = APR_SUCCESS;
sasl_pool = svn_pool_create(NULL);
+ sasl_ctx_count = 1;
apr_pool_cleanup_register(sasl_pool, NULL, sasl_done_cb,
apr_pool_cleanup_null);
#ifdef APR_HAS_THREADS
@@ -166,6 +170,8 @@
{
sasl_conn_t *sasl_ctx = data;
sasl_dispose(&sasl_ctx);
+ if (svn_atomic_dec(&sasl_ctx_count) == 0)
+ sasl_done();
return APR_SUCCESS;
}
@@ -187,6 +193,7 @@
return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
sasl_errstring(result, NULL, NULL));
+ svn_atomic_inc(&sasl_ctx_count);
apr_pool_cleanup_register(pool, *sasl_ctx, sasl_dispose_cb,
apr_pool_cleanup_null);
--
Vlad
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Server-side Cyrus SASL support
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 8/21/06, Vlad Georgescu <vg...@gmail.com> wrote:
> This patch adds Cyrus SASL support to svnserve. Security layers are
> not supported yet - I'll submit another patch for that when this one
> gets the 'stamp of approval'.
>
> [[[
> * subversion/include/svn_config.h
> (SVN_CONFIG_SECTION_SASL): New section.
> (SVN_CONFIG_OPTION_USE_SASL): New option.
>
> * subversion/libsvn_repos/repos.c
> (create_conf): Document the new option.
>
> * subversion/svnserve/sasl_auth.c: New file.
>
> * subversion/svnserve/main.c
> (main): Call sasl_init().
>
> * subversion/svnserve/serve.c:
> (server_baton_t, authn_type, authz_type, get_access): Moved to server.h.
> (simple_auth_request): Contains the code that was prieviously in auth_request.
> (auth_request): Call sasl_auth_request() or simple_auth_request().
> (must_have_access): Consider the value of b->use_sasl when
> determining whether authentication should be performed.
> (find_repos): Read the value of the use-sasl option into b->use_sasl.
> Use that value when determining whether access is allowed to the repository.
>
> * subversion/svnserve/server.h
> (server_baton_t): Moved here from serve.c. Has a new member 'use_sasl'
> (authn_type, authz_type, get_access): Moved here from serve.c.
> (sasl_init, sasl_auth_request): New declarations.
> ]]]
>
> SASL supports many password-checking methods. It can use an external
> daemon saslauthd that can authenticate against /etc/shadow, PAM, LDAP,
> etc. It only works with plaintext passwords, though, so we can't use
> it until we have SSL support in for ra_svn. Until then, we have to use
> the other method, auxprop, which fetches passwords from a database
> (which is usually stored in /etc/sasldb2, but could also be an
> external SQL server, or a LDAP database)
>
> To test this patch, create a configuration file Subversion.conf in
> /usr/lib/sasl2. This file will be read by Cyrus SASL at runtime to
> determine how authentication should be performed.
> See the SASL docs for more details. A simple example could be:
>
> pwcheck_method: auxprop
> mech_list: DIGEST-MD5 ANONYMOUS
>
> Then add users and passwords to Cyrus SASL's password database:
>
> saslpasswd2 -c user -u realm -a Subversion
>
> where 'realm' is the actual realm of your repository.
> Just like Subversion, Cyrus SASL supports the concept of
> "authentication realms". In fact, svnserve will treat the repository
> realm as a SASL realm when authenticating the user. This is necessary
> because the password database is global instead of per-repository, so
> it's the only way for Subversion to tell which users belong to which
> repository.
>
> Comments and criticism are welcome, as always.
Hey Vlad, I spent a fair amount of time looking at this today, and
it's looking pretty good, but I think I found a problem in the
existing sasl client code. I'm getting some strage deadlocks while
running the svnsync tests, specifically test number 13, "detect
non-svnsync commits in destination". The problem appears to be one of
pool destruction ordering. We end up calling sasl_done before we
destroy some of the pools that hold sasl contexts, so when sasl tries
to use the mutex wrappers the pool has already been destroyed, so it
tries to lock/unlock mutexes that are allocated in memory that's been
reused. Oops.
The following patch shows the problem:
Index: subversion/libsvn_ra_svn/sasl_auth.c
===================================================================
--- subversion/libsvn_ra_svn/sasl_auth.c (revision 21225)
+++ subversion/libsvn_ra_svn/sasl_auth.c (working copy)
@@ -40,6 +40,21 @@
#include "ra_svn.h"
#include "ra_svn_sasl.h"
+static void
+debug(const char *fmt, ...)
+{
+ FILE *f = fopen("/tmp/debug.out", "a+");
+ va_list ap;
+
+ fprintf(f, "%d ", getpid());
+
+ va_start(ap, fmt);
+ vfprintf(f, fmt, ap);
+ va_end(ap);
+
+ fclose(f);
+}
+
static volatile svn_atomic_t sasl_status;
static apr_pool_t *sasl_pool = NULL;
@@ -47,6 +62,7 @@
/* Pool cleanup called when sasl_pool is destroyed. */
static apr_status_t sasl_done_cb(void *data)
{
+ debug("sasl_done\n");
/* Reset sasl_status, in case the client calls
apr_initialize()/apr_terminate() more than once. */
sasl_status = 0;
@@ -76,6 +92,9 @@
apr_thread_mutex_t *mutex;
apr_status_t apr_err;
+ if (! sasl_status)
+ return NULL;
+
apr_err = apr_thread_mutex_lock(array_mutex);
if (apr_err != APR_SUCCESS)
return NULL;
@@ -164,6 +183,7 @@
static apr_status_t sasl_dispose_cb(void *data)
{
+ debug("sasl_dispose_cb: %d\n", (int) data);
sasl_conn_t *sasl_ctx = data;
sasl_dispose(&sasl_ctx);
return APR_SUCCESS;
Run it with svnsync_tests.py --url=svn://localhost/ 13
Then look in /tmp/debug.log, and you'll see the ordering problem. The
if (! sasl_status) bit is a hack so that we don't do the bogus stuff,
which avoids the deadlock, but obviously isn't the "right" solution.
Good enough to see the debug trace though...
I'm not entirely sure what the solution here is, need to think about
it some more.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Server-side Cyrus SASL support
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 9/15/06, Vlad Georgescu <vg...@gmail.com> wrote:
> On 9/6/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> > On 9/1/06, Vlad Georgescu <vg...@gmail.com> wrote:
> > > On 8/29/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> > > > On 8/29/06, Vlad Georgescu <vg...@gmail.com> wrote:
> > > >
> > > > > Ouch. I don't know if it's related to the crash or not, but I noticed
> > > > > I forgot to do any error-checking when calling sasl_init() in main. So
> > > > > SASL might not have been initialized.
> > > >
> > > > That could certainly be an issue.
> > >
> > > I figured out why your initialization failed. Turns out that once we
> > > added those if statements to the mutex callbacks they could no longer
> > > be used by the server because the server doesn't initialize
> > > sasl_status (The reason it worked for me when testing those changes is
> > > that I didn't bother to restart the server after recompiling). So I
> > > made sasl_status public (see the log message) and changed svnserve's
> > > sasl_init() to call svn_atomic_init_once().
> > >
> > > I also fixed a client-side bug where the value of the 'creds' variable
> > > wasn't checked as early as it should have been (see the second log
> > > message).
> >
> > For what it's worth, I've tried this, and it does indeed stop the
> > crashing, but I can't seem to get it to actually work. More debugging
> > later, as I'm still not sure I haven't just screwed something up in my
> > sasl configuration...
> >
> > -garrett
> >
>
> Hi,
>
> I attached a new version of my patch, with a couple of minor fixes.
>
> Also, before testing this, please apply the client-side fix that I
> mentioned in my previous e-mail. Without it the client will lose the
> last error message returned by the server (this happens because
> last_err is allocated in a subpool, so we have to use it before
> restarting the enclosing loop, which clears the pool). With that fix
> in place, if the authentication still fails, you should at least get a
> decent error message.
Ahh, good to know, I'll take a look at this later today.
Thanks!
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Server-side Cyrus SASL support
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 9/18/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> On 9/15/06, Vlad Georgescu <vg...@gmail.com> wrote:
>
> > Also, before testing this, please apply the client-side fix that I
> > mentioned in my previous e-mail. Without it the client will lose the
> > last error message returned by the server (this happens because
> > last_err is allocated in a subpool, so we have to use it before
> > restarting the enclosing loop, which clears the pool). With that fix
> > in place, if the authentication still fails, you should at least get a
> > decent error message.
>
> Committed the client side patch in r21529. Now on to trying the
> server side stuff again...
And hey, it turns out it work! If I had to guess I'd say that I
typoed something the last time I played with this, or one of your
recent bugfixes helped things.
Anyway, a few comments. It would be nice to stick a sasl-howto.txt
file in the notes directory, with examples of how to set this up (i.e.
the subversion.conf and saslpasswd2 stuff you need to be aware of).
Perhaps more importantly, it would be nice to have some instructions
for how to run the test suite over this stuff. I'm thinking a
--enable-sasl flag that would turn on the sasl stuff in svnserve.conf
for new repositories, and then the user would simply need to add a few
users/passwords via saslpasswd2 before starting the test suite.
Finally, now would be a good time to take a look at those outstanding
patches you had for additional sasl magic ;-)
Thanks for being so patient on this stuff!
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Server-side Cyrus SASL support
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 9/15/06, Vlad Georgescu <vg...@gmail.com> wrote:
> Also, before testing this, please apply the client-side fix that I
> mentioned in my previous e-mail. Without it the client will lose the
> last error message returned by the server (this happens because
> last_err is allocated in a subpool, so we have to use it before
> restarting the enclosing loop, which clears the pool). With that fix
> in place, if the authentication still fails, you should at least get a
> decent error message.
Committed the client side patch in r21529. Now on to trying the
server side stuff again...
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Server-side Cyrus SASL support
Posted by Vlad Georgescu <vg...@gmail.com>.
On 9/6/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> On 9/1/06, Vlad Georgescu <vg...@gmail.com> wrote:
> > On 8/29/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> > > On 8/29/06, Vlad Georgescu <vg...@gmail.com> wrote:
> > >
> > > > Ouch. I don't know if it's related to the crash or not, but I noticed
> > > > I forgot to do any error-checking when calling sasl_init() in main. So
> > > > SASL might not have been initialized.
> > >
> > > That could certainly be an issue.
> >
> > I figured out why your initialization failed. Turns out that once we
> > added those if statements to the mutex callbacks they could no longer
> > be used by the server because the server doesn't initialize
> > sasl_status (The reason it worked for me when testing those changes is
> > that I didn't bother to restart the server after recompiling). So I
> > made sasl_status public (see the log message) and changed svnserve's
> > sasl_init() to call svn_atomic_init_once().
> >
> > I also fixed a client-side bug where the value of the 'creds' variable
> > wasn't checked as early as it should have been (see the second log
> > message).
>
> For what it's worth, I've tried this, and it does indeed stop the
> crashing, but I can't seem to get it to actually work. More debugging
> later, as I'm still not sure I haven't just screwed something up in my
> sasl configuration...
>
> -garrett
>
Hi,
I attached a new version of my patch, with a couple of minor fixes.
Also, before testing this, please apply the client-side fix that I
mentioned in my previous e-mail. Without it the client will lose the
last error message returned by the server (this happens because
last_err is allocated in a subpool, so we have to use it before
restarting the enclosing loop, which clears the pool). With that fix
in place, if the authentication still fails, you should at least get a
decent error message.
--
Vlad
Re: [PATCH] Server-side Cyrus SASL support
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 9/5/06, Vlad Georgescu <vg...@gmail.com> wrote:
> On 9/6/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> > For what it's worth, I've tried this, and it does indeed stop the
> > crashing, but I can't seem to get it to actually work. More debugging
> > later, as I'm still not sure I haven't just screwed something up in my
> > sasl configuration...
> >
> > -garrett
> >
>
> I'm just guessing, but could this be a problem with permissions?
> Specifically, does the svnserve user have read access to the password
> database (/etc/sasldb2)?
Unfortunately, that doesn't seem to be the problem.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Server-side Cyrus SASL support
Posted by Vlad Georgescu <vg...@gmail.com>.
On 9/6/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> For what it's worth, I've tried this, and it does indeed stop the
> crashing, but I can't seem to get it to actually work. More debugging
> later, as I'm still not sure I haven't just screwed something up in my
> sasl configuration...
>
> -garrett
>
I'm just guessing, but could this be a problem with permissions?
Specifically, does the svnserve user have read access to the password
database (/etc/sasldb2)?
--
Vlad
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Server-side Cyrus SASL support
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 9/1/06, Vlad Georgescu <vg...@gmail.com> wrote:
> On 8/29/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> > On 8/29/06, Vlad Georgescu <vg...@gmail.com> wrote:
> >
> > > Ouch. I don't know if it's related to the crash or not, but I noticed
> > > I forgot to do any error-checking when calling sasl_init() in main. So
> > > SASL might not have been initialized.
> >
> > That could certainly be an issue.
>
> I figured out why your initialization failed. Turns out that once we
> added those if statements to the mutex callbacks they could no longer
> be used by the server because the server doesn't initialize
> sasl_status (The reason it worked for me when testing those changes is
> that I didn't bother to restart the server after recompiling). So I
> made sasl_status public (see the log message) and changed svnserve's
> sasl_init() to call svn_atomic_init_once().
>
> I also fixed a client-side bug where the value of the 'creds' variable
> wasn't checked as early as it should have been (see the second log
> message).
For what it's worth, I've tried this, and it does indeed stop the
crashing, but I can't seem to get it to actually work. More debugging
later, as I'm still not sure I haven't just screwed something up in my
sasl configuration...
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Server-side Cyrus SASL support
Posted by Vlad Georgescu <vg...@gmail.com>.
On 8/29/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> On 8/29/06, Vlad Georgescu <vg...@gmail.com> wrote:
>
> > Ouch. I don't know if it's related to the crash or not, but I noticed
> > I forgot to do any error-checking when calling sasl_init() in main. So
> > SASL might not have been initialized.
>
> That could certainly be an issue.
I figured out why your initialization failed. Turns out that once we
added those if statements to the mutex callbacks they could no longer
be used by the server because the server doesn't initialize
sasl_status (The reason it worked for me when testing those changes is
that I didn't bother to restart the server after recompiling). So I
made sasl_status public (see the log message) and changed svnserve's
sasl_init() to call svn_atomic_init_once().
I also fixed a client-side bug where the value of the 'creds' variable
wasn't checked as early as it should have been (see the second log
message).
[[[
Add Cyrus SASL support to svnserve.
* subversion/libsvn_ra_svn/sasl_auth.c:
(svn_ra_svn__sasl_status): Rename from sasl_status, change all uses and drop
the 'static' qualifier.
* subversion/libsvn_ra_svn/ra_svn_sasl.h
(svn_ra_svn__sasl_status): New declaration.
* subversion/include/svn_config.h
(SVN_CONFIG_SECTION_SASL): New section.
(SVN_CONFIG_OPTION_USE_SASL): New option.
* subversion/libsvn_repos/repos.c
(create_conf): Document the new option.
* subversion/svnserve/sasl_auth.c: New file.
* subversion/svnserve/main.c
(main): Call sasl_init().
* subversion/svnserve/serve.c:
(server_baton_t, authn_type, authz_type, get_access): Moved to server.h.
(simple_auth_request): Contains the code that was prieviously in auth_request.
(auth_request): Call sasl_auth_request() or simple_auth_request().
(must_have_access): Consider the value of b->use_sasl when
determining whether authentication should be performed.
(find_repos): Read the value of the use-sasl option into b->use_sasl.
Use that value when determining whether access is allowed to the repository.
* subversion/svnserve/server.h
(server_baton_t): Moved here from serve.c. Has a new member 'use_sasl'
(authn_type, authz_type, get_access): Moved here from serve.c.
(sasl_init, sasl_auth_request): New declarations.
]]]
[[[
Check that the credentials are valid as early as possible.
* subversion/libsvn_ra_svn/sasl_auth.c:
(handle_interact): Don't check that the 'creds' variable is non-NULL here.
(svn_ra_svn__do_auth): Instead do it here, right after the call to
sasl_auth_next_credentials().
]]]
--
Vlad
Re: [PATCH] Server-side Cyrus SASL support
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 8/29/06, Vlad Georgescu <vg...@gmail.com> wrote:
> Ouch. I don't know if it's related to the crash or not, but I noticed
> I forgot to do any error-checking when calling sasl_init() in main. So
> SASL might not have been initialized.
That could certainly be an issue.
> OTOH, it's probably not a good idea to initialize SASL in main anyway,
> because it's a potentially expensive operation (it loads a bunch of
> plugins, which might have their own dependencies etc.). That wouldn't
> be nice to people who run svn over ssh and/or don't care about Cyrus
> SASL. We could instead do the initialization in sasl_auth_request
> using svn_atomic_init_once, like we do on the client side. Thoughts?
I'd suggest making it work in main() first, then later moving to
something more complex ;-)
> > I've got anon-access = none and auth-access = write turned on in
> > svnserve.conf, and use-sasl = true set. In my
> > /usr/lib/sasl2/Subversion.conf file I have the following:
> >
> > pwcheck_method: auxprop
> > mech_list: DIGEST-MD5 ANONYMOUS
> >
> > And I gave myself a username and password via saslpasswd2, although I
> > haven't actually got myself to the point of being prompted for it.
> >
> > Other than the crash, a few questions. Why is the config file
> > capitalized? Is that a convention with SASL conf files? We've stuck
> > to all lowercase so far.
>
> Ok, I'll change it to lowercase.
>
> > Also, why do I have to run saslpasswd2 as
> > root? Is that normal? How do people generally control their sasl
> > passwords?
>
> I suppose saslpasswd2 is meant to be used by a system administrator.
Hmm. That's too bad. I suppose if you want the users to have the
ability to change their passwords you'll have to use a sasl config
that uses PAM or LDAP or something.
> > Also, one compile warning I noticed. In sasl_auth_request there's the
> > following bit of code:
> >
> > char *p;
> > const char *user;
> >
> > /* Get the authenticated username. */
> > result = sasl_getprop(sasl_ctx, SASL_USERNAME, (const void **)&user);
> >
> > This produces a warning about type punned pointers, you can avoid that
> > by passing a const void * instead of user, and then assigning it to
> > user later. I also find it a little odd that result is never checked
> > in that function...
>
> Yup, I'll have to fix that. Thanks.
Great! Looking forward to the next rev of this patch.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Server-side Cyrus SASL support
Posted by Vlad Georgescu <vg...@gmail.com>.
On 8/29/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> Ok, now that we're past the weird pool ordering bugs, I actually got
> to the point of trying out the SASL support. I think I've got it
> configured the way you suggested, but I'm getting the following crash
> when I try to run 'svn list' on a repository with sasl turned on.
>
> (gdb) run
> Starting program:
> /home/rooneg/code/svn-trunk/subversion/svnserve/.libs/lt-svnserve -d
> --foreground -r /home/rooneg/code/svn-trunk -T
> [Thread debugging using libthread_db enabled]
> [New Thread -1212417312 (LWP 13558)]
> [New Thread -1213510736 (LWP 13563)]
>
> Program received signal SIGSEGV, Segmentation fault.
> [Switching to Thread -1213510736 (LWP 13563)]
> 0xb7d5e817 in sasl_errdetail () from /usr/lib/libsasl2.so.2
> (gdb) where
> #0 0xb7d5e817 in sasl_errdetail () from /usr/lib/libsasl2.so.2
> #1 0x0804c775 in sasl_auth_request (conn=0x80716e8, pool=0x80713b8,
> b=0xb7ab43a0, required=READ_ACCESS, needs_username=0)
> at subversion/svnserve/sasl_auth.c:262
> #2 0x0804d2cb in auth_request (conn=0x80716e8, pool=0x80713b8, b=0xffffffff,
> required=READ_ACCESS, needs_username=0) at subversion/svnserve/serve.c:329
> #3 0x08051bf0 in serve (conn=0x80716e8, params=0xbfbb2518, pool=0x80713b8)
> at subversion/svnserve/serve.c:2284
> #4 0x0804b599 in serve_thread (tid=0x80737b8, data=0x8071270)
> at subversion/svnserve/main.c:253
> #5 0xb7e01992 in dummy_worker (opaque=0x0) at thread.c:105
> #6 0xb7d72341 in start_thread () from /lib/tls/i686/cmov/libpthread.so.0
> #7 0xb7cf04ee in clone () from /lib/tls/i686/cmov/libc.so.6
> (gdb) up
> #1 0x0804c775 in sasl_auth_request (conn=0x80716e8, pool=0x80713b8,
> b=0xb7ab43a0, required=READ_ACCESS, needs_username=0)
> at subversion/svnserve/sasl_auth.c:262
> 262 return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
> (gdb) list
> 257 secprops.security_flags |= SASL_SEC_NOANONYMOUS;
> 258
> 259 /* Set security properties. */
> 260 result = sasl_setprop(sasl_ctx, SASL_SEC_PROPS, &secprops);
> 261 if (result != SASL_OK)
> 262 return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
> 263 sasl_errdetail(sasl_ctx));
> 264
> 265 /* SASL needs to know if we are externally authenticated. */
> 266 if (b->tunnel_user)
> (gdb)
Ouch. I don't know if it's related to the crash or not, but I noticed
I forgot to do any error-checking when calling sasl_init() in main. So
SASL might not have been initialized.
OTOH, it's probably not a good idea to initialize SASL in main anyway,
because it's a potentially expensive operation (it loads a bunch of
plugins, which might have their own dependencies etc.). That wouldn't
be nice to people who run svn over ssh and/or don't care about Cyrus
SASL. We could instead do the initialization in sasl_auth_request
using svn_atomic_init_once, like we do on the client side. Thoughts?
> I've got anon-access = none and auth-access = write turned on in
> svnserve.conf, and use-sasl = true set. In my
> /usr/lib/sasl2/Subversion.conf file I have the following:
>
> pwcheck_method: auxprop
> mech_list: DIGEST-MD5 ANONYMOUS
>
> And I gave myself a username and password via saslpasswd2, although I
> haven't actually got myself to the point of being prompted for it.
>
> Other than the crash, a few questions. Why is the config file
> capitalized? Is that a convention with SASL conf files? We've stuck
> to all lowercase so far.
Ok, I'll change it to lowercase.
> Also, why do I have to run saslpasswd2 as
> root? Is that normal? How do people generally control their sasl
> passwords?
I suppose saslpasswd2 is meant to be used by a system administrator.
> Also, one compile warning I noticed. In sasl_auth_request there's the
> following bit of code:
>
> char *p;
> const char *user;
>
> /* Get the authenticated username. */
> result = sasl_getprop(sasl_ctx, SASL_USERNAME, (const void **)&user);
>
> This produces a warning about type punned pointers, you can avoid that
> by passing a const void * instead of user, and then assigning it to
> user later. I also find it a little odd that result is never checked
> in that function...
Yup, I'll have to fix that. Thanks.
--
Vlad
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Server-side Cyrus SASL support
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 8/21/06, Vlad Georgescu <vg...@gmail.com> wrote:
> This patch adds Cyrus SASL support to svnserve. Security layers are
> not supported yet - I'll submit another patch for that when this one
> gets the 'stamp of approval'.
>
> [[[
> * subversion/include/svn_config.h
> (SVN_CONFIG_SECTION_SASL): New section.
> (SVN_CONFIG_OPTION_USE_SASL): New option.
>
> * subversion/libsvn_repos/repos.c
> (create_conf): Document the new option.
>
> * subversion/svnserve/sasl_auth.c: New file.
>
> * subversion/svnserve/main.c
> (main): Call sasl_init().
>
> * subversion/svnserve/serve.c:
> (server_baton_t, authn_type, authz_type, get_access): Moved to server.h.
> (simple_auth_request): Contains the code that was prieviously in auth_request.
> (auth_request): Call sasl_auth_request() or simple_auth_request().
> (must_have_access): Consider the value of b->use_sasl when
> determining whether authentication should be performed.
> (find_repos): Read the value of the use-sasl option into b->use_sasl.
> Use that value when determining whether access is allowed to the repository.
>
> * subversion/svnserve/server.h
> (server_baton_t): Moved here from serve.c. Has a new member 'use_sasl'
> (authn_type, authz_type, get_access): Moved here from serve.c.
> (sasl_init, sasl_auth_request): New declarations.
> ]]]
>
> SASL supports many password-checking methods. It can use an external
> daemon saslauthd that can authenticate against /etc/shadow, PAM, LDAP,
> etc. It only works with plaintext passwords, though, so we can't use
> it until we have SSL support in for ra_svn. Until then, we have to use
> the other method, auxprop, which fetches passwords from a database
> (which is usually stored in /etc/sasldb2, but could also be an
> external SQL server, or a LDAP database)
>
> To test this patch, create a configuration file Subversion.conf in
> /usr/lib/sasl2. This file will be read by Cyrus SASL at runtime to
> determine how authentication should be performed.
> See the SASL docs for more details. A simple example could be:
>
> pwcheck_method: auxprop
> mech_list: DIGEST-MD5 ANONYMOUS
>
> Then add users and passwords to Cyrus SASL's password database:
>
> saslpasswd2 -c user -u realm -a Subversion
>
> where 'realm' is the actual realm of your repository.
> Just like Subversion, Cyrus SASL supports the concept of
> "authentication realms". In fact, svnserve will treat the repository
> realm as a SASL realm when authenticating the user. This is necessary
> because the password database is global instead of per-repository, so
> it's the only way for Subversion to tell which users belong to which
> repository.
>
> Comments and criticism are welcome, as always.
Ok, now that we're past the weird pool ordering bugs, I actually got
to the point of trying out the SASL support. I think I've got it
configured the way you suggested, but I'm getting the following crash
when I try to run 'svn list' on a repository with sasl turned on.
(gdb) run
Starting program:
/home/rooneg/code/svn-trunk/subversion/svnserve/.libs/lt-svnserve -d
--foreground -r /home/rooneg/code/svn-trunk -T
[Thread debugging using libthread_db enabled]
[New Thread -1212417312 (LWP 13558)]
[New Thread -1213510736 (LWP 13563)]
Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread -1213510736 (LWP 13563)]
0xb7d5e817 in sasl_errdetail () from /usr/lib/libsasl2.so.2
(gdb) where
#0 0xb7d5e817 in sasl_errdetail () from /usr/lib/libsasl2.so.2
#1 0x0804c775 in sasl_auth_request (conn=0x80716e8, pool=0x80713b8,
b=0xb7ab43a0, required=READ_ACCESS, needs_username=0)
at subversion/svnserve/sasl_auth.c:262
#2 0x0804d2cb in auth_request (conn=0x80716e8, pool=0x80713b8, b=0xffffffff,
required=READ_ACCESS, needs_username=0) at subversion/svnserve/serve.c:329
#3 0x08051bf0 in serve (conn=0x80716e8, params=0xbfbb2518, pool=0x80713b8)
at subversion/svnserve/serve.c:2284
#4 0x0804b599 in serve_thread (tid=0x80737b8, data=0x8071270)
at subversion/svnserve/main.c:253
#5 0xb7e01992 in dummy_worker (opaque=0x0) at thread.c:105
#6 0xb7d72341 in start_thread () from /lib/tls/i686/cmov/libpthread.so.0
#7 0xb7cf04ee in clone () from /lib/tls/i686/cmov/libc.so.6
(gdb) up
#1 0x0804c775 in sasl_auth_request (conn=0x80716e8, pool=0x80713b8,
b=0xb7ab43a0, required=READ_ACCESS, needs_username=0)
at subversion/svnserve/sasl_auth.c:262
262 return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
(gdb) list
257 secprops.security_flags |= SASL_SEC_NOANONYMOUS;
258
259 /* Set security properties. */
260 result = sasl_setprop(sasl_ctx, SASL_SEC_PROPS, &secprops);
261 if (result != SASL_OK)
262 return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
263 sasl_errdetail(sasl_ctx));
264
265 /* SASL needs to know if we are externally authenticated. */
266 if (b->tunnel_user)
(gdb)
I've got anon-access = none and auth-access = write turned on in
svnserve.conf, and use-sasl = true set. In my
/usr/lib/sasl2/Subversion.conf file I have the following:
pwcheck_method: auxprop
mech_list: DIGEST-MD5 ANONYMOUS
And I gave myself a username and password via saslpasswd2, although I
haven't actually got myself to the point of being prompted for it.
Other than the crash, a few questions. Why is the config file
capitalized? Is that a convention with SASL conf files? We've stuck
to all lowercase so far. Also, why do I have to run saslpasswd2 as
root? Is that normal? How do people generally control their sasl
passwords?
Also, one compile warning I noticed. In sasl_auth_request there's the
following bit of code:
char *p;
const char *user;
/* Get the authenticated username. */
result = sasl_getprop(sasl_ctx, SASL_USERNAME, (const void **)&user);
This produces a warning about type punned pointers, you can avoid that
by passing a const void * instead of user, and then assigning it to
user later. I also find it a little odd that result is never checked
in that function...
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org