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

chrony: Create state directory with correct owner. #97592

Merged
merged 1 commit into from Sep 10, 2020

Conversation

kevincox
Copy link
Contributor

@kevincox kevincox commented Sep 9, 2020

Fixes #97546

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ajs124
Copy link
Member

ajs124 commented Sep 9, 2020

Why does chrony need to be started as root?

@kevincox
Copy link
Contributor Author

kevincox commented Sep 9, 2020

I asked that question on #97546. It would be nice to fix and remove the tmpfiles rule instead. However I created this as the quick fix that we know works. It is weird because the config does give chrony a capability, but then runs as root anyways.

@ajs124
Copy link
Member

ajs124 commented Sep 9, 2020

Hm. What's weird is that I'm running chrony on two systems (on 20.03) and I don't see this issue. Both instances have /var/lib/chrony/chrony.drift files which changed yesterday, so they're definitely being written to. The folders belong to chrony on both systems. Since it's dropping privileges (-u chrony) this really shouldn't be a problem.

@kevincox
Copy link
Contributor Author

kevincox commented Sep 9, 2020

Hmm, that's odd. The documented behaviour of StateDirectory is clearly to chown that directory to root. At first I thought that having that file owned by chrony would be enough but I don't think it is because chrony does a create+rename to write it. So unless it has a less safe fallback for when it doesn't have permissions it would fail if that directory every got chowned to root. I am quite curious how it is working on your system. Does your chronyd.service file have the StateDirectory= set?

Since it's dropping privileges (-u chrony) this really shouldn't be a problem.

I'm not quite sure I understand. Do you mean that it shouldn't be a problem not running as root because it drops privileges anyways or it shouldn't be a problem to write that file because it is started as root?

@ajs124
Copy link
Member

ajs124 commented Sep 9, 2020

I'm not quite sure I understand. Do you mean that it shouldn't be a problem not running as root because it drops privileges anyways or it shouldn't be a problem to write that file because it is started as root?

Yeah… that was some faulty logic on my part, sorry.

In fact, my chronyd.services do not have StateDirectory set, because I'm on 20.03 and it was only introduced by @flokli in f25a301/#73934.

As for why chronyd runs as root in the first place, they seem to have a check for that, which can be bypassed as such:

  chrony = super.chrony.overrideAttrs (oA: {
    configureFlags = (oA.configureFlags or []) ++ [ "--with-pidfile=/run/chrony/chrony.pid" "--chronyrundir=/run/chrony" ];

    postPatch = ''
      ${oA.postPatch or ""}

      substituteInPlace main.c --replace 'LOG_FATAL("Not superuser")' '((void)0)'
    '';
  });

Where the configureFlags might not be strictly necessary.

With all that said, I'd say we could merge this as a quick fix, but in the long run it would definitely be better to not run this as root, in the first place.

@kevincox
Copy link
Contributor Author

I see. That all makes sense then. I propose the following:

  1. Merge this as the fix for now. If you approve I will merge.
  2. Contact the chrony devs to update to not check for root.
  3. If they are unresponsive we can consider patching crhony, however I would prefer to stick to upstream as possible.

@ajs124
Copy link
Member

ajs124 commented Sep 10, 2020

Can you link the upstream issue (or mail archive or whatever they have) here, once you've contacted them?

@kevincox kevincox merged commit 91032af into master Sep 10, 2020
@aanderse
Copy link
Member

@kevincox as part of cleanup can we delete this branch now?

@kevincox kevincox deleted the kevincox-chrony-state branch September 10, 2020 17:53
@kevincox
Copy link
Contributor Author

@ajs124 I filed #97718 to track the root issue.

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.

chronyd doesn't own state directory.
3 participants