On Thu, Sep 02, 2010 at 09:44:02AM -0400, Stephen Gallagher wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 07/09/2010 05:59 AM, Sumit Bose wrote:
> > On Fri, Mar 19, 2010 at 02:15:06PM +0100, Sumit Bose wrote:
> >> On Fri, Mar 19, 2010 at 08:48:49AM -0400, Simo Sorce wrote:
> >>> On Fri, 19 Mar 2010 12:00:47 +0100
> >>> Sumit Bose <sb...@redhat.com> wrote:
> >>>
> >>>> On Thu, Mar 18, 2010 at 05:51:13PM -0400, Simo Sorce wrote:
> >>>>>
> >>>>> Some time ago I added code to fetch the rootdse on connection, but
> >>>>> didn't publicize it too much.
> >>>>>
> >>>>> Attached find 2 patches.
> >>>>>
> >>>>> 1) Rework the way we store data fetched from the rootdse so the it
> >>>>> is more useful and is actually attached to the ldap handle.
> >>>>
> >>>> I'm in general fine with this one, although I haven't tested it
> >>>> throughly yet.
> >>>>>
> >>>>> 2) Check controls are supported before using them.
> >>>>
> >>>> I have two comments here:
> >>>>
> >>>> - sdap_is_control_supported() should be integrated in
> >>>> sss_ldap_control_create() or a new call should be created which
> >>>> calls sdap_is_control_supported() and then sss_ldap_control_create().
> >>>> - the debug message 'Server does not support the Password Policy
> >>>> control' should have a much lower level than 7. Because if the
> >>>> control is missing the user might not see the expected
> >>>> behaviour/performance.
> >>>
> >>> Sumit,
> >>> do you want to take over the second patch ?
> >>> I don't have a way to thoroughly test it anyway and I am a bit short on
> >>> time.
> >>
> >> sure
> >>
> >
> > Hi,
> >
> > not even 4 months later here are the revised versions of these patches.
> > I had it add two more changes to the first patch to make it build on
> > master because of the change to sdap_cli_connect_recv().
> >
> > In the second patch if have created a new call sdap_control_create()
> > which checks if the control is supported with
> > sdap_is_control_supported() and calls sss_ldap_control_create() on
> > success. I think this approach is more clean, because it leaves
> > sss_ldap_control_create() as a pure replacement function.
> >
>
> Naturally, we only got around to reviewing this two months after this
> revision :(
>
> Unfortunately, the changes made to the creation of LDAP operations in
> SSSD 1.3.0 conflict madly with these changes. Would it be possible for
> you to rebase this patch atop the current master?
>
> If not, I'll add it to my todo list post-1.4.0.
>
rebased versions attached.
bye,
Sumit
> - --
> Stephen Gallagher
> RHCE 804006346421761
>
> Delivering value year after year.
> Red Hat ranks #1 in value among software vendors.
> http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkx/qiIACgkQeiVVYja6o6MFHQCeIhpJQ4Ttk0+76fKo6uAsVHhB
> cy8AnRsnD3Fy736/lAJVEc7BioJGpYI8
> =KNjZ
> -----END PGP SIGNATURE-----
> _______________________________________________
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel
From 7952ef9953eec2d5a3d1b4100eb14ea140abcb99 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sb...@redhat.com>
Date: Thu, 9 Sep 2010 15:15:36 +0200
Subject: [PATCH 1/2] Store rootdse supported features in sdap_handler
---
src/providers/ipa/ipa_auth.c | 4 +-
src/providers/ldap/ldap_common.h | 3 +
src/providers/ldap/sdap.c | 84 ++++++++++++++++++++++------
src/providers/ldap/sdap.h | 23 +++++++-
src/providers/ldap/sdap_async.h | 8 +--
src/providers/ldap/sdap_async_connection.c | 50 ++++++----------
src/providers/ldap/sdap_id_op.c | 4 +-
7 files changed, 116 insertions(+), 60 deletions(-)
diff --git a/src/providers/ipa/ipa_auth.c b/src/providers/ipa/ipa_auth.c
index 2d91457..23095e5 100644
--- a/src/providers/ipa/ipa_auth.c
+++ b/src/providers/ipa/ipa_auth.c
@@ -92,7 +92,7 @@ static struct tevent_req
*get_password_migration_flag_send(TALLOC_CTX *memctx,
subreq = sdap_cli_connect_send(state, ev, sdap_auth_ctx->opts,
sdap_auth_ctx->be, sdap_auth_ctx->service,
- NULL);
+ true);
if (subreq == NULL) {
DEBUG(1, ("sdap_cli_connect_send failed.\n"));
goto fail;
@@ -118,7 +118,7 @@ static void get_password_migration_flag_auth_done(struct
tevent_req *subreq)
char *search_base;
const char **attrs;
- ret = sdap_cli_connect_recv(subreq, state, &state->sh, NULL);
+ ret = sdap_cli_connect_recv(subreq, state, &state->sh);
talloc_zfree(subreq);
if (ret) {
DEBUG(1, ("sdap_auth request failed.\n"));
diff --git a/src/providers/ldap/ldap_common.h b/src/providers/ldap/ldap_common.h
index 5b7298b..2a0e976 100644
--- a/src/providers/ldap/ldap_common.h
+++ b/src/providers/ldap/ldap_common.h
@@ -42,6 +42,9 @@ struct sdap_id_ctx {
struct fo_service *fo_service;
struct sdap_service *service;
+ /* what rootDSE returns */
+ struct sysdb_attrs *rootDSE;
+
/* LDAP connection cache */
struct sdap_id_conn_cache *conn_cache;
diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c
index 39c67cc..e0f5038 100644
--- a/src/providers/ldap/sdap.c
+++ b/src/providers/ldap/sdap.c
@@ -325,37 +325,85 @@ errno_t setup_tls_config(struct dp_option *basic_opts)
}
-bool sdap_rootdse_sasl_mech_is_supported(struct sysdb_attrs *rootdse,
- const char *sasl_mech)
+bool sdap_check_sup_list(struct sup_list *l, const char *val)
{
- struct ldb_message_element *el = NULL;
- struct ldb_val *val;
int i;
- if (!sasl_mech) return false;
+ if (!val) {
+ return false;
+ }
- for (i = 0; i < rootdse->num; i++) {
- if (strcasecmp(rootdse->a[i].name, "supportedSASLMechanisms")) {
+ for (i = 0; i < l->num_vals; i++) {
+ if (strcasecmp(val, (char *)l->vals[i])) {
continue;
}
- el = &rootdse->a[i];
- break;
+ return true;
}
- if (!el) {
- /* no supported SASL Mechanism at all ? */
- return false;
+ return false;
+}
+
+static int sdap_init_sup_list(TALLOC_CTX *memctx,
+ struct sup_list *list,
+ int num, struct ldb_val *vals)
+{
+ int i;
+
+ list->vals = talloc_array(memctx, char *, num);
+ if (!list->vals) {
+ return ENOMEM;
}
- for (i = 0; i < el->num_values; i++) {
- val = &el->values[i];
- if (strncasecmp(sasl_mech, (const char *)val->data, val->length)) {
- continue;
+ for (i = 0; i < num; i++) {
+ list->vals[i] = talloc_strndup(list->vals,
+ (char *)vals[i].data, vals[i].length);
+ if (!list->vals[i]) {
+ return ENOMEM;
}
- return true;
}
- return false;
+ list->num_vals = num;
+
+ return EOK;
+}
+
+int sdap_set_rootdse_supported_lists(struct sysdb_attrs *rootdse,
+ struct sdap_handle *sh)
+{
+ struct ldb_message_element *el = NULL;
+ int ret;
+ int i;
+
+ for (i = 0; i < rootdse->num; i++) {
+ el = &rootdse->a[i];
+ if (strcasecmp(el->name, "supportedControl") == 0) {
+
+ ret = sdap_init_sup_list(sh, &sh->supported_controls,
+ el->num_values, el->values);
+ if (ret) {
+ return ret;
+ }
+ }
+ if (strcasecmp(el->name, "supportedExtension") == 0) {
+
+ ret = sdap_init_sup_list(sh, &sh->supported_extensions,
+ el->num_values, el->values);
+ if (ret) {
+ return ret;
+ }
+ }
+ if (strcasecmp(el->name, "supportedSASLMechanisms") == 0) {
+
+ ret = sdap_init_sup_list(sh, &sh->supported_saslmechs,
+ el->num_values, el->values);
+ if (ret) {
+ return ret;
+ }
+ }
+ }
+
+ return EOK;
+
}
int build_attrs_from_map(TALLOC_CTX *memctx,
diff --git a/src/providers/ldap/sdap.h b/src/providers/ldap/sdap.h
index 61e7fe2..c7921c7 100644
--- a/src/providers/ldap/sdap.h
+++ b/src/providers/ldap/sdap.h
@@ -93,6 +93,11 @@ struct ldap_cb_data {
void *wakeup_cb_data;
};
+struct sup_list {
+ int num_vals;
+ char **vals;
+};
+
struct sdap_handle {
LDAP *ldap;
bool connected;
@@ -101,6 +106,10 @@ struct sdap_handle {
struct sdap_fd_events *sdap_fd_events;
+ struct sup_list supported_saslmechs;
+ struct sup_list supported_controls;
+ struct sup_list supported_extensions;
+
struct sdap_op *ops;
/* during release we need to lock access to the handler
@@ -288,8 +297,18 @@ int sdap_get_msg_dn(TALLOC_CTX *memctx, struct sdap_handle
*sh,
errno_t setup_tls_config(struct dp_option *basic_opts);
-bool sdap_rootdse_sasl_mech_is_supported(struct sysdb_attrs *rootdse,
- const char *sasl_mech);
+int sdap_set_rootdse_supported_lists(struct sysdb_attrs *rootdse,
+ struct sdap_handle *sh);
+bool sdap_check_sup_list(struct sup_list *l, const char *val);
+
+#define sdap_is_sasl_mech_supported(sh, sasl_mech) \
+ sdap_check_sup_list(&((sh)->supported_saslmechs), sasl_mech)
+
+#define sdap_is_control_supported(sh, ctrl_oid) \
+ sdap_check_sup_list(&((sh)->supported_controls), ctrl_oid)
+
+#define sdap_is_extension_supported(sh, ext_oid) \
+ sdap_check_sup_list(&((sh)->supported_extensions), ext_oid)
int build_attrs_from_map(TALLOC_CTX *memctx,
struct sdap_attr_map *map,
diff --git a/src/providers/ldap/sdap_async.h b/src/providers/ldap/sdap_async.h
index 6053506..e5a60da 100644
--- a/src/providers/ldap/sdap_async.h
+++ b/src/providers/ldap/sdap_async.h
@@ -110,16 +110,14 @@ struct tevent_req *sdap_cli_connect_send(TALLOC_CTX
*memctx,
struct sdap_options *opts,
struct be_ctx *be,
struct sdap_service *service,
- struct sysdb_attrs **rootdse);
+ bool skip_rootdse);
int sdap_cli_connect_recv(struct tevent_req *req,
TALLOC_CTX *memctx,
- struct sdap_handle **gsh,
- struct sysdb_attrs **rootdse);
+ struct sdap_handle **gsh);
int sdap_cli_connect_recv_ext(struct tevent_req *req,
TALLOC_CTX *memctx,
bool *can_retry,
- struct sdap_handle **gsh,
- struct sysdb_attrs **rootdse);
+ struct sdap_handle **gsh);
struct tevent_req *sdap_get_generic_send(TALLOC_CTX *memctx,
struct tevent_context *ev,
diff --git a/src/providers/ldap/sdap_async_connection.c
b/src/providers/ldap/sdap_async_connection.c
index 682d74c..35720c0 100644
--- a/src/providers/ldap/sdap_async_connection.c
+++ b/src/providers/ldap/sdap_async_connection.c
@@ -939,7 +939,6 @@ struct sdap_cli_connect_state {
struct be_ctx *be;
bool use_rootdse;
- struct sysdb_attrs *rootdse;
struct sdap_handle *sh;
@@ -961,7 +960,7 @@ struct tevent_req *sdap_cli_connect_send(TALLOC_CTX *memctx,
struct sdap_options *opts,
struct be_ctx *be,
struct sdap_service *service,
- struct sysdb_attrs **rootdse)
+ bool skip_rootdse)
{
struct sdap_cli_connect_state *state;
struct tevent_req *req;
@@ -977,12 +976,10 @@ struct tevent_req *sdap_cli_connect_send(TALLOC_CTX
*memctx,
state->srv = NULL;
state->be = be;
- if (rootdse) {
- state->use_rootdse = true;
- state->rootdse = *rootdse;
- } else {
+ if (skip_rootdse) {
state->use_rootdse = false;
- state->rootdse = NULL;
+ } else {
+ state->use_rootdse = true;
}
ret = sdap_cli_resolve_next(req);
@@ -1068,7 +1065,7 @@ static void sdap_cli_connect_done(struct tevent_req
*subreq)
return;
}
- if (state->use_rootdse && !state->rootdse) {
+ if (state->use_rootdse) {
/* fetch the rootDSE this time */
sdap_cli_rootdse_step(req);
return;
@@ -1078,8 +1075,7 @@ static void sdap_cli_connect_done(struct tevent_req
*subreq)
if (sasl_mech && state->use_rootdse) {
/* check if server claims to support GSSAPI */
- if (!sdap_rootdse_sasl_mech_is_supported(state->rootdse,
- sasl_mech)) {
+ if (!sdap_is_sasl_mech_supported(state->sh, sasl_mech)) {
tevent_req_error(req, ENOTSUP);
return;
}
@@ -1127,10 +1123,11 @@ static void sdap_cli_rootdse_done(struct tevent_req
*subreq)
struct tevent_req);
struct sdap_cli_connect_state *state = tevent_req_data(req,
struct sdap_cli_connect_state);
+ struct sysdb_attrs *rootdse;
const char *sasl_mech;
int ret;
- ret = sdap_get_rootdse_recv(subreq, state, &state->rootdse);
+ ret = sdap_get_rootdse_recv(subreq, state, &rootdse);
talloc_zfree(subreq);
if (ret) {
if (ret == ETIMEDOUT) { /* retry another server */
@@ -1158,12 +1155,18 @@ static void sdap_cli_rootdse_done(struct tevent_req
*subreq)
}
}
+ /* save rootdse data about supported features */
+ ret = sdap_set_rootdse_supported_lists(rootdse, state->sh);
+ if (ret) {
+ tevent_req_error(req, ret);
+ return;
+ }
+
sasl_mech = dp_opt_get_string(state->opts->basic, SDAP_SASL_MECH);
if (sasl_mech && state->use_rootdse) {
/* check if server claims to support GSSAPI */
- if (!sdap_rootdse_sasl_mech_is_supported(state->rootdse,
- sasl_mech)) {
+ if (!sdap_is_sasl_mech_supported(state->sh, sasl_mech)) {
tevent_req_error(req, ENOTSUP);
return;
}
@@ -1287,17 +1290,15 @@ static void sdap_cli_auth_done(struct tevent_req
*subreq)
int sdap_cli_connect_recv(struct tevent_req *req,
TALLOC_CTX *memctx,
- struct sdap_handle **gsh,
- struct sysdb_attrs **rootdse)
+ struct sdap_handle **gsh)
{
- return sdap_cli_connect_recv_ext(req, memctx, NULL, gsh, rootdse);
+ return sdap_cli_connect_recv_ext(req, memctx, NULL, gsh);
}
int sdap_cli_connect_recv_ext(struct tevent_req *req,
TALLOC_CTX *memctx,
bool *can_retry,
- struct sdap_handle **gsh,
- struct sysdb_attrs **rootdse)
+ struct sdap_handle **gsh)
{
struct sdap_cli_connect_state *state = tevent_req_data(req,
struct sdap_cli_connect_state);
@@ -1337,19 +1338,6 @@ int sdap_cli_connect_recv_ext(struct tevent_req *req,
talloc_zfree(state->sh);
}
- if (rootdse) {
- if (state->use_rootdse) {
- *rootdse = talloc_steal(memctx, state->rootdse);
- if (!*rootdse) {
- return ENOMEM;
- }
- } else {
- *rootdse = NULL;
- }
- } else {
- talloc_zfree(rootdse);
- }
-
return EOK;
}
diff --git a/src/providers/ldap/sdap_id_op.c b/src/providers/ldap/sdap_id_op.c
index a005f0f..4fa7733 100644
--- a/src/providers/ldap/sdap_id_op.c
+++ b/src/providers/ldap/sdap_id_op.c
@@ -460,7 +460,7 @@ static int sdap_id_op_connect_step(struct tevent_req *req)
conn_data->conn_cache = conn_cache;
subreq = sdap_cli_connect_send(conn_data, conn_cache->be->ev,
conn_cache->opts, conn_cache->be,
- conn_cache->service, &conn_data->rootDSE);
+ conn_cache->service, false);
if (!subreq) {
ret = ENOMEM;
goto done;
@@ -497,7 +497,7 @@ static void sdap_id_op_connect_done(struct tevent_req
*subreq)
int ret;
ret = sdap_cli_connect_recv_ext(subreq, conn_data, &can_retry,
- &conn_data->sh, &conn_data->rootDSE);
+ &conn_data->sh);
conn_data->connect_req = NULL;
talloc_zfree(subreq);
--
1.7.2.2
From f0d5d1d3817dc4c4eaa127c6c718390f80a23e18 Mon Sep 17 00:00:00 2001
From: Simo Sorce <sso...@redhat.com>
Date: Thu, 18 Mar 2010 17:24:00 -0400
Subject: [PATCH 2/2] Check if control is supported before using it.
---
src/providers/ldap/sdap.c | 18 ++++++++++++++++++
src/providers/ldap/sdap.h | 3 +++
src/providers/ldap/sdap_async.c | 17 ++++++++++-------
src/providers/ldap/sdap_async_connection.c | 16 +++++++++-------
4 files changed, 40 insertions(+), 14 deletions(-)
diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c
index e0f5038..370ffde 100644
--- a/src/providers/ldap/sdap.c
+++ b/src/providers/ldap/sdap.c
@@ -434,3 +434,21 @@ int build_attrs_from_map(TALLOC_CTX *memctx,
return EOK;
}
+int sdap_control_create(struct sdap_handle *sh, const char *oid, int
iscritical,
+ struct berval *value, int dupval, LDAPControl **ctrlp)
+{
+ int ret;
+
+ if (sdap_is_control_supported(sh, oid)) {
+ ret = sss_ldap_control_create(oid, iscritical, value, dupval, ctrlp);
+ if (ret != LDAP_SUCCESS) {
+ DEBUG(1, ("sss_ldap_control_create failed [%d][%s].\n",
+ ret, ldap_err2string(ret)));
+ }
+ } else {
+ DEBUG(3, ("Server does not support the requested control [%s].\n",
oid));
+ ret = LDAP_NOT_SUPPORTED;
+ }
+
+ return ret;
+}
diff --git a/src/providers/ldap/sdap.h b/src/providers/ldap/sdap.h
index c7921c7..91d35e0 100644
--- a/src/providers/ldap/sdap.h
+++ b/src/providers/ldap/sdap.h
@@ -313,4 +313,7 @@ bool sdap_check_sup_list(struct sup_list *l, const char
*val);
int build_attrs_from_map(TALLOC_CTX *memctx,
struct sdap_attr_map *map,
size_t size, const char ***_attrs);
+
+int sdap_control_create(struct sdap_handle *sh, const char *oid, int
iscritical,
+ struct berval *value, int dupval, LDAPControl **ctrlp);
#endif /* _SDAP_H_ */
diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
index 104a72e..5bde3b8 100644
--- a/src/providers/ldap/sdap_async.c
+++ b/src/providers/ldap/sdap_async.c
@@ -566,7 +566,9 @@ struct tevent_req *sdap_exop_modify_passwd_send(TALLOC_CTX
*memctx,
int ret;
BerElement *ber = NULL;
struct berval *bv = NULL;
- LDAPControl *request_controls[2];
+ int msgid;
+ LDAPControl **request_controls = NULL;
+ LDAPControl *ctrls[2] = { NULL, NULL };
req = tevent_req_create(memctx, &state,
struct sdap_exop_modify_passwd_state);
@@ -602,13 +604,14 @@ struct tevent_req
*sdap_exop_modify_passwd_send(TALLOC_CTX *memctx,
return NULL;
}
- ret = sss_ldap_control_create(LDAP_CONTROL_PASSWORDPOLICYREQUEST,
- 0, NULL, 0, &request_controls[0]);
- if (ret != LDAP_SUCCESS) {
- DEBUG(1, ("sss_ldap_control_create failed.\n"));
+ ret = sdap_control_create(state->sh, LDAP_CONTROL_PASSWORDPOLICYREQUEST,
+ 0, NULL, 0, &ctrls[0]);
+ if (ret != LDAP_SUCCESS && ret != LDAP_NOT_SUPPORTED) {
+ DEBUG(1, ("sdap_control_create failed to create "
+ "Password Policy control.\n"));
goto fail;
}
- request_controls[1] = NULL;
+ request_controls = ctrls;
DEBUG(4, ("Executing extended operation\n"));
@@ -616,7 +619,7 @@ struct tevent_req *sdap_exop_modify_passwd_send(TALLOC_CTX
*memctx,
LDAP_EXOP_MODIFY_PASSWD, bv,
request_controls, NULL);
ber_bvfree(bv);
- ldap_control_free(request_controls[0]);
+ if (ctrls[0]) ldap_control_free(ctrls[0]);
if (!subreq) {
DEBUG(1, ("ldap_extended_operation_send failed.\n"));
diff --git a/src/providers/ldap/sdap_async_connection.c
b/src/providers/ldap/sdap_async_connection.c
index 35720c0..f5c6511 100644
--- a/src/providers/ldap/sdap_async_connection.c
+++ b/src/providers/ldap/sdap_async_connection.c
@@ -308,8 +308,9 @@ static struct tevent_req *simple_bind_send(TALLOC_CTX
*memctx,
struct tevent_req *req;
struct simple_bind_state *state;
int ret = EOK;
- LDAPControl *request_controls[2];
struct tevent_req *subreq;
+ LDAPControl **request_controls = NULL;
+ LDAPControl *ctrls[2] = { NULL, NULL };
req = tevent_req_create(memctx, &state, struct simple_bind_state);
if (!req) return NULL;
@@ -325,19 +326,20 @@ static struct tevent_req *simple_bind_send(TALLOC_CTX
*memctx,
state->user_dn = user_dn;
state->pw = pw;
- ret = sss_ldap_control_create(LDAP_CONTROL_PASSWORDPOLICYREQUEST,
- 0, NULL, 0, &request_controls[0]);
- if (ret != LDAP_SUCCESS) {
- DEBUG(1, ("sss_ldap_control_create failed.\n"));
+ ret = sdap_control_create(state->sh, LDAP_CONTROL_PASSWORDPOLICYREQUEST,
+ 0, NULL, 0, &ctrls[0]);
+ if (ret != LDAP_SUCCESS && ret != LDAP_NOT_SUPPORTED) {
+ DEBUG(1, ("sdap_control_create failed to create "
+ "Password Policy control.\n"));
goto fail;
}
- request_controls[1] = NULL;
+ request_controls = ctrls;
DEBUG(4, ("Executing simple bind as: %s\n", state->user_dn));
subreq = ldap_sasl_bind_send(state, ev, sh, user_dn, LDAP_SASL_SIMPLE, pw,
request_controls, NULL);
- ldap_control_free(request_controls[0]);
+ if (ctrls[0]) ldap_control_free(ctrls[0]);
if (!subreq) goto fail;
tevent_req_set_callback(subreq, simple_bind_step, req);
--
1.7.2.2
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
