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

Commit PAM bindings for FreeBSD #885

Merged
merged 2 commits into from
Nov 4, 2024

Conversation

bjorn3
Copy link
Collaborator

@bjorn3 bjorn3 commented Oct 15, 2024

This is a prerequisite of compiling sudo-rs for FreeBSD, but not in itself enough to compile sudo-rs for FreeBSD. This also intentionally excluses a minor change to the PAM code that is necessary for FreeBSD as someone will need to carefully verify that PAM on FreeBSD behaves similar enough to PAM on Linux or if not adapt the PAM code in sudo-rs.

Part of #869

@rnijveld
Copy link
Collaborator

I'm still a little bit confused as to how we should handle PAM here, given that differences between different PAM implementations don't necessarily depend on the platform we are compiling for, but instead depend on which specific PAM implementation that is in use.

Really the most important thing though is the pam_conv definition, for which the first argument can be interpreted in two ways, where we at least know that the PAM shipped with Solaris (who cares?) does things differently compared to LinuxPAM. I think the PAM implementation shipped with FreeBSD (and macOS) is however a derivative of LinuxPAM, so that doesn't really matter. Additionally I believe most (all?) PAM modules these days only send a single message per conversation call anyway, so the semantic difference doesn't matter.

Some additional changes I've identified so far are some additional constants for some non-standard items to set/get. But I mean we have three items we set and one we get, I'm sure we'll be fine there.

All in all: I'm not really sure what our goal should be. Do we want to make sure that our subset of PAM is just generic enough that any PAM implementation should be supported, or do we want to make sure that we handle each PAM implementation specifically? If so, how do we discriminate between the different PAM implementations. As far as I'm aware there is no way to identify each of the PAM implementations specifically.

@bjorn3
Copy link
Collaborator Author

bjorn3 commented Oct 21, 2024

Does every OS only ever use a single PAM implementation? If so just cfg'ing based on target_os would work, right? If not we did either need a new cargo feature with which the user selects the PAM implementation they need or a build script which automatically detects it I would think.

@bjorn3 bjorn3 force-pushed the freebsd_pam_bindings branch 2 times, most recently from 5240e40 to 54ba749 Compare October 22, 2024 09:27
@rnijveld
Copy link
Collaborator

Does every OS only ever use a single PAM implementation? If so just cfg'ing based on target_os would work, right? If not we did either need a new cargo feature with which the user selects the PAM implementation they need or a build script which automatically detects it I would think.

In practice almost everyone on Linux uses LinuxPAM and almost everyone on FreeBSD runs OpenPAM, but they both run on either platform, and there are some (niche) Linux distros that actually ship OpenPAM by default.

But I guess my main thing here would be: is there even any practical difference between LinuxPAM and OpenPAM that justifies generating two different bindings for our use cases. As far as I'm aware all functions that we rely on in our implementation have identical signatures. And I believe all constants we need are defined in both implementations with the same values (but we should check those assertions just to make sure of course), I believe everything we use is in the XSSO PAM spec and is pretty basic.

One thing we should check however is if there is any difference in the pam_conv struct. LinuxPAM claims it does something different to the Solaris PAM implementation, whereas OpenPAM claims to try and follow Solaris PAM as closely as possible. Specifically that pam_conv struct contains a conv function pointer member. The second argument of that function pointer should be a const struct pam_message **msg. The LinuxPAM documentation has this to note about that argument:

[...] Linux-PAM interprets the msg argument as a pointer to an array of num_msg read only 'struct pam_message' pointers. Solaris' PAM implementation interprets this argument as a pointer to a pointer to an array of num_msg pam_message structures. Fortunately, perhaps, for most module/application developers when num_msg has a value of one these two definitions are entirely equivalent. [...]

Meanwhile the OpenPAM documentation has this to say:

The second argument is a pointer to an array of pointers to pam_message structures containing the actual messages.

Which to me makes it sound like it does the same thing as LinuxPAM, but at the same time they also say:

OpenPAM tries to remain compatible with Solaris, at the expense of XSSO conformance and Linux-PAM compatibility

All of this however is mostly theoretical, given that almost every PAM module in practice does single message conversations, making the two interpretations of the argument entirely equivalent. We could in our implementation just refuse to work with conversations of more than one message. Or we could assume that the way more sane LinuxPAM interpretation (which also appears to be the OpenPAM interpretation) is leading, and we could also assume that any PAM module that does send more messages in a single conversation actually does this double referencing trick that the LinuxPAM manual recommends: msg[n] = & (( *msg )[n]) which should mean that either interpretation always works anyway.

Anyway, I believe that both PAM implementations are pretty much identical and changes are mostly on the module development side, not on the application side that we're communicating with.

@bjorn3
Copy link
Collaborator Author

bjorn3 commented Oct 23, 2024

But I guess my main thing here would be: is there even any practical difference between LinuxPAM and OpenPAM that justifies generating two different bindings for our use cases. As far as I'm aware all functions that we rely on in our implementation have identical signatures. And I believe all constants we need are defined in both implementations with the same values (but we should check those assertions just to make sure of course), I believe everything we use is in the XSSO PAM spec and is pretty basic.

They are in fact not ABI compatible. I initially tried using the Linux bindings on FreeBSD and it would immediately return an error before even asking for a password.

@rnijveld
Copy link
Collaborator

We just discussed this a little bit: it appears the incompatibilities are only related to constants being defined with different values (ugh, a bit of a shame really, they were so close). But all the functions are the same and all constants we need are available in both implementations. So technically we could keep our function definitions in a single file for both platforms and then have a separate set of constants per platform (or really: per PAM implementation), but that just feels like more work than just generating bindings twice for now.

Secondary to that is still the issue that the PAM implementation is not necessarily based on the target platform, but is sort of a separate choice (i.e. LinuxPAM runs on FreeBSD, OpenPAM runs on Linux). However the 95% solution here is to just split based on platform. We could for those people using a non-standard solution then add feature flags allowing them to override the switch based on platform and use an alternative implementation instead.

I would suggest naming the files something like sys_linuxpam.rs and sys_openpam.rs to reflect that they are more associated with the target implementation instead of the target platform however.

@squell
Copy link
Member

squell commented Oct 23, 2024

Or we could assume that the way more sane LinuxPAM interpretation (which also appears to be the OpenPAM interpretation) is leading, and we could also assume that any PAM module that does send more messages in a single conversation actually does this double referencing trick that the LinuxPAM manual recommends: msg[n] = & (( *msg )[n]) which should mean that either interpretation always works anyway.

Let's just check the FreeBSD source code for their interpretation and confirm that this part of the code is good to go; we're not aiming for 'general portability' just yet.

That 'trick' actually has a dark side, since it a.) is very easy to implement by accident b.) actually makes it 'preferable' in a way to go with the faulty Solaris interpretation in the client side, since that will "always work". :-)

It's disheartening that a simple ptr ** in a C function prototype can cause and then cover up so much confusion in something so important.

@bjorn3
Copy link
Collaborator Author

bjorn3 commented Oct 23, 2024

The only builtin caller of the pam_conv function I found in openpam is pam_vprompt which always passes a single message: https://github.com/freebsd/freebsd-src/blob/419249c1cacca2cfdf5128ead2873a9fd57f112e/contrib/openpam/lib/libpam/pam_vprompt.c#L83 It is possible for PAM modules to directly call it by getting the PAM_CONV item from the PAM context though. For example the pam_unix module of openpam does this, but it too only passes a single message: https://github.com/freebsd/freebsd-src/blob/419249c1cacca2cfdf5128ead2873a9fd57f112e/contrib/openpam/modules/pam_unix/pam_unix.c#L89 The only other PAM modules part of the freebsd-src repo are pam_passwdqc (https://github.com/freebsd/freebsd-src/blob/419249c1cacca2cfdf5128ead2873a9fd57f112e/contrib/pam_modules/pam_passwdqc/pam_passwdqc.c#L150) which uses a single message only and pam_radius (https://github.com/freebsd/freebsd-src/blob/419249c1cacca2cfdf5128ead2873a9fd57f112e/lib/libpam/modules/pam_radius/pam_radius.c#L274) which looks to be putting all messages tightly packed into an array and then produce another array with pointers into the first array in the same order as the first array contains the messages and finally a pointer to the second array is passed to the pam_conv function.

@squell squell enabled auto-merge November 4, 2024 14:36
This is a prerequisite of compiling sudo-rs for FreeBSD, but not in
itself enough to compile sudo-rs for FreeBSD. This also intentionally
excluses a minor change to the PAM code that is necessary for FreeBSD as
someone will need to carefully verify that PAM on FreeBSD behaves
similar enough to PAM on Linux or if not adapt the PAM code in sudo-rs.
Also fix the Makefile to generate the bindings.
@squell squell merged commit 1f63cac into trifectatechfoundation:main Nov 4, 2024
13 checks passed
@bjorn3 bjorn3 deleted the freebsd_pam_bindings branch November 4, 2024 14:44
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