nixos/system-environment: fix variable substitution #70137

michaelpj · 2019-09-30T20:23:42Z

Motivation for this change

There were two issues:

Variables weren't being replaced in the values of variables, only in the profiles.
HOME isn't a PAM_ITEM so we need to use ${HOME}.

Fixes #69316.

Someone other than me should also test this!

Things done

Notify maintainers

cc @worldofpeace

worldofpeace

LGTM fixes the current issue.

Though there's more PAM_ITEMS that we could map and replace

https://linux.die.net/man/3/pam_get_item

and it would be good if somehow, all environment variables $SOMETHING
got substituted in the PAM format.

TBH, we should be using environment.d for all of these things.

michaelpj · 2019-10-01T14:19:58Z

Yeah, although we're doing something a bit weird here in matching up normal environment variables and PAM_ITEMS. I'm not sure that there are many things which have sensible interpretations as environment variables and also as PAM_ITEMS. We could try and replace all other environment variables with ${VAR}, but I'm not confident in writing something that will match all environment variable dereferences 😅

We were only replacing them in the profiles. We also need to do this in the values of variables, including both the session-relative variables and the non-session-relative variables.

`@` synax is for `PAM_ITEM`s, `HOME` needs to use `$`.

worldofpeace · 2019-10-02T01:21:03Z

Yeah, although we're doing something a bit weird here in matching up normal environment variables and PAM_ITEMS. I'm not sure that there are many things which have sensible interpretations as environment variables and also as PAM_ITEMS. We could try and replace all other environment variables with ${VAR}, but I'm not confident in writing something that will match all environment variable dereferences sweat_smile

Right, so it was only a suggestion for maybe someone else in the future to maybe notice 😄

Thanks again for fixing this 👍

nixos/system-environment: fix variable substitution (cherry picked from commit 49bc1bf)

dtzWill · 2019-10-05T13:24:21Z

nixos/modules/config/system-environment.nix

+        # We're trying to use the same syntax for PAM variables and env variables.
+        # That means we need to map the env variables that people might use to their
+        # equivalent PAM variable.
+        # Note: PAM_USER is a PAM_ITEM, HOME is an environment variable, they have 


So actually '@{HOME}is special, and there are important reasons for preferring it which are outlined inman pam_env.conf`:

(Possibly non-existent) environment variables may be used in values using the ${string} syntax and (possibly non-existent)
PAM_ITEMs as well as HOME and SHELL may be used in values using the @{string} syntax. Both the $ and @ characters can be
backslash escaped to be used as literal values values can be delimited with "", escaped " not supported. Note that many
environment variables that you would like to use may not be set by the time the module is called. For example, ${HOME} is
used below several times, but many PAM applications don't make it available by the time you need it. The special variables
@{HOME} and @{SHELL} are expanded to the values for the user from his passwd entry.

I remember running into problems when using $HOME instead of @{HOME} (as the quoted bit above says, HOME often isn't set by the time it's needed) but bouncing through commit logs on various machines I'm not finding evidence of such a (mis-)adventure. It may have been related to sshd/sudo, but regardless I am inclined to suggest we continue to prefer @{HOME} in pam-processed bits. That said I'm still sorting through this and similar recent changes and don't quite have it all in my head just yet :).

If the docs say that, it's still a fairly easy change to revert

- replaceEnvVars = replaceStrings ["$HOME" "$USER"] ["\${HOME}" "@{PAM_USER}"]; + replaceEnvVars = replaceStrings ["$HOME" "$USER"] ["@{HOME}" "@{PAM_USER}"];

and wasn't the motivation for this change anyways.

@dtzWill btw you'd be an excellent reviewer for #70430 (I think anyways) 😄

Ah, here we go. Test is sudo env-- with the latest changes it's clear everything is wrong (HOME isn't set as described above, so all the variables are borked), reverting to use @{HOME} fixes them again.

Please confirm/test and let me know what you find.

Possibly part of what's going on with #70418, but don't have a full story explaining that behavior just yet.

Ahh I see. Testing this change now. PR incoming since it should be completely fine.

@dtzWill Yep sudo env now looks right.

❤️ thanks for quick response, will take a look! And yes I don't think this will be hard to fix or require big reverts or anything :).

…e-subst" This reverts commit 08e03bf.

michaelpj · 2019-10-05T14:48:12Z

Rats, sorry, I read the documentation but evidently not enough! What I thought was a comprehensive list of PAM_ITEMS didn't have HOME.

dtzWill · 2019-10-05T14:50:41Z

No worries, I'm not even sure this means it's a "PAM_ITEM" or if pam_env handles them in addition to "actual" "PAM_ITEM"s or if we care if it's a "PAM_ITEM" or not 😁. I saw you mention checking pam_get_id which of course doesn't mention HOME, so dunno. I'm pretty sure I only know about this because I borked it myself before haha.

worldofpeace · 2019-10-05T14:54:59Z

Rats, sorry, I read the documentation but evidently not enough! What I thought was a comprehensive list of PAM_ITEMS didn't have HOME.

That's what I got from the doc as well. And no worries, you might have noticed even the best of break things regularly 😄 As long as the release doesn't see it we're usually fine.

…-variable-subst"" This reverts commit 9f9fea1.

ofborg bot added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Sep 30, 2019

worldofpeace approved these changes Oct 1, 2019

View reviewed changes

michaelpj added 2 commits October 1, 2019 21:18

nixos/system-environment: replace env vars in values of variables too

9d61dda

We were only replacing them in the profiles. We also need to do this in the values of variables, including both the session-relative variables and the non-session-relative variables.

nixos/system-environment: fix syntax for environment variables

d8b9742

`@` synax is for `PAM_ITEM`s, `HOME` needs to use `$`.

worldofpeace force-pushed the fix-pam-variable-subst branch from 81358ac to d8b9742 Compare October 2, 2019 01:20

worldofpeace changed the title ~~pam: fix variable substitution~~ nixos/system-environment: fix variable substitution Oct 2, 2019

worldofpeace merged commit 49bc1bf into NixOS:master Oct 2, 2019

ofborg bot removed 6.topic: nixos 8.has: module (update) labels Oct 2, 2019

dtzWill reviewed Oct 5, 2019

View reviewed changes

worldofpeace mentioned this pull request Oct 5, 2019

nixos/system-environment: use pam syntax for home variable #70465

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/system-environment: fix variable substitution #70137

nixos/system-environment: fix variable substitution #70137

michaelpj commented Sep 30, 2019

worldofpeace left a comment

michaelpj commented Oct 1, 2019

worldofpeace commented Oct 2, 2019 •

edited

Loading

dtzWill Oct 5, 2019

worldofpeace Oct 5, 2019

worldofpeace Oct 5, 2019

dtzWill Oct 5, 2019

worldofpeace Oct 5, 2019

worldofpeace Oct 5, 2019

dtzWill Oct 5, 2019

michaelpj commented Oct 5, 2019

dtzWill commented Oct 5, 2019

worldofpeace commented Oct 5, 2019

nixos/system-environment: fix variable substitution #70137

nixos/system-environment: fix variable substitution #70137

Conversation

michaelpj commented Sep 30, 2019

Motivation for this change

Things done

Notify maintainers

worldofpeace left a comment

Choose a reason for hiding this comment

TBH, we should be using environment.d for all of these things.

michaelpj commented Oct 1, 2019

worldofpeace commented Oct 2, 2019 • edited Loading

dtzWill Oct 5, 2019

Choose a reason for hiding this comment

worldofpeace Oct 5, 2019

Choose a reason for hiding this comment

worldofpeace Oct 5, 2019

Choose a reason for hiding this comment

dtzWill Oct 5, 2019

Choose a reason for hiding this comment

worldofpeace Oct 5, 2019

Choose a reason for hiding this comment

worldofpeace Oct 5, 2019

Choose a reason for hiding this comment

dtzWill Oct 5, 2019

Choose a reason for hiding this comment

michaelpj commented Oct 5, 2019

dtzWill commented Oct 5, 2019

worldofpeace commented Oct 5, 2019

worldofpeace commented Oct 2, 2019 •

edited

Loading