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

polkit rules generation #100

Merged
merged 6 commits into from
Jul 22, 2023
Merged

Conversation

tomeon
Copy link
Collaborator

@tomeon tomeon commented Apr 1, 2022

This is a draft pull request that introduces the ability to dynamically generate polkit rules that allow update-systemd-resolved to perform its various DBus calls when run as an unprivileged user.

Motivation

  1. Make it easier for folks who run their OpenVPN client under unprivileged users to create appropriate polkit rules, minimizing the amount of hand-editing required. On the happiest path, the generated rules can be used without any edits at all.
  2. Make it easier to update polkit rules in the future, if (for instance) update-systemd-resolved adds or removes org.freedesktop.resolve1 DBus call types.
  3. Support automated/on-demand rules generation for use-cases like creating appropriate polkit rules as part of building a NixOS system configuration.

Summary of changes

  1. Add the action update-systemd-resolved print-polkit-rules, with various options as documented in the README.md changes.
  2. Add NixOS-based tests intended to exercise the generated polkit ruleset by attempting to perform certain actions that should succeed only if the generated rules work as intended. Note that I implemented the tests using NixOS's test framework because (a) I'm familiar with it and (b) it's the easiest option I know of for performing systems-level testing that requires multiple machines and nontrivial setup steps. I'd be happy to re-implement the tests using whatever tooling the update-systemd-resolved maintainers prefer.

Caveats

  1. My facility with JavaScript is basic, at best. The generated polkit rules work (on my machine ;)), but I am not confident they are free of glaring style gaffes or other problems.
  2. The rules generation process involves printing the contents of an argument vector ("$@") as a JSON array. jq is used for this, if available, falling back to a pure-shell implementation on systems where jq is absent. This fallback implementation is, to put it kindly, dodgy in the extreme: it escapes " characters and passes everything else through untouched. This is a potential security hole, if a bad actor can somehow influence the list of arguments passed to the JSON-generating function. I could implement other fallback functions (e.g., using Python and the json or simplejson module, if available, using Perl and JSON::PP, JSON::XS, etc., if available, and so on). WDYT?

TODO

  1. Help text. Omitted for the time being because update-systemd-resolved does not yet implement --help for existing options. Going to save this for a future PR.
  2. Developer documentation. For example, guidance on installing the Nix package manager and instructions on how to update Nix flake inputs.
  3. Reconsider certain diagnostics. With the current changeset, update-systemd-resolved dumps a polkit rules definition to the system journal when certain operations fail (to let the user know that how they may need to update their polkit rules to allow update-systemd-resolved to do its thing). Instead, maybe update-systemd-resolved could print the polkit rules generation command to the journal -- e.g., update-system-resolved --polkit-allowed-user <uid> --polkit-allowed-group <gid>.

Thanks in advance for your consideration!

@tomeon tomeon marked this pull request as draft April 1, 2022 14:03
@tomeon
Copy link
Collaborator Author

tomeon commented Apr 1, 2022

Also: having trouble with my personal Travis CI account, so here's a link to a passing build on builds.sr.ht.

@tomeon tomeon force-pushed the polkit-rules-definition branch 4 times, most recently from fd9037e to 11f5ac5 Compare April 9, 2022 20:52
@tomeon tomeon force-pushed the polkit-rules-definition branch 6 times, most recently from 9d8e4c2 to 980072e Compare July 20, 2023 15:54
@tomeon tomeon marked this pull request as ready for review July 20, 2023 16:02
@tomeon
Copy link
Collaborator Author

tomeon commented Jul 20, 2023

@piotr-dobrogost -- since you gave this PR a 👍 a while back, any chance I could impose upon you for a review? Please and thank you 😃 .

@tomeon
Copy link
Collaborator Author

tomeon commented Jul 20, 2023

The rules generation process involves printing the contents of an argument vector ("$@") as a JSON array. jq is used for this, if available, falling back to a pure-shell implementation on systems where jq is absent. This fallback implementation is, to put it kindly, dodgy in the extreme: it escapes " characters and passes everything else through untouched. This is a potential security hole, if a bad actor can somehow influence the list of arguments passed to the JSON-generating function. I could implement other fallback functions (e.g., using Python and the json or simplejson module, if available, using Perl and JSON::PP, JSON::XS, etc., if available, and so on). WDYT?

I have implemented JSON serialization using Perl and Python.

@tomeon tomeon changed the title Draft/RFC: polkit rules generation polkit rules generation Jul 20, 2023
@tomeon tomeon force-pushed the polkit-rules-definition branch 4 times, most recently from 8acd6c2 to 5047262 Compare July 22, 2023 15:08
that prints a polkit rules definition to standard output. These rules
permit specified users and/or groups to perform the DBus calls necessary
for update-systemd-resolved's proper operation.
when the prerequisites for such are available.
when building polkit rules.

[skip ci]
@tomeon tomeon merged commit 41b1a36 into jonathanio:master Jul 22, 2023
@tomeon tomeon deleted the polkit-rules-definition branch September 8, 2023 01:24
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

Successfully merging this pull request may close these issues.

1 participant