From 754355e7f60cd7feff20b6654f784f0e311a15a2 Mon Sep 17 00:00:00 2001 From: h7x4 Date: Mon, 16 Sep 2024 19:33:45 +0200 Subject: [PATCH 1/9] nixos/{assertions,warnings}: use structured attrs --- nixos/modules/misc/assertions.nix | 196 ++++++++++++++++-- nixos/modules/system/activation/top-level.nix | 56 ++++- 2 files changed, 239 insertions(+), 13 deletions(-) diff --git a/nixos/modules/misc/assertions.nix b/nixos/modules/misc/assertions.nix index ae8fa710b2b8c..071ba359c4064 100644 --- a/nixos/modules/misc/assertions.nix +++ b/nixos/modules/misc/assertions.nix @@ -1,18 +1,108 @@ { lib, ... }: { - options = { - assertions = lib.mkOption { - type = lib.types.listOf lib.types.unspecified; + type = + let + assertionItemType = lib.types.submodule ( + { config, ... }: + { + options = { + enable = lib.mkOption { + description = '' + Whether to enable this assertion. + + This option is mostly useful for users, in order to forcefully disable assertions they believe to be + erroneous while waiting for someone to fix the assertion upstream. + ''; + type = lib.types.bool; + default = true; + example = false; + }; + + assertion = lib.mkOption { + description = "Condition to be asserted. If this is `false`, the evaluation will throw the error"; + type = lib.types.bool; + example = false; + }; + + message = lib.mkOption { + description = "The contents of the error message that should be shown upon triggering a false assertion"; + type = if config.lazy then lib.types.unspecified else lib.types.nonEmptyStr; + example = "This is an example error message"; + }; + + lazy = lib.mkOption { + description = '' + Whether to avoid evaluating the message contents until the assertion condition occurs. + + This will also disable typechecking. + + ::: {.note} + We do not recommend you enable this. It is mostly intended for backwards compatibility. + If you do need to enable it, make sure to double check that your `message` always will + evaluate successfully whenever the assertion would trigger. + ::: + ''; + type = lib.types.bool; + default = false; + example = true; + }; + }; + } + ); + + # This might be replaced when tagged submodules or similar are available, + # see https://github.com/NixOS/nixpkgs/pull/254790 + checkedAssertionItemType = + let + check = x: x ? assertion && x ? message; + in + lib.types.addCheck assertionItemType check; + + nestedAssertionAttrsType = + with lib.types; + let + nestedAssertionItemType = oneOf [ + checkedAssertionItemType + (attrsOf nestedAssertionItemType) + ]; + in + nestedAssertionItemType + // { + description = "nested attrs of (${assertionItemType.description})"; + }; + + # Backwards compatibility for assertions that are still written as attrs inside a list. + # The attribute name will be set to the sha256 sum of the assertion message, e.g. `assertions.legacy."" = { ... }` + coercedAssertionAttrs = + let + coerce = xs: { + legacy = lib.listToAttrs ( + lib.imap0 (i: assertion: lib.nameValuePair "anon-${toString i}" (assertion // { lazy = true; })) xs + ); + }; + in + with lib.types; + coercedTo (listOf (attrsOf unspecified)) coerce (submodule { + freeformType = nestedAssertionAttrsType; + }); + in + coercedAssertionAttrs; internal = true; - default = [ ]; - example = [ + default = { }; + example = lib.literalExpression '' { - assertion = false; - message = "you can't enable this for that reason"; + programs.foo.dontUseSomeOption = { + assertion = !config.programs.foo.settings.someOption; + message = "You can't enable foo's someOption for some reason"; + }; + services.bar.mutuallyExclusiveWithFoo = { + assertion = config.services.bar.enable -> !config.programs.foo.enable; + message = "You can't use the 'foo' program if you're using the 'bar' service"; + }; } - ]; + ''; description = '' This option allows modules to express conditions that must hold for the evaluation of the system configuration to @@ -21,16 +111,98 @@ }; warnings = lib.mkOption { + type = + let + warningItemType = lib.types.submodule { + options = { + enable = lib.mkOption { + description = '' + Whether to enable this warning. + + This option is mostly useful for users, in order to forcefully disable warnings they believe to be + erroneous while waiting for someone to fix the condition upstream. + ''; + type = lib.types.bool; + default = true; + example = false; + }; + + condition = lib.mkOption { + description = "Condition that triggers the warning message. If this is `true`, the warning will be shown"; + type = lib.types.bool; + example = true; + }; + + message = lib.mkOption { + description = "The contents of the warning message that should be shown upon triggering the condition"; + type = lib.types.nonEmptyStr; + example = "This is an example warning message"; + }; + }; + }; + + # This might be replaced when tagged submodules or similar are available, + # see https://github.com/NixOS/nixpkgs/pull/254790 + checkedWarningItemType = + let + check = x: x ? condition && x ? message; + in + lib.types.addCheck warningItemType check; + + nestedWarningAttrsType = + with lib.types; + let + nestedWarningItemType = oneOf [ + checkedWarningItemType + (attrsOf nestedWarningItemType) + ]; + in + nestedWarningItemType + // { + description = "nested attrs of (${warningItemType.description})"; + }; + + # Backwards compatibility for warnings that are still written as strings inside a list. + # The attribute name will be set to the sha256 sum of the warning message, e.g. `warnings."" = { ... }` + coercedWarningAttrs = + let + coerce = xs: { + legacy = lib.listToAttrs ( + map ( + message: + lib.nameValuePair (builtins.hashString "sha256" message) { + inherit message; + condition = true; + } + ) xs + ); + }; + in + with lib.types; + coercedTo (listOf str) coerce (submodule { + freeformType = nestedWarningAttrsType; + }); + in + coercedWarningAttrs; internal = true; - default = [ ]; - type = lib.types.listOf lib.types.str; - example = [ "The `foo' service is deprecated and will go away soon!" ]; + default = { }; + example = lib.literalExpression '' + { + services.foo.deprecationNotice = { + condition = config.services.foo.enable; + message = "The `foo' service is deprecated and will go away soon!"; + }; + services.bar.otherWarning = { + condition = !config.services.bar.settings.importantOption; + message = "You might want to enable services.bar.settings.importantOption, or everything is going to break"; + }; + } + ''; description = '' This option allows modules to show warnings to users during the evaluation of the system configuration. ''; }; - }; # impl of assertions is in # - diff --git a/nixos/modules/system/activation/top-level.nix b/nixos/modules/system/activation/top-level.nix index b8662e992965e..618262971c651 100644 --- a/nixos/modules/system/activation/top-level.nix +++ b/nixos/modules/system/activation/top-level.nix @@ -77,7 +77,61 @@ let ); # Handle assertions and warnings - baseSystemAssertWarn = lib.asserts.checkAssertWarn config.assertions config.warnings baseSystem; + collectUntil = + let + collectUntil' = + path: pred: dontProceedPred: value: + if pred value then + [ { inherit path value; } ] + else if dontProceedPred value then + [ ] + else if isAttrs value then + concatMap ({ name, value }: collectUntil' (path ++ [ name ]) pred dontProceedPred value) ( + attrsToList value + ) + else + [ ]; + in + collectUntil' [ ]; + + formatAttrPath = + let + escape = s: "\"" + (builtins.replaceStrings [ "\"" ] [ "\\\"" ] s) + "\""; + in + lib.concatMapStringsSep "." ( + p: if builtins.match "[a-zA-Z_-][a-zA-Z0-9_-]*" p == null then escape p else p + ); + + failedAssertions = + let + assertions = collectUntil ( + x: builtins.isAttrs x && !(x.assertion or true) && (x.enable or false) && x ? message + ) (x: builtins.isAttrs x && (x.lazy or false)) config.assertions; + in + map (x: "assertions." + (formatAttrPath x.path) + ":\n" + x.value.message) assertions; + + failedWarnings = + let + collect' = + let + collect'' = + path: pred: value: + if pred value then + [ { inherit path value; } ] + else if isAttrs value then + concatMap ({ name, value }: collect'' (path ++ [ name ]) pred value) (attrsToList value) + else + [ ]; + in + collect'' [ ]; + + warnings = collect' ( + x: isAttrs x && (x.condition or false) && (x.enable or false) && x ? message + ) config.warnings; + in + map (x: "warnings." + (formatAttrPath x.path) + ":\n" + x.value.message) warnings; + + baseSystemAssertWarn = lib.asserts.checkAssertWarn failedAssertions failedWarnings baseSystem; # Replace runtime dependencies system = From b52d1a3ffc8af8a4c1bbdf7ef689fb1024dd4c1d Mon Sep 17 00:00:00 2001 From: h7x4 Date: Mon, 16 Sep 2024 19:33:48 +0200 Subject: [PATCH 2/9] nixos/doc: document structured assertions/warnings --- .../manual/development/assertions.section.md | 27 ++++++++----------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/nixos/doc/manual/development/assertions.section.md b/nixos/doc/manual/development/assertions.section.md index ad68398bf830a..46ba1458a069c 100644 --- a/nixos/doc/manual/development/assertions.section.md +++ b/nixos/doc/manual/development/assertions.section.md @@ -12,16 +12,13 @@ This is an example of using `warnings`. { config, lib, ... }: { config = lib.mkIf config.services.foo.enable { - warnings = - if config.services.foo.bar then - [ - '' - You have enabled the bar feature of the foo service. - This is known to cause some specific problems in certain situations. - '' - ] - else - [ ]; + warnings.services.foo.beCarefulWithBar = { + condition = config.services.foo.bar; + message = '' + You have enabled the bar feature of the foo service. + This is known to cause some specific problems in certain situations. + ''; + }; }; } ``` @@ -34,12 +31,10 @@ This example, extracted from the [`syslogd` module](https://github.com/NixOS/nix { config, lib, ... }: { config = lib.mkIf config.services.syslogd.enable { - assertions = [ - { - assertion = !config.services.rsyslogd.enable; - message = "rsyslogd conflicts with syslogd"; - } - ]; + assertions.services.syslogd.rsyslogdConflict = { + assertion = !config.services.rsyslogd.enable; + message = "rsyslogd conflicts with syslogd"; + }; }; } ``` From bd9d89047ffcfe93a59072024504a92d1bcd9d86 Mon Sep 17 00:00:00 2001 From: h7x4 Date: Mon, 16 Sep 2024 19:33:48 +0200 Subject: [PATCH 3/9] lib/modules: use structured assertions/warnings --- lib/modules.nix | 47 ++++++++++++++----------- lib/tests/modules/doRename-warnings.nix | 27 ++++++++++++-- 2 files changed, 51 insertions(+), 23 deletions(-) diff --git a/lib/modules.nix b/lib/modules.nix index 3330579952dea..48426715a2734 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -1573,19 +1573,18 @@ let x: throw "The option `${showOption optionName}' can no longer be used since it's been removed. ${replacementInstructions}"; }); - config.assertions = + config.assertions = setAttrByPath (optionName ++ [ "removed" ]) ( let opt = getAttrFromPath optionName options; in - [ - { - assertion = !opt.isDefined; - message = '' - The option definition `${showOption optionName}' in ${showFiles opt.files} no longer has any effect; please remove it. - ${replacementInstructions} - ''; - } - ]; + { + assertion = !opt.isDefined; + message = '' + The option definition `${showOption optionName}' in ${showFiles opt.files} no longer has any effect; please remove it. + ${replacementInstructions} + ''; + } + ); }; /** @@ -1703,15 +1702,20 @@ let ); config = { - warnings = filter (x: x != "") ( + warnings = foldl' recursiveUpdate { } ( map ( - f: - let - val = getAttrFromPath f config; - opt = getAttrFromPath f options; - in - optionalString (val != "_mkMergedOptionModule") - "The option `${showOption f}' defined in ${showFiles opt.files} has been changed to `${showOption to}' that has a different type. Please read `${showOption to}' documentation and update your configuration accordingly." + path: + setAttrByPath (path ++ [ "mergedOption" ]) { + condition = (getAttrFromPath path config) != "_mkMergedOptionModule"; + message = + let + opt = getAttrFromPath path options; + in + '' + The option `${showOption path}' defined in ${showFiles opt.files} has been changed to `${showOption to}' that has a different type. + Please read `${showOption to}' documentation and update your configuration accordingly. + ''; + } ) from ); } @@ -1917,9 +1921,10 @@ let ); config = mkIf condition (mkMerge [ (optionalAttrs (options ? warnings) { - warnings = - optional (warn && fromOpt.isDefined) - "The option `${showOption from}' defined in ${showFiles fromOpt.files} has been renamed to `${showOption to}'."; + warnings = setAttrByPath (from ++ [ "aliased" ]) { + condition = warn && fromOpt.isDefined; + message = "The option `${showOption from}' defined in ${showFiles fromOpt.files} has been renamed to `${showOption to}'."; + }; }) ( if withPriority then diff --git a/lib/tests/modules/doRename-warnings.nix b/lib/tests/modules/doRename-warnings.nix index c0150ce4349ce..7342d5807fb41 100644 --- a/lib/tests/modules/doRename-warnings.nix +++ b/lib/tests/modules/doRename-warnings.nix @@ -17,12 +17,35 @@ }) ]; options = { - warnings = lib.mkOption { type = lib.types.listOf lib.types.str; }; + warnings = lib.mkOption { + type = + let + checkedWarningItemType = + let + check = x: x ? condition && x ? message; + in + lib.types.addCheck (lib.types.attrsOf lib.types.anything) check; + + nestedWarningAttrsType = + let + nestedWarningItemType = lib.types.either checkedWarningItemType ( + lib.types.attrsOf nestedWarningItemType + ); + in + nestedWarningItemType; + in + lib.types.submodule { freeformType = nestedWarningAttrsType; }; + }; + c.d.e = lib.mkOption { }; result = lib.mkOption { }; }; config = { a.b = 1234; - result = lib.concatStringsSep "%" config.warnings; + result = + let + warning = config.warnings.a.b.aliased; + in + lib.optionalString warning.condition warning.message; }; } From eadba4e353600016b7a24f329f506b8ce9dceef6 Mon Sep 17 00:00:00 2001 From: h7x4 Date: Mon, 16 Sep 2024 19:33:49 +0200 Subject: [PATCH 4/9] nixos/prometheus/exporters: use structured assertions/warnings --- .../monitoring/prometheus/exporters.nix | 105 +++++++++--------- 1 file changed, 55 insertions(+), 50 deletions(-) diff --git a/nixos/modules/services/monitoring/prometheus/exporters.nix b/nixos/modules/services/monitoring/prometheus/exporters.nix index 35a89ab1bf719..f50653b8c729d 100644 --- a/nixos/modules/services/monitoring/prometheus/exporters.nix +++ b/nixos/modules/services/monitoring/prometheus/exporters.nix @@ -416,16 +416,16 @@ in config = mkMerge ( [ { - assertions = [ - { + assertions.services.prometheus.exporters = { + ipmi.nonEmptyConfigFile = { assertion = cfg.ipmi.enable -> (cfg.ipmi.configFile != null) -> (!(lib.hasPrefix "/tmp/" cfg.ipmi.configFile)); message = '' Config file specified in `services.prometheus.exporters.ipmi.configFile' must not reside within /tmp - it won't be visible to the systemd service. ''; - } - { + }; + ipmi.nonEmptyWebConfigFile = { assertion = cfg.ipmi.enable -> (cfg.ipmi.webConfigFile != null) @@ -434,89 +434,89 @@ in Config file specified in `services.prometheus.exporters.ipmi.webConfigFile' must not reside within /tmp - it won't be visible to the systemd service. ''; - } - { + }; + restic.nonEmptyRepositoryFile = { assertion = cfg.restic.enable -> ((cfg.restic.repository == null) != (cfg.restic.repositoryFile == null)); message = '' Please specify either 'services.prometheus.exporters.restic.repository' or 'services.prometheus.exporters.restic.repositoryFile'. ''; - } - { + }; + snmp.nonEmptyConfiguration = { assertion = cfg.snmp.enable -> ((cfg.snmp.configurationPath == null) != (cfg.snmp.configuration == null)); message = '' Please ensure you have either `services.prometheus.exporters.snmp.configuration' or `services.prometheus.exporters.snmp.configurationPath' set! ''; - } - { + }; + mikrotik.nonEmptyConfiguration = { assertion = cfg.mikrotik.enable -> ((cfg.mikrotik.configFile == null) != (cfg.mikrotik.configuration == null)); message = '' Please specify either `services.prometheus.exporters.mikrotik.configuration' or `services.prometheus.exporters.mikrotik.configFile'. ''; - } - { + }; + mail.nonEmptyConfiguration = { assertion = cfg.mail.enable -> ((cfg.mail.configFile == null) != (cfg.mail.configuration == null)); message = '' Please specify either 'services.prometheus.exporters.mail.configuration' or 'services.prometheus.exporters.mail.configFile'. ''; - } - { + }; + mysqld.serverEnabledIfRunAsLocalSuperUser = { assertion = cfg.mysqld.runAsLocalSuperUser -> config.services.mysql.enable; message = '' The exporter is configured to run as 'services.mysql.user', but 'services.mysql.enable' is set to false. ''; - } - { + }; + nextcloud.nonEmptyPassword = { assertion = cfg.nextcloud.enable -> ((cfg.nextcloud.passwordFile == null) != (cfg.nextcloud.tokenFile == null)); message = '' Please specify either 'services.prometheus.exporters.nextcloud.passwordFile' or 'services.prometheus.exporters.nextcloud.tokenFile' ''; - } - { + }; + sql.nonEmptyConfiguration = { assertion = cfg.sql.enable -> ((cfg.sql.configFile == null) != (cfg.sql.configuration == null)); message = '' Please specify either 'services.prometheus.exporters.sql.configuration' or 'services.prometheus.exporters.sql.configFile' ''; - } - { + }; + scaphandre.platformIsx86_64 = { assertion = cfg.scaphandre.enable -> (pkgs.stdenv.targetPlatform.isx86_64 == true); message = '' Scaphandre only support x86_64 architectures. ''; - } - { + }; + scaphandre.kernelIsNewerThan511 = { assertion = cfg.scaphandre.enable -> ((lib.kernel.whenHelpers pkgs.linux.version).whenOlder "5.11" true).condition == false; message = '' Scaphandre requires a kernel version newer than '5.11', '${pkgs.linux.version}' given. ''; - } - { + }; + scaphandre.kernelModuleIntelRaplCommonIsEnabled = { assertion = cfg.scaphandre.enable -> (builtins.elem "intel_rapl_common" config.boot.kernelModules); message = '' Scaphandre needs 'intel_rapl_common' kernel module to be enabled. Please add it in 'boot.kernelModules'. ''; - } - { + }; + idrac.nonEmptyConfiguration = { assertion = cfg.idrac.enable -> ((cfg.idrac.configurationPath == null) != (cfg.idrac.configuration == null)); message = '' Please ensure you have either `services.prometheus.exporters.idrac.configuration' or `services.prometheus.exporters.idrac.configurationPath' set! ''; - } - { + }; + deluge.nonEmptyPassword = { assertion = cfg.deluge.enable -> ((cfg.deluge.delugePassword == null) != (cfg.deluge.delugePasswordFile == null)); @@ -524,8 +524,8 @@ in Please ensure you have either `services.prometheus.exporters.deluge.delugePassword' or `services.prometheus.exporters.deluge.delugePasswordFile' set! ''; - } - { + }; + pgbouncer.nonEmptyConnection = { assertion = cfg.pgbouncer.enable -> (xor (cfg.pgbouncer.connectionEnvFile == null) (cfg.pgbouncer.connectionString == null)); @@ -533,30 +533,35 @@ in Options `services.prometheus.exporters.pgbouncer.connectionEnvFile` and `services.prometheus.exporters.pgbouncer.connectionString` are mutually exclusive! ''; - } - ] - ++ (flip map (attrNames exporterOpts) (exporter: { - assertion = cfg.${exporter}.firewallFilter != null -> cfg.${exporter}.openFirewall; - message = '' - The `firewallFilter'-option of exporter ${exporter} doesn't have any effect unless - `openFirewall' is set to `true'! - ''; - })) - ++ config.services.prometheus.exporters.assertions; - warnings = [ - (mkIf - ( + }; + } + // (lib.listToAttrs ( + map ( + exporter: + lib.nameValuePair exporter { + assertion = cfg.${exporter}.firewallFilter != null -> cfg.${exporter}.openFirewall; + message = '' + The `firewallFilter'-option of exporter ${exporter} doesn't have any effect unless + `openFirewall' is set to `true'! + ''; + } + ) (attrNames exporterOpts) + )) + // config.services.prometheus.exporters.assertions; + + warnings = { + services.prometheus.exporters.idrac.configurationPathOverrides = { + condition = config.services.prometheus.exporters.idrac.enable - && config.services.prometheus.exporters.idrac.configurationPath != null - ) - '' + && config.services.prometheus.exporters.idrac.configurationPath != null; + message = '' Configuration file in `services.prometheus.exporters.idrac.configurationPath` may override `services.prometheus.exporters.idrac.listenAddress` and/or `services.prometheus.exporters.idrac.port`. Consider using `services.prometheus.exporters.idrac.configuration` instead. - '' - ) - ] - ++ config.services.prometheus.exporters.warnings; + ''; + }; + } + // config.services.prometheus.exporters.warnings; } ] ++ [ From 5eed7f558e27280e510165f94d60c85e1d0b93f6 Mon Sep 17 00:00:00 2001 From: h7x4 Date: Mon, 16 Sep 2024 19:33:49 +0200 Subject: [PATCH 5/9] nixos/filesystems: use structured assertions/warnings --- nixos/modules/tasks/filesystems.nix | 70 +++++++++++++++-------------- 1 file changed, 36 insertions(+), 34 deletions(-) diff --git a/nixos/modules/tasks/filesystems.nix b/nixos/modules/tasks/filesystems.nix index 84b66404f6f9f..c9aae84788eca 100644 --- a/nixos/modules/tasks/filesystems.nix +++ b/nixos/modules/tasks/filesystems.nix @@ -17,6 +17,7 @@ let flip head literalExpression + mapAttrs mkDefault mkEnableOption mkIf @@ -438,7 +439,7 @@ in config = { - assertions = + assertions.fileSystems = let ls = sep: concatMapStringsSep sep (x: x.mountPoint); resizableFSes = [ @@ -447,43 +448,44 @@ in "btrfs" "xfs" ]; - notAutoResizable = fs: fs.autoResize && !(builtins.elem fs.fsType resizableFSes); in - [ - { + { + topologicallySorted = { assertion = !(fileSystems' ? cycle); - message = "The ‘fileSystems’ option can't be topologically sorted: mountpoint dependency path ${ls " -> " fileSystems'.cycle} loops to ${ls ", " fileSystems'.loops}"; - } - { - assertion = !(any notAutoResizable fileSystems); - message = - let - fs = head (filter notAutoResizable fileSystems); - in - '' - Mountpoint '${fs.mountPoint}': 'autoResize = true' is not supported for 'fsType = "${fs.fsType}"' - ${optionalString (fs.fsType == "auto") "fsType has to be explicitly set and"} - only the following support it: ${lib.concatStringsSep ", " resizableFSes}. - ''; - } - { - assertion = !(any (fs: fs.formatOptions != null) fileSystems); message = '' - 'fileSystems..formatOptions' has been removed, since - systemd-makefs does not support any way to provide formatting - options. + The ‘fileSystems’ option can't be topologically sorted: + mountpoint dependency path ${ls " -> " fileSystems'.cycle or [ ]} loops to ${ + ls ", " fileSystems'.loops or [ ] + } ''; - } - ] - ++ lib.map (fs: { - assertion = fs.label != null -> fs.device == "/dev/disk/by-label/${escape fs.label}"; - message = '' - The filesystem with mount point ${fs.mountPoint} has its label and device set to inconsistent values: - label: ${toString fs.label} - device: ${toString fs.device} - 'filesystems..label' and 'filesystems..device' are mutually exclusive. Please set only one. - ''; - }) fileSystems; + }; + resizable = mapAttrs (name: fs: { + assertion = fs.autoResize -> builtins.elem fs.fsType resizableFSes; + message = '' + Mountpoint '${fs.mountPoint}': 'autoResize = true' is not supported for 'fsType = "${fs.fsType}"' + ${optionalString (fs.fsType == "auto") "fsType has to be explicitly set and"} + only the following support it: ${lib.concatStringsSep ", " resizableFSes}. + ''; + }) config.fileSystems; + formatOptionsDeprecated = mapAttrs (name: fs: { + assertion = fs.formatOptions == null; + message = '' + The filesystem with mount point ${fs.mountPoint} has its label and device set to inconsistent values: + label: ${toString fs.label} + device: ${toString fs.device} + 'filesystems..label' and 'filesystems..device' are mutually exclusive. Please set only one. + ''; + }) config.fileSystems; + consistentDeviceLabelMountPoint = mapAttrs (name: fs: { + assertion = fs.label != null -> fs.device == "/dev/disk/by-label/${escape fs.label}"; + message = '' + The filesystem with mount point ${fs.mountPoint} has its label and device set to inconsistent values: + label: ${toString fs.label} + device: ${toString fs.device} + 'filesystems..label' and 'filesystems..device' are mutually exclusive. Please set only one. + ''; + }) config.fileSystems; + }; # Export for use in other modules system.build.fileSystems = fileSystems; From cb547e17a5801ae7de07e3f9abcaee744f25748b Mon Sep 17 00:00:00 2001 From: h7x4 Date: Mon, 16 Sep 2024 19:33:50 +0200 Subject: [PATCH 6/9] nixos/networking: use structured assertions/warnings --- .../tasks/network-interfaces-scripted.nix | 24 ++--- nixos/modules/tasks/network-interfaces.nix | 91 ++++++++++--------- 2 files changed, 62 insertions(+), 53 deletions(-) diff --git a/nixos/modules/tasks/network-interfaces-scripted.nix b/nixos/modules/tasks/network-interfaces-scripted.nix index 9e6529e487b4c..38c0ed438fad9 100644 --- a/nixos/modules/tasks/network-interfaces-scripted.nix +++ b/nixos/modules/tasks/network-interfaces-scripted.nix @@ -63,17 +63,19 @@ let bond: (filterAttrs (attrName: attr: elem attrName deprecated && attr != null) bond); }; - bondWarnings = - let - oneBondWarnings = - bondName: bond: mapAttrsToList (bondText bondName) (bondDeprecation.filterDeprecated bond); - bondText = - bondName: optName: _: - "${bondName}.${optName} is deprecated, use ${bondName}.driverOptions"; - in - { - warnings = flatten (mapAttrsToList oneBondWarnings cfg.bonds); - }; + bondWarnings = { + warnings.networking.bonds.deprecatedOptions = lib.mapAttrs ( + bondName: bond: + lib.mergeAttrsList ( + map (optName: { + ${optName} = { + condition = (cfg.bonds.${bondName}.${optName} or null) != null; + message = "${bondName}.${optName} is deprecated, use ${bondName}.driverOptions"; + }; + }) bondDeprecation.deprecated + ) + ) cfg.bonds; + }; normalConfig = { systemd.network.links = diff --git a/nixos/modules/tasks/network-interfaces.nix b/nixos/modules/tasks/network-interfaces.nix index 09b6cec4e7ea4..b1a360a14662e 100644 --- a/nixos/modules/tasks/network-interfaces.nix +++ b/nixos/modules/tasks/network-interfaces.nix @@ -32,8 +32,6 @@ let ) ) (attrValues cfg.vswitches); - slaveIfs = map (i: cfg.interfaces.${i}) (filter (i: cfg.interfaces ? ${i}) slaves); - rstpBridges = flip filterAttrs cfg.bridges (_: { rstp, ... }: rstp); needsMstpd = rstpBridges != { }; @@ -1678,47 +1676,56 @@ in meta.maintainers = with lib.maintainers; [ rnhmjoj ]; config = { + warnings.networking.interfaces = lib.mapAttrs (_: i: i.warnings) cfg.interfaces; + warnings.networking.systemdNetworkdDhcpdClash = { + condition = config.systemd.network.enable && cfg.useDHCP && !cfg.useNetworkd; + message = '' + The combination of `systemd.network.enable = true`, `networking.useDHCP = true` and `networking.useNetworkd = false` + can cause both networkd and dhcpcd to manage the same interfaces. This can lead to loss of networking. + It is recommended you choose only one of networkd (by also enabling `networking.useNetworkd`) or scripting + (by disabling `systemd.network.enable`) + ''; + }; - warnings = - (concatMap (i: i.warnings) interfaces) - ++ (lib.optional (config.systemd.network.enable && cfg.useDHCP && !cfg.useNetworkd) '' - The combination of `systemd.network.enable = true`, `networking.useDHCP = true` and `networking.useNetworkd = false` can cause both networkd and dhcpcd to manage the same interfaces. This can lead to loss of networking. It is recommended you choose only one of networkd (by also enabling `networking.useNetworkd`) or scripting (by disabling `systemd.network.enable`) - ''); - - assertions = - (forEach interfaces (i: { - # With the linux kernel, interface name length is limited by IFNAMSIZ - # to 16 bytes, including the trailing null byte. - # See include/linux/if.h in the kernel sources - assertion = stringLength i.name < 16; - message = '' - The name of networking.interfaces."${i.name}" is too long, it needs to be less than 16 characters. - ''; - })) - ++ (forEach slaveIfs (i: { - assertion = i.ipv4.addresses == [ ] && i.ipv6.addresses == [ ]; - message = '' - The networking.interfaces."${i.name}" must not have any defined ips when it is a slave. - ''; - })) - ++ (forEach interfaces (i: { - assertion = i.tempAddress != "disabled" -> cfg.enableIPv6; - message = '' - Temporary addresses are only needed when IPv6 is enabled. - ''; - })) - ++ (forEach interfaces (i: { - assertion = (i.virtual && i.virtualType == "tun") -> i.macAddress == null; - message = '' - Setting a MAC Address for tun device ${i.name} isn't supported. - ''; - })) - ++ [ - { - assertion = cfg.hostId == null || (stringLength cfg.hostId == 8 && isHexString cfg.hostId); - message = "Invalid value given to the networking.hostId option."; - } - ]; + assertions.networking.interfaces = lib.flip lib.mapAttrs cfg.interfaces ( + iName: iCfg: { + nameLessThan16Chars = { + # With the linux kernel, interface name length is limited by IFNAMSIZ + # to 16 bytes, including the trailing null byte. + # See include/linux/if.h in the kernel sources + assertion = stringLength iName < 16; + message = '' + The name of networking.interfaces."${iName}" is too long, it needs to be less than 16 characters. + ''; + }; + + slaveInterfacesMustNotHaveIpAddresses = { + assertion = elem iName slaves -> (iCfg.ipv4.addresses == [ ] && iCfg.ipv6.addresses == [ ]); + message = '' + The networking.interfaces."${iName}" must not have any defined ips when it is a slave. + ''; + }; + + tempAddressOnlyForIpv6 = { + assertion = iCfg.tempAddress != "disabled" -> cfg.enableIPv6; + message = '' + Temporary addresses are only needed when IPv6 is enabled. + ''; + }; + + noMacAddressForTunDevices = { + assertion = (iCfg.virtual && iCfg.virtualType == "tun") -> iCfg.macAddress == null; + message = '' + Setting a MAC Address for tun device ${iName} isn't supported. + ''; + }; + } + ); + + assertions.networking.validHostId = { + assertion = cfg.hostId == null || (stringLength cfg.hostId == 8 && isHexString cfg.hostId); + message = "Invalid value given to the networking.hostId option."; + }; boot.kernelModules = [ ] From ad050b416bfe463ae375408677a71577fd65413b Mon Sep 17 00:00:00 2001 From: h7x4 Date: Mon, 16 Sep 2024 19:33:51 +0200 Subject: [PATCH 7/9] nixos/users: use structured assertions/warnings --- nixos/modules/config/users-groups.nix | 286 ++++++++++++-------------- 1 file changed, 127 insertions(+), 159 deletions(-) diff --git a/nixos/modules/config/users-groups.nix b/nixos/modules/config/users-groups.nix index 59f1b44483df8..49cc89c7744d9 100644 --- a/nixos/modules/config/users-groups.nix +++ b/nixos/modules/config/users-groups.nix @@ -20,6 +20,7 @@ let flatten flip foldr + genAttrs generators getAttr hasAttr @@ -27,6 +28,7 @@ let length listToAttrs literalExpression + mapAttrs mapAttrs' mapAttrsToList match @@ -621,11 +623,6 @@ let sdInitrdGidsAreUnique = idsAreUnique (filterAttrs ( n: g: g.gid != null ) config.boot.initrd.systemd.groups) "gid"; - groupNames = lib.mapAttrsToList (n: g: g.name) cfg.groups; - usersWithoutExistingGroup = lib.filterAttrs ( - n: u: u.group != "" && !lib.elem u.group groupNames - ) cfg.users; - usersWithNullShells = attrNames (filterAttrs (name: cfg: cfg.shell == null) cfg.users); spec = pkgs.writeText "users-groups.json" ( builtins.toJSON { @@ -1036,39 +1033,17 @@ in }; }; - assertions = [ - { + assertions.users = { + uidsGidsUnique = { assertion = !cfg.enforceIdUniqueness || (uidsAreUnique && gidsAreUnique); message = "UIDs and GIDs must be unique!"; - } - { + }; + systemdInitrdUidsGidsUnique = { assertion = !cfg.enforceIdUniqueness || (sdInitrdUidsAreUnique && sdInitrdGidsAreUnique); message = "systemd initrd UIDs and GIDs must be unique!"; - } - { - assertion = usersWithoutExistingGroup == { }; - message = - let - errUsers = lib.attrNames usersWithoutExistingGroup; - missingGroups = lib.unique (lib.mapAttrsToList (n: u: u.group) usersWithoutExistingGroup); - mkConfigHint = group: "users.groups.${group} = {};"; - in - '' - The following users have a primary group that is undefined: ${lib.concatStringsSep " " errUsers} - Hint: Add this to your NixOS configuration: - ${lib.concatStringsSep "\n " (map mkConfigHint missingGroups)} - ''; - } - { - assertion = !cfg.mutableUsers -> length usersWithNullShells == 0; - message = '' - users.mutableUsers = false has been set, - but found users that have their shell set to null. - If you wish to disable login, set their shell to pkgs.shadow (the default). - Misconfigured users: ${lib.concatStringsSep " " usersWithNullShells} - ''; - } - { + }; + + noLockout = { # If mutableUsers is false, to prevent users creating a # configuration that locks them out of the system, ensure that # there is at least one "privileged" account that has a @@ -1102,56 +1077,32 @@ in However you are most probably better off by setting users.mutableUsers = true; and manually running passwd root to set the root password. ''; - } - ] - ++ flatten ( - flip mapAttrsToList cfg.users ( - name: user: - [ - ( - let - # Things fail in various ways with especially non-ascii usernames. - # This regex mirrors the one from shadow's is_valid_name: - # https://github.com/shadow-maint/shadow/blob/bee77ffc291dfed2a133496db465eaa55e2b0fec/lib/chkname.c#L68 - # though without the trailing $, because Samba 3 got its last release - # over 10 years ago and is not in Nixpkgs anymore, - # while later versions don't appear to require anything like that. - nameRegex = "[a-zA-Z0-9_.][a-zA-Z0-9_.-]*"; - in - { - assertion = builtins.match nameRegex user.name != null; - message = "The username \"${user.name}\" is not valid, it does not match the regex \"${nameRegex}\"."; - } - ) - { + }; + + users = flip mapAttrs cfg.users ( + name: user: { + noColonInHashedPassword = { assertion = (user.hashedPassword != null) -> (match ".*:.*" user.hashedPassword == null); message = '' The password hash of user "${user.name}" contains a ":" character. This is invalid and would break the login system because the fields of /etc/shadow (file where hashes are stored) are colon-separated. - Please check the value of option `users.users."${user.name}".hashedPassword`.''; - } - { - assertion = user.isNormalUser && user.uid != null -> user.uid >= 1000; - message = '' - A user cannot have a users.users.${user.name}.uid set below 1000 and set users.users.${user.name}.isNormalUser. - Either users.users.${user.name}.isSystemUser must be set to true instead of users.users.${user.name}.isNormalUser - or users.users.${user.name}.uid must be changed to 1000 or above. + Please check the value of option `users.users."${user.name}".hashedPassword`. ''; - } - { + }; + + userKindDefined = { assertion = let - # we do an extra check on isNormalUser here, to not trigger this assertion when isNormalUser is set and uid to < 1000 - isEffectivelySystemUser = - user.isSystemUser || (user.uid != null && user.uid < 1000 && !user.isNormalUser); + isEffectivelySystemUser = user.isSystemUser || (user.uid != null && user.uid < 1000); in xor isEffectivelySystemUser user.isNormalUser; message = '' Exactly one of users.users.${user.name}.isSystemUser and users.users.${user.name}.isNormalUser must be set. ''; - } - { + }; + + userHasPrimaryGroup = { assertion = user.group != ""; message = '' users.users.${user.name}.group is unset. This used to default to @@ -1160,78 +1111,98 @@ in users.users.${user.name}.group = "${user.name}"; users.groups.${user.name} = {}; ''; - } - ] - ++ (map - (shell: { - assertion = - !user.ignoreShellProgramCheck - -> (user.shell == pkgs.${shell}) - -> (config.programs.${shell}.enable == true); + }; + + userPrimaryGroupExists = { + assertion = cfg.groups ? ${user.group}; message = '' - users.users.${user.name}.shell is set to ${shell}, but - programs.${shell}.enable is not true. This will cause the ${shell} - shell to lack the basic nix directories in its PATH and might make - logging in as that user impossible. You can fix it with: - programs.${shell}.enable = true; - - If you know what you're doing and you are fine with the behavior, - set users.users.${user.name}.ignoreShellProgramCheck = true; - instead. + users.users.${user.name}.group is set to "${user.group}", but this group is not defined. + + Hint: Add this to your NixOS configuration: + "users.groups.${user.group} = {};"; ''; - }) - [ - "fish" - "xonsh" - "zsh" - ] - ) - ) - ); + }; - warnings = - flip concatMap (attrValues cfg.users) ( - user: - let - passwordOptions = [ - "hashedPassword" - "hashedPasswordFile" - "password" - ] - ++ optionals cfg.mutableUsers [ - # For immutable users, initialHashedPassword is set to hashedPassword, - # so using these options would always trigger the assertion. - "initialHashedPassword" - "initialPassword" - ]; - unambiguousPasswordConfiguration = - 1 >= length (filter (x: x != null) (map (flip getAttr user) passwordOptions)); - in - optional (!unambiguousPasswordConfiguration) '' - The user '${user.name}' has multiple of the options - `initialHashedPassword`, `hashedPassword`, `initialPassword`, `password` - & `hashedPasswordFile` set to a non-null value. - - ${multiplePasswordsWarning} - ${overrideOrderText cfg.mutableUsers} - The values of these options are: - ${concatMapStringsSep "\n" ( - value: "* users.users.\"${user.name}\".${value}: ${generators.toPretty { } user.${value}}" - ) passwordOptions} - '' - ) - ++ filter (x: x != null) ( - flip mapAttrsToList cfg.users ( - _: user: - # This regex matches a subset of the Modular Crypto Format (MCF)[1] - # informal standard. Since this depends largely on the OS or the - # specific implementation of crypt(3) we only support the (sane) - # schemes implemented by glibc and BSDs. In particular the original - # DES hash is excluded since, having no structure, it would validate - # common mistakes like typing the plaintext password. - # - # [1]: https://en.wikipedia.org/wiki/Crypt_(C) + declarativeUserNoNullShell = { + assertion = !cfg.mutableUsers -> user.shell != null; + message = '' + users.mutableUsers = false has been set, + but user '${name}' have their shell set to null. + If you wish to disable login, set their shell to pkgs.shadow (the default). + ''; + }; + + shells = + genAttrs + [ + "fish" + "xonsh" + "zsh" + ] + (shell: { + assertion = + !user.ignoreShellProgramCheck + -> (user.shell == pkgs.${shell}) + -> (config.programs.${shell}.enable == true); + message = '' + users.users.${user.name}.shell is set to ${shell}, but + programs.${shell}.enable is not true. This will cause the ${shell} + shell to lack the basic nix directories in its PATH and might make + logging in as that user impossible. You can fix it with: + programs.${shell}.enable = true; + + If you know what you're doing and you are fine with the behavior, + set users.users.${user.name}.ignoreShellProgramCheck = true; + instead. + ''; + }); + } + ); + }; + + warnings.users.users = flip mapAttrs cfg.users ( + name: user: { + ambiguousPasswordConfiguration = let + unambiguousPasswordConfiguration = + 1 >= length ( + filter (x: x != null) ( + [ + user.hashedPassword + user.hashedPasswordFile + user.password + ] + ++ optionals cfg.mutableUsers [ + # For immutable users, initialHashedPassword is set to hashedPassword, + # so using these options would always trigger the assertion. + user.initialHashedPassword + user.initialPassword + ] + ) + ); + in + { + condition = !unambiguousPasswordConfiguration; + message = '' + The user '${user.name}' has multiple of the options + `hashedPassword`, `password`, `hashedPasswordFile`, `initialPassword` + & `initialHashedPassword` set to a non-null value. + The options silently discard others by the order of precedence + given above which can lead to surprising results. To resolve this warning, + set at most one of the options above to a non-`null` value. + ''; + }; + + invalidPasswordHash = + let + # This regex matches a subset of the Modular Crypto Format (MCF)[1] + # informal standard. Since this depends largely on the OS or the + # specific implementation of crypt(3) we only support the (sane) + # schemes implemented by glibc and BSDs. In particular the original + # DES hash is excluded since, having no structure, it would validate + # common mistakes like typing the plaintext password. + # + # [1]: https://en.wikipedia.org/wiki/Crypt_(C) sep = "\\$"; base64 = "[a-zA-Z0-9./]+"; id = cryptSchemeIdPatternGroup; @@ -1242,29 +1213,26 @@ in content = "${base64}${sep}${base64}(${sep}${base64})?"; mcf = "^${sep}${scheme}${sep}${content}$"; in - if - ( + { + condition = allowsLogin user.hashedPassword && user.hashedPassword != "" # login without password - && match mcf user.hashedPassword == null - ) - then - '' + && match mcf user.hashedPassword == null; + message = '' The password hash of user "${user.name}" may be invalid. You must set a valid hash or the user will be locked out of their account. Please - check the value of option `users.users."${user.name}".hashedPassword`.'' - else - null - ) - ++ flip mapAttrsToList cfg.users ( - name: user: - if user.passwordFile != null then - ''The option `users.users."${name}".passwordFile' has been renamed '' - + ''to `users.users."${name}".hashedPasswordFile'.'' - else - null - ) - ); - }; + check the value of option `users.users."${user.name}".hashedPassword`. + ''; + }; + passwordFileDeprecation = { + condition = user.passwordFile != null; + message = '' + The option `users.users."${user.name}".passwordFile' has been renamed + to `users.users."${user.name}".hashedPasswordFile'. + ''; + }; + } + ); + }; } From 24717ff0256f0fc0d28b109fce58f33366914905 Mon Sep 17 00:00:00 2001 From: h7x4 Date: Mon, 16 Sep 2024 19:33:51 +0200 Subject: [PATCH 8/9] nixos/security/wrappers: use structured assertions/warnings --- nixos/modules/security/wrappers/default.nix | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/nixos/modules/security/wrappers/default.nix b/nixos/modules/security/wrappers/default.nix index edbed8120e248..43b05f0651607 100644 --- a/nixos/modules/security/wrappers/default.nix +++ b/nixos/modules/security/wrappers/default.nix @@ -245,15 +245,18 @@ in }; ###### implementation - config = lib.mkIf config.security.enableWrappers { - - assertions = lib.mapAttrsToList (name: opts: { - assertion = opts.setuid || opts.setgid -> opts.capabilities == ""; - message = '' - The security.wrappers.${name} wrapper is not valid: - setuid/setgid and capabilities are mutually exclusive. - ''; - }) wrappers; + config = { + assertions.security.wrappers = lib.flip lib.mapAttrs wrappers ( + name: opts: { + setuidSetgidCapabilitiesAreMutuallyExclusive = { + assertion = opts.setuid || opts.setgid -> opts.capabilities == ""; + message = '' + The security.wrappers.${name} wrapper is not valid: + setuid/setgid and capabilities are mutually exclusive. + ''; + }; + } + ); security.wrappers = let From 757f726d0b25fa1d12f932c38157a8ee5c1ff94a Mon Sep 17 00:00:00 2001 From: h7x4 Date: Mon, 16 Sep 2024 19:33:53 +0200 Subject: [PATCH 9/9] nixos/fcgiwrap: use structured assertions/warnings --- .../modules/services/web-servers/fcgiwrap.nix | 46 +++++++++---------- 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/nixos/modules/services/web-servers/fcgiwrap.nix b/nixos/modules/services/web-servers/fcgiwrap.nix index 992f4a46ff4b5..8bbced5e45c95 100644 --- a/nixos/modules/services/web-servers/fcgiwrap.nix +++ b/nixos/modules/services/web-servers/fcgiwrap.nix @@ -119,30 +119,28 @@ in }; config = { - assertions = concatLists ( - mapAttrsToList (name: cfg: [ - { - assertion = cfg.socket.type == "unix" -> cfg.socket.user != null; - message = "Socket owner is required for the UNIX socket type."; - } - { - assertion = cfg.socket.type == "unix" -> cfg.socket.group != null; - message = "Socket owner is required for the UNIX socket type."; - } - { - assertion = cfg.socket.user != null -> cfg.socket.type == "unix"; - message = "Socket owner can only be set for the UNIX socket type."; - } - { - assertion = cfg.socket.group != null -> cfg.socket.type == "unix"; - message = "Socket owner can only be set for the UNIX socket type."; - } - { - assertion = cfg.socket.mode != null -> cfg.socket.type == "unix"; - message = "Socket mode can only be set for the UNIX socket type."; - } - ]) config.services.fcgiwrap.instances - ); + assertions.services.fcgiwrap.instances = mapAttrs (name: cfg: { + socketOwnerRequired = { + assertion = cfg.socket.type == "unix" -> cfg.socket.user != null; + message = "Socket owner is required for the UNIX socket type."; + }; + socketGroupRequired = { + assertion = cfg.socket.type == "unix" -> cfg.socket.group != null; + message = "Socket group is required for the UNIX socket type."; + }; + socketOwnerOnlyForUnix = { + assertion = cfg.socket.user != null -> cfg.socket.type == "unix"; + message = "Socket owner can only be set for the UNIX socket type."; + }; + socketGroupOnlyForUnix = { + assertion = cfg.socket.group != null -> cfg.socket.type == "unix"; + message = "Socket group can only be set for the UNIX socket type."; + }; + socketModeOnlyForUnix = { + assertion = cfg.socket.mode != null -> cfg.socket.type == "unix"; + message = "Socket mode can only be set for the UNIX socket type."; + }; + }) config.services.fcgiwrap.instances; systemd.services = forEachInstance (cfg: { after = [ "nss-user-lookup.target" ];