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

Properly support openPAM #18

Open
1wilkens opened this issue Sep 12, 2021 · 21 comments
Open

Properly support openPAM #18

1wilkens opened this issue Sep 12, 2021 · 21 comments

Comments

@1wilkens
Copy link
Owner

OpenPAM does not match Linux PAM 100% and thus is not compatible with this library in its current state.

To correctly support openPAM, we should get a comprehensive list of operating systems that use it, match that with the ones Rust builds for and properly handle API differences in a uniform way.

@coastalwhite
Copy link
Collaborator

I have looked at it, since we are using this library as well as the wrapper pam library in lemurs as well.

A lot is actually needed to do this.

TLDR on all the changed:

  • OpenPAM is used by macOS and most BSD-based distros (with the notable exception of OpenBSD)
  • OpenPAM (mostly) follows the XSSO specification made by the Solaris team. It adds some extensions, though. Linux PAM is quite different from the specification.
  • When it comes to many things, this library with OpenPAM is silently broken.
    • Status codes don't match
    • Flags don't match (e.g. PAM_SILENT being different)

I have been looking at fixing these things, since we experienced some problems with Ubuntu/Debian's PAM as well. I created a -sys of my own that addresses most of these issues. However, I didn't publish it, since it is really harmful to the ecosystem to have two -sys crates that link against the same system library.

Are you still willing to work on these crates, or have you moved on to other things? (which is totally understandable and fine) If so, I can submit a PR that addresses most of these issues. The problem being that we cannot use bindgen anymore, since we need to manually consider the differences between OpenPAM and Linux PAM.

How to actually deal with these differences is also a good question because somewhere the distinction between OpenPAM and Linux PAM needs to be made. I see a couple of possibilities.

  • pam-sys provides and labels both and lets pam-rs deal with the difference.
  • pam-sys decides based on the operating system (can be changed with an environment variable).
  • pam-sys decides based on cargo features.

I am curious whether you are willing to get involved with this.

@pvdrz
Copy link

pvdrz commented Feb 9, 2023

👋 bindgen contributor here. Is there any way I could help so you can use bindgen? Would it be feasible to run different bindgen invocations based on the target?

@coastalwhite
Copy link
Collaborator

Hey @pvdrz, I am not actually sure what the proper way to represent this would be. Take the library with the changes I am proposing. If you want to keep cross-compilation possibilities and not have people need to do build time configuration of a library 5 levels down the dependency tree, it is actually quite difficult. You would like to be able to detect the installed version at compile-time and have something to overwrite that detection to do cross-compilation or linking against a different binary.

@pvdrz
Copy link

pvdrz commented Feb 10, 2023

What I was thinking was basically your option of

  • pam-sys provides and labels both and lets pam-rs deal with the difference.

Where bindings are generated for linux-pam if the target is linux and openpam for macos and the rest of the bsd family.

I'd say that pam-rs should be the crate responsible of handling the differences and provide opaque abstractions around the flags and status code just like std::io::ErrorKind works right now. It should also keep the parts of the API that don't intersect behind extension traits in the same fashion as std::os::linux::fs::MetadataExt and other similar traits.

Of course this is easier said than done and I'd be happy to help.

@coastalwhite
Copy link
Collaborator

I feel like this is the best solution, but automatically generating proper bindings will present a problem I fear. The problem is that the target platform alone is not enough. There are BSD systems using Linux-PAM and Linux systems using OpenPAM.

OpenPAM is currently used by FreeBSD, PC-BSD, Dragonfly BSD, NetBSD, Mac OS X and a few Linux distributions.

We could detect the installed version at runtime by dynamically linking the library at runtime and seeing whether the openpam specific symbols are present.

@pvdrz
Copy link

pvdrz commented Feb 10, 2023

I'd be very interested in knowing which are those "few Linux distributions" because I did a quick search and got nothing 😅. Same for bsd, the only thing I know is that OpenBSD doesn't use PAM at all.

But yeah we could use libloading like you do instead and call the right bindgen invocation for each target.

@coastalwhite
Copy link
Collaborator

coastalwhite commented Feb 10, 2023

I'd be very interested in knowing which are those "few Linux distributions" because I did a quick search and got nothing sweat_smile.

As far as I know, and I have never seen anyone actually do it, some of the Anti-SystemD distributions have the option of using OpenPAM as it is supposedly more "lightweight".

It might be fine to use the target platform and have an environment variable to overwrite it if needed.

@pvdrz
Copy link

pvdrz commented Feb 10, 2023

yeah I think providing the "natural" default in a simple manner and allowing the user to select whatever they want with env-vars sounds convenient.

@1wilkens
Copy link
Owner Author

1wilkens commented Mar 2, 2023

Hey sorry for the late reply!
My intent is to continue maintaining the pam crate family, however work got the better of my time in the last months. It motivates me to read, that people are using these libraries as I kinda abandoned my original use-case (also a login/display manager). (I really have to take a look at lemurs as I kinda got lost in the ways that libpam and systemd interact.)

Some thoughts on the matter:

  • I would really like to keep using bindgen here, to keep this crate as simple as (reasonably) possible.
  • The crate should also link against the "natural default" unless otherwise specified to avoid the "configure the dependency deep down the tree"-problem. People that run a system that willingly deviates from the expected case are probably savvy enough to configure things right anyway.
  • Any API improvements in pam are greatly appreciated as I only briefly explored the login manager use-case and the API might not satisfy other scenarios.

@coastalwhite how should we proceed? I can likely spare some time on this, however likely not before June. Would you be interested in opening a PR to start the discussion? Also, as lemurs seems like a great application leveraging pam/pam-sys, would you be interested in co-maintaining the crates? :)

This was referenced Mar 2, 2023
@pvdrz
Copy link

pvdrz commented Mar 2, 2023

👋

On bindgen we are trying to keep in touch with the maintainers of sys crates like this one and figure out how to improve their experience. I also have a project from work that depends on pam so I'd be interested on contributing in any way I can.

Regarding the linux-pam vs open-pam issue. I think we agree on just exposing the bindings as they are and leave another crate figure out the differences between both. Am I right?

@coastalwhite
Copy link
Collaborator

Hey!

I would love to co-maintain these crates. I will have a look how to use bindgen to create a complete cover for both Linux-PAM, OpenPAM and any possible other implementations 😉.

I saw that greetd also uses pam-sys, they seemingly ran into the same problem I did a bit ago that the pam crate does some weird things with the environment.

I would really like to keep using bindgen here, to keep this crate as simple as (reasonably) possible.

I agree. I manually did it in libpam-sys, which allowed me to have fine control over all things that were generated. I do think we want to provide the values of linux-pam and openpam in a submodule and then have the "natural default" in root module. This natural default should probably be changable by an environment variable or possibly by a cargo feature. One problem I see with this is that we need a way to disable the module that is not the default since they symbols will be unresolvable.

Any API improvements in pam are greatly appreciated as I only briefly explored the login manager use-case and the API might not satisfy other scenarios.

I have been trying a bit on my own to come up with a better API for PAM clients. Specifically, I wanted to make PAM conversations easier, more secure and more expandable. I have a working prototype which can properly detail with org domains, username, password and OTP tokens. I will see what I can do to get it in a bit more presentable of state. Then you can see whether you want to merge the pam crate or whether you would rather have that be something separate.

One problem is that I currently don't really deal with modules. I am not sure about that for the moment.

Regarding the linux-pam vs open-pam issue. I think we agree on just exposing the bindings as they are and leave ?
another crate figure out the differences between both. Am I right?

Yes, I would agree with this statement.

In general, I will have a look at improving these bindings and then improving the API of the pam crate. I will keep you up to date.

@chipsenkbeil
Copy link

Is this the source of truth to figure out the status of this library (and pam) when it comes to OpenPAM support? I want to be able to integrate PAM as part of chipsenkbeil/distant#155 so I can provide alternative authentication when using distant and the associated neovim plugin, distant.nvim.

I'm also concerned about 1wilkens/pam#25 leading to segfaults on musl platforms, but I'm assuming that's a pam and not pam-sys issue, though? If not, seems like something to be fixed prior to 1.0 of this sys crate, too!

@coastalwhite
Copy link
Collaborator

Here is the current status of my investigations as of so far.

The OpenPAM support is a difficult problem. I got started with the issue, and there are numerous downsides to all solutions.

In essence, it is a not a "multiplatform" support problem, but a "multilibrary" support problem. I have tried this in a bunch of different ways, and I have come to the conclusion that having one pam-sys crate does not really make too much sense. Linux PAM and OpenPAM are just two different libraries. And although they share some shared interface, these interfaces are reasonably limited. Here are a couple of my suggestions on how to deal with that.

Separate crates

Have 2 crates: linuxpam-sys and openpam-sys. Both can then just use bindgen without any problems. Then, the pam-sys crate is merely there to merge these two and decide which one should be linked, depending on the platform and some environment variables. The API would provide only shared functions and reexport the non-shared functions as a submodule. This, to me, seems like the most ergonomic and proper solution.

The large problem with this solution. Only a single crate can links = "pam". If it is the pam-sys crate, the openpam-sys and linuxpam-sys crates do not really make sense on their own. It is the openpam-sys and linuxpam-sys crates, you need to decide with cargo features which one you want. The first seems like the more plausible option.

Single Crate, 2 Submodules

This is essentially equivalent to before and solves the links = "pam" problem. We have the singular pam-sys crate that has two submodules openpam and linux_pam. The root just exports which one got selected. Similar to how libpam-sys does it. This time using bindgen, however.

However, generating the documentation with bindgen will require for all the header files to be present in the repository, as the system may not have both linux_pam and openpam available. I am not convinced how logical that is. We should probably do that with git submodules. We also need to find a way to properly do reexports with documentation.

Those are the two solutions. The second solution seems the best, so will be focusing on that. I will have a look at doing this in the somewhat near future. It seems as if the ecosystem is already really split on pam-sys libraries:

I feel like PAM is a good target for Rust and that it is important that there is a good FFI library for it.

I would love to get the feedback from @1wilkens and @pvdrz.

And I hope this provided a status update for @chipsenkbeil.

@chipsenkbeil
Copy link

chipsenkbeil commented Apr 19, 2023

@1wilkens @pvdrz any thoughts on this? @coastalwhite has put a lot of work into this and I'd hate to lose the momentum!

A single higher-level crate with two separate sys crates has some precedence with wezterm-ssh using ssh2 and libssh-rs, albeit those sub-crates were themselves wrappers on top of the equivalent sys crates.

I'm also not opposed to the way that libpam-sys seems to approach it. I'm assuming there's never a situation where OpenPAM and LinuxPAM are both available? If there is, would the former high-level crate with two sys crates be easier? Or would the libpam-sys approach also work fine to include both?

My first thought from an end-user with a high-level library would be to have a primary export with further modules in the same way that we have windows and unix modules for platform-specific code, which I believe is how libpam-sys is built today with linux_pam and openpam modules.

@coastalwhite
Copy link
Collaborator

The problem with libpam-sys as @1wilkens said as well, is that we need to manually declare the bindings. I am not opposed to that as I feel like these bindings almost never change and when they do we want to know about it anyway, but I do get the argument for wanting to use bindgen.

I did manage to make quite some progress on what I said in my previous post. I will replace the old PR with a new PR hopefully this weekend.

@pvdrz
Copy link

pvdrz commented Apr 19, 2023

I like @coastalwhite first proposal as this is, as mentioned by themselves, a multilibrary issue, they just happen to have the same suffix so we have this blurry notion of PAM in our heads. So I agree, having two different sys crates makes total sense. Even though I agree there should be a crate that allows you to use whatever PAM library is available in your system,

I'm not sure if it should be a sys crate or not as it has been mentioned that both libraries differs in things like status codes. Having a non-sys crate with a regular Rust enum Status that doesn't expose its representation but matches the right status codes for linuxpam and openpam based on which one is being used sounds more ergonomic. That being said I'm not that familiar with PAM so take this with a grain of salt.

I'm not aware of any case where someone wants to use both PAM libraries at the same time so I wouldn't worry that much about the links issue. But who knows.

I'm not opposed to the second proposal, the only issue I see is that anyone who wants to do any authentication based on linux without caring about other OSes wouldn't have the option to use something like linuxpam-sys and would have to use the whole crate even if the crate has some limitations in its API for the sake of cross-compatibility.

I'm cc-ing @rnijveld as I think he might have some interesting opinions on this matter.

@coastalwhite
Copy link
Collaborator

coastalwhite commented Apr 22, 2023

Turns out the approach I was attempting with bindgen generating binding for both libraries just does not work. The reason it does not work is quite logical. Since C will not mangle the symbols, there will be symbols with the same name in the linker, which is not allowed. This is not solved by having two separate crates. I suspect that the libpam-sys approach is the only one that allows for the documentation of both bindings to be generated and a selection to be made.

@1wilkens
Copy link
Owner Author

1wilkens commented Nov 1, 2023

Hey all,

sorry it took so long for me to come back to this crate (and OSS) in general. I was busy finishing my PhD and had a subsequent job change which really captured all my energy.

I have caught up on the the posts here but will probably need another reread to properly disgest the available options. In the meantime I want to thank @coastalwhite for all your investigation and implementation so far!

As said in your PR I really would like to get back into things and release a 1.0 version of at least pam-sys if not pam this year. I'll add you as co-maintainer now but would like to discuss your proposed changes once more after I've had time to properly get what you are proposing.

@coastalwhite
Copy link
Collaborator

@pvdrz

How do you also feel about coordinating on this, so that in the future sudo-rs could possibly also use this crate and properly support OpenPAM as a backend?

@coastalwhite
Copy link
Collaborator

Another approach that can be used is to use Platform Specific Dependencies. Maybe, this is the better solution actually

@pvdrz
Copy link

pvdrz commented Mar 12, 2024

At the sudo project we try to keep the dependency tree as minimal as possible. I could see the project using this if it were to provide OpenPAM support as well as maintaining bindings for both libraries might be cumbersome. However, that's my personal opinion and other contributors might disagree.

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

No branches or pull requests

4 participants