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

Jump to bottom

nixos/krb5: allow lists in configuration #89748

Merged
merged 3 commits into from Sep 3, 2020
Merged

nixos/krb5: allow lists in configuration #89748

merged 3 commits into from Sep 3, 2020

Conversation

heinic
Copy link
Contributor

@heinic heinic commented Jun 7, 2020

Motivation for this change

Fix #89626

Kerberos allows multiple values for kdc to be specified, this should be possible by passing a list to config.krb5.realms.<realm>.kdc. In general, when specifying a list, it should end up as multiple lines of <attrName> = <list item> in krb5.conf.

Although there has been code to accept lists, evaluating would lead to attrs to be passed to concatMapStringsSep instead of a list as the last parameter, causing the evaluation to fail. Because of this, removing it is not a breaking change.

nixpkgs/nixos/modules/config/krb5/default.nix

Lines 52 to 53 in adc908a

else if (isList value) then
concatMapStringsSep " " mkVal { inherit value depth; }

Things done

Simplified indentation generation, allowing mkRelation to be more easily reused by mkVal (and vice versa). Added list handling to mkRelation and removed list handling from mkVal.

  • 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
    There is no documentation beside the examples, changing the relevant example would be an option.
  • Fits CONTRIBUTING.md.

@heinic
nixos/krb5: refactor indentation generation
5ed0924 
mkVal is no longer aware of the indentation depth. Indentation is
added to each line of the result when it is included in nested
entries.
@heinic
nixos/krb5: output lists as multiple config entries
2b694b1 
Fixes NixOS#89626
@Mic92 Mic92 requested a review from eqyiel June 8, 2020 13:57
@Mic92
Copy link
Member

Mic92 commented Jun 8, 2020

@GrahamcOfBorg test krb5

@eqyiel
Copy link
Contributor

eqyiel commented Aug 22, 2020

@heinic I'm sorry for sleeping on this for so long. Your work looks good to me. I'm not sure why the @GrahamcOfBorg command didn't work but I ran the tests locally, and it seems fine.

~/.nix-defexpr/nixpkgs pr-upstream-89748 eqyiel@hoshijiro
❯ git log -1
commit 2b694b1e9f69ba8053bb7006337b051154c44e86 (HEAD -> pr-upstream-89748)
Author: Nico Heitmann <nico.heitmann01@gmx.de>
Date:   Sun Jun 7 16:10:35 2020 +0200

    nixos/krb5: output lists as multiple config entries

    Fixes #89626

~/.nix-defexpr/nixpkgs pr-upstream-89748 eqyiel@hoshijiro
❯ nix-build nixos/tests/krb5/example-config.nix -I nixpkgs=$HOME/.nix-defexpr/nixpkgs

~/.nix-defexpr/nixpkgs pr-upstream-89748 eqyiel@hoshijiro
❯ nix-build nixos/tests/krb5/deprecated-config.nix -I nixpkgs=$HOME/.nix-defexpr/nixpkgs

Ideally cfg.realms would be types.attrsOf types.submodule with all the possible keys defined but I'm not confident that this list is exhaustive for both Heimdal and MIT Kerberos.

I think a reasonable compromise would be to change the snapshot test so that it covers list serialisation.

Would you mind making a change like this?

diff --git a/nixos/tests/krb5/example-config.nix b/nixos/tests/krb5/example-config.nix
index be195b51393..e2e10a9fda8 100644
--- a/nixos/tests/krb5/example-config.nix
+++ b/nixos/tests/krb5/example-config.nix
@@ -18,7 +18,10 @@ import ../make-test-python.nix ({ pkgs, ...} : {
         realms = {
           "ATHENA.MIT.EDU" = {
             admin_server = "athena.mit.edu";
-            kdc = "athena.mit.edu";
+            kdc = [
+              "athena01.mit.edu"
+              "athena02.mit.edu"
+            ];
           };
         };
         domain_realm = {
@@ -65,7 +68,8 @@ import ../make-test-python.nix ({ pkgs, ...} : {
       [realms]
         ATHENA.MIT.EDU = {
           admin_server = athena.mit.edu
-          kdc = athena.mit.edu
+          kdc = athena01.mit.edu
+          kdc = athena02.mit.edu
         }

       [domain_realm]

@heinic
nixos/krb5: add list to example configuration
0bee87c 
Updated the relevant nixos test to match the example configuration.
@heinic
Copy link
Contributor Author

heinic commented Aug 25, 2020

Updated the test like you suggested. I also changed the example for the nixos option, so it matches the updated test.

@heinic
Copy link
Contributor Author

heinic commented Aug 25, 2020

@ofborg test krb5

@eqyiel
Copy link
Contributor

eqyiel commented Aug 29, 2020

@heinic I don't have write access so someone else would have to merge this, but it looks good to me.

@Mic92 Mic92 merged commit 02a2649 into NixOS:master Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eqyiel eqyiel eqyiel approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

krb5.realms.<realm>.kdc should allow multiple hosts
4 participants
@heinic @Mic92 @eqyiel @Janik-Haag