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

nixos/knot: add keyFiles option #79266

Merged
merged 3 commits into from Feb 15, 2020
Merged

nixos/knot: add keyFiles option #79266

merged 3 commits into from Feb 15, 2020

Conversation

@Mic92
Copy link
Contributor

@Mic92 Mic92 commented Feb 5, 2020

Add an option to include configuration not in the nix store.
The use case for this are nixops like tools where the configuration
is not world-readable. Because of knot's DynamicUser we need to
install those files with the correct permission on startup into /var/lib/knot.

Motivation for this change
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.
@Mic92
Copy link
Contributor Author

@Mic92 Mic92 commented Feb 5, 2020

Because of DynamicUser the permission management is not yet very straight forward:

services.knot = {
  enable = true;
  checkConfig = false;
  extraConfig = ''
    include: /var/lib/knot/knot-he-key.conf
  '';
};
systemd.services.knot = {
  serviceConfig.PermissionsStartOnly = true;
  preStart = ''
    install -m700 --owner $USER /var/src/secrets/knot-he-key.conf /var/lib/knot/knot-he-key.conf
  '';
};

Maybe rather than checkConfig I could also add an option to add custom secret files?

Copy link
Member

@mweinelt mweinelt left a comment

I understand your use case and the change is fine with me.

@vcunat
vcunat approved these changes Feb 5, 2020
@vcunat
Copy link
Member

@vcunat vcunat commented Feb 5, 2020

I haven't really thought about best practices for managing these secrets (for knot service).

@Mic92
Copy link
Contributor Author

@Mic92 Mic92 commented Feb 5, 2020

I though about adding an systemd.services.knot.keyFiles = [ "/path/to/tsig.conf" ];.
Those would be copied to /var/lib/knot/keys.d and than included in knot's default configuration.

@vcunat
Copy link
Member

@vcunat vcunat commented Feb 5, 2020

I wonder if the risk of omitting the quotation marks is too high (leaking secrets by accident) 🤔

EDIT: I expect such mistakes can be detected in eval-time by some type-checking.

@Mic92
Copy link
Contributor Author

@Mic92 Mic92 commented Feb 5, 2020

I wonder if the risk of omitting the quotation marks is too high thinking

Usually when using nixops is using something like /run/keys, which is likely to fail on the developer machine. In theory all passwordFile options in nixos could be accidentally miss-used like that. However I see a fix for that outside of scope of this PR. This would be solved by a linter or custom nixos type option that check if the path is in the nix store.

@symphorien
Copy link
Contributor

@symphorien symphorien commented Feb 5, 2020

About passwordFile like options: #78640

@Mic92
Copy link
Contributor Author

@Mic92 Mic92 commented Feb 8, 2020

With the latest commit knot now can do automatic DNSSEC signing.

@ofborg ofborg bot requested a review from vcunat Feb 8, 2020
@Mic92 Mic92 force-pushed the Mic92:knot branch from 713bfcb to 9456fcd Feb 8, 2020
@Mic92 Mic92 changed the title nixos/knot: make configuration check optional nixos/knot: add keyFiles option Feb 8, 2020
@@ -5,14 +5,16 @@ with lib;
let
cfg = config.services.knot;

configFile = pkgs.writeText "knot.conf" cfg.extraConfig;
socketFile = "/run/knot/knot.sock";
configFile = pkgs.writeTextFile {

This comment has been minimized.

@Mic92

Mic92 Feb 8, 2020
Author Contributor

I tried to convert nix attrsets to yaml using remarshal. However the yaml subset chosen by knot is too restrictive for that (i.e. expect keys to appear in a certain order).

This comment has been minimized.

@vcunat

vcunat Feb 8, 2020
Member

Have you seen this thread? If a few people tries this out in practice, I suppose we could make it available as another optional attribute.

This comment has been minimized.

@vcunat

vcunat Feb 8, 2020
Member

I'm not aware of keys being expected in particular order, though (perhaps I misunderstand). In any case the yaml is stricter than what I see usually (and new yaml is even a super-set of json IIRC).

This comment has been minimized.

@Mic92

Mic92 Feb 8, 2020
Author Contributor

I had an issue with an acl rule where the id key was not the first key, which lead to an error. This broke already in the nixos test. Anyway this PR should not interfere with migrating to a different contribute type...

This comment has been minimized.

@vcunat

vcunat Feb 8, 2020
Member

Oh, that's a known restriction, though not by me until now.

This comment has been minimized.

@vcunat

vcunat Feb 22, 2020
Member

I think I have an idea how to approach this; hopefully I can get it to work soon. I'll link from here when that happens.

This comment has been minimized.

@vcunat

vcunat Mar 1, 2020
Member

It didn't really go the way I imagined, but let's continue on #81460

@Mic92
Copy link
Contributor Author

@Mic92 Mic92 commented Feb 8, 2020

Instead of adding an option to disable configuration checks I added a keyFiles option that implicitly disable configuration checks when used. This also gets permissions right in combination with knot's DynamicUser option.

@Mic92
Copy link
Contributor Author

@Mic92 Mic92 commented Feb 8, 2020

Here is a real-world example: https://github.com/Mic92/dotfiles/blob/34a97d3292b833fbad8f716f9c37a28eb4121eb1/nixos/eve/modules/knot/default.nix

Also the nixos test covers the new feature as well.

@Mic92 Mic92 mentioned this pull request Feb 12, 2020
2 of 10 tasks complete
Mic92 added 3 commits Feb 7, 2020
Otherwise knot tries to write to non-writable directories.
This for example breaks dnssec signing.
While it's possible to overwrite these path in the configuration,
having a sane defaults is nicer.
This makes it hard to include secret files.
Also using tools like keymgr becomes harder.
This useful to include tsig keys using nixops without adding those
world-readable to the nix store.
@Mic92 Mic92 force-pushed the Mic92:knot branch from 9456fcd to e2ef8b4 Feb 12, 2020
@Mic92
Copy link
Contributor Author

@Mic92 Mic92 commented Feb 12, 2020

I dropped DynamicUser. Also because of #79928

@Mic92
Copy link
Contributor Author

@Mic92 Mic92 commented Feb 13, 2020

@ju1m If you would have a short look at it and give me a thumbs up, I would go forward and merge it!

@Mic92 Mic92 merged commit 466c1df into NixOS:master Feb 15, 2020
13 checks passed
13 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@Mic92 Mic92 deleted the Mic92:knot branch Feb 15, 2020
dtzWill added a commit to dtzWill/nixpkgs that referenced this pull request Feb 15, 2020
nixos/knot: add keyFiles option

(cherry picked from commit 466c1df)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.