Using cache_req code for PAM responder

[Fabiano Fidêncio]

I've done a first WIP patch for this matter but Jakub pointed out the
approach is not correct as the PAM doesn't use the cache the same way
as other responders do.

Differently from the other responders, PAM tries to conatct the Data
Provider on almost every request.

Looking at the code, what's done is:
- While looping the domains in pam_check_user_search():
  - call pam_initgr_check_timeout()
    - in case the timeout is still valid:
      - get the entry from sysdb
    - otherwise
      - call the data provider first

As the using cache_req code for PAM responder has two main goals
(decrease code duplicaton and make it possible to log in with a
shortname to a trusted domain) Jakub suggested to, maybe write a new
cache_req plugin (specifically for PAM?) and decrease the number of
duplicated code by just reusing this new code from cache_req.

The main reason behind his idea is that he thinks we want to keep the
pam_initgr_check_timeout() while looping the domains in the cache_req
code.

So, as I'm not that much familiar with none of those two pieces of
code ... I'd like to know what's Pavel Březina opinion on these ideas.

Best Regards,
--
Fabiano Fidêncio

[Pavel Březina]

On 02/01/2017 10:39 AM, Fabiano Fidêncio wrote:
> I've done a first WIP patch for this matter but Jakub pointed out the
> approach is not correct as the PAM doesn't use the cache the same way
> as other responders do.

The patch uses wrong function, please use 
cache_req_initgroups_by_name(). (I'm sorry I didn't notice this earlier, 
I really barely read it in the private mail).
> Differently from the other responders, PAM tries to conatct the Data
> Provider on almost every request.
>
> Looking at the code, what's done is:
> While looping the domains in pam_check_user_search():
> call pam_initgr_check_timeout()
> in case the timeout is still valid:
> get the entry from sysdb
> otherwise
> call the data provider first

The timeout is there to ensure that we don't go to id provider too often 
(default 5 seconds) since PAM prefer to always go there and does not 
obey entry expiration time.

The whole point of this is to perform initgroups to refresh user's 
membership, we are not interested in the result.

The most generic solution here is to give cache_req the ability 
(parameter) to choose whether we wan't to go to data provider first and 
ignore expiration time and make the caller responsible for this decision.

The seconds solution is to create new initgroups plugin for PAM with 
bypass_cache = true and decide in PAM if it is called or not.
> As the using cache_req code for PAM responder has two main goals
> (decrease code duplicaton and make it possible to log in with a
> shortname to a trusted domain) Jakub suggested to, maybe write a new
> cache_req plugin (specifically for PAM?) and decrease the number of
> duplicated code by just reusing this new code from cache_req.
>
> The main reason behind his idea is that he thinks we want to keep the
> pam_initgr_check_timeout() while looping the domains in the cache_req
> code.
>
> So, as I'm not that much familiar with none of those two pieces of
> code ... I'd like to know what's Pavel Březina opinion on these ideas.
> Best Regards,
>
> Fabiano Fidêncio

[Pavel Březina]

On 02/01/2017 11:15 AM, Pavel Březina wrote:
> On 02/01/2017 10:39 AM, Fabiano Fidêncio wrote:
> > I've done a first WIP patch for this matter but Jakub pointed out the
> > approach is not correct as the PAM doesn't use the cache the same way
> > as other responders do.
>
> The patch uses wrong function, please use
> cache_req_initgroups_by_name(). (I'm sorry I didn't notice this earlier,
> I really barely read it in the private mail).
> > Differently from the other responders, PAM tries to conatct the Data
> > Provider on almost every request.
> >
> > Looking at the code, what's done is:
> > While looping the domains in pam_check_user_search():
> > call pam_initgr_check_timeout()
> > in case the timeout is still valid:
> > get the entry from sysdb
> > otherwise
> > call the data provider first
>
> The timeout is there to ensure that we don't go to id provider too often
> (default 5 seconds) since PAM prefer to always go there and does not
> obey entry expiration time.
>
> The whole point of this is to perform initgroups to refresh user's
> membership, we are not interested in the result.

I looked at wrong variable, we use user's name from the result since 
login name might be alias or upn. Therefore this is the solution.
> The most generic solution here is to give cache_req the ability
> (parameter) to choose whether we wan't to go to data provider first and
> ignore expiration time and make the caller responsible for this decision.
>
> The seconds solution is to create new initgroups plugin for PAM with
> bypass_cache = true and decide in PAM if it is called or not.
> > As the using cache_req code for PAM responder has two main goals
> > (decrease code duplicaton and make it possible to log in with a
> > shortname to a trusted domain) Jakub suggested to, maybe write a new
> > cache_req plugin (specifically for PAM?) and decrease the number of
> > duplicated code by just reusing this new code from cache_req.
> >
> > The main reason behind his idea is that he thinks we want to keep the
> > pam_initgr_check_timeout() while looping the domains in the cache_req
> > code.
> >
> > So, as I'm not that much familiar with none of those two pieces of
> > code ... I'd like to know what's Pavel Březina opinion on these ideas.
> > Best Regards,
> >
> > Fabiano Fidêncio
>
> sssd-devel mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]

[Sumit Bose]

On Wed, Feb 01, 2017 at 10:39:07AM +0100, Fabiano Fidêncio wrote:
> I've done a first WIP patch for this matter but Jakub pointed out the
> approach is not correct as the PAM doesn't use the cache the same way
> as other responders do.
>
> Differently from the other responders, PAM tries to conatct the Data
> Provider on almost every request.
>
> Looking at the code, what's done is:
> While looping the domains in pam_check_user_search():
> call pam_initgr_check_timeout()
> in case the timeout is still valid:
> get the entry from sysdb
> otherwise
> call the data provider first
>
> As the using cache_req code for PAM responder has two main goals
> (decrease code duplicaton and make it possible to log in with a
> shortname to a trusted domain) Jakub suggested to, maybe write a new
> cache_req plugin (specifically for PAM?) and decrease the number of
> duplicated code by just reusing this new code from cache_req.
>
> The main reason behind his idea is that he thinks we want to keep the
> pam_initgr_check_timeout() while looping the domains in the cache_req
> code.

Would it be possible to manage the hash table used by
pam_initgr_check_timeout() and friends outside of the cache_req code and
add some new _FORCE cache_req types like CACHE_REQ_INITGROUPS_FORCE
which can be used by the PAM responder if the given user in not in hash
table anymore?

bye,
Sumit
> So, as I'm not that much familiar with none of those two pieces of
> code ... I'd like to know what's Pavel Březina opinion on these ideas.
> Best Regards,
>
> Fabiano Fidêncio
> _______________________________________________
> sssd-devel mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]

[Pavel Březina]

On 02/01/2017 11:41 AM, Sumit Bose wrote:
> On Wed, Feb 01, 2017 at 10:39:07AM +0100, Fabiano Fidêncio wrote:
> > I've done a first WIP patch for this matter but Jakub pointed out the
> > approach is not correct as the PAM doesn't use the cache the same way
> > as other responders do.
> >
> > Differently from the other responders, PAM tries to conatct the Data
> > Provider on almost every request.
> >
> > Looking at the code, what's done is:
> > While looping the domains in pam_check_user_search():
> > call pam_initgr_check_timeout()
> > in case the timeout is still valid:
> > get the entry from sysdb
> > otherwise
> > call the data provider first
> >
> > As the using cache_req code for PAM responder has two main goals
> > (decrease code duplicaton and make it possible to log in with a
> > shortname to a trusted domain) Jakub suggested to, maybe write a new
> > cache_req plugin (specifically for PAM?) and decrease the number of
> > duplicated code by just reusing this new code from cache_req.
> >
> > The main reason behind his idea is that he thinks we want to keep the
> > pam_initgr_check_timeout() while looping the domains in the cache_req
> > code.
>
> Would it be possible to manage the hash table used by
> pam_initgr_check_timeout() and friends outside of the cache_req code and
> add some new _FORCE cache_req types like CACHE_REQ_INITGROUPS_FORCE
> which can be used by the PAM responder if the given user in not in hash
> table anymore?

What I have in mind:

Prerequisites:
1) PAM manages the hash table with timeouts it self, no change here. It 
uses logon name as key.
2) Cache_req has the ability to talk to dp first per caller decision 
(already there as plugin decision)

Changes in cache_req:
1) Add bypass_cache parameter into cache_req_data and create a setter 
for it.
2) Alter cache_req_search_send:281
if (!cr->plugin->bypass_cache) { --> if (!cr->plugin->bypass_cache && 
!cr->data->bypass_cache) {

Changes in PAM:
1) Call cache_req initgroups
2) If timeout is not set, set data->bypass_cache = true (talk dp first)
3) If timeout is set, set data->bypass_cache = false (search cache first)
> bye,
> Sumit
> > So, as I'm not that much familiar with none of those two pieces of
> > code ... I'd like to know what's Pavel Březina opinion on these ideas.
> > Best Regards,
> >
> > Fabiano Fidêncio
> > _______________________________________________
> > sssd-devel mailing list -- [e-mail]
> > To unsubscribe send an email to [e-mail]
>
> sssd-devel mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]

[Sumit Bose]

On Wed, Feb 01, 2017 at 11:57:14AM +0100, Pavel Březina wrote:
> On 02/01/2017 11:41 AM, Sumit Bose wrote:
> > On Wed, Feb 01, 2017 at 10:39:07AM +0100, Fabiano Fidêncio wrote:
> > > I've done a first WIP patch for this matter but Jakub pointed out the
> > > approach is not correct as the PAM doesn't use the cache the same way
> > > as other responders do.
> > >
> > > Differently from the other responders, PAM tries to conatct the Data
> > > Provider on almost every request.
> > >
> > > Looking at the code, what's done is:
> > > While looping the domains in pam_check_user_search():
> > > call pam_initgr_check_timeout()
> > > in case the timeout is still valid:
> > > get the entry from sysdb
> > > otherwise
> > > call the data provider first
> > >
> > > As the using cache_req code for PAM responder has two main goals
> > > (decrease code duplicaton and make it possible to log in with a
> > > shortname to a trusted domain) Jakub suggested to, maybe write a new
> > > cache_req plugin (specifically for PAM?) and decrease the number of
> > > duplicated code by just reusing this new code from cache_req.
> > >
> > > The main reason behind his idea is that he thinks we want to keep the
> > > pam_initgr_check_timeout() while looping the domains in the cache_req
> > > code.
> >
> > Would it be possible to manage the hash table used by
> > pam_initgr_check_timeout() and friends outside of the cache_req code and
> > add some new _FORCE cache_req types like CACHE_REQ_INITGROUPS_FORCE
> > which can be used by the PAM responder if the given user in not in hash
> > table anymore?
>
> What I have in mind:
>
> Prerequisites:
> PAM manages the hash table with timeouts it self, no change here. It uses
>
> logon name as key.
> 2) Cache_req has the ability to talk to dp first per caller decision
> (already there as plugin decision)
>
> Changes in cache_req:
> Add bypass_cache parameter into cache_req_data and create a setter for
>
> it.
> 2) Alter cache_req_search_send:281
> if (!cr->plugin->bypass_cache) { --> if (!cr->plugin->bypass_cache &&
> !cr->data->bypass_cache) {
>
> Changes in PAM:
> Call cache_req initgroups
> If timeout is not set, set data->bypass_cache = true (talk dp first)
> If timeout is set, set data->bypass_cache = false (search cache first)

This is in agreement to what I had in mind. I just wasn't sure what
would be the best way to tell cache_req to talk directly to the backend.

bye,
Sumit
> > bye,
> > Sumit
> > > So, as I'm not that much familiar with none of those two pieces of
> > > code ... I'd like to know what's Pavel Březina opinion on these ideas.
> > > Best Regards,
> > >
> > > Fabiano Fidêncio
> > > _______________________________________________
> > > sssd-devel mailing list -- [e-mail]
> > > To unsubscribe send an email to [e-mail]
> >
> > sssd-devel mailing list -- [e-mail]
> > To unsubscribe send an email to [e-mail]
>
> sssd-devel mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]