apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Paul J. Reder" <rede...@remulak.net>
Subject Re: [Patch]: Add basic function to APR for LDAP rebind callback support.
Date Mon, 03 Dec 2007 19:09:47 GMT
Graham,

Your changes look good to me (assuming the ldap struct does get passed in to
the cleanup). I made a few minor alterations to your patch and attached it
here. I like the simplification of automatically registering the callback
and better name space protection.

I'm a little concerned about the pool cleanup though. I'll probably still
explicitly call the remove because I'm concerned that in a busy system there
could be a window where the ldap struct is freed and reused by another request
before the first request processing is completed and the cleanup is paged in
and run. In that case the old xref might be found (even though the new one will
likely have been added). Calling the remove explicitly allows the xref to be
removed as soon as the ldap struct is no longer required rather than waiting
for the rest of the HTTP processing to happen before the pool is cleaned up.

Thanks for the review and effort to commit this.

Paul J. Reder

Graham Leggett wrote:
> Paul J. Reder wrote:
> 
>> I addressed the comments and haven't heard back from anyone
>> (Bojan or others). I can't commit to APR and can't commit the
>> Apache portion until the APR part has been committed. I know
>> folks were busy with the latest APR update... Has the dust
>> settled?
> 
> Looking at this in more detail.
> 
> It took me a while to figure out exactly what the rebind code was trying 
> to do (as opposed to generally knowing what it does), and it seems to be 
> "an implementation of a callback mechanism able to take advantage of the 
> LDAP referral callback feature".
> 
> Or in other words, use of this particular API is optional, someone using 
> the APR interface may choose to use this particular implementation, or 
> they may choose some other implementation of their own.
> 
> Based on this, I think the API should all be in a namespace like 
> apr_ldap_rebind.
> 
> Looking further, unless I am missing something, I think this could 
> probably be significantly simplified.
> 
> In theory, the apr_ldap_set_rebind_callback() function can be called by 
> apr_ldap_xref_add(), hiding apr_ldap_set_rebind_callback().
> 
> In theory, there should be a way to register a pool cleanup for 
> apr_ldap_rebind_remove() as well.
> 
> I have updated the patch, which is attached - will this do?
> 
> Regards,
> Graham
> -- 
> 
> 
> ------------------------------------------------------------------------
> 
> Index: ldap/apr_ldap_rebind.c
> ===================================================================
> --- ldap/apr_ldap_rebind.c	(revision 0)
> +++ ldap/apr_ldap_rebind.c	(revision 0)
> @@ -0,0 +1,251 @@
> +/* Licensed to the Apache Software Foundation (ASF) under one or more
> + * contributor license agreements.  See the NOTICE file distributed with
> + * this work for additional information regarding copyright ownership.
> + * The ASF licenses this file to You under the Apache License, Version 2.0
> + * (the "License"); you may not use this file except in compliance with
> + * the License.  You may obtain a copy of the License at
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +/*  apr_ldap_rebind.c -- LDAP rebind callbacks for referrals
> + *
> + *  The LDAP SDK allows a callback to be set to enable rebinding
> + *  for referral processing.
> + *
> + */
> +
> +#include "apr.h"
> +#include "apu.h"
> +#include "apr_ldap.h"
> +#include "apr_errno.h"
> +#include "apr_strings.h"
> +#include "apr_ldap_rebind.h"
> +
> +#include "stdio.h"
> +
> +#if APR_HAS_THREADS
> +static apr_thread_mutex_t *apr_ldap_xref_lock = NULL;
> +#endif
> +
> +/* Used to store information about connections for use in the referral rebind callback.
*/
> +struct apr_ldap_rebind_entry {
> +    LDAP *index;
> +    const char *bindDN;
> +    const char *bindPW;
> +    struct apr_ldap_rebind_entry *next;
> +};
> +typedef struct apr_ldap_rebind_entry apr_ldap_rebind_entry_t;
> +
> +static apr_ldap_rebind_entry_t *xref_head = NULL;
> +
> +static int apr_ldap_rebind_set_callback(LDAP *ld);
> +static apr_status_t apr_ldap_rebind_remove_helper(void *data);
> +
> +/* APR utility routine used to create the xref_lock. */
> +APU_DECLARE(apr_status_t) apr_ldap_rebind_init(apr_pool_t *pool)
> +{
> +    apr_status_t retcode = APR_SUCCESS;
> +
> +#if APR_HAS_THREADS
> +    if (apr_ldap_xref_lock == NULL) {
> +        retcode = apr_thread_mutex_create(&apr_ldap_xref_lock, APR_THREAD_MUTEX_DEFAULT,
pool);
> +    }
> +#endif
> +
> +    return(retcode);
> +}
> +
> +
> +/*************************************************************************************/
> +APU_DECLARE(apr_status_t) apr_ldap_rebind_add(apr_pool_t *pool, LDAP *ld, const char
*bindDN, const char *bindPW)
> +{
> +    apr_status_t retcode = APR_SUCCESS;
> +    apr_ldap_rebind_entry_t *new_xref;
> +
> +    new_xref = (apr_ldap_rebind_entry_t *)apr_pcalloc(pool, sizeof(apr_ldap_rebind_entry_t));
> +    if (new_xref) {
> +        new_xref->index = ld;
> +        if (bindDN) {
> +            new_xref->bindDN = apr_pstrdup(pool, bindDN);
> +        }
> +        if (bindPW) {
> +            new_xref->bindPW = apr_pstrdup(pool, bindPW);
> +        }
> +    
> +#if APR_HAS_THREADS
> +       apr_thread_mutex_lock(apr_ldap_xref_lock);
> +#endif
> +    
> +        new_xref->next = xref_head;
> +        xref_head = new_xref;
> +    
> +#if APR_HAS_THREADS
> +        apr_thread_mutex_unlock(apr_ldap_xref_lock);
> +#endif
> +    }
> +    else {
> +        return(APR_ENOMEM);
> +    }
> +
> +    retcode = apr_ldap_rebind_set_callback(ld);
> +    if (APR_SUCCESS != retcode) {
> +        apr_ldap_rebind_remove(ld);
> +        return retcode;
> +    }
> +
> +    apr_pool_cleanup_register(pool, ld,
> +                              apr_ldap_rebind_remove_helper,
> +                              apr_pool_cleanup_null);
> +
> +    return(APR_SUCCESS);
> +}
> +
> +/*************************************************************************************/
> +APU_DECLARE(apr_status_t) apr_ldap_rebind_remove(LDAP *ld)
> +{
> +    apr_ldap_rebind_entry_t *tmp_xref, *prev = NULL;
> +
> +#if APR_HAS_THREADS
> +    apr_thread_mutex_lock(apr_ldap_xref_lock);
> +#endif
> +    tmp_xref = xref_head;
> +
> +    while ((tmp_xref) && (tmp_xref->index != ld)) {
> +        prev = tmp_xref;
> +        tmp_xref = tmp_xref->next;
> +    }
> +
> +    if (tmp_xref) {
> +        if (tmp_xref == xref_head) {
> +            xref_head = xref_head->next;
> +        }
> +        else {
> +            prev->next = tmp_xref->next;
> +        }
> +        /* tmp_xref and its contents were pool allocated so they don't need to be freed
here. */
> +    }
> +
> +#if APR_HAS_THREADS
> +    apr_thread_mutex_unlock(apr_ldap_xref_lock);
> +#endif
> +    return APR_SUCCESS;
> +}
> +
> +static apr_status_t apr_ldap_rebind_remove_helper(void *data)
> +{
> +    LDAP *ld = (LDAP *)data;
> +    apr_ldap_rebind_remove(ld);
> +    return APR_SUCCESS;
> +}
> +
> +/*************************************************************************************/
> +static apr_ldap_rebind_entry_t *apr_ldap_rebind_lookup(LDAP *ld)
> +{
> +    apr_ldap_rebind_entry_t *tmp_xref, *match = NULL;
> +
> +#if APR_HAS_THREADS
> +    apr_thread_mutex_lock(apr_ldap_xref_lock);
> +#endif
> +    tmp_xref = xref_head;
> +
> +    while (tmp_xref) {
> +        if (tmp_xref->index == ld) {
> +            match = tmp_xref;
> +            tmp_xref = NULL;
> +        }
> +        else {
> +            tmp_xref = tmp_xref->next;
> +        }
> +    }
> +
> +#if APR_HAS_THREADS
> +    apr_thread_mutex_unlock(apr_ldap_xref_lock);
> +#endif
> +
> +    return (match);
> +}
> +
> +/* LDAP_rebindproc() ITDS style
> + *     Rebind callback function. Called when chasing referrals. See API docs.
> + * ON ENTRY:
> + *     ld       Pointer to an LDAP control structure. (input only)
> + *     binddnp  Pointer to an Application DName used for binding (in *or* out)
> + *     passwdp  Pointer to the password associated with the DName (in *or* out)
> + *     methodp  Pointer to the Auth method (output only)
> + *     freeit   Flag to indicate if this is a lookup or a free request (input only)
> + */
> +#if APR_HAS_TIVOLI_LDAPSDK
> +static int LDAP_rebindproc(LDAP *ld, char **binddnp, char **passwdp, int *methodp, int
freeit)
> +{
> +    if (!freeit) {
> +        apr_ldap_rebind_entry_t *my_conn;
> +
> +        *methodp = LDAP_AUTH_SIMPLE;
> +        my_conn = apr_ldap_xref_lookup(ld);
> +
> +        if ((my_conn) && (my_conn->bindDN != NULL)) {
> +            *binddnp = strdup(my_conn->bindDN);
> +            *passwdp = strdup(my_conn->bindPW);
> +        } else {
> +            *binddnp = NULL;
> +            *passwdp = NULL;
> +        }
> +    } else {
> +        if (*binddnp) {
> +            free(*binddnp);
> +        }
> +        if (*passwdp) {
> +            free(*passwdp);
> +        }
> +    }
> +
> +    return LDAP_SUCCESS;
> +}
> +
> +static int apr_ldap_rebind_set_callback(LDAP *ld)
> +{
> +    ldap_set_rebind_proc(ld, (LDAPRebindProc)LDAP_rebindproc);
> +    return APR_SUCCESS;
> +}
> +
> +#elif APR_HAS_OPENLDAP_LDAPSDK
> +
> +/* LDAP_rebindproc() openLDAP V3 style */
> +static int LDAP_rebindproc(LDAP *ld, LDAP_CONST char *url, ber_tag_t request, ber_int_t
msgid, void *params)
> +{
> +    apr_ldap_rebind_entry_t *my_conn;
> +    const char *bindDN = NULL;
> +    const char *bindPW = NULL;
> +
> +    my_conn = apr_ldap_rebind_lookup(ld);
> +
> +    if ((my_conn) && (my_conn->bindDN != NULL)) {
> +        bindDN = my_conn->bindDN;
> +        bindPW = my_conn->bindPW;
> +    }
> +
> +    return (ldap_bind_s(ld, bindDN, bindPW, LDAP_AUTH_SIMPLE));
> +}
> +
> +static int apr_ldap_rebind_set_callback(LDAP *ld)
> +{
> +    ldap_set_rebind_proc(ld, LDAP_rebindproc, NULL);
> +    return APR_SUCCESS;
> +}
> +
> +#else         /* Implementation not recognised */
> +
> +static int apr_ldap_rebind_set_callback(LDAP *ld)
> +{
> +    return APR_ENOTIMPL;
> +}
> +
> +#endif
> +
> Index: ldap/NWGNUmakefile
> ===================================================================
> --- ldap/NWGNUmakefile	(revision 599173)
> +++ ldap/NWGNUmakefile	(working copy)
> @@ -231,6 +231,7 @@
>  	$(OBJDIR)/apr_ldap_init.o \
>  	$(OBJDIR)/apr_ldap_option.o \
>  	$(OBJDIR)/apr_ldap_url.o \
> +	$(OBJDIR)/apr_ldap_rebind.o \
>  	$(EOLIST)
>  
>  #
> Index: include/apr_ldap_rebind.h
> ===================================================================
> --- include/apr_ldap_rebind.h	(revision 0)
> +++ include/apr_ldap_rebind.h	(revision 0)
> @@ -0,0 +1,80 @@
> +/* Licensed to the Apache Software Foundation (ASF) under one or more
> + * contributor license agreements.  See the NOTICE file distributed with
> + * this work for additional information regarding copyright ownership.
> + * The ASF licenses this file to You under the Apache License, Version 2.0
> + * (the "License"); you may not use this file except in compliance with
> + * the License.  You may obtain a copy of the License at
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +/**
> + * The APR LDAP rebind functions provide an implementation of
> + * a rebind procedure that can be used to allow clients to chase referrals,
> + * using the same credentials used to log in originally.
> + *
> + * Use of this implementation is optional.
> + *
> + * @file apu_ldap_rebind.h
> + * @brief Apache LDAP library
> + */
> +
> +#ifndef APU_LDAP_REBIND_H
> +#define APU_LDAP_REBIND_H
> +
> +/**
> + * APR LDAP initialize rebind lock
> + *
> + * This function creates the lock for controlling access to the xref list..
> + * @param pool Pool to use when creating the xref_lock.
> + */
> +APU_DECLARE(apr_status_t) apr_ldap_rebind_init(apr_pool_t *pool);
> +
> +
> +/**
> + * APR LDAP rebind_add function
> + *
> + * This function creates a cross reference entry for the specified ldap
> + * connection. The rebind callback function will look up this ldap 
> + * connection so it can retrieve the bindDN and bindPW for use in any 
> + * binds while referrals are being chased.
> + *
> + * This function will add the callback to the LDAP handle passed in.
> + *
> + * A cleanup is registered within the pool provided to remove this
> + * entry when the pool is removed. Alternatively apr_ldap_rebind_remove()
> + * can be called to explicitly remove the entry at will.
> + *
> + * @param pool The pool to use
> + * @param ld The LDAP connectionhandle
> + * @param bindDN The bind DN to be used for any binds while chasing 
> + *               referrals on this ldap connection.
> + * @param bindPW The bind Password to be used for any binds while 
> + *               chasing referrals on this ldap connection.
> + */
> +APU_DECLARE(apr_status_t) apr_ldap_rebind_add(apr_pool_t *pool,
> +                                              LDAP *ld,
> +                                              const char *bindDN,
> +                                              const char *bindPW);
> +
> +/**
> + * APR LDAP rebind_remove function
> + *
> + * This function removes the rebind cross reference entry for the
> + * specified ldap connection.
> + *
> + * If not explicitly removed, this function will be called automatically
> + * when the pool is cleaned up.
> + *
> + * @param ld The LDAP connectionhandle
> + */
> +APU_DECLARE(apr_status_t) apr_ldap_rebind_remove(LDAP *ld);
> +
> +#endif /* APU_LDAP_REBIND_H */
> +
> Index: include/apr_ldap.h.in
> ===================================================================
> --- include/apr_ldap.h.in	(revision 599173)
> +++ include/apr_ldap.h.in	(working copy)
> @@ -154,6 +154,7 @@
>  #include "apr_ldap_url.h"
>  #include "apr_ldap_init.h"
>  #include "apr_ldap_option.h"
> +#include "apr_ldap_rebind.h"
>  
>  /** @} */
>  #endif /* APR_HAS_LDAP */

-- 
Paul J. Reder
-----------------------------------------------------------
"The strength of the Constitution lies entirely in the determination of each
citizen to defend it.  Only if every single citizen feels duty bound to do
his share in this defense are the constitutional rights secure."
-- Albert Einstein


Mime
View raw message