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 permissions on folder '/run/NetworkManager/' #422

Merged
merged 2 commits into from
Nov 23, 2023
Merged

Conversation

matdug
Copy link

@matdug matdug commented Nov 15, 2023

When the Netplan systemd generator runs before the NetworkManager service, Netplan will preempt NetworkManager from creating folder '/run/NetworkManager' and will assign it permissions 700.
This is a side effect of the call to umask in function write_nm_conf_access_point, made to restrict the permissions on subfolder '/run/NetworkManger/system-connections'. A regular user needs to read from /run/NetworkManager where, in some systems, files such as resolv.conf ultimately reside.

Description

In my Yocto image /etc/resolv.conf is a symlink to /etc/resolv-conf.NetworkManager itself to /run/Netw
orkManager/resolv.conf. Because the /run/NetworkManager folder created by Netplan has 700 permissions a regular user has no access to resolv.conf which breaks domain name resolution.

This fix assigns the proper access to rights to /run/NetworkManager, 755, if Netplan happens to be the first to create it, without elevating privileges to /run/NetworkManager/system-connections, which is what was intended by the original code.

Checklist

  • Runs make check successfully.
  • Retains 100% code coverage (make check-coverage).
  • New/changed keys in YAML format are documented.
  • (Optional) Adds example YAML for new feature.
  • (Optional) Closes an open bug in Launchpad.

@slyon slyon added the community This PR has been proposed by somebody outside of the Netplan team and roadmap commitments. label Nov 20, 2023
@slyon slyon self-requested a review November 23, 2023 13:52
Mathieu Dugal and others added 2 commits November 23, 2023 15:30
When the Netplan systemd generator runs before the NetworkManager service, Netplan will
preempt NetworkManager from creating folder '/run/NetworkManager' and will assign it
permissions 700.
This is a side effect of the call to umask in function write_nm_conf_access_point,
made to restrict the permissions on subfolder '/run/NetworkManger/system-connections'.
A regular user needs to read from /run/NetworkManager where, in some
systems, files such as resolv.conf ultimately reside.
Newer versions of pyflakes (3.0.0+) deprecated python2.x "# type:" comments (PEP 484).
Let's avoid that failure.
Copy link
Collaborator

@slyon slyon left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution to Netplan!

This is a good catch and you're very right! /run/NetworkManager/ should be 0o755, while only /run/NetworkManager/system-connections/ should be using 0o700 permissions (and we have tests in tests/generator/base.py to actually check for the latter).

PS: I also pushed a small additional commit to avoid the pyflakes 3.0.0+ linting error in the CI.

PPS: The failure in the "Autopkgtest CI" tunnels (gre6) test case are well known (LP#2037667) and unrelated to this PR. Ignoring.

* Letting the next invokation of safe_mkdir_p_dir do it would result in
* more restrictive access because of the call to umask. */
nm_run_path = g_strjoin(G_DIR_SEPARATOR_S, rootdir ?: "", "run/NetworkManager/", NULL);
if (!g_file_test(nm_run_path, G_FILE_TEST_EXISTS))
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: you had some leading whitespace here, which doesn't match our code style, so I quickly fixed this up.

@slyon slyon merged commit 2ae3b9e into canonical:main Nov 23, 2023
14 of 15 checks passed
@matdug
Copy link
Author

matdug commented Nov 24, 2023

Thanks for the merge, and the edits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community This PR has been proposed by somebody outside of the Netplan team and roadmap commitments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants