ldap: update shadow last change in sysdb as well

[pbrezina]

Otherwise pam can use the changed information whe id chaching is
enabled, so next authentication that fits into the id timeout
(5 seconds by default) will still sees the password as expired.

Resolves: #6477

[sumit-bose]

Hi,

I was thinking if it would be better to force a refresh in pam_check_user_search_next() if the entry is expired. Especially since the sssd.conf man page currently says for pam_initgroups_scheme never "Never force an online lookup, use the data from the cache as long as they are not expired".

But I guess the intention of the PAM initgroups cache and pam_initgroups_scheme is to really use cached data and skip the online lookup even if the entry is expired (the man page entry should be fixed in this case). What do you think?

bye,
Sumit

[pbrezina]

Hi,

I was thinking if it would be better to force a refresh in pam_check_user_search_next() if the entry is expired. Especially since the sssd.conf man page currently says for pam_initgroups_scheme never "Never force an online lookup, use the data from the cache as long as they are not expired".

Hmm, that is a good idea.

But I guess the intention of the PAM initgroups cache and pam_initgroups_scheme is to really use cached data and skip the online lookup even if the entry is expired (the man page entry should be fixed in this case). What do you think?

It depends. Unless someone sets entry_cache_timeout to really low value, which probably does not happen in production, it makes sense to refresh an expired entry. But I don't precisely remember why we needed to implement pam_initgroups_schema. Since you are the author, you can make a better call.

bye, Sumit

[pbrezina]

However, man page change will not fit the release unless it is postponed to next week.

[sumit-bose]

Hi,
I was thinking if it would be better to force a refresh in pam_check_user_search_next() if the entry is expired. Especially since the sssd.conf man page currently says for pam_initgroups_scheme never "Never force an online lookup, use the data from the cache as long as they are not expired".

Hmm, that is a good idea.

But I guess the intention of the PAM initgroups cache and pam_initgroups_scheme is to really use cached data and skip the online lookup even if the entry is expired (the man page entry should be fixed in this case). What do you think?

It depends. Unless someone sets entry_cache_timeout to really low value, which probably does not happen in production, it makes sense to refresh an expired entry. But I don't precisely remember why we needed to implement pam_initgroups_schema. Since you are the author, you can make a better call.

Hi,

pam_initgroups_schema was implemented to switch the default to the no_session behavior so that e.g. sudo does not require a refresh but can work from cache. My concern was that users might miss the old/original always so the option was added. While implementing it I added never as a third option. So the original behavior was a mix, the initgroups cache didn't care about expired entries and if the entry was not in the cache if was refreshed unconditionally.

I can't remember the details but I guess while implementing it I didn't thought much about expired entry and just assumed that cache_req will refresh expired entries.

Maybe it would make sense to open a new ticket to make more time to evaluate this and make a note in the new ticket that depending on the outcome this patch here might be reversed since it wouldn't be needed anymore. I 'll ACK this patch.

bye,
Sumit

bye, Sumit

[pbrezina]

cache_req is first called with bypass_dp = true so backend is not contacted at all. We could add boolean to struct cache_req_result to indicate if the object returned from cache_req was expired or not. Although adding new things to cache_reqs is no longer fun :-)

[pbrezina]

This did not fit into the release as it lacks the second ack.

[thalman]

Codewise it looks good. The commit message has a typo

whe id chaching

could you please fix the message?

Thanks

[thalman]

We are a bit on hurry with this PR, so let's proceed with typo in commit message.

[alexey-tikhonov]

Pushed PR: #6478

• master
  • 7e8b97c - ldap: update shadow last change in sysdb as well
• sssd-2-8
  • d7da296 - ldap: update shadow last change in sysdb as well