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

Remove distro specific PAM modules #1856

Merged
merged 3 commits into from Mar 4, 2024
Merged

Remove distro specific PAM modules #1856

merged 3 commits into from Mar 4, 2024

Conversation

evelikov
Copy link
Contributor

In light of #1842 instead of getting more distro-specific PAM modules in sddm, we obtained a way to not install the default ones. Here we remove the Debian and FreeBSD variants.

As result upstream devs does not need to worry if #1058 is applicable for the distro PAM modules, workarounds like #1833 become obsolete. Distributions can change their PAM policy without any patching or PRs.

/cc @arrowd and @carlosdem who introduced them.

Team looking at both distros - Debian and FreeBSD - it seems like they both patch a) some paths and b) the default theme. To me it seems like these can be made cmake -Dfoobar=baz modules?

HTH o/

@arrowd
Copy link
Contributor

arrowd commented Jan 24, 2024

I don't get it. What are we supposed to do on the FreeBSD side now?

@evelikov
Copy link
Contributor Author

I don't get it. What are we supposed to do on the FreeBSD side now?

  • build with cmake -DINSTALL_PAM_CONFIGURATION=OFF ....
  • keep the complete pam files locally in freebsd ports

In case it's not obvious, I'm not 100% sure if this will fly with maintainers. So consider their input more than mine ;-)

@arrowd
Copy link
Contributor

arrowd commented Jan 24, 2024

I don't see the rationale for this change. It also breaks installing SDDM directly from source (well not installing, but using SDDM installed from source).

@Conan-Kudo
Copy link
Contributor

If we were going to do this, we'd drop all of them including the Arch ones. The reason why I did #1842 wasn't because I agreed with the base premise, but because it makes sense to offer the ability to not install them and use what the system already has.

That said, even GDM keeps PAM files for the various operating systems that are supported because PAM configs vary wildly and having the ability to update them when the GDM code changes is valuable.

@carlosdem
Copy link
Contributor

debian specific pam modules were introduced in #1440 by @hsitter . the original premise is still valid i think : " this prevents people from ending up with
a broken setup because we install unsuitable pam configs."

@evelikov
Copy link
Contributor Author

Are you saying that the nameless ones (aka sddm.pam and co) are used solely by Arch? My assumption was that they're used across multiple distros, although I could be wrong. Especially since Arch doesn't use elogind.

Glancing across GDM - as an example, fwiw - it has Arch, Exterbo, LFS, OE and RH/Fedora PAM config in-tree.

Debian seemingly generates a pseudo(?) config in their packaging which then gets updated via their custom tool, which gets called by a patched gdm.service. Overall it seems a bit over-engineered although I could easily be missing something.

Regardless: my initial train of thought stands, although take it with a pinch of salt.

Possible alternative is to rename INSTALL_PAM_CONFIGURATION to PAM_CONFIGURATION, which takes freebsd, debian, arch and none as tokens. With the default being "none".

@Conan-Kudo
Copy link
Contributor

Are you saying that the nameless ones (aka sddm.pam and co) are used solely by Arch? My assumption was that they're used across multiple distros, although I could be wrong. Especially since Arch doesn't use elogind.

Yes. They are exclusively used for Arch and are not used by any other distribution. They cannot be, since they depend on how Arch sets up PAM.

@evelikov
Copy link
Contributor Author

Fwiw: Glancing over at Gentoo - it uses the default (Arch?) PAM with a trivial change s/include/substack/

@hsitter
Copy link
Collaborator

hsitter commented Feb 20, 2024

The basic requirement here is that if you install sddm from source you should not ever end up with a broken pam setup; that was the original intent behind multiple configs. The sddm.pam didn't work 100% on Debian so installing it on Debian just broke things.

The way I see it we have a fork in the road:

Option a) Drop all configs. 99% of the time the user has sddm already installed and will have a distro provided config. No need to install one really.

Option b) Keep things mostly the same and maintain configs in-tree like GDM does. Also following Emil's idea of renaming INSTALL_PAM_CONFIGURATION to PAM_CONFIGURATION=none|debian|freesbd|.... With one additional thought: I'd still default to detect the system if possible. I wouldn't want to put it on the user|contributor to know that they have to set things up - things should just work.

@davidedmundson @Vogtinator @aleixpol @plfiorini do you have a take on this?

@Conan-Kudo
Copy link
Contributor

I'm fine with either path, and if we do option B, then I'll submit to include the redhat/fedora PAM configuration too.

@Vogtinator
Copy link
Contributor

I'm in favor of a), see also my comments in #1842.

@plfiorini
Copy link
Member

I'm in favor of a) because maintaining distro specific stuff here is difficult

@evelikov
Copy link
Contributor Author

evelikov commented Feb 20, 2024

Thanks for the input everyone.

Rather busy this week. When I'm back I will:

  • update the PR removing all the PAM modules - option a)
  • poke the distribution maintainers so they're aware -> reduce odds of accidental breakage

Having a per-distribution set in the upstream project (sddm) does not
scale well. Instead sddm can provide a default set that distributions
can opt out of with "cmake -DINSTALL_PAM_CONFIGURATION=off ..."

Glancing through the FreeBSD ports - all the pam modules are patched
already, so dropping them from here should reduce maintenance on both
sides.
Having a per-distribution set in the upstream project (sddm) does not
scale well. Instead sddm can provide a default set that distributions
can opt out of with "cmake -DINSTALL_PAM_CONFIGURATION=off ..."
PAM modules are distribution specific and are ultimately better fit in
the distribution packaging.

Having them in SDDM brings extra burden to the already stretched
developers and maintainers. Plus a handful of distributions currently
patch the (Arch) PAM modules, where having the full set in their own
tree would make for cleaner solution.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
@evelikov
Copy link
Contributor Author

evelikov commented Mar 2, 2024

Updated the PR to also drop the the final (Arch) PAM files - aka implemented a)

Tagging some people:

Team, with this PR the PAM files are removed from SDDM as the general inclination is that those are better suited within the distribution packaging/repo. Please consider updating the SDDM package and sharing this news with fellow package maintainers.

Thanks in advance o/

@hsitter hsitter merged commit ae072f9 into sddm:develop Mar 4, 2024
12 of 13 checks passed
@evelikov evelikov deleted the star-tmp branch March 5, 2024 18:25
@edt-xx
Copy link

edt-xx commented Mar 5, 2024

This may be a stupid question. What has to change in the package for this change to work? I am on Arch and the aur package maintainer seems to think the problem is here, and here you are saying the package needs updates... Please provide a little more info so this can be resolved.

@Vogtinator
Copy link
Contributor

The package has to provide PAM service files suitable for the target distro. SDDM no longer provides any.

@Vogtinator
Copy link
Contributor

(There should ideally be some documentation around this, but for git builds it's always possible to have breaking changes)

@edt-xx
Copy link

edt-xx commented Mar 5, 2024

I've no problems with builds breaking. I'd just love to see docs - it okay for builds to break, its a real PITA when there is a lack of info on how to fix them. For now, guess I will need to wait (and use an older sddm-git build) for someone with more pam/sddm/arch know how to fix things on the Arch side. BTW your very prompt replies are appreciated :-)

@felixonmars
Copy link

The AUR maintainer appears to have applied a quick revert of this commit for the time being: https://aur.archlinux.org/cgit/aur.git/commit/?h=sddm-git&id=6b182d021decff2983182ecbe2e49eb26d93f154 so updating should be fine for now.

The Arch official repository package will be adjusted to provide the pam files later whenever a new release appears.

@sirlucjan
Copy link

The AUR maintainer appears to have applied a quick revert of this commit for the time being: https://aur.archlinux.org/cgit/aur.git/commit/?h=sddm-git&id=6b182d021decff2983182ecbe2e49eb26d93f154 so updating should be fine for now.

The Arch official repository package will be adjusted to provide the pam files later whenever a new release appears.

I got the first feedback and the temporary solution works.

@Vogtinator
Copy link
Contributor

(There should ideally be some documentation around this, but for git builds it's always possible to have breaking changes)

#1883 adds at least some info.

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.

None yet

10 participants