Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update verify_user_pam.c #2547

Closed
wants to merge 1 commit into from
Closed

Conversation

yifanjiang
Copy link
Contributor

Search the pam vendor directory /usr/lib/pam.d. See also: https://bugzilla.opensuse.org/show_bug.cgi?id=1208121

Search the pam vendor directory /usr/lib/pam.d. See also:
https://bugzilla.opensuse.org/show_bug.cgi?id=1208121
@matt335672
Copy link
Member

Hi @yifanjiang

I don't think this repository is the best place for this patch.

Our own install scripts hard-write the installation path for the PAM file in instfiles/pam.d:-

pamddir = $(sysconfdir)/pam.d

so that's what we check.

If we accept this patch as it is, you've got some logic to move the PAM file in https://build.opensuse.org/package/show/home:yfjiang:branches:X11:RemoteDesktop/xrdp and some here.

I'd suggest either doing all the patching in your own repository, or adding logic to XRDP to allow the pamddir to be specified as part of the build so you can get rid of all the patching.

Or am I misunderstanding something?

@yifanjiang
Copy link
Contributor Author

@matt335672 Thanks, as what you guided, handling it in downstream seems sane enough. Though the mentioned hard-coded pam.d path is worthy of taking a look, I'll handle it as a separate thing. Close this by now.

@thkukuk
Copy link

thkukuk commented Feb 15, 2023

Our own install scripts hard-write the installation path for the PAM file in instfiles/pam.d:-

pamddir = $(sysconfdir)/pam.d

so that's what we check.

If we accept this patch as it is, you've got some logic to move the PAM file in https://build.opensuse.org/package/show/home:yfjiang:branches:X11:RemoteDesktop/xrdp and some here.

I'd suggest either doing all the patching in your own repository, or adding logic to XRDP to allow the pamddir to be specified as part of the build so you can get rid of all the patching.

Or am I misunderstanding something?

@matt335672 as upstream Linux-PAM maintainer I can tell you, that /usr/lib/pam.d is an official directory from upstream Linux-PAM and handled in the same way as /etc/pam.d. If you hardcode parts of the search path in your application, your application is broken, as I does no longer behaves correct.
I have no idea why xrdp thinks it needs to be more clever than the PAM, but if you think you need to be more clever, you need to check for the complete search path, not only the single part of it in which you install something.
Problem: there is no way to get the full pam.d search path out of libpam, this is a configure option. But /etc/pam.d and /usr/lib/pam.d are standard upstream directories and if xrdp wants to behave correct it has to look at minimal in both.

Linux-PAM searches the configuration files in this order in this directories:
<vendor specific dir>:/usr/lib/pam.d:/etc/pam.d
Later directories overwrites earlier one.
xrdp PAM config file can be in all three directories, which is a correct and common installation (at least for image based Linux distributions, on which maybe xrdp is not commonly used) and will be used more and more by traditional distributions, too.

@matt335672
Copy link
Member

Thanks @thkukuk - that's useful information.

I'll look into how we can combine this with #2552 to come up with something which works well for Linux-PAM and OpenPAM. OpenPAM is less configurable than Linux PAM in this area.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants