[PATCH] LDAP: Detect the presence of POSIX attributes

[Jakub Hrozek]

Hi,

When the schema is set to AD and ID mapping is used, there is a one-time
check ran when searching for users to detect the presence of POSIX attributes
in LDAP. If this check fails, the search fails as if no entry was found
and returns a special error code.

If the AD identity lookup finds this error code, the GC is disabled for
the next search.

The sdap_server_opts structure is filled every time a client connects to
a server so the posix check boolean is reset to false again on connecting
to the server.

It might be better to move the check to where the rootDSE is retrieved,
but the check depends on several features that are not known to the code
that retrieves the rootDSE (or the connection code for example) such as what
the attribute mappings are or the authentication method that should be used.

Currently this patch only runs the check when users are requested. It
would be trivial to add the same code (about 70 lines) to the group
request as well.

Additionally, I wonder if the absence of POSIX attributes in GC should
be reported louder. Currently there is just MINOR_FAILURE.

We could go as far as report to syslog when a user or a group from
subdomains is requested and the GC was already disabled, but I wanted to
check with the other developers before implementing this.

[Jakub Hrozek]

On Mon, Jan 27, 2014 at 11:33:36PM +0100, Jakub Hrozek wrote:
>  Hi,
>  
>  When the schema is set to AD and ID mapping is used, there is a one-time
>  check ran when searching for users to detect the presence of POSIX attributes
>  in LDAP. If this check fails, the search fails as if no entry was found
>  and returns a special error code.
>  
>  If the AD identity lookup finds this error code, the GC is disabled for
>  the next search.
>  
>  The sdap_server_opts structure is filled every time a client connects to
>  a server so the posix check boolean is reset to false again on connecting
>  to the server.
>  
>  It might be better to move the check to where the rootDSE is retrieved,
>  but the check depends on several features that are not known to the code
>  that retrieves the rootDSE (or the connection code for example) such as what
>  the attribute mappings are or the authentication method that should be used.
>  
>  Currently this patch only runs the check when users are requested. It
>  would be trivial to add the same code (about 70 lines) to the group
>  request as well.
>  
>  Additionally, I wonder if the absence of POSIX attributes in GC should
>  be reported louder. Currently there is just MINOR_FAILURE.
>  
>  We could go as far as report to syslog when a user or a group from
>  subdomains is requested and the GC was already disabled, but I wanted to
>  check with the other developers before implementing this. 
One additional question -- currently the AD ID provider always retries
in LDAP if search in GC didn't find the result. With the possibility of
detecting the POSIX attributes, do we want to remove this fallback if we
were able to run the detection and know that GC is available but doesn't
contain the POSIX attributes?

I would say the fallback should stay, because there is still a chance
other required attribute will be missing, after all the user can
configure additional filter and this fallback only affects negative
searches.

[Pavel Březina]

On 01/27/2014 11:33 PM, Jakub Hrozek wrote:
>  Hi,
>
>  When the schema is set to AD and ID mapping is used, there is a one-time 
you wanted to say "when ID mapping is *not* used"

>  check ran when searching for users to detect the presence of POSIX
> attributes
>  in LDAP. If this check fails, the search fails as if no entry was found
>  and returns a special error code.
>
>  If the AD identity lookup finds this error code, the GC is disabled for
>  the next search.
>
>  The sdap_server_opts structure is filled every time a client connects to
>  a server so the posix check boolean is reset to false again on connecting
>  to the server. 
GC lookups should not be disabled permanently. Administrator can setup 
POSIX attribute propagation to GC any time and SSSD should notice it
without restarting.

So we should rather perform the check periodically and not only disable 
GC lookups, but also re-enable it if it succeeds.

>  It might be better to move the check to where the rootDSE is
> retrieved,
>  but the check depends on several features that are not known to the code
>  that retrieves the rootDSE (or the connection code for example) such as what
>  the attribute mappings are or the authentication method that should be used.
>
>  Currently this patch only runs the check when users are requested. It
>  would be trivial to add the same code (about 70 lines) to the group
>  request as well.
>
>  Additionally, I wonder if the absence of POSIX attributes in GC should
>  be reported louder. Currently there is just MINOR_FAILURE. 
We have SSSDBG_IMPORTANT_INFO for such things.

>  We could go as far as report to syslog when a user or a group from
>  subdomains is requested and the GC was already disabled, but I wanted to
>  check with the other developers before implementing this. 
I haven't made up my mind on this question yet, but I'm more inclined to 
avoid using syslog for this situation. :-)

>  One additional question -- currently the AD ID provider always
> retries
>  in LDAP if search in GC didn't find the result. With the possibility of
>  detecting the POSIX attributes, do we want to remove this fallback if we
>  were able to run the detection and know that GC is available but doesn't
>  contain the POSIX attributes?
>
>  I would say the fallback should stay, because there is still a chance
>  other required attribute will be missing, after all the user can
>  configure additional filter and this fallback only affects negative
>  searches. 
I'm not sure if I understand you. Isn't the whole point of this patch to 
actually avoid the fallback? I mean if we detect that GC doesn't have 
POSIX attributes we want to go directly to LDAP on next queries, don't we?

[Sumit Bose]

On Wed, Jan 29, 2014 at 03:39:41PM +0100, Pavel Březina wrote:
>  On 01/27/2014 11:33 PM, Jakub Hrozek wrote:
>  >Hi,
>  >
>  >When the schema is set to AD and ID mapping is used, there is a one-time
>  
>  you wanted to say "when ID mapping is *not* used"
>  
>  >check ran when searching for users to detect the presence of POSIX attributes
>  >in LDAP. If this check fails, the search fails as if no entry was found
>  >and returns a special error code.
>  >
>  >If the AD identity lookup finds this error code, the GC is disabled for
>  >the next search.
>  >
>  >The sdap_server_opts structure is filled every time a client connects to
>  >a server so the posix check boolean is reset to false again on connecting
>  >to the server.
>  
>  GC lookups should not be disabled permanently. Administrator can
>  setup POSIX attribute propagation to GC any time and SSSD should
>  notice it
>  without restarting.
>  
>  So we should rather perform the check periodically and not only
>  disable GC lookups, but also re-enable it if it succeeds. 
It is not disabled permanently but only for the given connection. On
every reconnect it should be checked again. I think a periodic task is
not needed here.

>  
>  >It might be better to move the check to where the rootDSE is retrieved,
>  >but the check depends on several features that are not known to the code
>  >that retrieves the rootDSE (or the connection code for example) such as what
>  >the attribute mappings are or the authentication method that should be used.
>  >
>  >Currently this patch only runs the check when users are requested. It
>  >would be trivial to add the same code (about 70 lines) to the group
>  >request as well.
>  >
>  >Additionally, I wonder if the absence of POSIX attributes in GC should
>  >be reported louder. Currently there is just MINOR_FAILURE.
>  
>  We have SSSDBG_IMPORTANT_INFO for such things.
>  
>  >We could go as far as report to syslog when a user or a group from
>  >subdomains is requested and the GC was already disabled, but I wanted to
>  >check with the other developers before implementing this.
>  
>  I haven't made up my mind on this question yet, but I'm more
>  inclined to avoid using syslog for this situation. :-)
>  
>  >One additional question -- currently the AD ID provider always retries
>  >in LDAP if search in GC didn't find the result. With the possibility of
>  >detecting the POSIX attributes, do we want to remove this fallback if we
>  >were able to run the detection and know that GC is available but doesn't
>  >contain the POSIX attributes?
>  >
>  >I would say the fallback should stay, because there is still a chance
>  >other required attribute will be missing, after all the user can
>  >configure additional filter and this fallback only affects negative
>  >searches.
>  
>  I'm not sure if I understand you. Isn't the whole point of this
>  patch to actually avoid the fallback? I mean if we detect that GC 
yes, the fallback should be avoided but I agree with Jakub that we
should still allow it.

bye,
Sumit

>  doesn't have POSIX attributes we want to go directly to LDAP on
> next
>  queries, don't we?
>  
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Březina]

On 01/29/2014 04:09 PM, Sumit Bose wrote:
>  On Wed, Jan 29, 2014 at 03:39:41PM +0100, Pavel Březina wrote:
> > On 01/27/2014 11:33 PM, Jakub Hrozek wrote:
> >> Hi,
> >>
> >> When the schema is set to AD and ID mapping is used, there is a one-time
> >
> > you wanted to say "when ID mapping is *not* used"
> >
> >> check ran when searching for users to detect the presence of POSIX attributes
> >> in LDAP. If this check fails, the search fails as if no entry was found
> >> and returns a special error code.
> >>
> >> If the AD identity lookup finds this error code, the GC is disabled for
> >> the next search.
> >>
> >> The sdap_server_opts structure is filled every time a client connects to
> >> a server so the posix check boolean is reset to false again on connecting
> >> to the server.
> >
> > GC lookups should not be disabled permanently. Administrator can
> > setup POSIX attribute propagation to GC any time and SSSD should
> > notice it
> > without restarting.
> >
> > So we should rather perform the check periodically and not only
> > disable GC lookups, but also re-enable it if it succeeds.
>
>  It is not disabled permanently but only for the given connection. On
>  every reconnect it should be checked again. I think a periodic task is
>  not needed here. 
Yes, the check is performed on each new connection, however if it 
detects no POSIX attributes in GC, AD_ENABLE_GC is set to false but 
never to true again. Therefore the next tests are done only against LDAP 
-- if I'm not missing anything.

> >
> >> It might be better to move the check to where the rootDSE is retrieved,
> >> but the check depends on several features that are not known to the code
> >> that retrieves the rootDSE (or the connection code for example) such as what
> >> the attribute mappings are or the authentication method that should be used.
> >>
> >> Currently this patch only runs the check when users are requested. It
> >> would be trivial to add the same code (about 70 lines) to the group
> >> request as well.
> >>
> >> Additionally, I wonder if the absence of POSIX attributes in GC should
> >> be reported louder. Currently there is just MINOR_FAILURE.
> >
> > We have SSSDBG_IMPORTANT_INFO for such things.
> >
> >> We could go as far as report to syslog when a user or a group from
> >> subdomains is requested and the GC was already disabled, but I wanted to
> >> check with the other developers before implementing this.
> >
> > I haven't made up my mind on this question yet, but I'm more
> > inclined to avoid using syslog for this situation. :-)
> >
> >> One additional question -- currently the AD ID provider always retries
> >> in LDAP if search in GC didn't find the result. With the possibility of
> >> detecting the POSIX attributes, do we want to remove this fallback if we
> >> were able to run the detection and know that GC is available but doesn't
> >> contain the POSIX attributes?
> >>
> >> I would say the fallback should stay, because there is still a chance
> >> other required attribute will be missing, after all the user can
> >> configure additional filter and this fallback only affects negative
> >> searches.
> >
> > I'm not sure if I understand you. Isn't the whole point of this
> > patch to actually avoid the fallback? I mean if we detect that GC
>
>  yes, the fallback should be avoided but I agree with Jakub that we
>  should still allow it.
>
>  bye,
>  Sumit
>
> > doesn't have POSIX attributes we want to go directly to LDAP on next
> > queries, don't we?
> >
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel(a)lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>

[Jakub Hrozek]

On Wed, Jan 29, 2014 at 03:39:41PM +0100, Pavel Březina wrote:
>  On 01/27/2014 11:33 PM, Jakub Hrozek wrote:
>  >Hi,
>  >
>  >When the schema is set to AD and ID mapping is used, there is a one-time
>  
>  you wanted to say "when ID mapping is *not* used"
>  
>  >check ran when searching for users to detect the presence of POSIX attributes
>  >in LDAP. If this check fails, the search fails as if no entry was found
>  >and returns a special error code.
>  >
>  >If the AD identity lookup finds this error code, the GC is disabled for
>  >the next search.
>  >
>  >The sdap_server_opts structure is filled every time a client connects to
>  >a server so the posix check boolean is reset to false again on connecting
>  >to the server.
>  
>  GC lookups should not be disabled permanently. Administrator can
>  setup POSIX attribute propagation to GC any time and SSSD should
>  notice it
>  without restarting.
>  
>  So we should rather perform the check periodically and not only
>  disable GC lookups, but also re-enable it if it succeeds. 
See the attached patch, I added a timeout of 3600 seconds for the next
detection. I don't think we want to have the timeout configurable but
maybe we want the timeout to be reset on changing the online status as
well?

>  
>  >It might be better to move the check to where the rootDSE is retrieved,
>  >but the check depends on several features that are not known to the code
>  >that retrieves the rootDSE (or the connection code for example) such as what
>  >the attribute mappings are or the authentication method that should be used.
>  >
>  >Currently this patch only runs the check when users are requested. It
>  >would be trivial to add the same code (about 70 lines) to the group
>  >request as well.
>  >
>  >Additionally, I wonder if the absence of POSIX attributes in GC should
>  >be reported louder. Currently there is just MINOR_FAILURE.
>  
>  We have SSSDBG_IMPORTANT_INFO for such things. 
Done.

>  
>  >We could go as far as report to syslog when a user or a group from
>  >subdomains is requested and the GC was already disabled, but I wanted to
>  >check with the other developers before implementing this.
>  
>  I haven't made up my mind on this question yet, but I'm more
>  inclined to avoid using syslog for this situation. :-) 
Yes, me too.

>  
>  >One additional question -- currently the AD ID provider always retries
>  >in LDAP if search in GC didn't find the result. With the possibility of
>  >detecting the POSIX attributes, do we want to remove this fallback if we
>  >were able to run the detection and know that GC is available but doesn't
>  >contain the POSIX attributes?
>  >
>  >I would say the fallback should stay, because there is still a chance
>  >other required attribute will be missing, after all the user can
>  >configure additional filter and this fallback only affects negative
>  >searches.
>  
>  I'm not sure if I understand you. Isn't the whole point of this
>  patch to actually avoid the fallback? I mean if we detect that GC
>  doesn't have POSIX attributes we want to go directly to LDAP on next
>  queries, don't we? 
I meant the other way. Currently even if we know that GC *has* POSIX
attributes and we don't find the user in GC, we still retry LDAP. I was
wondering whether we can remove this fallback if we know that POSIX
attributes are present. But I think we shouldn't remove the fallback for
the general case because admins can still configure a custom filter in
the search base and this custom filter can contain any attributes.

[Sumit Bose]

On Wed, Jan 29, 2014 at 05:11:59PM +0100, Jakub Hrozek wrote:
>  On Wed, Jan 29, 2014 at 03:39:41PM +0100, Pavel Březina wrote:
>  > On 01/27/2014 11:33 PM, Jakub Hrozek wrote:
>  > >Hi,
>  > >
>  > >When the schema is set to AD and ID mapping is used, there is a one-time
>  > 
>  > you wanted to say "when ID mapping is *not* used"
>  > 
>  > >check ran when searching for users to detect the presence of POSIX attributes
>  > >in LDAP. If this check fails, the search fails as if no entry was found
>  > >and returns a special error code.
>  > >
>  > >If the AD identity lookup finds this error code, the GC is disabled for
>  > >the next search.
>  > >
>  > >The sdap_server_opts structure is filled every time a client connects to
>  > >a server so the posix check boolean is reset to false again on connecting
>  > >to the server.
>  > 
>  > GC lookups should not be disabled permanently. Administrator can
>  > setup POSIX attribute propagation to GC any time and SSSD should
>  > notice it
>  > without restarting.
>  > 
>  > So we should rather perform the check periodically and not only
>  > disable GC lookups, but also re-enable it if it succeeds.
>  
>  See the attached patch, I added a timeout of 3600 seconds for the next
>  detection. I don't think we want to have the timeout configurable but
>  maybe we want the timeout to be reset on changing the online status as
>  well? 
I think we do not have to reset it when changing the online status since
having the POSIX IDs in the GC or not is a global feature of the forest
and changes should be quite rare.

>  
>  > 
>  > >It might be better to move the check to where the rootDSE is retrieved,
>  > >but the check depends on several features that are not known to the code
>  > >that retrieves the rootDSE (or the connection code for example) such as what
>  > >the attribute mappings are or the authentication method that should be used.
>  > >
>  > >Currently this patch only runs the check when users are requested. It
>  > >would be trivial to add the same code (about 70 lines) to the group
>  > >request as well. 
I think it is not called when enumeration is the first task running. Can
you add it here as well? And while you are on it it might be worth to
add it to group lookups as well, just for completeness.

Please find more comments inline.

bye,
Sumit

>  > >
>  > >Additionally, I wonder if the absence of POSIX attributes in GC should
>  > >be reported louder. Currently there is just MINOR_FAILURE.
>  > 
>  > We have SSSDBG_IMPORTANT_INFO for such things.
>  
>  Done.
>  
>  > 
>  > >We could go as far as report to syslog when a user or a group from
>  > >subdomains is requested and the GC was already disabled, but I wanted to
>  > >check with the other developers before implementing this.
>  > 
>  > I haven't made up my mind on this question yet, but I'm more
>  > inclined to avoid using syslog for this situation. :-)
>  
>  Yes, me too.
>  
>  > 
>  > >One additional question -- currently the AD ID provider always retries
>  > >in LDAP if search in GC didn't find the result. With the possibility of
>  > >detecting the POSIX attributes, do we want to remove this fallback if we
>  > >were able to run the detection and know that GC is available but doesn't
>  > >contain the POSIX attributes?
>  > >
>  > >I would say the fallback should stay, because there is still a chance
>  > >other required attribute will be missing, after all the user can
>  > >configure additional filter and this fallback only affects negative
>  > >searches.
>  > 
>  > I'm not sure if I understand you. Isn't the whole point of this
>  > patch to actually avoid the fallback? I mean if we detect that GC
>  > doesn't have POSIX attributes we want to go directly to LDAP on next
>  > queries, don't we?
>  
>  I meant the other way. Currently even if we know that GC *has* POSIX
>  attributes and we don't find the user in GC, we still retry LDAP. I was
>  wondering whether we can remove this fallback if we know that POSIX
>  attributes are present. But I think we shouldn't remove the fallback for
>  the general case because admins can still configure a custom filter in
>  the search base and this custom filter can contain any attributes. 
>  From c76a1058f1aea30b83f3dd133230a093b1de5b44 Mon Sep 17 00:00:00
> 2001
>  From: Jakub Hrozek <jhrozek(a)redhat.com&gt;
>  Date: Mon, 16 Dec 2013 18:36:12 +0100
>  Subject: [PATCH] LDAP: Detect the presence of POSIX attributes
>  
>  When the schema is set to AD and ID mapping is used, there is a one-time 
                                                ^ 'not' missing ?

>  check ran when searching for users to detect the presence of POSIX
>  attributes in LDAP. If this check fails, the search fails as if no entry
>  was found and returns a special error code.
>  
>  The sdap_server_opts structure is filled every time a client connects to
>  a server so the posix check boolean is reset to false again on connecting
>  to the server.
>  
>  It might be better to move the check to where the rootDSE is retrieved,
>  but the check depends on several features that are not known to the code
>  that retrieves the rootDSE (or the connection code for example) such as what
>  the attribute mappings are or the authentication method that should be used.
>  ---
>   src/providers/ad/ad_common.h          |   3 +
>   src/providers/ad/ad_id.c              |  45 ++++++++-
>   src/providers/ad/ad_id.h              |   1 +
>   src/providers/ipa/ipa_subdomains_id.c |   2 +-
>   src/providers/ldap/ldap_id.c          |  79 ++++++++++++++-
>   src/providers/ldap/sdap.h             |   1 +
>   src/providers/ldap/sdap_async.c       | 185 ++++++++++++++++++++++++++++++++++
>   src/providers/ldap/sdap_async.h       |   9 ++
>   src/util/util_errors.c                |   1 +
>   src/util/util_errors.h                |   1 +
>   10 files changed, 320 insertions(+), 7 deletions(-)
>  
>  diff --git a/src/providers/ad/ad_common.h b/src/providers/ad/ad_common.h
>  index d370cef69124c127f41d7c4cbaa25713363e7752..d70d4656c1d1878a882f261e481a2507f8e58c55
> 100644
>  --- a/src/providers/ad/ad_common.h
>  +++ b/src/providers/ad/ad_common.h
>  @@ -69,6 +69,9 @@ struct ad_options {
>       struct sdap_options *id;
>       struct ad_id_ctx *id_ctx;
>   
>  +    /* POSIX detection */
>  +    time_t gc_check_timer;
>  +
>       /* Auth and chpass Provider */
>       struct krb5_ctx *auth_ctx;
>   
>  diff --git a/src/providers/ad/ad_id.c b/src/providers/ad/ad_id.c
>  index db921b11850c16cc7e818cad481705acbb4b9127..164855fb13a9ae88fcf244529b357824762178a8
> 100644
>  --- a/src/providers/ad/ad_id.c
>  +++ b/src/providers/ad/ad_id.c
>  @@ -27,6 +27,8 @@
>   #include "providers/ldap/sdap_async_enum.h"
>   #include "providers/ldap/sdap_idmap.h"
>   
>  +#define GC_POSIX_CHECK_TIMEOUT  3600
>  +
>   struct ad_handle_acct_info_state {
>       struct be_req *breq;
>       struct be_acct_req *ar;
>  @@ -34,6 +36,7 @@ struct ad_handle_acct_info_state {
>       struct sdap_id_conn_ctx **conn;
>       struct sdap_domain *sdom;
>       size_t cindex;
>  +    struct ad_options *ad_options;
>   
>       int dp_error;
>       const char *err;
>  @@ -47,6 +50,7 @@ ad_handle_acct_info_send(TALLOC_CTX *mem_ctx,
>                            struct be_req *breq,
>                            struct be_acct_req *ar,
>                            struct sdap_id_ctx *ctx,
>  +                         struct ad_options *ad_options,
>                            struct sdap_domain *sdom,
>                            struct sdap_id_conn_ctx **conn)
>   {
>  @@ -64,6 +68,7 @@ ad_handle_acct_info_send(TALLOC_CTX *mem_ctx,
>       state->ctx = ctx;
>       state->sdom = sdom;
>       state->conn = conn;
>  +    state->ad_options = ad_options;
>       state->cindex = 0;
>   
>       ret = ad_handle_acct_info_step(req);
>  @@ -137,12 +142,29 @@ ad_handle_acct_info_done(struct tevent_req *subreq)
>       if (sdap_err == EOK) {
>           tevent_req_done(req);
>           return;
>  +    } else if (sdap_err == ERR_NO_POSIX) {
>  +        if (dp_opt_get_bool(state->ad_options->basic, AD_ENABLE_GC)) {
>  +            DEBUG(SSSDBG_IMPORTANT_INFO, ("POSIX attributes were requested "
>  +                  "but are not present on the server side. Global Catalog "
>  +                  "lookups will be disabled for %d seconds\n",
>  +                  GC_POSIX_CHECK_TIMEOUT));
>  +
>  +            state->ad_options->gc_check_timer = time(NULL); 
I would like to suggest rename gc_check_timer to gc_next_check and set
it to time(NULL)+GC_POSIX_CHECK_TIMEOUT here. This will save you an add
operation later which might be called more often (ok, compared to
calling time() the cycles spend for an add are negligible, but still the
check might be easier to read).

>  +
>  +            ret = dp_opt_set_bool(state->ad_options->basic,
>  +                                  AD_ENABLE_GC, false);
>  +            if (ret != EOK) {
>  +                DEBUG(SSSDBG_MINOR_FAILURE,
>  +                      ("Could not turn off GC support\n"));
>  +                /* Not fatal */
>  +            }
>  +        }
>       } else if (sdap_err != ENOENT) {
>           tevent_req_error(req, EIO);
>           return;
>       }
>   
>  -    /* Ret is only ENOENT now. Try the next connection */
>  +    /* Ret is only ENOENT or ERR_NO_POSIX now. Try the next connection */
>       state->cindex++;
>       ret = ad_handle_acct_info_step(req);
>       if (ret != EAGAIN) {
>  @@ -188,6 +210,25 @@ get_conn_list(struct be_req *breq, struct ad_id_ctx *ad_ctx,
>                 struct sss_domain_info *dom, struct be_acct_req *ar)
>   {
>       struct sdap_id_conn_ctx **clist;
>  +    time_t now;
>  +    errno_t ret;
>  +
>  +    /* If the last POSIX check was too far in the past, we might need to
>  +     * re-check again
>  +     */
>  +    now = time(NULL);
>  +    if (ad_ctx->ad_options->gc_check_timer &&
>  +            ad_ctx->ad_options->gc_check_timer + GC_POSIX_CHECK_TIMEOUT < now)
> { 
see above

>  +        ad_ctx->sdap_id_ctx->srv_opts->posix_checked =
> false;
>  +
>  +        ret = dp_opt_set_bool(ad_ctx->ad_options->basic,
>  +                              AD_ENABLE_GC, true);
>  +        if (ret != EOK) {
>  +            DEBUG(SSSDBG_MINOR_FAILURE,
>  +                    ("Could not turn on GC support\n"));
>  +            /* Not fatal */
>  +        }
>  +    }
>   
>       switch (ar->entry_type & BE_REQ_TYPE_MASK) {
>       case BE_REQ_USER: /* user */ 
...

>  +static errno_t sdap_posix_check_parse(struct sdap_handle *sh,
>  +                                      struct sdap_msg *msg,
>  +                                      void *pvt)
>  +{
>  +    struct berval **vals;
>  +    struct sdap_posix_check_state *state =
>  +        talloc_get_type(pvt, struct sdap_posix_check_state);
>  +    char *dn;
>  +
>  +    dn = ldap_get_dn(sh->ldap, msg->msg);
>  +    if (dn == NULL) {
>  +        DEBUG(SSSDBG_TRACE_LIBS,
>  +              ("Search did not find any entry with POSIX attributes\n"));
>  +        goto done;
>  +    }
>  +    DEBUG(SSSDBG_TRACE_LIBS, ("Found [%s] with POSIX attributes\n", dn));
>  +    ldap_memfree(dn);
>  +
>  +    vals = ldap_get_values_len(sh->ldap, msg->msg,
>  +                               state->opts->user_map[SDAP_AT_USER_UID].name);
>  +    if (vals == NULL) {
>  +        vals = ldap_get_values_len(sh->ldap, msg->msg,
>  +                               state->opts->group_map[SDAP_AT_GROUP_GID].name);
>  +        if (vals == NULL) {
>  +            DEBUG(SSSDBG_TRACE_LIBS, ("Entry does not have POSIX attrs?\n"));
>  +            goto done;
>  +        }
>  +    } 
If you really want to check if one of the attributes is present you
might also want to check if the value is really an integer?

>  +    ldap_value_free_len(vals);
>  +    state->has_posix = true;
>  +
>  +done:
>  +    return EOK;
>  +}
>  +

[Jakub Hrozek]

On Thu, Jan 30, 2014 at 11:17:06AM +0100, Sumit Bose wrote:
>  On Wed, Jan 29, 2014 at 05:11:59PM +0100, Jakub Hrozek wrote:
>  > On Wed, Jan 29, 2014 at 03:39:41PM +0100, Pavel Březina wrote:
>  > > On 01/27/2014 11:33 PM, Jakub Hrozek wrote:
>  > > >Hi,
>  > > >
>  > > >When the schema is set to AD and ID mapping is used, there is a one-time
>  > > 
>  > > you wanted to say "when ID mapping is *not* used"
>  > > 
>  > > >check ran when searching for users to detect the presence of POSIX
> attributes
>  > > >in LDAP. If this check fails, the search fails as if no entry was found
>  > > >and returns a special error code.
>  > > >
>  > > >If the AD identity lookup finds this error code, the GC is disabled for
>  > > >the next search.
>  > > >
>  > > >The sdap_server_opts structure is filled every time a client connects to
>  > > >a server so the posix check boolean is reset to false again on connecting
>  > > >to the server.
>  > > 
>  > > GC lookups should not be disabled permanently. Administrator can
>  > > setup POSIX attribute propagation to GC any time and SSSD should
>  > > notice it
>  > > without restarting.
>  > > 
>  > > So we should rather perform the check periodically and not only
>  > > disable GC lookups, but also re-enable it if it succeeds.
>  > 
>  > See the attached patch, I added a timeout of 3600 seconds for the next
>  > detection. I don't think we want to have the timeout configurable but
>  > maybe we want the timeout to be reset on changing the online status as
>  > well?
>  
>  I think we do not have to reset it when changing the online status since
>  having the POSIX IDs in the GC or not is a global feature of the forest
>  and changes should be quite rare. 
After some discussion last week with Simo I decided to remove the timer
altogether. The decision to change AD schema is so rare it makes sense
to let admins restart a service on clients.

>  
>  > 
>  > > 
>  > > >It might be better to move the check to where the rootDSE is retrieved,
>  > > >but the check depends on several features that are not known to the code
>  > > >that retrieves the rootDSE (or the connection code for example) such as
> what
>  > > >the attribute mappings are or the authentication method that should be
> used.
>  > > >
>  > > >Currently this patch only runs the check when users are requested. It
>  > > >would be trivial to add the same code (about 70 lines) to the group
>  > > >request as well.
>  
>  I think it is not called when enumeration is the first task running. Can
>  you add it here as well? And while you are on it it might be worth to
>  add it to group lookups as well, just for completeness. 
Done. During the testing of the AD enumeration I found some other issues
that are on the list right now as separate patches.

>  
>  Please find more comments inline.
>  
>  bye,
>  Sumit
>  
>  > > >
>  > > >Additionally, I wonder if the absence of POSIX attributes in GC should
>  > > >be reported louder. Currently there is just MINOR_FAILURE.
>  > > 
>  > > We have SSSDBG_IMPORTANT_INFO for such things.
>  > 
>  > Done.
>  > 
>  > > 
>  > > >We could go as far as report to syslog when a user or a group from
>  > > >subdomains is requested and the GC was already disabled, but I wanted to
>  > > >check with the other developers before implementing this.
>  > > 
>  > > I haven't made up my mind on this question yet, but I'm more
>  > > inclined to avoid using syslog for this situation. :-)
>  > 
>  > Yes, me too.
>  > 
>  > > 
>  > > >One additional question -- currently the AD ID provider always retries
>  > > >in LDAP if search in GC didn't find the result. With the possibility
> of
>  > > >detecting the POSIX attributes, do we want to remove this fallback if we
>  > > >were able to run the detection and know that GC is available but
> doesn't
>  > > >contain the POSIX attributes?
>  > > >
>  > > >I would say the fallback should stay, because there is still a chance
>  > > >other required attribute will be missing, after all the user can
>  > > >configure additional filter and this fallback only affects negative
>  > > >searches.
>  > > 
>  > > I'm not sure if I understand you. Isn't the whole point of this
>  > > patch to actually avoid the fallback? I mean if we detect that GC
>  > > doesn't have POSIX attributes we want to go directly to LDAP on next
>  > > queries, don't we?
>  > 
>  > I meant the other way. Currently even if we know that GC *has* POSIX
>  > attributes and we don't find the user in GC, we still retry LDAP. I was
>  > wondering whether we can remove this fallback if we know that POSIX
>  > attributes are present. But I think we shouldn't remove the fallback for
>  > the general case because admins can still configure a custom filter in
>  > the search base and this custom filter can contain any attributes.
>  
>  > From c76a1058f1aea30b83f3dd133230a093b1de5b44 Mon Sep 17 00:00:00 2001
>  > From: Jakub Hrozek <jhrozek(a)redhat.com&gt;
>  > Date: Mon, 16 Dec 2013 18:36:12 +0100
>  > Subject: [PATCH] LDAP: Detect the presence of POSIX attributes
>  > 
>  > When the schema is set to AD and ID mapping is used, there is a one-time
>  
>                                                  ^ 'not' missing ? 
Right, fixed

>  
>  > check ran when searching for users to detect the presence of POSIX
>  > attributes in LDAP. If this check fails, the search fails as if no entry
>  > was found and returns a special error code.
>  > 
>  > The sdap_server_opts structure is filled every time a client connects to
>  > a server so the posix check boolean is reset to false again on connecting
>  > to the server.
>  > 
>  > It might be better to move the check to where the rootDSE is retrieved,
>  > but the check depends on several features that are not known to the code
>  > that retrieves the rootDSE (or the connection code for example) such as what
>  > the attribute mappings are or the authentication method that should be used.
>  > ---
>  >  src/providers/ad/ad_common.h          |   3 +
>  >  src/providers/ad/ad_id.c              |  45 ++++++++-
>  >  src/providers/ad/ad_id.h              |   1 +
>  >  src/providers/ipa/ipa_subdomains_id.c |   2 +-
>  >  src/providers/ldap/ldap_id.c          |  79 ++++++++++++++-
>  >  src/providers/ldap/sdap.h             |   1 +
>  >  src/providers/ldap/sdap_async.c       | 185 ++++++++++++++++++++++++++++++++++
>  >  src/providers/ldap/sdap_async.h       |   9 ++
>  >  src/util/util_errors.c                |   1 +
>  >  src/util/util_errors.h                |   1 +
>  >  10 files changed, 320 insertions(+), 7 deletions(-)
>  > 
>  > diff --git a/src/providers/ad/ad_common.h b/src/providers/ad/ad_common.h
>  > index
> d370cef69124c127f41d7c4cbaa25713363e7752..d70d4656c1d1878a882f261e481a2507f8e58c55 100644
>  > --- a/src/providers/ad/ad_common.h
>  > +++ b/src/providers/ad/ad_common.h
>  > @@ -69,6 +69,9 @@ struct ad_options {
>  >      struct sdap_options *id;
>  >      struct ad_id_ctx *id_ctx;
>  >  
>  > +    /* POSIX detection */
>  > +    time_t gc_check_timer;
>  > +
>  >      /* Auth and chpass Provider */
>  >      struct krb5_ctx *auth_ctx;
>  >  
>  > diff --git a/src/providers/ad/ad_id.c b/src/providers/ad/ad_id.c
>  > index
> db921b11850c16cc7e818cad481705acbb4b9127..164855fb13a9ae88fcf244529b357824762178a8 100644
>  > --- a/src/providers/ad/ad_id.c
>  > +++ b/src/providers/ad/ad_id.c
>  > @@ -27,6 +27,8 @@
>  >  #include "providers/ldap/sdap_async_enum.h"
>  >  #include "providers/ldap/sdap_idmap.h"
>  >  
>  > +#define GC_POSIX_CHECK_TIMEOUT  3600
>  > +
>  >  struct ad_handle_acct_info_state {
>  >      struct be_req *breq;
>  >      struct be_acct_req *ar;
>  > @@ -34,6 +36,7 @@ struct ad_handle_acct_info_state {
>  >      struct sdap_id_conn_ctx **conn;
>  >      struct sdap_domain *sdom;
>  >      size_t cindex;
>  > +    struct ad_options *ad_options;
>  >  
>  >      int dp_error;
>  >      const char *err;
>  > @@ -47,6 +50,7 @@ ad_handle_acct_info_send(TALLOC_CTX *mem_ctx,
>  >                           struct be_req *breq,
>  >                           struct be_acct_req *ar,
>  >                           struct sdap_id_ctx *ctx,
>  > +                         struct ad_options *ad_options,
>  >                           struct sdap_domain *sdom,
>  >                           struct sdap_id_conn_ctx **conn)
>  >  {
>  > @@ -64,6 +68,7 @@ ad_handle_acct_info_send(TALLOC_CTX *mem_ctx,
>  >      state->ctx = ctx;
>  >      state->sdom = sdom;
>  >      state->conn = conn;
>  > +    state->ad_options = ad_options;
>  >      state->cindex = 0;
>  >  
>  >      ret = ad_handle_acct_info_step(req);
>  > @@ -137,12 +142,29 @@ ad_handle_acct_info_done(struct tevent_req *subreq)
>  >      if (sdap_err == EOK) {
>  >          tevent_req_done(req);
>  >          return;
>  > +    } else if (sdap_err == ERR_NO_POSIX) {
>  > +        if (dp_opt_get_bool(state->ad_options->basic, AD_ENABLE_GC)) {
>  > +            DEBUG(SSSDBG_IMPORTANT_INFO, ("POSIX attributes were requested
> "
>  > +                  "but are not present on the server side. Global Catalog
> "
>  > +                  "lookups will be disabled for %d seconds\n",
>  > +                  GC_POSIX_CHECK_TIMEOUT));
>  > +
>  > +            state->ad_options->gc_check_timer = time(NULL);
>  
>  I would like to suggest rename gc_check_timer to gc_next_check and set
>  it to time(NULL)+GC_POSIX_CHECK_TIMEOUT here. This will save you an add
>  operation later which might be called more often (ok, compared to
>  calling time() the cycles spend for an add are negligible, but still the
>  check might be easier to read). 
The timer is gone now :-)

>  
>  > +
>  > +            ret = dp_opt_set_bool(state->ad_options->basic,
>  > +                                  AD_ENABLE_GC, false);
>  > +            if (ret != EOK) {
>  > +                DEBUG(SSSDBG_MINOR_FAILURE,
>  > +                      ("Could not turn off GC support\n"));
>  > +                /* Not fatal */
>  > +            }
>  > +        }
>  >      } else if (sdap_err != ENOENT) {
>  >          tevent_req_error(req, EIO);
>  >          return;
>  >      }
>  >  
>  > -    /* Ret is only ENOENT now. Try the next connection */
>  > +    /* Ret is only ENOENT or ERR_NO_POSIX now. Try the next connection */
>  >      state->cindex++;
>  >      ret = ad_handle_acct_info_step(req);
>  >      if (ret != EAGAIN) {
>  > @@ -188,6 +210,25 @@ get_conn_list(struct be_req *breq, struct ad_id_ctx *ad_ctx,
>  >                struct sss_domain_info *dom, struct be_acct_req *ar)
>  >  {
>  >      struct sdap_id_conn_ctx **clist;
>  > +    time_t now;
>  > +    errno_t ret;
>  > +
>  > +    /* If the last POSIX check was too far in the past, we might need to
>  > +     * re-check again
>  > +     */
>  > +    now = time(NULL);
>  > +    if (ad_ctx->ad_options->gc_check_timer &&
>  > +            ad_ctx->ad_options->gc_check_timer + GC_POSIX_CHECK_TIMEOUT <
> now) {
>  
>  see above
>  
>  > +        ad_ctx->sdap_id_ctx->srv_opts->posix_checked = false;
>  > +
>  > +        ret = dp_opt_set_bool(ad_ctx->ad_options->basic,
>  > +                              AD_ENABLE_GC, true);
>  > +        if (ret != EOK) {
>  > +            DEBUG(SSSDBG_MINOR_FAILURE,
>  > +                    ("Could not turn on GC support\n"));
>  > +            /* Not fatal */
>  > +        }
>  > +    }
>  >  
>  >      switch (ar->entry_type & BE_REQ_TYPE_MASK) {
>  >      case BE_REQ_USER: /* user */
>  
>  ...
>  
>  > +static errno_t sdap_posix_check_parse(struct sdap_handle *sh,
>  > +                                      struct sdap_msg *msg,
>  > +                                      void *pvt)
>  > +{
>  > +    struct berval **vals;
>  > +    struct sdap_posix_check_state *state =
>  > +        talloc_get_type(pvt, struct sdap_posix_check_state);
>  > +    char *dn;
>  > +
>  > +    dn = ldap_get_dn(sh->ldap, msg->msg);
>  > +    if (dn == NULL) {
>  > +        DEBUG(SSSDBG_TRACE_LIBS,
>  > +              ("Search did not find any entry with POSIX
> attributes\n"));
>  > +        goto done;
>  > +    }
>  > +    DEBUG(SSSDBG_TRACE_LIBS, ("Found [%s] with POSIX attributes\n",
> dn));
>  > +    ldap_memfree(dn);
>  > +
>  > +    vals = ldap_get_values_len(sh->ldap, msg->msg,
>  > +                              
> state->opts->user_map[SDAP_AT_USER_UID].name);
>  > +    if (vals == NULL) {
>  > +        vals = ldap_get_values_len(sh->ldap, msg->msg,
>  > +                              
> state->opts->group_map[SDAP_AT_GROUP_GID].name);
>  > +        if (vals == NULL) {
>  > +            DEBUG(SSSDBG_TRACE_LIBS, ("Entry does not have POSIX
> attrs?\n"));
>  > +            goto done;
>  > +        }
>  > +    }
>  
>  If you really want to check if one of the attributes is present you
>  might also want to check if the value is really an integer? 
I'm not sure, this code specifically checks the presence of the
attributes, the semantics is checked later when the attributes are used.
But the performance hit is very small and the additional checks might
bring some nicer error messages, so I've added them.

Thank you for the review a new patch is attached.

[Pavel Březina]

On 02/11/2014 09:33 PM, Jakub Hrozek wrote:
>  On Thu, Jan 30, 2014 at 11:17:06AM +0100, Sumit Bose wrote:
> > On Wed, Jan 29, 2014 at 05:11:59PM +0100, Jakub Hrozek wrote:
> >> On Wed, Jan 29, 2014 at 03:39:41PM +0100, Pavel Březina wrote:
> >>> On 01/27/2014 11:33 PM, Jakub Hrozek wrote:
> >>>> Hi,
> >>>>
> >>>> When the schema is set to AD and ID mapping is used, there is a one-time
> >>>
> >>> you wanted to say "when ID mapping is *not* used"
> >>>
> >>>> check ran when searching for users to detect the presence of POSIX
> attributes
> >>>> in LDAP. If this check fails, the search fails as if no entry was found
> >>>> and returns a special error code.
> >>>>
> >>>> If the AD identity lookup finds this error code, the GC is disabled for
> >>>> the next search.
> >>>>
> >>>> The sdap_server_opts structure is filled every time a client connects to
> >>>> a server so the posix check boolean is reset to false again on
> connecting
> >>>> to the server.
> >>>
> >>> GC lookups should not be disabled permanently. Administrator can
> >>> setup POSIX attribute propagation to GC any time and SSSD should
> >>> notice it
> >>> without restarting.
> >>>
> >>> So we should rather perform the check periodically and not only
> >>> disable GC lookups, but also re-enable it if it succeeds.
> >>
> >> See the attached patch, I added a timeout of 3600 seconds for the next
> >> detection. I don't think we want to have the timeout configurable but
> >> maybe we want the timeout to be reset on changing the online status as
> >> well?
> >
> > I think we do not have to reset it when changing the online status since
> > having the POSIX IDs in the GC or not is a global feature of the forest
> > and changes should be quite rare.
>
>  After some discussion last week with Simo I decided to remove the timer
>  altogether. The decision to change AD schema is so rare it makes sense
>  to let admins restart a service on clients.
>
> >
> >>
> >>>
> >>>> It might be better to move the check to where the rootDSE is retrieved,
> >>>> but the check depends on several features that are not known to the code
> >>>> that retrieves the rootDSE (or the connection code for example) such as
> what
> >>>> the attribute mappings are or the authentication method that should be
> used.
> >>>>
> >>>> Currently this patch only runs the check when users are requested. It
> >>>> would be trivial to add the same code (about 70 lines) to the group
> >>>> request as well.
> >
> > I think it is not called when enumeration is the first task running. Can
> > you add it here as well? And while you are on it it might be worth to
> > add it to group lookups as well, just for completeness.
>
>  Done. During the testing of the AD enumeration I found some other issues
>  that are on the list right now as separate patches.
>
> >
> > Please find more comments inline.
> >
> > bye,
> > Sumit
> >
> >>>>
> >>>> Additionally, I wonder if the absence of POSIX attributes in GC should
> >>>> be reported louder. Currently there is just MINOR_FAILURE.
> >>>
> >>> We have SSSDBG_IMPORTANT_INFO for such things.
> >>
> >> Done.
> >>
> >>>
> >>>> We could go as far as report to syslog when a user or a group from
> >>>> subdomains is requested and the GC was already disabled, but I wanted to
> >>>> check with the other developers before implementing this.
> >>>
> >>> I haven't made up my mind on this question yet, but I'm more
> >>> inclined to avoid using syslog for this situation. :-)
> >>
> >> Yes, me too.
> >>
> >>>
> >>>> One additional question -- currently the AD ID provider always retries
> >>>> in LDAP if search in GC didn't find the result. With the possibility
> of
> >>>> detecting the POSIX attributes, do we want to remove this fallback if we
> >>>> were able to run the detection and know that GC is available but
> doesn't
> >>>> contain the POSIX attributes?
> >>>>
> >>>> I would say the fallback should stay, because there is still a chance
> >>>> other required attribute will be missing, after all the user can
> >>>> configure additional filter and this fallback only affects negative
> >>>> searches.
> >>>
> >>> I'm not sure if I understand you. Isn't the whole point of this
> >>> patch to actually avoid the fallback? I mean if we detect that GC
> >>> doesn't have POSIX attributes we want to go directly to LDAP on next
> >>> queries, don't we?
> >>
> >> I meant the other way. Currently even if we know that GC *has* POSIX
> >> attributes and we don't find the user in GC, we still retry LDAP. I was
> >> wondering whether we can remove this fallback if we know that POSIX
> >> attributes are present. But I think we shouldn't remove the fallback for
> >> the general case because admins can still configure a custom filter in
> >> the search base and this custom filter can contain any attributes.
> >
> >>  From c76a1058f1aea30b83f3dd133230a093b1de5b44 Mon Sep 17 00:00:00 2001
> >> From: Jakub Hrozek <jhrozek(a)redhat.com&gt;
> >> Date: Mon, 16 Dec 2013 18:36:12 +0100
> >> Subject: [PATCH] LDAP: Detect the presence of POSIX attributes
> >>
> >> When the schema is set to AD and ID mapping is used, there is a one-time
> >
> >                                                  ^ 'not' missing ?
>
>  Right, fixed
>
> >
> >> check ran when searching for users to detect the presence of POSIX
> >> attributes in LDAP. If this check fails, the search fails as if no entry
> >> was found and returns a special error code.
> >>
> >> The sdap_server_opts structure is filled every time a client connects to
> >> a server so the posix check boolean is reset to false again on connecting
> >> to the server.
> >>
> >> It might be better to move the check to where the rootDSE is retrieved,
> >> but the check depends on several features that are not known to the code
> >> that retrieves the rootDSE (or the connection code for example) such as what
> >> the attribute mappings are or the authentication method that should be used.
> >> ---
> >>   src/providers/ad/ad_common.h          |   3 +
> >>   src/providers/ad/ad_id.c              |  45 ++++++++-
> >>   src/providers/ad/ad_id.h              |   1 +
> >>   src/providers/ipa/ipa_subdomains_id.c |   2 +-
> >>   src/providers/ldap/ldap_id.c          |  79 ++++++++++++++-
> >>   src/providers/ldap/sdap.h             |   1 +
> >>   src/providers/ldap/sdap_async.c       | 185 ++++++++++++++++++++++++++++++++++
> >>   src/providers/ldap/sdap_async.h       |   9 ++
> >>   src/util/util_errors.c                |   1 +
> >>   src/util/util_errors.h                |   1 +
> >>   10 files changed, 320 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/src/providers/ad/ad_common.h b/src/providers/ad/ad_common.h
> >> index
> d370cef69124c127f41d7c4cbaa25713363e7752..d70d4656c1d1878a882f261e481a2507f8e58c55 100644
> >> --- a/src/providers/ad/ad_common.h
> >> +++ b/src/providers/ad/ad_common.h
> >> @@ -69,6 +69,9 @@ struct ad_options {
> >>       struct sdap_options *id;
> >>       struct ad_id_ctx *id_ctx;
> >>
> >> +    /* POSIX detection */
> >> +    time_t gc_check_timer;
> >> +
> >>       /* Auth and chpass Provider */
> >>       struct krb5_ctx *auth_ctx;
> >>
> >> diff --git a/src/providers/ad/ad_id.c b/src/providers/ad/ad_id.c
> >> index
> db921b11850c16cc7e818cad481705acbb4b9127..164855fb13a9ae88fcf244529b357824762178a8 100644
> >> --- a/src/providers/ad/ad_id.c
> >> +++ b/src/providers/ad/ad_id.c
> >> @@ -27,6 +27,8 @@
> >>   #include "providers/ldap/sdap_async_enum.h"
> >>   #include "providers/ldap/sdap_idmap.h"
> >>
> >> +#define GC_POSIX_CHECK_TIMEOUT  3600
> >> +
> >>   struct ad_handle_acct_info_state {
> >>       struct be_req *breq;
> >>       struct be_acct_req *ar;
> >> @@ -34,6 +36,7 @@ struct ad_handle_acct_info_state {
> >>       struct sdap_id_conn_ctx **conn;
> >>       struct sdap_domain *sdom;
> >>       size_t cindex;
> >> +    struct ad_options *ad_options;
> >>
> >>       int dp_error;
> >>       const char *err;
> >> @@ -47,6 +50,7 @@ ad_handle_acct_info_send(TALLOC_CTX *mem_ctx,
> >>                            struct be_req *breq,
> >>                            struct be_acct_req *ar,
> >>                            struct sdap_id_ctx *ctx,
> >> +                         struct ad_options *ad_options,
> >>                            struct sdap_domain *sdom,
> >>                            struct sdap_id_conn_ctx **conn)
> >>   {
> >> @@ -64,6 +68,7 @@ ad_handle_acct_info_send(TALLOC_CTX *mem_ctx,
> >>       state->ctx = ctx;
> >>       state->sdom = sdom;
> >>       state->conn = conn;
> >> +    state->ad_options = ad_options;
> >>       state->cindex = 0;
> >>
> >>       ret = ad_handle_acct_info_step(req);
> >> @@ -137,12 +142,29 @@ ad_handle_acct_info_done(struct tevent_req *subreq)
> >>       if (sdap_err == EOK) {
> >>           tevent_req_done(req);
> >>           return;
> >> +    } else if (sdap_err == ERR_NO_POSIX) {
> >> +        if (dp_opt_get_bool(state->ad_options->basic, AD_ENABLE_GC)) {
> >> +            DEBUG(SSSDBG_IMPORTANT_INFO, ("POSIX attributes were requested
> "
> >> +                  "but are not present on the server side. Global Catalog
> "
> >> +                  "lookups will be disabled for %d seconds\n",
> >> +                  GC_POSIX_CHECK_TIMEOUT));
> >> +
> >> +            state->ad_options->gc_check_timer = time(NULL);
> >
> > I would like to suggest rename gc_check_timer to gc_next_check and set
> > it to time(NULL)+GC_POSIX_CHECK_TIMEOUT here. This will save you an add
> > operation later which might be called more often (ok, compared to
> > calling time() the cycles spend for an add are negligible, but still the
> > check might be easier to read).
>
>  The timer is gone now :-)
>
> >
> >> +
> >> +            ret = dp_opt_set_bool(state->ad_options->basic,
> >> +                                  AD_ENABLE_GC, false);
> >> +            if (ret != EOK) {
> >> +                DEBUG(SSSDBG_MINOR_FAILURE,
> >> +                      ("Could not turn off GC support\n"));
> >> +                /* Not fatal */
> >> +            }
> >> +        }
> >>       } else if (sdap_err != ENOENT) {
> >>           tevent_req_error(req, EIO);
> >>           return;
> >>       }
> >>
> >> -    /* Ret is only ENOENT now. Try the next connection */
> >> +    /* Ret is only ENOENT or ERR_NO_POSIX now. Try the next connection */
> >>       state->cindex++;
> >>       ret = ad_handle_acct_info_step(req);
> >>       if (ret != EAGAIN) {
> >> @@ -188,6 +210,25 @@ get_conn_list(struct be_req *breq, struct ad_id_ctx
> *ad_ctx,
> >>                 struct sss_domain_info *dom, struct be_acct_req *ar)
> >>   {
> >>       struct sdap_id_conn_ctx **clist;
> >> +    time_t now;
> >> +    errno_t ret;
> >> +
> >> +    /* If the last POSIX check was too far in the past, we might need to
> >> +     * re-check again
> >> +     */
> >> +    now = time(NULL);
> >> +    if (ad_ctx->ad_options->gc_check_timer &&
> >> +            ad_ctx->ad_options->gc_check_timer + GC_POSIX_CHECK_TIMEOUT
> < now) {
> >
> > see above
> >
> >> +        ad_ctx->sdap_id_ctx->srv_opts->posix_checked = false;
> >> +
> >> +        ret = dp_opt_set_bool(ad_ctx->ad_options->basic,
> >> +                              AD_ENABLE_GC, true);
> >> +        if (ret != EOK) {
> >> +            DEBUG(SSSDBG_MINOR_FAILURE,
> >> +                    ("Could not turn on GC support\n"));
> >> +            /* Not fatal */
> >> +        }
> >> +    }
> >>
> >>       switch (ar->entry_type & BE_REQ_TYPE_MASK) {
> >>       case BE_REQ_USER: /* user */
> >
> > ...
> >
> >> +static errno_t sdap_posix_check_parse(struct sdap_handle *sh,
> >> +                                      struct sdap_msg *msg,
> >> +                                      void *pvt)
> >> +{
> >> +    struct berval **vals;
> >> +    struct sdap_posix_check_state *state =
> >> +        talloc_get_type(pvt, struct sdap_posix_check_state);
> >> +    char *dn;
> >> +
> >> +    dn = ldap_get_dn(sh->ldap, msg->msg);
> >> +    if (dn == NULL) {
> >> +        DEBUG(SSSDBG_TRACE_LIBS,
> >> +              ("Search did not find any entry with POSIX
> attributes\n"));
> >> +        goto done;
> >> +    }
> >> +    DEBUG(SSSDBG_TRACE_LIBS, ("Found [%s] with POSIX attributes\n",
> dn));
> >> +    ldap_memfree(dn);
> >> +
> >> +    vals = ldap_get_values_len(sh->ldap, msg->msg,
> >> +                              
> state->opts->user_map[SDAP_AT_USER_UID].name);
> >> +    if (vals == NULL) {
> >> +        vals = ldap_get_values_len(sh->ldap, msg->msg,
> >> +                              
> state->opts->group_map[SDAP_AT_GROUP_GID].name);
> >> +        if (vals == NULL) {
> >> +            DEBUG(SSSDBG_TRACE_LIBS, ("Entry does not have POSIX
> attrs?\n"));
> >> +            goto done;
> >> +        }
> >> +    }
> >
> > If you really want to check if one of the attributes is present you
> > might also want to check if the value is really an integer?
>
>  I'm not sure, this code specifically checks the presence of the
>  attributes, the semantics is checked later when the attributes are used.
>  But the performance hit is very small and the additional checks might
>  bring some nicer error messages, so I've added them.
>
>  Thank you for the review a new patch is attached. 
Ack.

[Jakub Hrozek]

On Wed, Feb 12, 2014 at 11:30:24AM +0100, Pavel Březina wrote:
>  On 02/11/2014 09:33 PM, Jakub Hrozek wrote:
>  >On Thu, Jan 30, 2014 at 11:17:06AM +0100, Sumit Bose wrote:
>  >>On Wed, Jan 29, 2014 at 05:11:59PM +0100, Jakub Hrozek wrote:
>  >>>On Wed, Jan 29, 2014 at 03:39:41PM +0100, Pavel Březina wrote:
>  >>>>On 01/27/2014 11:33 PM, Jakub Hrozek wrote:
>  >>>>>Hi,
>  >>>>>
>  >>>>>When the schema is set to AD and ID mapping is used, there is a
> one-time
>  >>>>
>  >>>>you wanted to say "when ID mapping is *not* used"
>  >>>>
>  >>>>>check ran when searching for users to detect the presence of POSIX
> attributes
>  >>>>>in LDAP. If this check fails, the search fails as if no entry was
> found
>  >>>>>and returns a special error code.
>  >>>>>
>  >>>>>If the AD identity lookup finds this error code, the GC is disabled
> for
>  >>>>>the next search.
>  >>>>>
>  >>>>>The sdap_server_opts structure is filled every time a client connects
> to
>  >>>>>a server so the posix check boolean is reset to false again on
> connecting
>  >>>>>to the server.
>  >>>>
>  >>>>GC lookups should not be disabled permanently. Administrator can
>  >>>>setup POSIX attribute propagation to GC any time and SSSD should
>  >>>>notice it
>  >>>>without restarting.
>  >>>>
>  >>>>So we should rather perform the check periodically and not only
>  >>>>disable GC lookups, but also re-enable it if it succeeds.
>  >>>
>  >>>See the attached patch, I added a timeout of 3600 seconds for the next
>  >>>detection. I don't think we want to have the timeout configurable but
>  >>>maybe we want the timeout to be reset on changing the online status as
>  >>>well?
>  >>
>  >>I think we do not have to reset it when changing the online status since
>  >>having the POSIX IDs in the GC or not is a global feature of the forest
>  >>and changes should be quite rare.
>  >
>  >After some discussion last week with Simo I decided to remove the timer
>  >altogether. The decision to change AD schema is so rare it makes sense
>  >to let admins restart a service on clients.
>  >
>  >>
>  >>>
>  >>>>
>  >>>>>It might be better to move the check to where the rootDSE is
> retrieved,
>  >>>>>but the check depends on several features that are not known to the
> code
>  >>>>>that retrieves the rootDSE (or the connection code for example) such
> as what
>  >>>>>the attribute mappings are or the authentication method that should
> be used.
>  >>>>>
>  >>>>>Currently this patch only runs the check when users are requested.
> It
>  >>>>>would be trivial to add the same code (about 70 lines) to the group
>  >>>>>request as well.
>  >>
>  >>I think it is not called when enumeration is the first task running. Can
>  >>you add it here as well? And while you are on it it might be worth to
>  >>add it to group lookups as well, just for completeness.
>  >
>  >Done. During the testing of the AD enumeration I found some other issues
>  >that are on the list right now as separate patches.
>  >
>  >>
>  >>Please find more comments inline.
>  >>
>  >>bye,
>  >>Sumit
>  >>
>  >>>>>
>  >>>>>Additionally, I wonder if the absence of POSIX attributes in GC
> should
>  >>>>>be reported louder. Currently there is just MINOR_FAILURE.
>  >>>>
>  >>>>We have SSSDBG_IMPORTANT_INFO for such things.
>  >>>
>  >>>Done.
>  >>>
>  >>>>
>  >>>>>We could go as far as report to syslog when a user or a group from
>  >>>>>subdomains is requested and the GC was already disabled, but I wanted
> to
>  >>>>>check with the other developers before implementing this.
>  >>>>
>  >>>>I haven't made up my mind on this question yet, but I'm more
>  >>>>inclined to avoid using syslog for this situation. :-)
>  >>>
>  >>>Yes, me too.
>  >>>
>  >>>>
>  >>>>>One additional question -- currently the AD ID provider always
> retries
>  >>>>>in LDAP if search in GC didn't find the result. With the
> possibility of
>  >>>>>detecting the POSIX attributes, do we want to remove this fallback if
> we
>  >>>>>were able to run the detection and know that GC is available but
> doesn't
>  >>>>>contain the POSIX attributes?
>  >>>>>
>  >>>>>I would say the fallback should stay, because there is still a
> chance
>  >>>>>other required attribute will be missing, after all the user can
>  >>>>>configure additional filter and this fallback only affects negative
>  >>>>>searches.
>  >>>>
>  >>>>I'm not sure if I understand you. Isn't the whole point of this
>  >>>>patch to actually avoid the fallback? I mean if we detect that GC
>  >>>>doesn't have POSIX attributes we want to go directly to LDAP on next
>  >>>>queries, don't we?
>  >>>
>  >>>I meant the other way. Currently even if we know that GC *has* POSIX
>  >>>attributes and we don't find the user in GC, we still retry LDAP. I was
>  >>>wondering whether we can remove this fallback if we know that POSIX
>  >>>attributes are present. But I think we shouldn't remove the fallback for
>  >>>the general case because admins can still configure a custom filter in
>  >>>the search base and this custom filter can contain any attributes.
>  >>
>  >>> From c76a1058f1aea30b83f3dd133230a093b1de5b44 Mon Sep 17 00:00:00 2001
>  >>>From: Jakub Hrozek <jhrozek(a)redhat.com&gt;
>  >>>Date: Mon, 16 Dec 2013 18:36:12 +0100
>  >>>Subject: [PATCH] LDAP: Detect the presence of POSIX attributes
>  >>>
>  >>>When the schema is set to AD and ID mapping is used, there is a one-time
>  >>
>  >>                                                 ^ 'not' missing ?
>  >
>  >Right, fixed
>  >
>  >>
>  >>>check ran when searching for users to detect the presence of POSIX
>  >>>attributes in LDAP. If this check fails, the search fails as if no entry
>  >>>was found and returns a special error code.
>  >>>
>  >>>The sdap_server_opts structure is filled every time a client connects to
>  >>>a server so the posix check boolean is reset to false again on connecting
>  >>>to the server.
>  >>>
>  >>>It might be better to move the check to where the rootDSE is retrieved,
>  >>>but the check depends on several features that are not known to the code
>  >>>that retrieves the rootDSE (or the connection code for example) such as what
>  >>>the attribute mappings are or the authentication method that should be used.
>  >>>---
>  >>>  src/providers/ad/ad_common.h          |   3 +
>  >>>  src/providers/ad/ad_id.c              |  45 ++++++++-
>  >>>  src/providers/ad/ad_id.h              |   1 +
>  >>>  src/providers/ipa/ipa_subdomains_id.c |   2 +-
>  >>>  src/providers/ldap/ldap_id.c          |  79 ++++++++++++++-
>  >>>  src/providers/ldap/sdap.h             |   1 +
>  >>>  src/providers/ldap/sdap_async.c       | 185
> ++++++++++++++++++++++++++++++++++
>  >>>  src/providers/ldap/sdap_async.h       |   9 ++
>  >>>  src/util/util_errors.c                |   1 +
>  >>>  src/util/util_errors.h                |   1 +
>  >>>  10 files changed, 320 insertions(+), 7 deletions(-)
>  >>>
>  >>>diff --git a/src/providers/ad/ad_common.h b/src/providers/ad/ad_common.h
>  >>>index
> d370cef69124c127f41d7c4cbaa25713363e7752..d70d4656c1d1878a882f261e481a2507f8e58c55 100644
>  >>>--- a/src/providers/ad/ad_common.h
>  >>>+++ b/src/providers/ad/ad_common.h
>  >>>@@ -69,6 +69,9 @@ struct ad_options {
>  >>>      struct sdap_options *id;
>  >>>      struct ad_id_ctx *id_ctx;
>  >>>
>  >>>+    /* POSIX detection */
>  >>>+    time_t gc_check_timer;
>  >>>+
>  >>>      /* Auth and chpass Provider */
>  >>>      struct krb5_ctx *auth_ctx;
>  >>>
>  >>>diff --git a/src/providers/ad/ad_id.c b/src/providers/ad/ad_id.c
>  >>>index
> db921b11850c16cc7e818cad481705acbb4b9127..164855fb13a9ae88fcf244529b357824762178a8 100644
>  >>>--- a/src/providers/ad/ad_id.c
>  >>>+++ b/src/providers/ad/ad_id.c
>  >>>@@ -27,6 +27,8 @@
>  >>>  #include "providers/ldap/sdap_async_enum.h"
>  >>>  #include "providers/ldap/sdap_idmap.h"
>  >>>
>  >>>+#define GC_POSIX_CHECK_TIMEOUT  3600
>  >>>+
>  >>>  struct ad_handle_acct_info_state {
>  >>>      struct be_req *breq;
>  >>>      struct be_acct_req *ar;
>  >>>@@ -34,6 +36,7 @@ struct ad_handle_acct_info_state {
>  >>>      struct sdap_id_conn_ctx **conn;
>  >>>      struct sdap_domain *sdom;
>  >>>      size_t cindex;
>  >>>+    struct ad_options *ad_options;
>  >>>
>  >>>      int dp_error;
>  >>>      const char *err;
>  >>>@@ -47,6 +50,7 @@ ad_handle_acct_info_send(TALLOC_CTX *mem_ctx,
>  >>>                           struct be_req *breq,
>  >>>                           struct be_acct_req *ar,
>  >>>                           struct sdap_id_ctx *ctx,
>  >>>+                         struct ad_options *ad_options,
>  >>>                           struct sdap_domain *sdom,
>  >>>                           struct sdap_id_conn_ctx **conn)
>  >>>  {
>  >>>@@ -64,6 +68,7 @@ ad_handle_acct_info_send(TALLOC_CTX *mem_ctx,
>  >>>      state->ctx = ctx;
>  >>>      state->sdom = sdom;
>  >>>      state->conn = conn;
>  >>>+    state->ad_options = ad_options;
>  >>>      state->cindex = 0;
>  >>>
>  >>>      ret = ad_handle_acct_info_step(req);
>  >>>@@ -137,12 +142,29 @@ ad_handle_acct_info_done(struct tevent_req *subreq)
>  >>>      if (sdap_err == EOK) {
>  >>>          tevent_req_done(req);
>  >>>          return;
>  >>>+    } else if (sdap_err == ERR_NO_POSIX) {
>  >>>+        if (dp_opt_get_bool(state->ad_options->basic, AD_ENABLE_GC))
> {
>  >>>+            DEBUG(SSSDBG_IMPORTANT_INFO, ("POSIX attributes were
> requested "
>  >>>+                  "but are not present on the server side. Global
> Catalog "
>  >>>+                  "lookups will be disabled for %d seconds\n",
>  >>>+                  GC_POSIX_CHECK_TIMEOUT));
>  >>>+
>  >>>+            state->ad_options->gc_check_timer = time(NULL);
>  >>
>  >>I would like to suggest rename gc_check_timer to gc_next_check and set
>  >>it to time(NULL)+GC_POSIX_CHECK_TIMEOUT here. This will save you an add
>  >>operation later which might be called more often (ok, compared to
>  >>calling time() the cycles spend for an add are negligible, but still the
>  >>check might be easier to read).
>  >
>  >The timer is gone now :-)
>  >
>  >>
>  >>>+
>  >>>+            ret = dp_opt_set_bool(state->ad_options->basic,
>  >>>+                                  AD_ENABLE_GC, false);
>  >>>+            if (ret != EOK) {
>  >>>+                DEBUG(SSSDBG_MINOR_FAILURE,
>  >>>+                      ("Could not turn off GC support\n"));
>  >>>+                /* Not fatal */
>  >>>+            }
>  >>>+        }
>  >>>      } else if (sdap_err != ENOENT) {
>  >>>          tevent_req_error(req, EIO);
>  >>>          return;
>  >>>      }
>  >>>
>  >>>-    /* Ret is only ENOENT now. Try the next connection */
>  >>>+    /* Ret is only ENOENT or ERR_NO_POSIX now. Try the next connection */
>  >>>      state->cindex++;
>  >>>      ret = ad_handle_acct_info_step(req);
>  >>>      if (ret != EAGAIN) {
>  >>>@@ -188,6 +210,25 @@ get_conn_list(struct be_req *breq, struct ad_id_ctx
> *ad_ctx,
>  >>>                struct sss_domain_info *dom, struct be_acct_req *ar)
>  >>>  {
>  >>>      struct sdap_id_conn_ctx **clist;
>  >>>+    time_t now;
>  >>>+    errno_t ret;
>  >>>+
>  >>>+    /* If the last POSIX check was too far in the past, we might need to
>  >>>+     * re-check again
>  >>>+     */
>  >>>+    now = time(NULL);
>  >>>+    if (ad_ctx->ad_options->gc_check_timer &&
>  >>>+            ad_ctx->ad_options->gc_check_timer +
> GC_POSIX_CHECK_TIMEOUT < now) {
>  >>
>  >>see above
>  >>
>  >>>+        ad_ctx->sdap_id_ctx->srv_opts->posix_checked = false;
>  >>>+
>  >>>+        ret = dp_opt_set_bool(ad_ctx->ad_options->basic,
>  >>>+                              AD_ENABLE_GC, true);
>  >>>+        if (ret != EOK) {
>  >>>+            DEBUG(SSSDBG_MINOR_FAILURE,
>  >>>+                    ("Could not turn on GC support\n"));
>  >>>+            /* Not fatal */
>  >>>+        }
>  >>>+    }
>  >>>
>  >>>      switch (ar->entry_type & BE_REQ_TYPE_MASK) {
>  >>>      case BE_REQ_USER: /* user */
>  >>
>  >>...
>  >>
>  >>>+static errno_t sdap_posix_check_parse(struct sdap_handle *sh,
>  >>>+                                      struct sdap_msg *msg,
>  >>>+                                      void *pvt)
>  >>>+{
>  >>>+    struct berval **vals;
>  >>>+    struct sdap_posix_check_state *state =
>  >>>+        talloc_get_type(pvt, struct sdap_posix_check_state);
>  >>>+    char *dn;
>  >>>+
>  >>>+    dn = ldap_get_dn(sh->ldap, msg->msg);
>  >>>+    if (dn == NULL) {
>  >>>+        DEBUG(SSSDBG_TRACE_LIBS,
>  >>>+              ("Search did not find any entry with POSIX
> attributes\n"));
>  >>>+        goto done;
>  >>>+    }
>  >>>+    DEBUG(SSSDBG_TRACE_LIBS, ("Found [%s] with POSIX
> attributes\n", dn));
>  >>>+    ldap_memfree(dn);
>  >>>+
>  >>>+    vals = ldap_get_values_len(sh->ldap, msg->msg,
>  >>>+                              
> state->opts->user_map[SDAP_AT_USER_UID].name);
>  >>>+    if (vals == NULL) {
>  >>>+        vals = ldap_get_values_len(sh->ldap, msg->msg,
>  >>>+                              
> state->opts->group_map[SDAP_AT_GROUP_GID].name);
>  >>>+        if (vals == NULL) {
>  >>>+            DEBUG(SSSDBG_TRACE_LIBS, ("Entry does not have POSIX
> attrs?\n"));
>  >>>+            goto done;
>  >>>+        }
>  >>>+    }
>  >>
>  >>If you really want to check if one of the attributes is present you
>  >>might also want to check if the value is really an integer?
>  >
>  >I'm not sure, this code specifically checks the presence of the
>  >attributes, the semantics is checked later when the attributes are used.
>  >But the performance hit is very small and the additional checks might
>  >bring some nicer error messages, so I've added them.
>  >
>  >Thank you for the review a new patch is attached.
>  
>  Ack. 
Thank you both for the review, pushed to master and sssd-1-11

[Pavel Březina]

On 01/29/2014 05:11 PM, Jakub Hrozek wrote:
>  On Wed, Jan 29, 2014 at 03:39:41PM +0100, Pavel Březina wrote:
> > On 01/27/2014 11:33 PM, Jakub Hrozek wrote:
> >> Hi,
> >>
> >> When the schema is set to AD and ID mapping is used, there is a one-time
> >
> > you wanted to say "when ID mapping is *not* used"
> >
> >> check ran when searching for users to detect the presence of POSIX attributes
> >> in LDAP. If this check fails, the search fails as if no entry was found
> >> and returns a special error code.
> >>
> >> If the AD identity lookup finds this error code, the GC is disabled for
> >> the next search.
> >>
> >> The sdap_server_opts structure is filled every time a client connects to
> >> a server so the posix check boolean is reset to false again on connecting
> >> to the server.
> >
> > GC lookups should not be disabled permanently. Administrator can
> > setup POSIX attribute propagation to GC any time and SSSD should
> > notice it
> > without restarting.
> >
> > So we should rather perform the check periodically and not only
> > disable GC lookups, but also re-enable it if it succeeds.
>
>  See the attached patch, I added a timeout of 3600 seconds for the next
>  detection. I don't think we want to have the timeout configurable but
>  maybe we want the timeout to be reset on changing the online status as
>  well?
>
> >
> >> It might be better to move the check to where the rootDSE is retrieved,
> >> but the check depends on several features that are not known to the code
> >> that retrieves the rootDSE (or the connection code for example) such as what
> >> the attribute mappings are or the authentication method that should be used.
> >>
> >> Currently this patch only runs the check when users are requested. It
> >> would be trivial to add the same code (about 70 lines) to the group
> >> request as well.
> >>
> >> Additionally, I wonder if the absence of POSIX attributes in GC should
> >> be reported louder. Currently there is just MINOR_FAILURE.
> >
> > We have SSSDBG_IMPORTANT_INFO for such things.
>
>  Done.
>
> >
> >> We could go as far as report to syslog when a user or a group from
> >> subdomains is requested and the GC was already disabled, but I wanted to
> >> check with the other developers before implementing this.
> >
> > I haven't made up my mind on this question yet, but I'm more
> > inclined to avoid using syslog for this situation. :-)
>
>  Yes, me too.
>
> >
> >> One additional question -- currently the AD ID provider always retries
> >> in LDAP if search in GC didn't find the result. With the possibility of
> >> detecting the POSIX attributes, do we want to remove this fallback if we
> >> were able to run the detection and know that GC is available but doesn't
> >> contain the POSIX attributes?
> >>
> >> I would say the fallback should stay, because there is still a chance
> >> other required attribute will be missing, after all the user can
> >> configure additional filter and this fallback only affects negative
> >> searches.
> >
> > I'm not sure if I understand you. Isn't the whole point of this
> > patch to actually avoid the fallback? I mean if we detect that GC
> > doesn't have POSIX attributes we want to go directly to LDAP on next
> > queries, don't we?
>
>  I meant the other way. Currently even if we know that GC *has* POSIX
>  attributes and we don't find the user in GC, we still retry LDAP. I was
>  wondering whether we can remove this fallback if we know that POSIX
>  attributes are present. But I think we shouldn't remove the fallback for
>  the general case because admins can still configure a custom filter in
>  the search base and this custom filter can contain any attributes. 
Ah, OK. I agree.