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.
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.
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.
--
/ Alexander Bokovoy
>From de956384778bc7ebae8eec80c8e2127ed25f87df Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy <aboko...@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 principal
into ipasam long term private struct.
Rework ccache handling to avoid re-initing every time callback is called
Please note that you need to re-run ipa-adtrust-install as smb.conf option
and its meaning has been changed (ipasam:principal vs ipasam:principal template)
---
daemons/ipa-sam/ipa_sam.c | 117 +++++++++++++++++++++++++--------------
install/share/smb.conf.template | 2 +-
2 files changed, 76 insertions(+), 43 deletions(-)
diff --git a/daemons/ipa-sam/ipa_sam.c b/daemons/ipa-sam/ipa_sam.c
index
9baac1b2d35a640c36fe7f95a6154ec8582649d8..4666e0f02c19c124f858b0c491af27f42e0a1519
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) {
@@ -3158,21 +3168,27 @@ static void bind_callback_cleanup(struct
ipasam_sasl_interact_priv *data)
}
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 +3198,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,7 +3263,10 @@ 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;
}
@@ -3263,7 +3282,7 @@ 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;
+ const char *princ_template = NULL;
LDAPMessage *result = NULL;
LDAPMessage *entry = NULL;
@@ -3293,9 +3312,9 @@ static NTSTATUS pdb_init_ipasam(struct pdb_methods
**pdb_method,
}
trim_char( uri, '\"', '\"' );
- service_principal = lp_parm_const_string(-1, "ipasam", "principal",
NULL);
+ princ_template = lp_parm_const_string(-1, "ipasam", "principal
template", NULL);
- if (service_principal == NULL) {
+ if (princ_template == NULL) {
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;
@@ -3304,13 +3323,27 @@ static NTSTATUS pdb_init_ipasam(struct pdb_methods
**pdb_method,
uri, false, bind_dn, bind_secret,
&ldap_state->smbldap_state);
} else {
+ ldap_state->ipasam_privates->client_princ =
talloc_asprintf(ldap_state->ipasam_privates,
+ "cifs/%s",
+ princ_template);
+ if (ldap_state->ipasam_privates->client_princ == NULL) {
+ DEBUG(0, ("Failed to create ipasam client principal out
of the template.\n"));
+ return NT_STATUS_NO_MEMORY;
+ }
+ ldap_state->ipasam_privates->server_princ =
talloc_asprintf(ldap_state->ipasam_privates,
+ "ldap/%s",
+ princ_template);
+ if (ldap_state->ipasam_privates->client_princ == NULL) {
+ DEBUG(0, ("Failed to create ipasam server principal out
of the template.\n"));
+ return NT_STATUS_NO_MEMORY;
+ }
/* We authenticate via GSSAPI and thus will use kerberos
principal to bind our access */
status = smbldap_init(*pdb_method, pdb_get_tevent_context(),
uri, false, NULL, NULL,
&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..e0954e1661542eb43ee78adb4675cc2e18250dcc
100644
--- a/install/share/smb.conf.template
+++ b/install/share/smb.conf.template
@@ -18,7 +18,7 @@ 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
+ipasam:principal template = $FQDN@$REALM
rpc_server:epmapper = external
rpc_server:lsarpc = external
rpc_server:lsass = external
--
1.7.10.4
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
