Skip to content

pam: properly support UPN logon names#271

Closed
sumit-bose wants to merge 3 commits intoSSSD:masterfrom
sumit-bose:master
Closed

pam: properly support UPN logon names#271
sumit-bose wants to merge 3 commits intoSSSD:masterfrom
sumit-bose:master

Conversation

@sumit-bose
Copy link
Contributor

Many logon applications like /bin/login or sshd canonicalize the user
name before they call pam_start() and hence the UPN is not seen by
SSSD's pam responder. But some like e.g. gdm don't and authentication
might fail if a UPN is used.

The reason is that currently the already parsed short name of the user
was used in the cache_req and hence the cache_req was not able to fall
back to the UPN lookup code. This patch uses the name originally
provided by the user as input to allow the fallback to the UPN lookup.

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

@fidencio
Copy link
Contributor

@sumit-bose, the patch itself looks fine.
I'll do some tests on my side here and get back with a final ACK by Tomorrow (probably).

@fidencio fidencio self-assigned this May 15, 2017
@jhrozek
Copy link
Contributor

jhrozek commented May 22, 2017

retest this please

Many logon applications like /bin/login or sshd canonicalize the user
name before they call pam_start() and hence the UPN is not seen by
SSSD's pam responder. But some like e.g. gdm don't and authentication
might fail if a UPN is used.

The reason is that currently the already parsed short name of the user
was used in the cache_req and hence the cache_req was not able to fall
back to the UPN lookup code. This patch uses the name originally
provided by the user as input to allow the fallback to the UPN lookup.

Resolves https://pagure.io/SSSD/sssd/issue/3240
@sumit-bose
Copy link
Contributor Author

@fidencio found an issue with the patch while testing, cache_req_send() should not be called with the domain name to allow cache_req to properly split a fully-qualified name.

As a result a I found that the Initgroups by UPN does not use the right negative cache, this is fixed in 'cache_req: use the right negative cache for initgroups by upn'. @lslebodn, do we need a separate ticket/PR for this?

Finally I addded 'test: make sure p11_child is build for pam-srv-tests' so that 'make ./pam-srv-tests' can be called in a clean environment and the test will still pass.

@fidencio
Copy link
Contributor

@sumit-bose: Thanks for patches. This new version does work as expected.

IMO, you could just add "Related to: ..." in the cache_req patch and avoid a separate ticket/PR for this one.

ACK!

@jhrozek
Copy link
Contributor

jhrozek commented May 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants