On 6/16/06, Vlad Georgescu <vgeorgescu@gmail.com> wrote:
> [[[
> Implement support for Cyrus SASL on the client side.
>
> * configure.in
> Define SVN_HAVE_SASL
>
> * subversion/libsvn_ra_svn/sasl_auth.c: New file.
>
> * subversion/libsvn_ra_svn/trivial_auth.c: New file. Contains
> functions taken from client.c.
>
> * subversion/libsvn_ra_svn/client.c
> (ra_svn_session_baton_t): Moved to ra_svn.h and renamed to
> svn_ra_svn__session_baton_t. Changed all occurances to reflect this.
> (find_mech, read_success): Moved to trivial_auth.c.
> (do_auth): Moved to trivial_auth.c and renamed to svn_ra_svn__handle_auth.
> (auth_response): Renamed to svn_ra_svn__auth_response.
> (svn_ra_svn__init): Call svn_ra_svn__sasl_init.
> (open_session): Call svn_ra_svn__sasl_new.
> Updated comments to reflect changes.
>
> * subversion/libsvn_ra_svn/ra_svn.h
> Conditionally #include <sasl/sasl.h>
> (svn_ra_svn_conn_st): Add new member: sasl_conn.
> (svn_ra_svn__handle_auth,
> svn_ra_svn__auth_response,
> svn_ra_svn__sasl_init,
> svn_ra_svn__sasl_new): New declarations.
> ]]]
Ok, more good stuff, but there are a few things I've noticed that
we're going to want to look into/fix.
1) The new files need to have a license block at the beginning, just
like the rest of the files in the tree.
2) This doesn't seem to do anything with regard to thread safety.
Looking at the cyrus sasl headers there seems to be a sasl_set_mutex
function that we're going to need to call. Since they've given us a
somewhat bone-headed design to work with, it probably means we're
going to have to jump through a few hoops to make it work with APR.
I'm envisioning something along the lines of a global pool that we
allocate mutexes out of or something like that, with our free function
returning the mutexes to a free list or something...
If we don't call sasl_set_mutex what is the behavior? Does sasl use
its own internal mutex allocation code? Or does it just not lock?
3) A few minor points I noticed in the code:
+ /* make sure the SASL context is properly disposed of */
+ apr_pool_cleanup_register(pool, &sess->conn->sasl_conn,
+ &sasl_dispose_wrapper,
+ apr_pool_cleanup_null);
No need for the & before sasl_dispose_wrapper.
+ /* Set security properties */
+ secprops.min_ssf = 0;
+ secprops.max_ssf = 256;
I have no idea what this stuff does, but magic numbers like that make
me nervous. Where did those values come from, and could they be
#defined someplace so they have more meaningful names?
+ 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;
+ }
+ }
Are there any other possible values for prompt->id?
+/* base64-encode src and put it in *dest */
+static svn_error_t *encode(char **dest, const char *src, int srclen,
+ apr_pool_t *pool)
+{
+ int destlen = srclen;
+ while (destlen % 3)
+ destlen++;
+ destlen = (destlen * 4) / 3;
+ *dest = apr_palloc(pool, destlen + 1);
+ SVN_SASL_ERR(sasl_encode64(src, srclen, *dest, destlen, NULL),
+ "Could not encode client response");
+ (*dest)[destlen] = '\0';
+ return SVN_NO_ERROR;
+}
I feel like we've already got base64 encoding functions in svn someplace...
+ /* Some mechs send data with the last server response.
+ * We need to call sasl_client_step one last time */
+ SVN_SASL_ERR(sasl_client_step(sess->conn->sasl_conn,
+ response,
+ strlen(response),
+ &client_interact,
+ &out, /* Not used */
+ &outlen), /* Not used */
+ "SASL failure during authentication");
Indenting is off here.
+svn_error_t *svn_ra_svn__handle_auth(svn_ra_svn__session_baton_t *sess,
+ apr_array_header_t *mechlist,
+ const char *realm, apr_pool_t *pool)
+{
+ const char *realmstring, *mechstring;
+ svn_ra_svn_item_t *elt;
+ svn_auth_iterstate_t *iterstate = NULL;
+ int i;
+ svn_boolean_t try_more_creds = TRUE;
+ svn_boolean_t compat = (realm == NULL);
+
+ realmstring = realm ?
+ apr_psprintf(pool, "%s %s", sess->realm_prefix, realm)
+ : sess->realm_prefix;
+
+ apr_pool_t *subpool = svn_pool_create(pool);
Don't declare variables part way into a function, it's not valid C (at
least not for the versions of C we support).
+ /* create a string containing the list of mechanisms, separated by spaces */
+ elt = &((svn_ra_svn_item_t *) mechlist->elts)[0];
+ mechstring = elt->u.word;
You could use APR_ARRAY_IDX here, for readability. In other places too, FWIW.
Looking good though. I'll try and apply this patch and take it for a
spin sometime today...
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Jun 16 17:17:46 2006