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

Fix env parsing off by one #35

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

serpilliere
Copy link
Contributor

Fix issue #34

@serpilliere
Copy link
Contributor Author

This PR fixes the off by one described in #34

Note:
It seems that the PAM environment only selects arbitrary variables

self.set_env("HOME", user.home_dir().to_str().unwrap())?;

        self.set_env("HOME", user.home_dir().to_str().unwrap())?;
        self.set_env("PWD", user.home_dir().to_str().unwrap())?;
        self.set_env("SHELL", user.shell().to_str().unwrap())?;

But the PAM environment can set important other variables: for example in a kerberos environement, you have the KRB5CCNAME which points to the kerberos ticket for the user freshly logged in using pam.
This variable is in most system forwarded to the user environment (for example in SSH).

So if you are ok with this, maybe we could modify this code to inherit every environment variable positioned by the PAM to the process, instead of just arbitrary one.

Or if you are not ok with this principle, maybe we could expose an API for get all pam variable to the use api, something in the client.rs like:

    /// Retreive environement variable set by pam
    pub fn get_env_list(&mut self) -> Vec<(String, String)> {
        let pam_env = getenvlist(self.handle);
        let mut env = vec![];
        for (name, value) in pam_env {
            env.push((name, value));
        }
        env
    }

So the end user could do this on its own

Note: I think in an old version of your code, you forwarded every environment variables if I remember correctly.

@serpilliere
Copy link
Contributor Author

Hi @1wilkens
I added the get_env_list function to the client, so the user will be able to retrieve PAM environment variable.
Moreover, I added a set of those environment variable to the initialize_environment so by default it will be pushed to the logged env.

fx-chun added a commit to fx-chun/pam that referenced this pull request Jun 26, 2024
Squashed commit of the following:

commit a65741d
Author: Antonio Gurgel <antonio@goorzhel.com>
Date:   Mon Aug 22 17:30:24 2022 -0700

    Portably refer to C char type

commit 56a3e24
Author: Fabrice Desclaux <fabrice.desclaux@cea.fr>
Date:   Thu Jul 21 11:17:00 2022 +0200

    Set environement retrived by pam module

commit be06986
Author: Fabrice Desclaux <fabrice.desclaux@cea.fr>
Date:   Fri Jul 8 11:47:59 2022 +0200

    Fix env parsing off by one

1wilkens#35

Co-authored-by: Fabrice Desclaux <fabrice.desclaux@cea.fr>
Co-authored-by: Antonio Gurgel <antonio@goorzhel.com>
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.

2 participants