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