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

Warn on deprecated /var/run in shell paths #60996

Closed
wants to merge 3 commits into from

Conversation

layus
Copy link
Member

@layus layus commented May 5, 2019

Motivation for this change

The /var/run deprecation in nixpkgs removed
/var/run/current-system/sw/ from environment.shells and hence
from /etc/shells without any warning to users still using
/var/run/.. paths in users.defaultUserShell or in users.*.shell.

This PR tries to smooth the migration by throwing an error on such
shells in configs.

It may be better to warn instead of throwing an error, but at the same time you do not want users to be locked out of their machine because ssh thinks that their shell is invalid/not allowed.

The issue was raised by @Mic92 in @peterhoeg 's PR #47856 (review) but was lost in the other comments and the migration to another PR.

Things done

/cc @bobvanderlinden, @globin, @flokli for working on #51918

@layus layus requested review from edolstra and nbp as code owners May 5, 2019 16:52
/var/run is deprecated, shells should not use it anymore.
It would also be the right place to check that store paths are not used.
@layus layus changed the title Revert /var/run -> /run for shell paths Warn on deprecated /var/run in shell paths May 5, 2019
lib/types.nix Outdated Show resolved Hide resolved
lib/types.nix Outdated Show resolved Hide resolved
@bobvanderlinden
Copy link
Member

Thanks a lot for this PR. I wasn't aware of this problem. I can indeed imagine that people will be locked out. Just to be sure: this will not happen to ssh sessions and the like? If it does impact ssh sessions, would it make sense to always try to rewrite the path?

@layus
Copy link
Member Author

layus commented May 5, 2019

From tests here it does not lock you out from ssh. But it does indeed tamper with services that look in /etc/shell to test for normal vs systems users, like gdm via accounts-daemon, or ftp servers apparently[1].

I discovered the issue with gdm that suddenly had no user to show in the users selection dropdown. All were considered system users for their shells were not in /etc/shells.

@layus
Copy link
Member Author

layus commented May 5, 2019

Now that ssh seems not to be impacted, throwing an error seems a bit overkill. I turned it into a warning, but kept the evaluation failing. If we prefer not to fail, we just need to flip the boolean.

BTW, we definitely need a mailing list for changes like this. Like Arch does.

@matthewbauer
Copy link
Member

@layus for now you can post these in Discourse: https://discourse.nixos.org/

but an "announcements" mailing list could definitely be useful.

@worldofpeace
Copy link
Contributor

I can say there's at least one confirmed report of this causing problems for a user

@doronbehar
Copy link
Contributor

(triage) What say you @worldofpeace & @matthewbauer ?

@flokli
Copy link
Contributor

flokli commented May 23, 2020

I'd assume this is already "too late" - users upgrading to a new release in the last 1.5 years most likely already stumbled into this and migrated away from the old config.

@Mic92
Copy link
Member

Mic92 commented May 24, 2020

Agreed.

@Mic92 Mic92 closed this May 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants