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

Setup $USER and $HOME if passwd for $USER has different uid #8879

Merged
merged 4 commits into from
May 2, 2022

Conversation

faho
Copy link
Member

@faho faho commented Apr 15, 2022

This gets the passwd entry for $USER (if it is set). If that gives the
same uid that getuid() gives us, we assume the data is correct.

If not, we reset $HOME and $USER from the passwd value for our UID.

Fixes #8583

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

This gets the passwd entry for $USER (if it is set). If that gives the
same uid that getuid() gives us, we assume the data is correct.

If not, we reset $HOME and $USER from the passwd value for our UID.

Fixes fish-shell#8583
@faho faho added this to the fish 3.5.0 milestone Apr 15, 2022
Copy link
Contributor

@septatrix septatrix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think that implementing this as a fish variable (e.g. fish_prompt_name) is preferable and more in line with (what user came to expect from) other shells. Setting USER and HOME when empty is fine however.

Modifying the users environment comes at the risk of doing something which the user did not intend. For example now LOGNAME and USER (the former of which is actually required by posix whereas the latter apparently is not) can become out if sync. Setting a fish variable instead provides users with the same behaviour regarding the environment variables they came to expect from bash/zsh et al. while providing a clear source for the name to display. This way one might also get rid of the technique proposed by faho which while fantastic still suffers from problems like duplicate usernames.

src/env.cpp Outdated Show resolved Hide resolved
src/env.cpp Outdated Show resolved Hide resolved
src/env.cpp Show resolved Hide resolved
src/env.cpp Outdated Show resolved Hide resolved
@faho
Copy link
Member Author

faho commented Apr 15, 2022

Modifying the users environment comes at the risk of doing something which the user did not intend. For example now LOGNAME and USER (the former of which is actually required by posix whereas the latter apparently is not) can become out if sync.

Again: $USER is what people will actually use. And it is what we already use, and what we document people use.

We aren't posix-compatible, so it's fine to simply define $USER as the thing to use.

I still think that implementing this as a fish variable (e.g. fish_prompt_name) is preferable and more in line with (what user came to expect from) other shells.

Fish is not other shells, and calling this something else just means you have now made the wrong way and the right way. (what is $USER even for if it's not corrected?)

Instead let's simply make the one right way to get the user.

This worked before, there's no real issue with su and stuff.
It feels weird to have $EUID but use the real UID here.
@septatrix
Copy link
Contributor

Well my underlying problem with this is that modern su implementations made the conscious choice to not set USER and LOGNAME for example if a script wants to lower privileges from root back to the UID of the invoking user. This is already broken by the current implementation making this use case impossible. And for chroot it is the same story.
Setting up anything besides SHELL and filling in empty variables is not something the shell should about and instead leave it to the login manager. However seeing as we both are very reluctant to change our stance this is something @ridiculousfish should probably finally decide...

@faho
Copy link
Member Author

faho commented Apr 15, 2022

However seeing as we both are very reluctant to change our stance this is something @ridiculousfish should probably finally decide...

Sorry, I am entirely capable of making decisions here. This is not how any of this works.

modern su implementations made the conscious choice

That's quite the assumption. The way I know these things, modern su implementations just do the same thing that su implementations have always done.

if a script wants to lower privileges from root back to the UID of the invoking user.

That's both not a common use case and doable in other ways.

This might be a case for a "$ORIGINAL_USER" variable.

But, like I said, we should make the default work, make the simple thing easy, instead of making the naive solution broken and the correct solution annoying.

@septatrix
Copy link
Contributor

However seeing as we both are very reluctant to change our stance this is something @ridiculousfish should probably finally decide...

Sorry, I am entirely capable of making decisions here. This is not how any of this works.

Given that you made a PR for this I figured that you wanted feedback from some of your colleagues and I do not think we will come to an agreement I do not see a point in us repeating our arguments ad infinitum and simply see what your colleagues think.

modern su implementations made the conscious choice

That's quite the assumption. The way I know these things, modern su implementations just do the same thing that su implementations have always done.

Yes they reimplement the behaviour from previous versions instead of changing it to preserve backwards compatibility and here we are talking about a behavior which would nullifylies their effort. I also asked this in the util-linux repo and he confirmed by gist about providing information about the invoking user and performing minimal environment changes unless the login flag is passed.

if a script wants to lower privileges from root back to the UID of the invoking user.

That's both not a common use case and doable in other ways.

It is a more common use case that starting a fish shell inside a chroot and no there is no possible way to know the invoking user if fish overrides that variable. Well actually there is LOGNAME but only because fish does not keep it in sync with USER.

This might be a case for a "$ORIGINAL_USER" variable.

Then you would implement a workaround variable for a workaround and everyone would have to special case if there was a fish shell between su and the program changing USER and setting ORIGINAL_USER

@faho
Copy link
Member Author

faho commented Apr 16, 2022

Given that you made a PR for this I figured that you wanted feedback from some of your colleagues and I do not think we will come to an agreement I do not see a point in us repeating our arguments ad infinitum and simply see what your colleagues think.

Yes, and they will give their feedback if they have any, once they have the time.

There is no need to @ specific people, and this specific example comes across a bit like you are asking for my manager to overrule me.


Then you would implement a workaround variable for a workaround

I do not believe it is a "workaround" to have someone asking for specifically the unusual "original user" as opposed to the more typical "current user" to have to specify that.

performing minimal environment changes unless the login flag is passed.

Yes, and you have been arguing that "There is no reason to use with su without --login/-l/-".

With "--login" it changes $USER.

(tbh I would naively expect it to be the other way around - $LOGNAME, i.e. "login name" should only be changed with "--login". Oh well)

have to special case if there was a fish shell between su and the program changing USER

None of this is standardized or guaranteed. If you are relying on $USER here you are making unportable and brittle assumptions already. Relying on $USER to give you the "original" user can very easily be circumvented already. What if I set $USER to someone else before starting your script? Will it then become that other user instead of me? That doesn't sound like a good idea!

@septatrix
Copy link
Contributor

septatrix commented Apr 16, 2022

have to special case if there was a fish shell between su and the program changing USER

None of this is standardized or guaranteed. If you are relying on $USER here you are making unportable and brittle assumptions already. Relying on $USER to give you the "original" user can very easily be circumvented already.

Yes but if I ship installation instructions with my software and tell the to su to a different user and call some scripts I can very well expect the same behaviour for all shells but fish.

What if I set $USER to someone else before starting your script? Will it then become that other user instead of me? That doesn't sound like a good idea!

Which is why my proposal would be to introduce a fish variable (which could be made read-only to prevent such tampering) which always returns the current pw_name. Also the EUID variable would be more suited for a fish variable instead of an environment variable.

This way fish could always show the correct user in the prompt and properly check if a red root prompt should be displayed (if caching is not required it would even be possible to update the username direct in the prompt without a religion). User and developers could then be sure that fish does not touch their environment and everything is as one is used from other shells.

Then you would implement a workaround variable for a workaround

I do not believe it is a "workaround" to have someone asking for specifically the unusual "original user" as opposed to the more typical "current user" to have to specify that.

No the workaround is to update the user variable upon shell start (and afterwards trusting it which could still be confusing) instead of always having a live variable which grants reliable access to such information. Also while unusual you now made it impossible to get the original user when using su (please correct me if I am wrong). Also what if someone uses LOGNAME instead of USER? They will now be very surprised. Instead in scripts one should either use id -un (for compatibility with other shells) or a proposed fish_displayname to save a subprocess call.

@faho
Copy link
Member Author

faho commented May 2, 2022

Okay, over two weeks with no comment from anyone else. That means I'm making an executive decision and merging this.

If we get any reports of problems, we can revisit this. I don't foresee any because we've already been doing this in the most common case (when uid is 0).

@faho faho merged commit dd95e0a into fish-shell:master May 2, 2022
@faho faho removed the RFC label May 2, 2022
@faho faho deleted the fix-home-and-user branch May 24, 2022 09:04
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't trust $USER
2 participants