Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sudo: always use server highest usn for smart refresh #806

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
3 participants
@pbrezina
Copy link
Member

commented Apr 24, 2019

The sudo attributes may not be indexed on the server, therefore if
smart refresh filter is run on the server it may first search using
the indexed entryusn attribute and run the rest of the filter on
non-sudo objects. The number of objects that are filtered may increased
dramatically if sudo rules are not changed for a long time (and thus
keeping smaller and smaller last sudo usn number).

This patch makes sure that highest sudo usn number is always set to
the highest server usn number after each refresh.

Resolves:
https://pagure.io/SSSD/sssd/issue/3997

@pbrezina pbrezina requested a review from sumit-bose Apr 29, 2019

@sumit-bose

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

Hi Pavel,

after looking at the patch for some time I wonder what is the modification that actually changed the behavior? The call to 'sysdb_compare_usn()' is removed but since the 'if (usn_number > srv_opts->last_usn)' is still present I would expected that the overall behavior stays the same? Is there a chance that some other patch is missing?

bye,
Sumit

@pbrezina

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

Old code

srv_opts->max_sudo_value contains the largest usn value from found sudo rule (i.e. possibly from ou=sudoers subtree).

    errno = 0;
    usn_number = strtoul(usn, &endptr, 10);
    if (errno != 0) {
        ret = errno;
        DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s [%d]: %s\n",
              usn, ret, sss_strerror(ret));
        return;
    }

    if (usn_number == 0) {
        /* Zero means that there were no rules on the server, so we have
         * nothing to store. */
        DEBUG(SSSDBG_TRACE_FUNC, "SUDO USN value is empty.\n");
        return;
    }

    newusn = sdap_sudo_new_usn(srv_opts, usn_number, endptr);
    if (newusn == NULL) {
        return;
    }

    /* NOTE: We compare largest value found in the search result with the latest value from previous result */
    if (sysdb_compare_usn(newusn, srv_opts->max_sudo_value) > 0) {
        talloc_zfree(srv_opts->max_sudo_value);
        srv_opts->max_sudo_value = newusn;
    } else {
        talloc_zfree(newusn);
    }

    /* Then we update last_usn if needed. */
    if (usn_number > srv_opts->last_usn) {
        srv_opts->last_usn = usn_number;
    }

New code

We always use last_usn regardless of what was returned in the search result.

    errno = 0;
    usn_number = strtoul(usn, &endptr, 10);
    if (errno != 0) {
        ret = errno;
        DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s [%d]: %s\n",
              usn, ret, sss_strerror(ret));
        return;
    }

    if (usn_number > srv_opts->last_usn) {
        srv_opts->last_usn = usn_number;
    }

    newusn = sdap_sudo_new_usn(srv_opts, srv_opts->last_usn, endptr);
    if (newusn == NULL) {
        return;
    }

    talloc_zfree(srv_opts->max_sudo_value);
    srv_opts->max_sudo_value = newusn;
@pbrezina

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

Because the search result contains only sudo object (or nothing), it is quite possible that srv_opts->max_sudo_value <= newusn < srv_opts->last_usn therefore before the patch we would set srv_opts->max_sudo_value to a value that is lower than srv_opts->last_usn. With the patch we always use the highest known usn value and it will get updated with each refresh.

@sumit-bose

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

Because the search result contains only sudo object (or nothing), it is quite possible that srv_opts->max_sudo_value <= newusn < srv_opts->last_usn therefore before the patch we would set srv_opts->max_sudo_value to a value that is lower than srv_opts->last_usn. With the patch we always use the highest known usn value and it will get updated with each refresh.

Thanks for the explanation. But in my testing the looks like srv_opts->last_usn is basically never updated. It is set at the first connection setup. On every reconnect (by default every 15min) it is read again, but as long as the server stays the same it is not updated as can be see here where I used a shorter ldap_connection_expire timeout:

(Fri May  3 17:59:26 2019) [sssd[be[ad.devel]]] [sdap_id_op_connect_done] (0x2000): Old USN: 2405656, New USN: 2406238
(Fri May  3 17:59:36 2019) [sssd[be[ad.devel]]] [sdap_sudo_smart_refresh_send] (0x0400): Issuing a smart refresh of sudo rules (USN >= 2405657)
(Fri May  3 17:59:36 2019) [sssd[be[ad.devel]]] [sdap_id_op_connect_done] (0x2000): Old USN: 2405656, New USN: 2406239
(Fri May  3 17:59:46 2019) [sssd[be[ad.devel]]] [sdap_sudo_smart_refresh_send] (0x0400): Issuing a smart refresh of sudo rules (USN >= 2405657)
(Fri May  3 17:59:46 2019) [sssd[be[ad.devel]]] [sdap_id_op_connect_done] (0x2000): Old USN: 2405656, New USN: 2406240
(Fri May  3 17:59:56 2019) [sssd[be[ad.devel]]] [sdap_sudo_smart_refresh_send] (0x0400): Issuing a smart refresh of sudo rules (USN >= 2405657)
(Fri May  3 17:59:56 2019) [sssd[be[ad.devel]]] [sdap_id_op_connect_done] (0x2000): Old USN: 2405656, New USN: 2406241
(Fri May  3 18:00:06 2019) [sssd[be[ad.devel]]] [sdap_sudo_smart_refresh_send] (0x0400): Issuing a smart refresh of sudo rules (USN >= 2405657)
(Fri May  3 18:00:06 2019) [sssd[be[ad.devel]]] [sdap_id_op_connect_done] (0x2000): Old USN: 2405656, New USN: 2406242
(Fri May  3 18:00:16 2019) [sssd[be[ad.devel]]] [sdap_sudo_smart_refresh_send] (0x0400): Issuing a smart refresh of sudo rules (USN >= 2405657)
(Fri May  3 18:00:16 2019) [sssd[be[ad.devel]]] [sdap_id_op_connect_done] (0x2000): Old USN: 2405656, New USN: 2406243
(Fri May  3 18:00:26 2019) [sssd[be[ad.devel]]] [sdap_sudo_smart_refresh_send] (0x0400): Issuing a smart refresh of sudo rules (USN >= 2405657)
(Fri May  3 18:00:26 2019) [sssd[be[ad.devel]]] [sdap_id_op_connect_done] (0x2000): Old USN: 2405656, New USN: 2406244
(Fri May  3 18:00:36 2019) [sssd[be[ad.devel]]] [sdap_sudo_smart_refresh_send] (0x0400): Issuing a smart refresh of sudo rules (USN >= 2405657)
(Fri May  3 18:00:36 2019) [sssd[be[ad.devel]]] [sdap_id_op_connect_done] (0x2000): Old USN: 2405656, New USN: 2406245
(Fri May  3 18:00:46 2019) [sssd[be[ad.devel]]] [sdap_sudo_smart_refresh_send] (0x0400): Issuing a smart refresh of sudo rules (USN >= 2405657)
(Fri May  3 18:00:46 2019) [sssd[be[ad.devel]]] [sdap_id_op_connect_done] (0x2000): Old USN: 2405656, New USN: 2406246
(Fri May  3 18:00:56 2019) [sssd[be[ad.devel]]] [sdap_sudo_smart_refresh_send] (0x0400): Issuing a smart refresh of sudo rules (USN >= 2405657)
(Fri May  3 18:00:56 2019) [sssd[be[ad.devel]]] [sdap_id_op_connect_done] (0x2000): Old USN: 2405656, New USN: 2406247
(Fri May  3 18:01:06 2019) [sssd[be[ad.devel]]] [sdap_sudo_smart_refresh_send] (0x0400): Issuing a smart refresh of sudo rules (USN >= 2405657)
(Fri May  3 18:01:06 2019) [sssd[be[ad.devel]]] [sdap_id_op_connect_done] (0x2000): Old USN: 2405656, New USN: 2406248
(Fri May  3 18:01:16 2019) [sssd[be[ad.devel]]] [sdap_sudo_smart_refresh_send] (0x0400): Issuing a smart refresh of sudo rules (USN >= 2405657)
(Fri May  3 18:01:16 2019) [sssd[be[ad.devel]]] [sdap_id_op_connect_done] (0x2000): Old USN: 2405656, New USN: 2406249
(Fri May  3 18:01:26 2019) [sssd[be[ad.devel]]] [sdap_sudo_smart_refresh_send] (0x0400): Issuing a smart refresh of sudo rules (USN >= 2405657)
(Fri May  3 18:01:26 2019) [sssd[be[ad.devel]]] [sdap_id_op_connect_done] (0x2000): Old USN: 2405656, New USN: 2406250

If I add

diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c
index e81aaf4..4a3670e 100644
--- a/src/providers/ldap/sdap.c
+++ b/src/providers/ldap/sdap.c
@@ -1460,8 +1460,9 @@ void sdap_steal_server_opts(struct sdap_id_ctx *id_ctx,
     }
 
     /* discard if same as previous so we do not reset max usn values
-     * unnecessarily */
+     * unnecessarily, only update last_usn. */
     if (strcmp(id_ctx->srv_opts->server_id, (*srv_opts)->server_id) == 0) {
+        id_ctx->srv_opts->last_usn = (*srv_opts)->last_usn;
         talloc_zfree(*srv_opts);
         return;
     }

I see

(Fri May  3 18:19:37 2019) [sssd[be[ad.devel]]] [sdap_id_op_connect_done] (0x2000): Old USN: 2406361, New USN: 2406362
(Fri May  3 18:19:47 2019) [sssd[be[ad.devel]]] [sdap_sudo_smart_refresh_send] (0x0400): Issuing a smart refresh of sudo rules (USN >= 2406363)
(Fri May  3 18:19:47 2019) [sssd[be[ad.devel]]] [sdap_id_op_connect_done] (0x2000): Old USN: 2406362, New USN: 2406363
(Fri May  3 18:19:57 2019) [sssd[be[ad.devel]]] [sdap_sudo_smart_refresh_send] (0x0400): Issuing a smart refresh of sudo rules (USN >= 2406364)
(Fri May  3 18:19:57 2019) [sssd[be[ad.devel]]] [sdap_id_op_connect_done] (0x2000): Old USN: 2406363, New USN: 2406368
(Fri May  3 18:20:07 2019) [sssd[be[ad.devel]]] [sdap_sudo_smart_refresh_send] (0x0400): Issuing a smart refresh of sudo rules (USN >= 2406369)
(Fri May  3 18:20:07 2019) [sssd[be[ad.devel]]] [sdap_id_op_connect_done] (0x2000): Old USN: 2406368, New USN: 2406369
(Fri May  3 18:20:17 2019) [sssd[be[ad.devel]]] [sdap_sudo_smart_refresh_send] (0x0400): Issuing a smart refresh of sudo rules (USN >= 2406370)
(Fri May  3 18:20:17 2019) [sssd[be[ad.devel]]] [sdap_id_op_connect_done] (0x2000): Old USN: 2406369, New USN: 2406370
(Fri May  3 18:20:27 2019) [sssd[be[ad.devel]]] [sdap_sudo_smart_refresh_send] (0x0400): Issuing a smart refresh of sudo rules (USN >= 2406371)
(Fri May  3 18:20:27 2019) [sssd[be[ad.devel]]] [sdap_id_op_connect_done] (0x2000): Old USN: 2406370, New USN: 2406376
(Fri May  3 18:20:37 2019) [sssd[be[ad.devel]]] [sdap_sudo_smart_refresh_send] (0x0400): Issuing a smart refresh of sudo rules (USN >= 2406377)
(Fri May  3 18:20:37 2019) [sssd[be[ad.devel]]] [sdap_id_op_connect_done] (0x2000): Old USN: 2406376, New USN: 2406377

But even in this case it has to be documented that the highest USN is not updated after each smart refresh but only during a reconnect. Since both timeout values (reconnect and smart refresh) are 15min by default is will effectively be the case but if one of the values is modified this might change.

bye,
Sumit

@pbrezina

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

I'm not sure what do you mean. From what I read in the code I see that:

  1. srv_opts->last_usn is updated by reading lastUSN attribute from rootDSE when new connection is established (sdap id op code or online check code)
  2. It gets also updated when enumerating users/groups/services and sudo smart refresh if any object found by the enumeration have entryUSN > srv_opts->last_usn.

So it does not necessarily corresponds to real value of lastUSN attribute but it contains greatest USN value known to SSSD.

@sumit-bose

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

I'm not sure what do you mean. From what I read in the code I see that:

1. `srv_opts->last_usn` is updated by reading `lastUSN` attribute from `rootDSE` when new connection is established (sdap id op code or online check code)

It is unfortunately not updated if the old and the new server are the same (see my patch above). And since we try to stick to one server this will happen most of the time.

2. It gets also updated when enumerating users/groups/services and sudo smart refresh if any object found by the enumeration have `entryUSN` > `srv_opts->last_usn`.

yes, but typically we do not recommend to enable enumeration, so you cannot rely on it.

So it does not necessarily corresponds to real value of lastUSN attribute but it contains greatest USN value known to SSSD.

@pbrezina

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

I see. So if enumeration is disabled we are stuck in the same situation as we are now.

@sumit-bose

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

I see. So if enumeration is disabled we are stuck in the same situation as we are now.

Yes, that's my finding as well. Using the patch for sdap_steal_server_opts() helps to mitigate this by refreshing last_usn at least on every reconnect. The alternative would be to read lastUSN from the rootDSE explicitly with every smart refresh.

@pbrezina

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

I think your diff is still quite a good improvement and we do not have to necessarily to send another ldap search for rootdse. Would you mind sending me the patch so I can include it in this PR?

@sumit-bose

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

@pbrezina

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

Thank you. Patch was included.

@sumit-bose

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

Thanks Pavel,

the patches work well in my testing. I'd like to ask to update your commit message a bit because "This patch makes sure that highest sudo usn number is always set to the highest server usn number after each refresh." is misleading, better would be something like "... to the highest usn number known by SSSD ...". Additionally I wonder if some details about how the highest USN is updated (reconnect, enumeration) can be added to the man pages entry for the smart refresh?

Not sure if we need another reviewer or if we can ACK each others patches?

bye,
Sumit

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

FWIW, personally I'm fine if you ack each other's patches. I tried to review them but it would take more time to understand the code around than I now have :-) so if you prefer, just go ahead and push.

@pbrezina pbrezina force-pushed the pbrezina:sudousn-update branch from b2758e6 to a43ce28 May 10, 2019

@pbrezina

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

I amended the commit message.

Additionally I wonder if some details about how the highest USN is updated (reconnect, enumeration) can be added to the man pages entry for the smart refresh?

In my opinion it is not needed. It can not be configured and it does not change the result. It is merely an optimization.

Ack to the Sumit's patch :-)

@sumit-bose

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

I amended the commit message.

Additionally I wonder if some details about how the highest USN is updated (reconnect, enumeration) can be added to the man pages entry for the smart refresh?

In my opinion it is not needed. It can not be configured and it does not change the result. It is merely an optimization.

I think it can be configured. Since the highest USN is only updated during reconnect or enumeration the related timeouts can be important here. I'm thinking of cases where ldap_sudo_smart_refresh_interval is lowered because this ticket make it sound safe to run smart refresh more often. But the logs will show that the USN in the search filter might be only updated every 15min after a reconnect.

Ack to the Sumit's patch :-)

pbrezina and others added some commits Apr 23, 2019

sudo: always use server highest known usn for smart refresh
The sudo attributes may not be indexed on the server, therefore if
smart refresh filter is run on the server it may first search using
the indexed entryusn attribute and run the rest of the filter on
non-sudo objects. The number of objects that are filtered may increased
dramatically if sudo rules are not changed for a long time (and thus
keeping smaller and smaller last sudo usn number).

This patch makes sure that highest sudo usn number is always set to
the highest server usn number known by SSSD after each refresh.

Resolves:
https://pagure.io/SSSD/sssd/issue/3997
sdap: update last_usn on reconnect
If we are reconnecting to the same server it makes sense the keep the
stored maximum USN values for the different object classes. But since
the highest USN is already read from the rootDSE during the reconnect it
make sense to keep this value to be able to update the maximum USN
values even of no new object where found during the related searches.

@pbrezina pbrezina force-pushed the pbrezina:sudousn-update branch from a43ce28 to b17949a May 21, 2019

@pbrezina

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

I attached new patch that updates documentation of ldap_sudo_smart_refresh_interval.

@sumit-bose

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

Thank you @pbrezina. ACK to your patches.

Given that all patches are now ACKed by somebody not being the author I'll switch the ticket state to Accepted.

bye,
Sumit

@sumit-bose sumit-bose added the Accepted label May 21, 2019

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

@jhrozek jhrozek closed this May 21, 2019

@jhrozek jhrozek added the Pushed label May 21, 2019

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

It's not clear to me whether you want to cherry-pick the patches to the stable branch or not..if yes, just please tell me.

(I was about to push the patches to sssd-1-16 as well, btu wasn't sure if this is more of a straight bug fix or a change of behaviour, even if the correct change)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.