[Freeipa-devel] [PATCH] bynd-dyndb-ldap: Add separate keytab principal option

[Simo Sorce]

On Thu, 16 Dec 2010 19:47:41 +0100
Zoran Pericic <zpericic at inet.hr> wrote:

> This patch separate sasl_user from keytab.
> 
> For some reason OpenLDAP refuse login if I use sasl_user. OpenLDAP
> try to do proxy login which always fail. openldap clients
> (ldapsearch) sends empty sasl_user for GSSAPI login. To send empty
> sasl_user we need separate option for krb5 principal so we could init
> principal from keytab.
> 
> Here is sample config:
> 
> dynamic-db "ldapdns" {
>      llibrary "ldap.so";
>      arg "connections 2";
>      arg "uri ldap://myhost";
>      arg "base ou=DNS,dc=mybase";
>      arg "cache_ttl 300";
>      arg "auth_method sasl";
>      arg "krb5_keytab FILE:/etc/named/named.keytab";
>      arg "krb5_principal dns/myhost";
>      arg "sasl_mech GSSAPI";
> }
> 
> 
> Best regards,
> Zoran Pericic
> 
> 
> ---
> 
> diff -urN bind-dyndb-ldap-0.1.0b.org/src/ldap_helper.c 
> bind-dyndb-ldap-0.1.0b.krb5_principal/src/ldap_helper.c
> --- bind-dyndb-ldap-0.1.0b.org/src/ldap_helper.c    2010-03-24 
> 11:55:30.000000000 +0100
> +++ bind-dyndb-ldap-0.1.0b.krb5_principal/src/ldap_helper.c    
> 2010-11-18 00:17:57.503920016 +0100
> @@ -128,6 +128,7 @@
>       ldap_auth_t        auth_method;
>       ld_string_t        *bind_dn;
>       ld_string_t        *password;
> +    ld_string_t        *krb5_principal;

are you using spaces instead of tabs ?
although we prefer 4 spaces indentations in general in the Freeipa
project it appears that the bind-dyndb-ldap plugin uses tabs for
indentation, so this patch should do the same.

>       ld_string_t        *sasl_mech;
>       ld_string_t        *sasl_user;
>       ld_string_t        *sasl_auth_name;
> @@ -293,6 +294,7 @@
>           { "auth_method", default_string("none")        },
>           { "bind_dn",     default_string("")        },
>           { "password",     default_string("")        },
> +        { "krb5_principal",     default_string("")    },
^^ tab

>           { "sasl_mech",     default_string("GSSAPI")    },
>           { "sasl_user",     default_string("")        },
>           { "sasl_auth_name", default_string("")        },
> @@ -330,6 +332,7 @@
>       CHECK(str_new(mctx, &ldap_inst->base));
>       CHECK(str_new(mctx, &ldap_inst->bind_dn));
>       CHECK(str_new(mctx, &ldap_inst->password));
> +    CHECK(str_new(mctx, &ldap_inst->krb5_principal));
^^ tab

>       CHECK(str_new(mctx, &ldap_inst->sasl_mech));
>       CHECK(str_new(mctx, &ldap_inst->sasl_user));
>       CHECK(str_new(mctx, &ldap_inst->sasl_auth_name));
> @@ -346,6 +349,7 @@
>       ldap_settings[i++].target = auth_method_str;
>       ldap_settings[i++].target = ldap_inst->bind_dn;
>       ldap_settings[i++].target = ldap_inst->password;
> +    ldap_settings[i++].target = ldap_inst->krb5_principal;
^^ tab

>       ldap_settings[i++].target = ldap_inst->sasl_mech;
>       ldap_settings[i++].target = ldap_inst->sasl_user;
>       ldap_settings[i++].target = ldap_inst->sasl_auth_name;
> @@ -380,13 +384,24 @@
>       }
> 
>       /* check we have the right data when SASL/GSSAPI is selected */
> -    if ((ldap_inst->auth_method == AUTH_SASL) &&
> -         (str_casecmp_char(ldap_inst->sasl_mech, "GSSAPI") == 0)) {
> -        if ((ldap_inst->sasl_user == NULL) ||
> -            (str_len(ldap_inst->sasl_user) == 0)) {
> -            log_error("Sasl mech GSSAPI defined but sasl_user is
> empty");
> -            result = ISC_R_FAILURE;
> -            goto cleanup;
> +    if((ldap_inst->auth_method == AUTH_SASL) &&

please keep using 'if (' and not 'if(' as the rest of the code.

> +        (str_casecmp_char(ldap_inst->sasl_mech, "GSSAPI") == 0)) {
> +        if((ldap_inst->krb5_principal == NULL) &&
> +            (str_len(ldap_inst->krb5_principal) == 0)) {
> +            if((ldap_inst->sasl_user == NULL) &&
> +                (str_len(ldap_inst->sasl_user) == 0)) {

the above 2 statements seem wrong to me, the original one had:
	if ((cond 1) || (cond 2)) {
while you changed it into:
	if ((cond 1) && (cond 2)) {
This fails to do the check that is intended.


> +                char hostname[255];
> +                if(gethostname(hostname, 255) != 0) {
> +                    log_error("SASL mech GSSAPI defined but 
> krb5_principal and sasl_user are empty. Could not get hostname");
> +                    result = ISC_R_FAILURE;
> +                    goto cleanup;
> +                } else {
> +                    str_sprintf(ldap_inst->krb5_principal, "dns/%s", 
> hostname);

This should probably be "DNS/%s", Kerberos is generally case sensitive
and t5he default for bind is to use the service name part in all caps.

> +                    log_debug(2, "SASL mech GSSAPI defined but 
> krb5_principal and sasl_user are empty, using default %s", 
> str_buf(ldap_inst->krb5_principal));

It would be preferable, but not necessary, to split long lines so that
they stay within 80 columns limits and avoid wrapping.

> +                }
> +            } else {
> +                str_copy(ldap_inst->krb5_principal,
> ldap_inst->sasl_user);
> +            }
>           }
>       }
> 
> @@ -447,6 +462,7 @@
>       str_destroy(&ldap_inst->base);
>       str_destroy(&ldap_inst->bind_dn);
>       str_destroy(&ldap_inst->password);
> +    str_destroy(&ldap_inst->krb5_principal);
>       str_destroy(&ldap_inst->sasl_mech);
>       str_destroy(&ldap_inst->sasl_user);
>       str_destroy(&ldap_inst->sasl_auth_name);
> @@ -1618,7 +1634,7 @@
>               isc_result_t result;
>               LOCK(&ldap_inst->kinit_lock);
>               result = get_krb5_tgt(ldap_inst->mctx,
> -                          str_buf(ldap_inst->sasl_user),
> +                          str_buf(ldap_inst->krb5_principal),
>                             str_buf(ldap_inst->krb5_keytab));
>               UNLOCK(&ldap_inst->kinit_lock);
>               if (result != ISC_R_SUCCESS)

Generally the direction looks ok, if you fix the above issues it will
get an ack.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York