1wilkens / pam

Safe Rust API to the Linux Pluggable Authentication Modules (PAM)
https://docs.rs/pam/
Apache License 2.0
88 stars 37 forks source link

Introduce Converse trait for user conversations #11

Closed elinorbgr closed 5 years ago

elinorbgr commented 5 years ago

Hi, I'm interested in using pam-auth for some project, and I'm likely gong to need some more advanced functionality than what is currently exposed, so I thought I'd help as much as I can while I learn how PAM works.

So, this PR is an attempt at doing the groundwork for supporting user-provided conversation functions. This is done like so:

Please let me know what you think of this approach :)

1wilkens commented 5 years ago

Hey thanks for the PR!

When writing this code, I already thought it might be too restrictive but never got around to fix this. I like the approach and would be happy to merge it. I am going to fully review this today to tomorrow.

Again thanks so much for your work! EDIT: One question, are you by any chance using pam-auth for something display manager related? I also tried my hand at one (rdm) but due to my limited understanding of both pam and display managers, I eventually stopped, but would be very interested to hear about your findings :)

elinorbgr commented 5 years ago

One question, are you by any chance using pam-auth for something display manager related?

Not yet, but I intend to. I'm going to attempt at something using smithay to handle all the input/graphics part of the login manager. Notably, I don't intend for it to be X-aware (or anything like that), but simply it'd be configured by users to just run some command (that could be startx, sway, or any custom script) once login is successful.

So for now I'm in the process of trying to understand PAM, since that's the part I understand the least currently. And I find it helps my understanding to try and remap the C API into something more rust-y, hence the PR to pam-auth, which will probably not be the last one if you are okay with that. :)

1wilkens commented 5 years ago

I just pushed some changes that I didn't get around to do until now (mainly porting to 2018 edition and using ?). I also cleaned up some usings which I would have marked in your PR as well, so I though I would just do it myself. After thinking some more about you approach, I would be happy to merge it. Initially I thought Authenticator should take a generic Converse but I am not sure how we could that expose nicely in the current API.

And I find it helps my understanding to try and remap the C API into something more rust-y, hence the PR to pam-auth, which will probably not be the last one if you are okay with that. :)

Sure I'd love to make the API more rusty. The current design of the Authenticator is basically how far I got in understanding the process of session opening in pam but it still has some problems and could be improved.

elinorbgr commented 5 years ago

Initially I thought Authenticator should take a generic Converse but I am not sure how we could that expose nicely in the current API.

I was thinking of something like that too, but restrained myself to non-breaking changes for the PR at first.

I had something like that in mind:

struct Password { ... }

impl Converse for Password { ... }

impl Password {
    fn set_credentials(&mut self, login: &str, passwd: &str) { ... }
}

struct Authenticator<C: Converse> { ... }

impl Authenticator<Password> {
    fn new_password() -> Authenticator<Password> { ... }
}

impl<C: Converse> Authenticator<C> {
    fn with_converse(converse: C) -> Authenticator<C> { ... }

    fn converse(&mut self) -> &mut C { ... }

    fn authenticate(&mut self) -> Result<()> { ... }

    fn open_session(&mut self) -> Result<()> { ... }
}

Where a simple use could be something like:

let mut authenticator = Authenticator::new_password();
authenticator.converse().set_credentials("me", "I like trains!");
authenticator.authenticate()?;

If that looks good to you, I can integrate that into the PR (and the necessary docs, of course).

1wilkens commented 5 years ago

LGTM! I should probably release a new version once the new API is integrated.

One thing I would still like to add are real tests, but I have not yet figured out how that would work on a real system.. Maybe tests that only pass in CI with predefined users or something. However that's for another commit. Thanks so much for your work thus far!

elinorbgr commented 5 years ago

Here is the rework, I also added an example that's a "sudo-like" app that spawns /bin/bash. I used it to ensure that the code works on my system.

1wilkens commented 5 years ago

Thanks! My next steps are:

  1. Merge this PR
  2. Rename this repo to just pam
  3. Publish 0.7.0 of pam to crates.io (the original author of this crate offered the name to me, as he did not really use it)