[Freeipa-devel] [PATCH] ipasam SASL bind callback fixes
Alexander Bokovoy
abokovoy at redhat.com
Thu Jul 5 13:27:01 UTC 2012
On Thu, 05 Jul 2012, Simo Sorce wrote:
>On Wed, 2012-07-04 at 23:10 +0300, Alexander Bokovoy wrote:
>> On Wed, 04 Jul 2012, Sumit Bose wrote:
>> >On Wed, Jul 04, 2012 at 08:57:44PM +0300, Alexander Bokovoy wrote:
>> >> Hi,
>> >>
>> >> when chasing what looked like ccache corruption with Sumit, I've found
>> >> yet another issue: use of local stack variable in long-time living code.
>> >>
>> >> This local stack use was absent in the original patch version and was
>> >> proposed by Sumit on one of reviews. It worked for us by luck, it should
>> >> not have.
>> >>
>> >> Hence, the patch that fixes the issue by moving service principal to a
>> >> longer term storage (ipasam private struct).
>> >>
>> >> In order to avoid ccache corruption we also need to move back to
>> >> in-memory ccache. When multiple LSASD and smbd processes try to auth
>> >> against LDAP in ipasam, they may write to the same ccache (common
>> >> ccache) when another process reads from it. This is not what we need.
>> >>
>> >> And writing to a persistent on-disk ccache is not needed anyway, as
>> >> smbldap connections re-authenticate themselves (smbldap connections
>> >> expire in few minutes).
>> >>
>> >> The patch also removes kerberos operations that are not needed when
>> >> using memory ccache.
>> >>
>> >> --
>> >> / Alexander Bokovoy
>> >
>> >This patch works for me and I have no objections using the in-memory
>> >ccache, so ACK from my side, but please wait for Simo's answer before
>> >pushing.
>> >
>> >Simo, you were in favour of using the default cache, do you
>> >agree as well?
>> >
>> >Before pushing please fix:
>> >
>> >ipa_sam.c:3164:8: warning: unused variable 'ccache_name' [-Wunused-variable]
>>
>> After talking with Sumit and researching I've changed back to use
>> on-disk ccache and avoid re-initing on every callback call.
>
>Thank, this is what I would have asked anyway.
>
>> Attached patch should be good. The only trouble is that you'll need to
>> re-run 'ipa-adtrust-install' as smb.conf's parameter we use has been
>> changed to a different one. That or you can manually run
>>
>> net conf setparm 'global' 'ipasam:principal template' '$FQDN@$REALM'
>>
>> where $FQDN is the fully-qualified domain name of the IPA server where
>> smbd will run and $REALM is IPA realm name.
>
>We do not need to store this information in the configuration
>explicitly, we have all that info already.
>
>So NACK, please remove this configuration option completely and fetch
>fqdn from the system (gethostname) and the realm from the krb5 context.
>
>> Note that I had to do this change to avoid manipulating by ipasam:principal
>> name as I need both client and server principals to use krb5_get_credentials().
>> With this change ipasam will build proper principal names by itself.
>>
>> Note that we cannot use use ipasam_privates.realm value because it is
>> not available at the point when callback is called. It is fetched from
>> ldap later, after callback has been successfully used.
>
>See above, we have all the information we need via simple API calls to
>glibc or krb5 libs, we do not need to retrieve that information by other
>means.
New patch is attached. Works for me without any smb.conf parameters.
--
/ Alexander Bokovoy
-------------- next part --------------
>From a10a96ce0c1d024fa4c25ade692d8e2abff1ba44 Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy <abokovoy at redhat.com>
Date: Wed, 4 Jul 2012 20:47:03 +0300
Subject: [PATCH 2/2] ipasam: improve SASL bind callback
SASL bind callback due to refactoring was referencing local variable which
didn't exist all the time. Fix that by including a copy of service principals
into ipasam long term private struct.
Rework ccache handling to avoid re-initing every time callback is called
---
daemons/ipa-sam/ipa_sam.c | 180 +++++++++++++++++++++++++++++----------
install/share/smb.conf.template | 1 -
2 files changed, 137 insertions(+), 44 deletions(-)
diff --git a/daemons/ipa-sam/ipa_sam.c b/daemons/ipa-sam/ipa_sam.c
index 9baac1b2d35a640c36fe7f95a6154ec8582649d8..ab766dce87563ed7f5dba1fc08f3eea1d5cea727 100644
--- a/daemons/ipa-sam/ipa_sam.c
+++ b/daemons/ipa-sam/ipa_sam.c
@@ -177,6 +177,8 @@ struct ipasam_privates {
char *trust_dn;
char *flat_name;
char *fallback_primary_group;
+ char *server_princ;
+ char *client_princ;
};
static LDAP *priv2ld(struct ldapsam_privates *priv)
@@ -3125,12 +3127,20 @@ static int ldap_sasl_interact(LDAP *ld, unsigned flags, void *priv_data, void *s
return ret;
}
-static void bind_callback_cleanup(struct ipasam_sasl_interact_priv *data)
+static void bind_callback_cleanup(struct ipasam_sasl_interact_priv *data, krb5_error_code rc)
{
+ const char *errstring = NULL;
+
if (!data->context) {
return;
}
+ if (rc) {
+ errstring = krb5_get_error_message(data->context, rc);
+ DEBUG(0,("kerberos error: code=%d, message=%s\n", rc, errstring));
+ krb5_free_error_message(data->context, errstring);
+ }
+
krb5_free_cred_contents(data->context, &data->creds);
if (data->options) {
@@ -3157,22 +3167,27 @@ static void bind_callback_cleanup(struct ipasam_sasl_interact_priv *data)
data->context = NULL;
}
-extern const char *lp_parm_const_string(int snum, const char *type, const char *option, const char *def);
-static int bind_callback(LDAP *ldap_struct, struct smbldap_state *ldap_state, void* ipasam_principal)
+static int bind_callback(LDAP *ldap_struct, struct smbldap_state *ldap_state, void* ipasam_priv)
{
- char *ccache_name = NULL;
krb5_error_code rc;
+ krb5_creds *out_creds = NULL;
+ krb5_creds in_creds;
struct ipasam_sasl_interact_priv data;
+ struct ipasam_privates *ipasam_private = NULL;
int ret;
memset(&data, 0, sizeof(struct ipasam_sasl_interact_priv));
- data.name = (const char*)ipasam_principal;
- if (data.name == NULL) {
- DEBUG(0, ("bind_callback: ipasam:principal is not set, cannot use GSSAPI bind\n"));
+ memset(&in_creds, 0, sizeof(krb5_creds));
+
+ ipasam_private = (struct ipasam_privates*)ipasam_priv;
+
+ if ((ipasam_private->client_princ == NULL) && (ipasam_private->server_princ == NULL)) {
+ DEBUG(0, ("bind_callback: 'ipasam:principal template' is not set, cannot use GSSAPI bind\n"));
return LDAP_LOCAL_ERROR;
}
+ data.name = ipasam_private->client_princ;
data.name_len = strlen(data.name);
rc = krb5_init_context(&data.context);
@@ -3182,60 +3197,60 @@ static int bind_callback(LDAP *ldap_struct, struct smbldap_state *ldap_state, vo
rc = krb5_parse_name(data.context, data.name, &data.principal);
if (rc) {
- bind_callback_cleanup(&data);
+ bind_callback_cleanup(&data, rc);
return LDAP_LOCAL_ERROR;
}
rc = krb5_cc_default(data.context, &data.ccache);
- if (rc) {
- bind_callback_cleanup(&data);
- return LDAP_LOCAL_ERROR;
- }
-
- rc = krb5_cc_initialize(data.context, data.ccache, data.principal);
- if (rc) {
- bind_callback_cleanup(&data);
- return LDAP_LOCAL_ERROR;
- }
-
- rc = krb5_cc_get_full_name(data.context, data.ccache, &ccache_name);
- if (rc) {
- if (ccache_name) {
- krb5_free_string(data.context, ccache_name);
- }
- bind_callback_cleanup(&data);
- return LDAP_LOCAL_ERROR;
- }
- rc = krb5_cc_set_default_name(data.context, ccache_name);
if (rc) {
- bind_callback_cleanup(&data);
+ bind_callback_cleanup(&data, rc);
return LDAP_LOCAL_ERROR;
}
rc = krb5_kt_resolve(data.context, "FILE:/etc/samba/samba.keytab", &data.keytab);
if (rc) {
- bind_callback_cleanup(&data);
+ bind_callback_cleanup(&data, rc);
return LDAP_LOCAL_ERROR;
}
- rc = krb5_get_init_creds_opt_alloc(data.context, &data.options);
+ rc = krb5_parse_name(data.context, ipasam_private->client_princ, &in_creds.client);
if (rc) {
- bind_callback_cleanup(&data);
+ krb5_free_principal(data.context, data.creds.client);
+ bind_callback_cleanup(&data, rc);
return LDAP_LOCAL_ERROR;
}
- rc = krb5_get_init_creds_opt_set_out_ccache(data.context, data.options, data.ccache);
+ rc = krb5_parse_name(data.context, ipasam_private->server_princ, &in_creds.server);
if (rc) {
- bind_callback_cleanup(&data);
+ krb5_free_principal(data.context, in_creds.server);
+ bind_callback_cleanup(&data, rc);
return LDAP_LOCAL_ERROR;
}
- rc = krb5_get_init_creds_keytab(data.context, &data.creds, data.principal, data.keytab,
- 0, NULL, data.options);
+ rc = krb5_get_credentials(data.context, KRB5_GC_CACHED, data.ccache, &in_creds, &out_creds);
+ krb5_free_principal(data.context, in_creds.server);
+ krb5_free_principal(data.context, in_creds.client);
+
if (rc) {
- bind_callback_cleanup(&data);
- return LDAP_LOCAL_ERROR;
+ rc = krb5_get_init_creds_opt_alloc(data.context, &data.options);
+ if (rc) {
+ bind_callback_cleanup(&data, rc);
+ return LDAP_LOCAL_ERROR;
+ }
+
+ rc = krb5_get_init_creds_opt_set_out_ccache(data.context, data.options, data.ccache);
+ if (rc) {
+ bind_callback_cleanup(&data, rc);
+ return LDAP_LOCAL_ERROR;
+ }
+
+ rc = krb5_get_init_creds_keytab(data.context, &data.creds, data.principal, data.keytab,
+ 0, NULL, data.options);
+ if (rc) {
+ bind_callback_cleanup(&data, rc);
+ return LDAP_LOCAL_ERROR;
+ }
}
ret = ldap_sasl_interactive_bind_s(ldap_struct,
@@ -3247,10 +3262,90 @@ static int bind_callback(LDAP *ldap_struct, struct smbldap_state *ldap_state, vo
DEBUG(0, ("bind_callback: cannot perform interactive SASL bind with GSSAPI\n"));
}
- bind_callback_cleanup(&data);
+ if (out_creds) {
+ krb5_free_creds(data.context, out_creds);
+ }
+ bind_callback_cleanup(&data, 0);
return ret;
}
+static NTSTATUS ipasam_generate_principals(struct ipasam_privates *privates) {
+
+ krb5_error_code rc;
+ int ret;
+ krb5_context context;
+ NTSTATUS status = NT_STATUS_UNSUCCESSFUL;
+ char hostname[255];
+ char *default_realm = NULL;
+
+ if (!privates) {
+ return status;
+ }
+
+ rc = krb5_init_context(&context);
+ if (rc) {
+ return status;
+ }
+
+ ret = gethostname(hostname, sizeof(hostname));
+ if (ret == -1) {
+ DEBUG(1, ("gethostname failed.\n"));
+ goto done;
+ }
+ hostname[sizeof(hostname)-1] = '\0';
+
+ rc = krb5_get_default_realm(context, &default_realm);
+ if (rc) {
+ goto done;
+ };
+
+ if (privates->client_princ) {
+ talloc_free(privates->client_princ);
+ privates->client_princ = NULL;
+ }
+
+ privates->client_princ = talloc_asprintf(privates,
+ "cifs/%s@%s",
+ hostname,
+ default_realm);
+
+ if (privates->client_princ == NULL) {
+ DEBUG(0, ("Failed to create ipasam client principal.\n"));
+ status = NT_STATUS_NO_MEMORY;
+ goto done;
+ }
+
+ if (privates->server_princ) {
+ talloc_free(privates->server_princ);
+ privates->server_princ = NULL;
+ }
+
+ privates->server_princ = talloc_asprintf(privates,
+ "ldap/%s@%s",
+ hostname,
+ default_realm);
+
+ if (privates->server_princ == NULL) {
+ DEBUG(0, ("Failed to create ipasam server principal.\n"));
+ status = NT_STATUS_NO_MEMORY;
+ goto done;
+ }
+
+ status = NT_STATUS_OK;
+
+done:
+
+ if (default_realm) {
+ krb5_free_default_realm(context, default_realm);
+ }
+
+ if (context) {
+ krb5_free_context(context);
+ }
+ return status;
+}
+
+
static NTSTATUS pdb_init_ipasam(struct pdb_methods **pdb_method,
const char *location)
{
@@ -3263,7 +3358,6 @@ static NTSTATUS pdb_init_ipasam(struct pdb_methods **pdb_method,
struct dom_sid ldap_domain_sid;
char *bind_dn = NULL;
char *bind_secret = NULL;
- const char *service_principal = NULL;
LDAPMessage *result = NULL;
LDAPMessage *entry = NULL;
@@ -3293,9 +3387,9 @@ static NTSTATUS pdb_init_ipasam(struct pdb_methods **pdb_method,
}
trim_char( uri, '\"', '\"' );
- service_principal = lp_parm_const_string(-1, "ipasam", "principal", NULL);
+ status = ipasam_generate_principals(ldap_state->ipasam_privates);
- if (service_principal == NULL) {
+ if (!NT_STATUS_IS_OK(status)) {
if (!fetch_ldap_pw(&bind_dn, &bind_secret)) {
DEBUG(0, ("pdb_init_ipasam: Failed to retrieve LDAP password from secrets.tdb\n"));
return NT_STATUS_NO_MEMORY;
@@ -3310,7 +3404,7 @@ static NTSTATUS pdb_init_ipasam(struct pdb_methods **pdb_method,
&ldap_state->smbldap_state);
if (NT_STATUS_IS_OK(status)) {
ldap_state->smbldap_state->bind_callback = bind_callback;
- ldap_state->smbldap_state->bind_callback_data = service_principal;
+ ldap_state->smbldap_state->bind_callback_data = ldap_state->ipasam_privates;
}
}
diff --git a/install/share/smb.conf.template b/install/share/smb.conf.template
index 3107350aa6e94514354b73f0152846e1d01e1e68..086b0fcfe5cff2bc3582f2a89962a99c9095b4bb 100644
--- a/install/share/smb.conf.template
+++ b/install/share/smb.conf.template
@@ -18,7 +18,6 @@ ldap suffix = $SUFFIX
ldap user suffix = cn=users,cn=accounts
ldap group suffix = cn=groups,cn=accounts
ldap machine suffix = cn=computers,cn=accounts
-ipasam:principal = cifs/$FQDN@$REALM
rpc_server:epmapper = external
rpc_server:lsarpc = external
rpc_server:lsass = external
--
1.7.10.4
More information about the Freeipa-devel
mailing list