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

pam_env: read config from .config or XDG_CONFIG_HOME #7

Closed
ghost opened this issue Feb 14, 2017 · 18 comments
Closed

pam_env: read config from .config or XDG_CONFIG_HOME #7

ghost opened this issue Feb 14, 2017 · 18 comments

Comments

@ghost
Copy link

ghost commented Feb 14, 2017

Follows XDG Base Directory Specification
https://specifications.freedesktop.org/basedir-spec/latest/

Suggestions:

  • read XDG_CONFIG_HOME/security/pam_env.conf if first one set
  • else fallback on HOME/.config/security/pam_env.conf
  • if there is no file above read HOME/.pam_environment to not break user setups
  • document new location

Here is pseudo-patch (bottom is especially horrible):

diff --git a/modules/pam_env/pam_env.c b/modules/pam_env/pam_env.c
index 3846e35..dec54ab 100644
--- a/modules/pam_env/pam_env.c
+++ b/modules/pam_env/pam_env.c
@@ -10,6 +10,8 @@
 #define DEFAULT_READ_ENVFILE    1
 
 #define DEFAULT_USER_ENVFILE    ".pam_environment"
+#define DEFAULT_USER_CONFDIR    ".config"
+#define DEFAULT_USER_CONFFILE   "security/pam_env.conf"
 #define DEFAULT_USER_READ_ENVFILE 1
 
 #include "config.h"
@@ -800,9 +802,13 @@ handle_env (pam_handle_t *pamh, int argc, const char **argv)
 
   if(user_readenv && retval == PAM_SUCCESS) {
     char *envpath = NULL;
+    char *confpath = NULL;
+    char *user_conf_dir = DEFAULT_USER_CONFDIR;
+    char *user_conf_file = DEFAULT_USER_CONFFILE;
     struct passwd *user_entry = NULL;
     const char *username;
     struct stat statbuf;
+    struct stat statbuf2;
 
     username = _pam_get_item_byname(pamh, "PAM_USER");
 
@@ -812,24 +818,45 @@ handle_env (pam_handle_t *pamh, int argc, const char **argv)
       pam_syslog(pamh, LOG_ERR, "No such user!?");
     }
     else {
+      const char *xdgconfighome = pam_getenv(pamh, "XDG_CONFIG_HOME");
+      if (xdgconfighome)
+	user_conf_dir = xdgconfighome;
+      if (asprintf(&confpath, "%s/%s/%s", user_entry->pw_dir, user_conf_dir,
+		   user_conf_file) < 0)
+	{
+	  pam_syslog(pamh, LOG_CRIT, "Out of memory");
+	  return PAM_BUF_ERR;
+	}
       if (asprintf(&envpath, "%s/%s", user_entry->pw_dir, user_env_file) < 0)
 	{
 	  pam_syslog(pamh, LOG_CRIT, "Out of memory");
 	  return PAM_BUF_ERR;
 	}
-      if (stat(envpath, &statbuf) == 0) {
+      if (stat(confpath, &statbuf) == 0) {
 	PAM_MODUTIL_DEF_PRIVS(privs);
 
 	if (pam_modutil_drop_priv(pamh, &privs, user_entry)) {
 	  retval = PAM_SESSION_ERR;
 	} else {
-	  retval = _parse_config_file(pamh, ctrl, envpath);
+	  retval = _parse_config_file(pamh, ctrl, confpath);
 	  if (pam_modutil_regain_priv(pamh, &privs))
 	    retval = PAM_SESSION_ERR;
 	}
-        if (retval == PAM_IGNORE)
-          retval = PAM_SUCCESS;
+	if (retval == PAM_IGNORE) {
+	  if (stat(envpath, &statbuf2) == 0) {
+	    if (pam_modutil_drop_priv(pamh, &privs, user_entry)) {
+	      retval = PAM_SESSION_ERR;
+	    } else {
+	      retval = _parse_config_file(pamh, ctrl, envpath);
+	      if (pam_modutil_regain_priv(pamh, &privs))
+	        retval = PAM_SESSION_ERR;
+	    }
+	    if (retval == PAM_IGNORE)
+	       retval = PAM_SUCCESS;
+	  }
+	}
       }
+      free(confpath);
       free(envpath);
     }
   }
@ayekat
Copy link

ayekat commented Sep 4, 2017

Chicken-or-egg question: Where are XDG_CONFIG_HOME and the other XDG basedir variables set? I personally use my ~/.pam_environment to set them in the first place.

@ghost
Copy link
Author

ghost commented Sep 21, 2017

@ayekat You can use /etc/security/pam_env.conf to set them for all users.

/etc/security/pam_env.conf

XDG_CACHE_HOME   DEFAULT=@{HOME}/.cache
XDG_CONFIG_HOME  DEFAULT=@{HOME}/.config
XDG_DATA_HOME    DEFAULT=@{HOME}/.local/share

With suggested change user can override those with:

  • XDG_CONFIG_HOME is set: XDG_CONFIG_HOME/security/pam_env.conf
  • XDG_CONFIG_HOME unset: HOME/.config/security/pam_env.conf
  • user pam_env.conf does not exist: ~/.pam_environment

Implementation of this change allows setting user variables from the same directory where user config files are located and if XDG_CONFIG_HOME is set for the system actually place it in other directory than HOME or HOME/.config.

@ayekat
Copy link

ayekat commented Sep 21, 2017

Implementation of this change allows setting user variables from the same directory where user config files are located

This assumes that a user wants to use the same values as the system administrator, but what happens if that is not the case? E.g. the sysadmin might set it to ~/.config system-wide, but the user might prefer ~/.local/config. In this case it would be easier for users to just stick to ~/.pam_environment, because that one is guaranteed to work on all systems, regardless of what the system administrator decides.

@ghost
Copy link
Author

ghost commented Sep 21, 2017

@ayekat I don't assume user can't figure out if environment variables work on another such system.

If you want to keep using ~/.pam_environment this does not change anything for you.

@ayekat
Copy link

ayekat commented Oct 1, 2017

@ayekat I don't assume user can't figure out if environment variables work on another such system.

I don't doubt that. But that was not my point at all.

  • Users who manage their config files in a VCS like git (to synchronise them among multiple machines) will not find it very useful if the location of the PAM user configuration changes from system to system, so they will stick to ~/.pam_environment.
  • Users who do not manage their config files in a VCS but still want to have a different layout than dictated by the system administrator (notably a different XDG_CONFIG_HOME) will not find this useful either, and also stick to ~/.pam_environment.
  • Users who do not manage their configuration files (i.e. do not write a PAM user configuration file) but still like to have a clean home directory will complain to the corresponding projects that generate files in their home directory (PAM is not one of them).
  • Users who do not care about litter in their home directory don't care about the location of the (non-existent) PAM user configuration file.

So in order to profit from this change, a user must

  1. not manage/synchronise their dotfiles in a VCS among multiple machines,
  2. be OK to adapt their personal files according to whatever the local sysadmin decides, and
  3. still have strong feelings about how their home directory content looks like.

Allow me to make the claim that such a type of user is quite a rare sight (especially since points 2 and 3 somehow contradict).

To clarify, I don't oppose this change (but only because you say that ~/.pam_environment will continue to work). I'm also all in for getting XDG basedir spec support into applications. I just don't see any benefits in this particular case (or with this particular approach).

@soc
Copy link

soc commented Oct 1, 2017

I want to properly manage and backup my config files and have them in a standardized place. That's why they shouldn't be spread all over the home directory, but placed in the location specified by the XDG spec so that they can be added to a VCS without having to manage a raft of exclude rules.

So I would like pam_env to follow standards because:

  1. I manage/synchronize dotfiles between systems,
  2. I'm OK to use the well-established rules of where they should be placed, and I'm fine with overriding it where necessary (which is very unlikely to happen), and
  3. I have strong feelings about how my home directory content needs to looks like.

@soc
Copy link

soc commented Oct 3, 2017

.pam_environment is the last dot-file in my home directory:

$ ls -Ap
.atom/	   .cargo/	 .eclipse/	       .java/	    .pam_environment  .scalaide/
Audios	   .config/	 Games		       .local/	    Pictures	      .ssh/
Backup	   .dbus/	 .gem/		       .m2/	    .pki/	      .subversion/
.bintray/  Desktop	 .gnupg/	       .minecraft/  Public	      .swt/
.bundle/   Development/  Incoming	       .mozilla/    .purple/	      Templates
.bundles/  Documents	 .IntelliJIdea2017.2/  .nano/	    .rustup/	      .thunderbird/
.cache/    Downloads	 .ivy2/		       .npm/	    .sbt/	      Videos

It would really be great to get this issue resolved.

@baj0k
Copy link

baj0k commented Nov 4, 2020

Hello,
I would like to debate a little, regarding this issue.

I don't agree with arguments against implementing XDG basedir spec into linux-pam. Basically there are scenarios where this solution will not be useful, that's true. But this isn't about whether it will be commonly used, but the possibility of a user to do so. There is a lot of discussion going about hardcoded paths and disability to control where the user-specific data is stored among most of the widely used software and since there is a pretty good standard developed and used, it should be (and often is) respected.

Personally, (and I am speaking from sysadmin perspective) I can relate to one problem.

  • There are rare scenarios in which hardcoded ~/.paths make the software unusable and there is almost never a secure workaround option. And usually if there is a workaround than the amount of work / code that's needed to be done is daunting and can lead into security vulnerabilities. At the same time the general solution is usually writing few lines of code to respect one environment variable and to treat ~/.path as a fallback.
    This is frustrating.

At last, as @soc said 3 years ago, I want to properly manage and backup my config files and have them in a standardized place. So do other people. Why not just let them.

@t8m
Copy link
Member

t8m commented Nov 4, 2020

The user environment support in pam_env should be removed completely at some point.

@ldv-alt what is your opinion?
If you agree I'd write a notice to NEWS and pam_env manpage that the user environment support is deprecated and will be removed completely at some point in the future.

@ldv-alt
Copy link
Member

ldv-alt commented Nov 4, 2020 via email

@guihkx
Copy link

guihkx commented Nov 4, 2020

The user environment support in pam_env should be removed completely at some point.

May I ask you guys to reconsider this? I believe that using ~/.pam_environment to set per-user environment variables for the entire session is the only "standardized" method available today, no? Some people will recommend using ~/.profile for that, but then some other people would argue that isn't the display managers' job to source ~/.profile: sddm/sddm#448 (comment)

If you guys really do remove it, it's implied that some setups will break, but regardless, I believe it would make sense to provide some other good alternative for it.

@ayekat
Copy link

ayekat commented Nov 4, 2020

I believe that using ~/.pam_environment to set per-user environment variables for the entire session is the only "standardized" method available today, no?

There is also systemd's environment.d, but based on what I've seen so far, it has two issues:

  • The chicken-or-egg issue I mentioned above: it reads user configuration from $XDG_CONFIG_HOME/environment.d, and if $XDG_CONFIG_HOME hasn't been set before (currently the job of ~/.pam_environment), it will read from ~/.config/environment.d, therefore requiring users to keep around ~/.config even if they set $XDG_CONFIG_HOME to a different value.
  • It only applies to the systemd user daemon (the user@{uid}.service) containing all the user services, but not the user session. One therefore has to process the output of systemctl show-environment e.g. in the login shell's profile file to get a uniform environment across both the daemon and the session.

So while in my comments above I voiced concerns about adopting the XDG basedir spec in PAM for the user environment file, this was only because of that bootstrapping issue.

But that is simultaneously also the reason why I'm a bit concerned about user environments having been disabled by default recently, and now the intention to remove this entirely: What is the reasonable alternative for users to set their environment variables for their session in a clean way? Currently, I only see ~/.pam_environment providing this.

@t8m
Copy link
Member

t8m commented Nov 4, 2020

The problem is pam is being run as root and that is really a recipe for security issues if you source user owned file from there and adjust environment variables based on it. It should be really a new standardized mechanism that is run with the user credentials after the fork from the login/display manager before the user shell/display environment is started.

@t8m
Copy link
Member

t8m commented Nov 4, 2020

BTW, .profile was the right and standardized place to set environment variables until "everybody" decided that shell is no longer something inherent and mandatory on unix-like systems. Perhaps deprecation and removal of the .pam_environment support will be the right incentive to reconsider this.

@soredake
Copy link

soredake commented Nov 4, 2020

fish shell for example does not read ~/.profile file.

@soc
Copy link

soc commented Nov 5, 2020

Dropping support for setting user envs feels like throwing out the baby with the bathwater – but in the end, at the most fundamental level, the bootstrapping issue remains:

A proper way of dealing with the bootstrap issue would be to specify the "bootstrap path" in a new column in /etc/passwd, but we can all imagine how "well-received" this is going to be by the usual crowd.

Moving things from .pam_environment to .profile is not really enticing, not to mention all the pain involved in coming up with something that would work across various shells.

@t8m
Copy link
Member

t8m commented Nov 5, 2020

Then a different universal mechanism has to be proposed and implemented. .pam_environment is not the right place for security reasons. Period.

@soc
Copy link

soc commented Nov 6, 2020

Makes sense, will be happy to review your proposal!

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

No branches or pull requests

7 participants