[sssd PR#141][opened] PAM: Use cache_req_user_by_name_*()

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Author: fidencio
 Title: #141: PAM: Use cache_req_user_by_name_*()
Action: opened

PR body:
"""
PAM responder has been already using a lot from cache_req and one of the
missing parts (most likely the only one?) was that
pam_check_user_search() has been using sss_dp_get_account_send(), which
could be dropped (and a lot of code around it) in favour of using
cache_req_user_by_name_*() instead.

Resolves:
https://fedorahosted.org/sssd/ticket/1126

Signed-off-by: Fabiano Fidêncio 
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/141/head:pr141
git checkout pr141

[sssd PR#141][+Changes requested] PAM: Use cache_req_user_by_name_*()

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req_user_by_name_*()

Label: +Changes requested

[sssd PR#141][comment] PAM: Use cache_req_user_by_name_*()

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req_user_by_name_*()

fidencio commented:
"""
pam_srv tests also need to be changed.
"""

See the full comment at https://github.com/SSSD/sssd/pull/141#issuecomment-276496494

[sssd PR#141][synchronized] PAM: Use cache_req_user_by_name_*()

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Author: fidencio
 Title: #141: PAM: Use cache_req_user_by_name_*()
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/141/head:pr141
git checkout pr141

[sssd PR#141][edited] PAM: Use cache_req to perform initgroups lookups

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Author: fidencio
 Title: #141: PAM: Use cache_req to perform initgroups lookups
Action: edited

Changed field: title
Original value:
"""
PAM: Use cache_req_user_by_name_*()
"""

[sssd PR#141][edited] PAM: Use cache_req to perform initgroups lookups

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Author: fidencio
 Title: #141: PAM: Use cache_req to perform initgroups lookups
Action: edited

Changed field: body
Original value:
"""
PAM responder has been already using a lot from cache_req and one of the
missing parts (most likely the only one?) was that
pam_check_user_search() has been using sss_dp_get_account_send(), which
could be dropped (and a lot of code around it) in favour of using
cache_req_user_by_name_*() instead.

Resolves:
https://fedorahosted.org/sssd/ticket/1126

Signed-off-by: Fabiano Fidêncio 
"""

[sssd PR#141][-Changes requested] PAM: Use cache_req to perform initgroups lookups

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

Label: -Changes requested

[sssd PR#141][comment] PAM: Use cache_req to perform initgroups lookups

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

fidencio commented:
"""
@pbrezina, commits are ready to be reviewed. @spbnick, you're not (totally) blocked anymore.
"""

See the full comment at https://github.com/SSSD/sssd/pull/141#issuecomment-277919987

[sssd PR#141][comment] PAM: Use cache_req to perform initgroups lookups

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

fidencio commented:
"""
CI: http://sssd-ci.duckdns.org/logs/job/61/72/summary.html
"""

See the full comment at https://github.com/SSSD/sssd/pull/141#issuecomment-277927439

[sssd PR#141][comment] PAM: Use cache_req to perform initgroups lookups

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

pbrezina commented:
"""
Ack from me. Can someone else do review as well since I was helping Fabiano with those patches?
"""

See the full comment at https://github.com/SSSD/sssd/pull/141#issuecomment-277975708

[sssd PR#141][synchronized] PAM: Use cache_req to perform initgroups lookups

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Author: fidencio
 Title: #141: PAM: Use cache_req to perform initgroups lookups
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/141/head:pr141
git checkout pr141

[sssd PR#141][comment] PAM: Use cache_req to perform initgroups lookups

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

fidencio commented:
"""
Patch set has been rebased on top of git master.
"""

See the full comment at https://github.com/SSSD/sssd/pull/141#issuecomment-279357985

[sssd PR#141][comment] PAM: Use cache_req to perform initgroups lookups

[spbnick]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

spbnick commented:
"""
I'll try to review this.
"""

See the full comment at https://github.com/SSSD/sssd/pull/141#issuecomment-279670819

[sssd PR#141][+Changes requested] PAM: Use cache_req to perform initgroups lookups

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

Label: +Changes requested

[sssd PR#141][comment] PAM: Use cache_req to perform initgroups lookups

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

fidencio commented:
"""
@spbnick: Thanks for the review.
I'll submit a new version when someone else does the review as well (yeah, laziness, sorry).

About rephrasing the commit message, do you have any suggestion?
"""

See the full comment at https://github.com/SSSD/sssd/pull/141#issuecomment-281001450

[sssd PR#141][comment] PAM: Use cache_req to perform initgroups lookups

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

pbrezina commented:
"""
Try to describe how PAM responder works with cache and where it differes with other responders in few words. If you have time, do it now before next review. You want to do it anyway and typing `git push` is not that hard in the end ;-)
"""

See the full comment at https://github.com/SSSD/sssd/pull/141#issuecomment-281072688

[sssd PR#141][comment] PAM: Use cache_req to perform initgroups lookups

[spbnick]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

spbnick commented:
"""
@fidencio Sorry, I was able to understand very little from the comment, so I can't really suggest a better commit message.
"""

See the full comment at https://github.com/SSSD/sssd/pull/141#issuecomment-281073850

[sssd PR#141][comment] PAM: Use cache_req to perform initgroups lookups

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

sumit-bose commented:
"""
I've checked and tested the patches and all worked well. So ACK and if spbnick's comments are addressed the patches can be pushed.

While testing UPN/email/Kerberos alias login names I came across a general issue with UPN related cache_req lookup  https://fedorahosted.org/sssd/ticket/3313 . This is unrelated to this patch set, I junst want to mention that testing the patches with UPNs might fail.
"""

See the full comment at https://github.com/SSSD/sssd/pull/141#issuecomment-281092132

[sssd PR#141][synchronized] PAM: Use cache_req to perform initgroups lookups

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Author: fidencio
 Title: #141: PAM: Use cache_req to perform initgroups lookups
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/141/head:pr141
git checkout pr141

[sssd PR#141][-Changes requested] PAM: Use cache_req to perform initgroups lookups

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

Label: -Changes requested

[sssd PR#141][comment] PAM: Use cache_req to perform initgroups lookups

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

jhrozek commented:
"""
@fidencio , can you rebase this branch atop Sumit's PKINIT patches so we can merge it?
"""

See the full comment at https://github.com/SSSD/sssd/pull/141#issuecomment-281947395

[sssd PR#141][+Changes requested] PAM: Use cache_req to perform initgroups lookups

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

Label: +Changes requested

[sssd PR#141][comment] PAM: Use cache_req to perform initgroups lookups

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

fidencio commented:
"""
@jhrozek: I'm doing that, but it's going to take some time and I do believe patches will have to be reviewed again (as more changes are being done in order to have the tests passing ...)
"""

See the full comment at https://github.com/SSSD/sssd/pull/141#issuecomment-281972197

[sssd PR#141][comment] PAM: Use cache_req to perform initgroups lookups

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

fidencio commented:
"""
Big thanks to @sumit-bose who helped me with the tests not passing due to the rebase and for pointing out that a whole piece of code that could also be dropped as part of the "PAM: Use cache_req to perform initgroups lookups" patch.

I'm updating this new series here and I'd highly recommend to have this patchset re-tested by the reviewers.

"""

See the full comment at https://github.com/SSSD/sssd/pull/141#issuecomment-282144466

[sssd PR#141][-Changes requested] PAM: Use cache_req to perform initgroups lookups

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

Label: -Changes requested

[sssd PR#141][synchronized] PAM: Use cache_req to perform initgroups lookups

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Author: fidencio
 Title: #141: PAM: Use cache_req to perform initgroups lookups
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/141/head:pr141
git checkout pr141

[sssd PR#141][comment] PAM: Use cache_req to perform initgroups lookups

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

fidencio commented:
"""
CI: http://sssd-ci.duckdns.org/logs/job/63/43/summary.html
"""

See the full comment at https://github.com/SSSD/sssd/pull/141#issuecomment-282157874

[sssd PR#141][comment] PAM: Use cache_req to perform initgroups lookups

[spbnick]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

spbnick commented:
"""
All my comments were addressed, ACK on my points. Thanks, @fidencio!
"""

See the full comment at https://github.com/SSSD/sssd/pull/141#issuecomment-282267591

[sssd PR#141][synchronized] PAM: Use cache_req to perform initgroups lookups

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Author: fidencio
 Title: #141: PAM: Use cache_req to perform initgroups lookups
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/141/head:pr141
git checkout pr141

[sssd PR#141][comment] PAM: Use cache_req to perform initgroups lookups

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

jhrozek commented:
"""
I'm afraid Coverity found something it doesn't like now:
````
Error: NULL_RETURNS (CWE-476):
sssd-1.15.1/src/responder/pam/pamsrv_cmd.c:1462: returned_null: "cache_req_data_name" returns null (checked 9 out of 10 times).
sssd-1.15.1/src/responder/common/cache_req/cache_req_data.c:250:5: null_return: Calling "cache_req_data_create" which might return null.
sssd-1.15.1/src/responder/common/cache_req/cache_req_data.c:78:9: return_null: Explicitly returning null.
sssd-1.15.1/src/responder/common/cache_req/cache_req_data.c:250:5: return_null_fn: Returning the return value of "cache_req_data_create", which might be null.
sssd-1.15.1/src/responder/common/cache_req/plugins/cache_req_group_by_filter.c:148: example_assign: Example 1: Assigning: "data" = return value from "cache_req_data_name(mem_ctx, CACHE_REQ_GROUP_BY_FILTER, filter)".
sssd-1.15.1/src/responder/common/cache_req/plugins/cache_req_group_by_filter.c:149: example_checked: Example 1 (cont.): "data" has its value checked in "data == NULL".
sssd-1.15.1/src/responder/common/cache_req/plugins/cache_req_group_by_name.c:213: example_assign: Example 2: Assigning: "data" = return value from "cache_req_data_name(mem_ctx, CACHE_REQ_GROUP_BY_NAME, name)".
sssd-1.15.1/src/responder/common/cache_req/plugins/cache_req_group_by_name.c:214: example_checked: Example 2 (cont.): "data" has its value checked in "data == NULL".
sssd-1.15.1/src/responder/common/cache_req/plugins/cache_req_initgroups_by_name.c:228: example_assign: Example 3: Assigning: "data" = return value from "cache_req_data_name(mem_ctx, CACHE_REQ_INITGROUPS, name)".
sssd-1.15.1/src/responder/common/cache_req/plugins/cache_req_initgroups_by_name.c:229: example_checked: Example 3 (cont.): "data" has its value checked in "data == NULL".
sssd-1.15.1/src/responder/common/cache_req/plugins/cache_req_netgroup_by_name.c:147: example_assign: Example 4: Assigning: "data" = return value from "cache_req_data_name(mem_ctx, CACHE_REQ_NETGROUP_BY_NAME, name)".
sssd-1.15.1/src/responder/common/cache_req/plugins/cache_req_netgroup_by_name.c:148: example_checked: Example 4 (cont.): "data" has its value checked in "data == NULL".
sssd-1.15.1/src/responder/common/cache_req/plugins/cache_req_user_by_filter.c:148: example_assign: Example 5: Assigning: "data" = return value from "cache_req_data_name(mem_ctx, CACHE_REQ_USER_BY_FILTER, filter)".
sssd-1.15.1/src/responder/common/cache_req/plugins/cache_req_user_by_filter.c:149: example_checked: Example 5 (cont.): "data" has its value checked in "data == NULL".
sssd-1.15.1/src/responder/pam/pamsrv_cmd.c:1462: var_assigned: Assigning: "data" = null return value from "cache_req_data_name".
sssd-1.15.1/src/responder/pam/pamsrv_cmd.c:1475: dereference: Dereferencing a pointer that might be null "data" when calling "cache_req_data_set_bypass_cache".
sssd-1.15.1/src/responder/common/cache_req/cache_req_data.c:365:5: deref_parm: Directly dereferencing parameter "data".
#  363|                                   bool bypass_cache)
#  364|   {
#  365|->     data->bypass_cache = bypass_cache;
#  366|   }
````

"""

See the full comment at https://github.com/SSSD/sssd/pull/141#issuecomment-283039533

[sssd PR#141][+Changes requested] PAM: Use cache_req to perform initgroups lookups

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

Label: +Changes requested

[sssd PR#141][synchronized] PAM: Use cache_req to perform initgroups lookups

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Author: fidencio
 Title: #141: PAM: Use cache_req to perform initgroups lookups
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/141/head:pr141
git checkout pr141

[sssd PR#141][-Changes requested] PAM: Use cache_req to perform initgroups lookups

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

Label: -Changes requested

[sssd PR#141][comment] PAM: Use cache_req to perform initgroups lookups

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

fidencio commented:
"""
New patch set pushed.
"""

See the full comment at https://github.com/SSSD/sssd/pull/141#issuecomment-283163599

[sssd PR#141][synchronized] PAM: Use cache_req to perform initgroups lookups

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Author: fidencio
 Title: #141: PAM: Use cache_req to perform initgroups lookups
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/141/head:pr141
git checkout pr141

[sssd PR#141][synchronized] PAM: Use cache_req to perform initgroups lookups

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Author: fidencio
 Title: #141: PAM: Use cache_req to perform initgroups lookups
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/141/head:pr141
git checkout pr141

[sssd PR#141][comment] PAM: Use cache_req to perform initgroups lookups

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

jhrozek commented:
"""
CI: http://sssd-ci.duckdns.org/logs/job/63/64/summary.html

(yes, I know we need to squash patches later and I'm running some manual tests, but so far so good)
"""

See the full comment at https://github.com/SSSD/sssd/pull/141#issuecomment-283624071

[sssd PR#141][comment] PAM: Use cache_req to perform initgroups lookups

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

jhrozek commented:
"""
retest this please
"""

See the full comment at https://github.com/SSSD/sssd/pull/141#issuecomment-283624101

[sssd PR#141][comment] PAM: Use cache_req to perform initgroups lookups

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

jhrozek commented:
"""
(this was for Jenkins, I don't know why the centos CI failed, so I'm just re-running it)
"""

See the full comment at https://github.com/SSSD/sssd/pull/141#issuecomment-283624181

[sssd PR#141][comment] PAM: Use cache_req to perform initgroups lookups

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

jhrozek commented:
"""
I found one bug in the patches that I pushed into my branch: https://github.com/jhrozek/sssd/tree/pam_cache_req

@fidencio if you agree, please squash it in, squash the two fixups and resubmit the patches.

There were also two unrelated bugs I found, one is in PR#179 and one in PR#176
"""

See the full comment at https://github.com/SSSD/sssd/pull/141#issuecomment-283714150

[sssd PR#141][+Changes requested] PAM: Use cache_req to perform initgroups lookups

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

Label: +Changes requested

[sssd PR#141][synchronized] PAM: Use cache_req to perform initgroups lookups

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Author: fidencio
 Title: #141: PAM: Use cache_req to perform initgroups lookups
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/141/head:pr141
git checkout pr141

[sssd PR#141][comment] PAM: Use cache_req to perform initgroups lookups

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

fidencio commented:
"""
@jhrozek: done!
"""

See the full comment at https://github.com/SSSD/sssd/pull/141#issuecomment-283803546

[sssd PR#141][-Changes requested] PAM: Use cache_req to perform initgroups lookups

[fidencio]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

Label: -Changes requested

[sssd PR#141][comment] PAM: Use cache_req to perform initgroups lookups

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

jhrozek commented:
"""
ACK

CI: http://sssd-ci.duckdns.org/logs/job/63/74/summary.html
"""

See the full comment at https://github.com/SSSD/sssd/pull/141#issuecomment-283935188

[sssd PR#141][+Accepted] PAM: Use cache_req to perform initgroups lookups

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

Label: +Accepted

[sssd PR#141][comment] PAM: Use cache_req to perform initgroups lookups

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

jhrozek commented:
"""
master:
 * c99bcc91e58988ff37403d362d4322469413c2ff
 * 5aaaf081765b3f23e1518b5f299c289afb9d3f13
 * ed891c0c55985cd25de05f65e82debf4452987e1
 * 189db5329db0277425bb3bdd785c19aee23bb364
"""

See the full comment at https://github.com/SSSD/sssd/pull/141#issuecomment-283946815

[sssd PR#141][closed] PAM: Use cache_req to perform initgroups lookups

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/141
Author: fidencio
 Title: #141: PAM: Use cache_req to perform initgroups lookups
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/141/head:pr141
git checkout pr141

[sssd PR#141][+Pushed] PAM: Use cache_req to perform initgroups lookups

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

Label: +Pushed

[sssd PR#141][-Accepted] PAM: Use cache_req to perform initgroups lookups

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

Label: -Accepted

[sssd PR#141][+Accepted] PAM: Use cache_req to perform initgroups lookups

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/141
Title: #141: PAM: Use cache_req to perform initgroups lookups

Label: +Accepted