URL: https://github.com/SSSD/sssd/pull/806 Author: pbrezina Title: #806: sudo: always use server highest usn for smart refresh Action: opened
PR body: """ 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 """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/806/head:pr806 git checkout pr806
URL: https://github.com/SSSD/sssd/pull/806 Title: #806: sudo: always use server highest usn for smart refresh
sumit-bose commented: """ 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 """
See the full comment at https://github.com/SSSD/sssd/pull/806#issuecomment-488576529
URL: https://github.com/SSSD/sssd/pull/806 Title: #806: sudo: always use server highest usn for smart refresh
pbrezina commented: """ #### Old code `srv_opts->max_sudo_value` contains the largest usn value from found sudo rule (i.e. possibly from ou=sudoers subtree).
```c {.line-numbers} 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 retuned in the search result.
```c {.line-numbers} 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; ``` """
See the full comment at https://github.com/SSSD/sssd/pull/806#issuecomment-488623227
URL: https://github.com/SSSD/sssd/pull/806 Title: #806: sudo: always use server highest usn for smart refresh
pbrezina commented: """ #### Old code `srv_opts->max_sudo_value` contains the largest usn value from found sudo rule (i.e. possibly from ou=sudoers subtree).
```c {.line-numbers} 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.
```c {.line-numbers} 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; ``` """
See the full comment at https://github.com/SSSD/sssd/pull/806#issuecomment-488623227
URL: https://github.com/SSSD/sssd/pull/806 Title: #806: sudo: always use server highest usn for smart refresh
pbrezina commented: """ 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. """
See the full comment at https://github.com/SSSD/sssd/pull/806#issuecomment-488623909
URL: https://github.com/SSSD/sssd/pull/806 Title: #806: sudo: always use server highest usn for smart refresh
sumit-bose commented: """
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 """
See the full comment at https://github.com/SSSD/sssd/pull/806#issuecomment-489157607
URL: https://github.com/SSSD/sssd/pull/806 Title: #806: sudo: always use server highest usn for smart refresh
pbrezina commented: """ 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.
"""
See the full comment at https://github.com/SSSD/sssd/pull/806#issuecomment-489600986
URL: https://github.com/SSSD/sssd/pull/806 Title: #806: sudo: always use server highest usn for smart refresh
sumit-bose commented: """
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.
"""
See the full comment at https://github.com/SSSD/sssd/pull/806#issuecomment-489603482
URL: https://github.com/SSSD/sssd/pull/806 Title: #806: sudo: always use server highest usn for smart refresh
pbrezina commented: """ I see. So if enumeration is disabled we are stuck in the same situation as we are now. """
See the full comment at https://github.com/SSSD/sssd/pull/806#issuecomment-489607714
URL: https://github.com/SSSD/sssd/pull/806 Title: #806: sudo: always use server highest usn for smart refresh
sumit-bose commented: """
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. """
See the full comment at https://github.com/SSSD/sssd/pull/806#issuecomment-489967231
URL: https://github.com/SSSD/sssd/pull/806 Title: #806: sudo: always use server highest usn for smart refresh
pbrezina commented: """ 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? """
See the full comment at https://github.com/SSSD/sssd/pull/806#issuecomment-489982837
URL: https://github.com/SSSD/sssd/pull/806 Title: #806: sudo: always use server highest usn for smart refresh
sumit-bose commented: """ Here you are:
[0001-sdap-update-last_usn-on-reconnect.patch.gz](https://github.com/SSSD/sssd/files/3151831/0001-sdap-update-last_usn-on-reco...)
"""
See the full comment at https://github.com/SSSD/sssd/pull/806#issuecomment-490006618
URL: https://github.com/SSSD/sssd/pull/806 Author: pbrezina Title: #806: sudo: always use server highest usn for smart refresh Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/806/head:pr806 git checkout pr806
URL: https://github.com/SSSD/sssd/pull/806 Title: #806: sudo: always use server highest usn for smart refresh
pbrezina commented: """ Thank you. Patch was included. """
See the full comment at https://github.com/SSSD/sssd/pull/806#issuecomment-490828336
URL: https://github.com/SSSD/sssd/pull/806 Title: #806: sudo: always use server highest usn for smart refresh
sumit-bose commented: """ 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 """
See the full comment at https://github.com/SSSD/sssd/pull/806#issuecomment-490979706
URL: https://github.com/SSSD/sssd/pull/806 Title: #806: sudo: always use server highest usn for smart refresh
jhrozek commented: """ 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. """
See the full comment at https://github.com/SSSD/sssd/pull/806#issuecomment-491038321
URL: https://github.com/SSSD/sssd/pull/806 Author: pbrezina Title: #806: sudo: always use server highest usn for smart refresh Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/806/head:pr806 git checkout pr806
URL: https://github.com/SSSD/sssd/pull/806 Title: #806: sudo: always use server highest usn for smart refresh
pbrezina commented: """ 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 :-) """
See the full comment at https://github.com/SSSD/sssd/pull/806#issuecomment-491241262
URL: https://github.com/SSSD/sssd/pull/806 Title: #806: sudo: always use server highest usn for smart refresh
sumit-bose commented: """
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 :-)
"""
See the full comment at https://github.com/SSSD/sssd/pull/806#issuecomment-492176604
URL: https://github.com/SSSD/sssd/pull/806 Author: pbrezina Title: #806: sudo: always use server highest usn for smart refresh Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/806/head:pr806 git checkout pr806
URL: https://github.com/SSSD/sssd/pull/806 Title: #806: sudo: always use server highest usn for smart refresh
pbrezina commented: """ I attached new patch that updates documentation of ldap_sudo_smart_refresh_interval. """
See the full comment at https://github.com/SSSD/sssd/pull/806#issuecomment-494293107
URL: https://github.com/SSSD/sssd/pull/806 Title: #806: sudo: always use server highest usn for smart refresh
sumit-bose commented: """ 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 """
See the full comment at https://github.com/SSSD/sssd/pull/806#issuecomment-494309653
URL: https://github.com/SSSD/sssd/pull/806 Title: #806: sudo: always use server highest usn for smart refresh
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/806 Title: #806: sudo: always use server highest usn for smart refresh
jhrozek commented: """ * master: * f8bae064ebbc038616fb45e99cf02a996730e866 * f1ce524ed0a623ce131830f122fbce5abd39ec45 * 819d70ef6e6fa0e736ebd60a7f8a26f672927d57 """
See the full comment at https://github.com/SSSD/sssd/pull/806#issuecomment-494537046
URL: https://github.com/SSSD/sssd/pull/806 Author: pbrezina Title: #806: sudo: always use server highest usn for smart refresh Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/806/head:pr806 git checkout pr806
URL: https://github.com/SSSD/sssd/pull/806 Title: #806: sudo: always use server highest usn for smart refresh
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/806 Title: #806: sudo: always use server highest usn for smart refresh
jhrozek commented: """ 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) """
See the full comment at https://github.com/SSSD/sssd/pull/806#issuecomment-494538048
sssd-devel@lists.fedorahosted.org
-
jhrozek -
pbrezina
-
sumit-bose