[PATCH] Check for controls before using them

[Simo Sorce]

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.

2) Check controls are supported before using them.

Simo.

[Sumit Bose]

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.
> 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.
> 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.

bye,
Sumit
> Simo.
>
> -- 
> Simo Sorce * Red Hat, Inc * New York
> > From be64c4245db58200db783623a6eaa791fe4339d6 Mon Sep 17 00:00:00 2001
>
> From: Simo Sorce 
> Date: Thu, 18 Mar 2010 16:58:02 -0400
> Subject: [PATCH 1/2] Store rootdse supported features in sdap_handler
>
> src/providers/ipa/ipa_access.c             |   10 +--
>  src/providers/ldap/ldap_common.h           |    3 -
>  src/providers/ldap/ldap_id.c               |   15 ++---
>  src/providers/ldap/ldap_id_enum.c          |   10 +--
>  src/providers/ldap/sdap.c                  |   84 ++++++++++++++++++++++------
>  src/providers/ldap/sdap.h                  |   23 +++++++-
>  src/providers/ldap/sdap_async.h            |    5 +-
>  src/providers/ldap/sdap_async_connection.c |   45 ++++++---------
>  8 files changed, 120 insertions(+), 75 deletions(-)
>
> diff --git a/src/providers/ipa/ipa_access.c b/src/providers/ipa/ipa_access.c
> index 7dfe1fd..6740f09 100644
> --- a/src/providers/ipa/ipa_access.c
> +++ b/src/providers/ipa/ipa_access.c
> @@ -437,7 +437,7 @@ static struct tevent_req *hbac_get_host_info_send(TALLOC_CTX *memctx,
>          }
>      subreq = sdap_cli_connect_send(state, ev, sdap_ctx->opts,
>                                   sdap_ctx->be, sdap_ctx->service, NULL);
>                                   sdap_ctx->be, sdap_ctx->service, false);
>    if (!subreq) {
>        DEBUG(1, ("sdap_cli_connect_send failed.\n"));
>        ret = ENOMEM;
>
> @@ -482,8 +482,7 @@ static void hbac_get_host_info_connect_done(struct tevent_req *subreq)
>                                                 struct hbac_get_host_info_state);
>      int ret;
> ret = sdap_cli_connect_recv(subreq, state->sdap_ctx, &state->sdap_ctx->gsh,
>                            NULL);
> ret = sdap_cli_connect_recv(subreq, state->sdap_ctx, &state->sdap_ctx->gsh);
> talloc_zfree(subreq);
> if (ret) {
>     tevent_req_error(req, ret);
>
> @@ -946,7 +945,7 @@ static struct tevent_req *hbac_get_rules_send(TALLOC_CTX *memctx,
>          }
>      subreq = sdap_cli_connect_send(state, ev, sdap_ctx->opts,
>                                   sdap_ctx->be, sdap_ctx->service, NULL);
>                                   sdap_ctx->be, sdap_ctx->service, false);
>    if (!subreq) {
>        DEBUG(1, ("sdap_cli_connect_send failed.\n"));
>        ret = ENOMEM;
>
> @@ -991,8 +990,7 @@ static void hbac_get_rules_connect_done(struct tevent_req *subreq)
>                                                       struct hbac_get_rules_state);
>      int ret;
> ret = sdap_cli_connect_recv(subreq, state->sdap_ctx, &state->sdap_ctx->gsh,
>                            NULL);
> ret = sdap_cli_connect_recv(subreq, state->sdap_ctx, &state->sdap_ctx->gsh);
> talloc_zfree(subreq);
> if (ret) {
>     tevent_req_error(req, ret);
>
> diff --git a/src/providers/ldap/ldap_common.h b/src/providers/ldap/ldap_common.h
> index ff1ffb7..cbacf83 100644
> --- a/src/providers/ldap/ldap_common.h
> +++ b/src/providers/ldap/ldap_common.h
> @@ -39,9 +39,6 @@ struct sdap_id_ctx {
>      struct fo_service *fo_service;
>      struct sdap_service *service;
> /* what rootDSE returns */
> struct sysdb_attrs *rootDSE;
> /* global sdap handler */
>    struct sdap_handle *gsh;
>
> diff --git a/src/providers/ldap/ldap_id.c b/src/providers/ldap/ldap_id.c
> index 4bbc07a..7a646a3 100644
> --- a/src/providers/ldap/ldap_id.c
> +++ b/src/providers/ldap/ldap_id.c
> @@ -106,7 +106,7 @@ struct tevent_req *users_get_send(TALLOC_CTX *memctx,
>           * or SASL/GSSAPI, etc ... */
>          subreq = sdap_cli_connect_send(state, ev, ctx->opts,
>                                         ctx->be, ctx->service,
>                                   &ctx->rootDSE);
>                                   false);
>    if (!subreq) {
>        ret = ENOMEM;
>        goto fail;
>
> @@ -143,8 +143,7 @@ static void users_get_connect_done(struct tevent_req *subreq)
>                                                       struct users_get_state);
>      int ret;
> ret = sdap_cli_connect_recv(subreq, state->ctx,
>                            &state->ctx->gsh, &state->ctx->rootDSE);
> ret = sdap_cli_connect_recv(subreq, state->ctx, &state->ctx->gsh);
> talloc_zfree(subreq);
> if (ret) {
>     if (ret == ENOTSUP) {
>
> @@ -329,7 +328,7 @@ struct tevent_req *groups_get_send(TALLOC_CTX *memctx,
>           * or SASL/GSSAPI, etc ... */
>          subreq = sdap_cli_connect_send(state, ev, ctx->opts,
>                                         ctx->be, ctx->service,
>                                   &ctx->rootDSE);
>                                   false);
>    if (!subreq) {
>        ret = ENOMEM;
>        goto fail;
>
> @@ -366,8 +365,7 @@ static void groups_get_connect_done(struct tevent_req *subreq)
>                                                       struct groups_get_state);
>      int ret;
> ret = sdap_cli_connect_recv(subreq, state->ctx,
>                            &state->ctx->gsh, &state->ctx->rootDSE);
> ret = sdap_cli_connect_recv(subreq, state->ctx, &state->ctx->gsh);
> talloc_zfree(subreq);
> if (ret) {
>     if (ret == ENOTSUP) {
>
> @@ -517,7 +515,7 @@ static struct tevent_req *groups_by_user_send(TALLOC_CTX *memctx,
>           * or SASL/GSSAPI, etc ... */
>          subreq = sdap_cli_connect_send(state, ev, ctx->opts,
>                                         ctx->be, ctx->service,
>                                   &ctx->rootDSE);
>                                   false);
>    if (!subreq) {
>        ret = ENOMEM;
>        goto fail;
>
> @@ -555,8 +553,7 @@ static void groups_by_user_connect_done(struct tevent_req *subreq)
>                                                       struct groups_by_user_state);
>      int ret;
> ret = sdap_cli_connect_recv(subreq, state->ctx,
>                            &state->ctx->gsh, &state->ctx->rootDSE);
> ret = sdap_cli_connect_recv(subreq, state->ctx, &state->ctx->gsh);
> talloc_zfree(subreq);
> if (ret) {
>     if (ret == ENOTSUP) {
>
> diff --git a/src/providers/ldap/ldap_id_enum.c b/src/providers/ldap/ldap_id_enum.c
> index bc06e8b..cf01a5b 100644
> --- a/src/providers/ldap/ldap_id_enum.c
> +++ b/src/providers/ldap/ldap_id_enum.c
> @@ -363,7 +363,7 @@ static struct tevent_req *enum_users_send(TALLOC_CTX *memctx,
>           * or SASL/GSSAPI, etc ... */
>          subreq = sdap_cli_connect_send(state, ev, ctx->opts,
>                                         ctx->be, ctx->service,
>                                   &ctx->rootDSE);
>                                   false);
>    if (!subreq) {
>        ret = ENOMEM;
>        goto fail;
>
> @@ -402,8 +402,7 @@ static void enum_users_connect_done(struct tevent_req *subreq)
>                                                       struct enum_users_state);
>      int ret;
> ret = sdap_cli_connect_recv(subreq, state->ctx,
>                            &state->ctx->gsh, &state->ctx->rootDSE);
> ret = sdap_cli_connect_recv(subreq, state->ctx, &state->ctx->gsh);
> talloc_zfree(subreq);
> if (ret) {
>     if (ret == ENOTSUP) {
>
> @@ -518,7 +517,7 @@ static struct tevent_req *enum_groups_send(TALLOC_CTX *memctx,
>           * or SASL/GSSAPI, etc ... */
>          subreq = sdap_cli_connect_send(state, ev, ctx->opts,
>                                         ctx->be, ctx->service,
>                                   &ctx->rootDSE);
>                                   false);
>    if (!subreq) {
>        ret = ENOMEM;
>        goto fail;
>
> @@ -556,8 +555,7 @@ static void enum_groups_connect_done(struct tevent_req *subreq)
>                                                   struct enum_groups_state);
>      int ret;
> ret = sdap_cli_connect_recv(subreq, state->ctx,
>                            &state->ctx->gsh, &state->ctx->rootDSE);
> ret = sdap_cli_connect_recv(subreq, state->ctx, &state->ctx->gsh);
> talloc_zfree(subreq);
> if (ret) {
>     if (ret == ENOTSUP) {
>
> 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 007185f..a5574fc 100644
> --- a/src/providers/ldap/sdap.h
> +++ b/src/providers/ldap/sdap.h
> @@ -67,6 +67,11 @@ struct ldap_cb_data {
>      struct fd_event_item *fd_list;
>  };
>
> +struct sup_list {
> int num_vals;
> char **vals;
>
> +};
>
> struct sdap_handle {
>      LDAP *ldap;
>      bool connected;
> @@ -77,6 +82,10 @@ struct sdap_handle {
>      struct tevent_fd *fde;
>  #endif
> struct sup_list supported_saslmechs;
> struct sup_list supported_controls;
> struct sup_list supported_extensions;
> struct sdap_op *ops;
>
> };
>
> @@ -250,8 +259,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 3c52d23..1139d46 100644
> --- a/src/providers/ldap/sdap_async.h
> +++ b/src/providers/ldap/sdap_async.h
> @@ -103,11 +103,10 @@ 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);
>
> 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 586733f..f0b8c2e 100644
> --- a/src/providers/ldap/sdap_async_connection.c
> +++ b/src/providers/ldap/sdap_async_connection.c
> @@ -810,7 +810,6 @@ struct sdap_cli_connect_state {
>      struct sdap_service *service;
>  bool use_rootdse;
>
> struct sysdb_attrs *rootdse;
>
> struct sdap_handle *sh;
>
> @@ -831,7 +830,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 tevent_req *req, *subreq;
>      struct sdap_cli_connect_state *state;
> @@ -844,12 +843,10 @@ struct tevent_req *sdap_cli_connect_send(TALLOC_CTX *memctx,
>      state->service = service;
>      state->srv = NULL;
> 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;
>
> }
>
> /* NOTE: this call may cause service->uri to be refreshed
>
> @@ -908,7 +905,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;
>
> @@ -918,8 +915,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;
>    }
>
> @@ -970,22 +966,29 @@ 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) {
>     tevent_req_error(req, ret);
>     return;
> }
>
> /* 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;
>    }
>
> @@ -1094,8 +1097,7 @@ 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)
>
> {
>      struct sdap_cli_connect_state *state = tevent_req_data(req,
>                                               struct sdap_cli_connect_state);
> @@ -1125,19 +1127,6 @@ int sdap_cli_connect_recv(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;
>
> }
>
> -- 
> 1.6.6.1
> > From d8981c63d348ee53c017c82c476ee35bf82e38d3 Mon Sep 17 00:00:00 2001
>
> From: Simo Sorce 
> 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_async.c            |   22 ++++++++++++++--------
>  src/providers/ldap/sdap_async_connection.c |   22 ++++++++++++++--------
>  2 files changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
> index 959c08a..030bf8d 100644
> --- a/src/providers/ldap/sdap_async.c
> +++ b/src/providers/ldap/sdap_async.c
> @@ -550,7 +550,8 @@ struct tevent_req *sdap_exop_modify_passwd_send(TALLOC_CTX *memctx,
>      BerElement *ber = NULL;
>      struct berval *bv = NULL;
>      int msgid;
> LDAPControl *request_controls[2];
>
> LDAPControl **request_controls = NULL;
>
> LDAPControl *ctrls[2] = { NULL, NULL };
>
> req = tevent_req_create(memctx, &state,
>                         struct sdap_exop_modify_passwd_state);
>
> @@ -585,20 +586,25 @@ 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"));
>    goto fail;
> if (sdap_is_control_supported(state->sh,
>                              LDAP_CONTROL_PASSWORDPOLICYREQUEST)) {
>    ret = sss_ldap_control_create(LDAP_CONTROL_PASSWORDPOLICYREQUEST,
>                                  0, NULL, 0, &ctrls[0]);
>    if (ret != LDAP_SUCCESS) {
>        DEBUG(1, ("sss_ldap_control_create failed.\n"));
>        goto fail;
>    }
>    request_controls = ctrls;
> } else {
>    DEBUG(7, ("Server does not support the Password Policy control.\n"));
> }
>
> request_controls[1] = NULL;
>
> DEBUG(4, ("Executing extended operation\n"));
>
> ret = ldap_extended_operation(state->sh->ldap, LDAP_EXOP_MODIFY_PASSWD,
>                               bv, request_controls, NULL, &msgid);
> ber_bvfree(bv);
>
> ldap_control_free(request_controls[0]);
> if (ctrls[0]) ldap_control_free(ctrls[0]);
> if (ret == -1 || msgid == -1) {
>     DEBUG(1, ("ldap_extended_operation failed\n"));
>     goto fail;
>
> diff --git a/src/providers/ldap/sdap_async_connection.c b/src/providers/ldap/sdap_async_connection.c
> index f0b8c2e..d86e4cc 100644
> --- a/src/providers/ldap/sdap_async_connection.c
> +++ b/src/providers/ldap/sdap_async_connection.c
> @@ -296,7 +296,8 @@ static struct tevent_req *simple_bind_send(TALLOC_CTX *memctx,
>      int ret = EOK;
>      int msgid;
>      int ldap_err;
> LDAPControl *request_controls[2];
>
> LDAPControl **request_controls = NULL;
>
> LDAPControl *ctrls[2] = { NULL, NULL };
>
> req = tevent_req_create(memctx, &state, struct simple_bind_state);
> if (!req) return NULL;
>
> @@ -312,19 +313,24 @@ 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"));
>    goto fail;
> if (sdap_is_control_supported(state->sh,
>                              LDAP_CONTROL_PASSWORDPOLICYREQUEST)) {
>    ret = sss_ldap_control_create(LDAP_CONTROL_PASSWORDPOLICYREQUEST,
>                                  0, NULL, 0, &ctrls[0]);
>    if (ret != LDAP_SUCCESS) {
>        DEBUG(1, ("sss_ldap_control_create failed.\n"));
>        goto fail;
>    }
>    request_controls = ctrls;
> } else {
>    DEBUG(7, ("Server does not support the Password Policy control.\n"));
> }
>
> request_controls[1] = NULL;
>
> DEBUG(4, ("Executing simple bind as: %s\n", state->user_dn));
>
> ret = ldap_sasl_bind(state->sh->ldap, state->user_dn, LDAP_SASL_SIMPLE,
>                      state->pw, request_controls, NULL, &msgid);
>
> ldap_control_free(request_controls[0]);
> if (ctrls[0]) ldap_control_free(ctrls[0]);
> if (ret == -1 || msgid == -1) {
>     ret = ldap_get_option(state->sh->ldap,
>                           LDAP_OPT_RESULT_CODE, &ldap_err);
>
> -- 
> 1.6.6.1
> sssd-devel mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/sssd-devel

[Simo Sorce]

On Fri, 19 Mar 2010 12:00:47 +0100
Sumit Bose  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.
> > 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.
> > 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.

Simo.

[Sumit Bose]

On Fri, Mar 19, 2010 at 08:48:49AM -0400, Simo Sorce wrote:
> On Fri, 19 Mar 2010 12:00:47 +0100
> Sumit Bose  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.
> > > 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.
> > > 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

bye,
Sumit
> Simo.
>
> -- 
> Simo Sorce * Red Hat, Inc * New York
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/sssd-devel

[Sumit Bose]

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  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.
> > > > 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.
> > > > 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.

bye,
Sumit

[Stephen Gallagher]

-----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  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.
> > > > > 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.
> > > > > 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.

- -- 
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-----

[Sumit Bose]

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  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.
> > > > > > 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.
> > > > > > 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
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On 09/10/2010 11:16 AM, Sumit Bose wrote:
> rebased versions attached.
>
> bye,
> Sumit

Ack to both patches, but may I suggest that the attached patch be 
squashed in? Perhaps it would make for a little more readable code.

[Sumit Bose]

On Fri, Sep 10, 2010 at 05:05:21PM +0200, Jakub Hrozek wrote:
> On 09/10/2010 11:16 AM, Sumit Bose wrote:
> > rebased versions attached.
> >
> > bye,
> > Sumit
>
> Ack to both patches, but may I suggest that the attached patch be
> squashed in? Perhaps it would make for a little more readable code.

yes, please do, ACK to both changes.

bye,
Sumit

[Jakub Hrozek]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/10/2010 05:40 PM, Sumit Bose wrote:
> On Fri, Sep 10, 2010 at 05:05:21PM +0200, Jakub Hrozek wrote:
> > > On 09/10/2010 11:16 AM, Sumit Bose wrote:
> > > > > rebased versions attached.
> > > > >
> > > > > bye,
> > > > > Sumit
> > >
> > > Ack to both patches, but may I suggest that the attached patch be
> > > squashed in? Perhaps it would make for a little more readable code.
>
> yes, please do, ACK to both changes.
>
> bye,
> Sumit

Attaching both patches, the first one contains my squashed mini-patch
acked by Sumit.
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkyNAQAACgkQHsardTLnvCWG3gCggRZx6Qd0T3GXuW65OTDwUwFs
> +9wAn0tVfNTeW6aM+AWW5mZsT8+TCiRj
> =pjGN
> -----END PGP SIGNATURE-----

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/12/2010 12:34 PM, Jakub Hrozek wrote:
> On 09/10/2010 05:40 PM, Sumit Bose wrote:
> > On Fri, Sep 10, 2010 at 05:05:21PM +0200, Jakub Hrozek wrote:
> > > > On 09/10/2010 11:16 AM, Sumit Bose wrote:
> > > > > > rebased versions attached.
> > > > > >
> > > > > > bye,
> > > > > > Sumit
> > > >
> > > > Ack to both patches, but may I suggest that the attached patch be
> > > > squashed in? Perhaps it would make for a little more readable code.
> >
> > yes, please do, ACK to both changes.
> > bye,
> > Sumit
>
> Attaching both patches, the first one contains my squashed mini-patch
> acked by Sumit.

Pushed both patches to master.

Patch 0002 needed to be rebased (again) because of the async bind patch
being reverted. However, the patch from earlier in this thread applied
cleanly now :)

- -- 
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/
>
> iEYEARECAAYFAkyQ7zoACgkQeiVVYja6o6NCDACdG3YNKK4zgV7+Yvou8vY43tYq
> wgcAn14XIuGhYe+cWYsiBJV5V6Yzq0h0
> =CnhD
> -----END PGP SIGNATURE-----