-
-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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/dovecot: Add proper escaping and support for more value types to plugin settings #286184
base: master
Are you sure you want to change the base?
Conversation
108957b
to
ba2668f
Compare
Sorry for bumping this, but it would be really great if this was merged before 24.05 (since it’d be a backward-compatibility hazard afterwards) @2xsaiko (name in maintainers appears to be dated, btw) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool, thanks!
(I didn't see this until now. Maybe I have to additionally add myself to the maintainers file to get reliably notified about stuff I maintain.)
(name in maintainers appears to be dated, btw)
Hm? It's not.
then | ||
concatMapStringsSep " " mkDovecotValue value | ||
else | ||
abort "mkDovecotValue: value not supported: ${toPretty {} value}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
abort "mkDovecotValue: value not supported: ${toPretty {} value}" | |
throw "mkDovecotValue: value not supported: ${toPretty {} value}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, abort works too, I suppose. I just see throw usually, so use whichever you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think I found abort in the docs somewhere, seen throw elsewhere too, I’ll change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Note for future readers: Difference between abort and throw is that throw can be caught during tryEval, while abort cannot, so you should generally use throw.)
escapeDovecotString = string: | ||
let | ||
escapedString = escape ["\\" "\""] string; | ||
in if match "[[:space:]]*<.*|.*[[:space:]#\"\\].*|" string != null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in if match "[[:space:]]*<.*|.*[[:space:]#\"\\].*|" string != null | |
in if match ''[[:space:]]*<.*|.*[[:space:]#"\].*|'' string != null |
so you can copy it verbatim
value: ( | ||
if isString value || isPath value || isDerivation value | ||
then | ||
escapeDovecotString (toString value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this, you'll have to pass the list as-is to pluginSettings for sieve.plugins, sieve.extensions and sieve.globalExtensions, otherwise it will quote the values:
plugin {
[...]
sieve_extensions = "+notify +imapflags +vnd.dovecot.filter"
sieve_global_extensions = "+vnd.dovecot.environment +vnd.dovecot.pipe"
sieve_pipe_bin_dir = /nix/store/9g0l06qac224hd2m0ixj0mhf25mh0662-sieve-pipe-bins
sieve_plugins = "sieve_imapsieve sieve_extprograms"
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this about having to write
services.dovecot = {
pluginSettings = {
sieve_extensions = ["+notify" "+imapflags" "+vnd.dovecot.filter"];
};
};
vs
services.dovecot = {
pluginSettings = {
sieve_extensions = "+notify +imapflags +vnd.dovecot.filter";
};
};
?
If so, that is intended behaviour since the former denotes a list of strings (which this is), while the latter denotes a since string containing spaces. (Having it declared as a list also means others can later use mkBefore
/mkAfter
in their own configs to easily extend it without conflicts or mkForce
.)
Dovecot isn’t very strict about these type differences in practice, so both of the above actually work. On the hand, a configuration value of sieve_extensions = "+notify" "+imapflags" "+vnd.dovecot.filter"
doesn’t work, even though the documentation says it should. As far as I can tell, all of those “should work but doesn’t” cases boil down to being space-separated words which the regex mentioned in the previous comment gracefully handles by making sure the quoting is only applied when it is actually needed due to the contents containing “special characters” (spaces, quotation marks, backslashes, file redirects, comment signs, empty strings). I actually have an extended version of what I’m proposing here (uses the same code also for global assignments, mailboxes, services and service listeners) in use and as long as one follows the only-quote-when-needed rule, even complex configurations are successfully generated and parsed by Dovecot with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is about the settings that I mentioned setting your second variant instead of the first. Even if it works (tbh I didn't assume it would) I think it would be good to change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess with this at least sieve.plugins could be removed since it just passes the value through directly.
If you do that though then I'd make pluginSettings a submodule with freeformType like here so the description and example can be kept.
ba2668f
to
8cac481
Compare
Updated to replace (mkRenamedOptionModule [ "services" "dovecot2" "sieve" "plugins" ] [ "services" "dovecot2" "pluginSettings" "sieve_plugins" ])
(mkChangedOptionModule [ "services" "dovecot2" "sieve" "extensions" ] [ "services" "dovecot2" "pluginsSettings" "sieve_extensions" ]
(config: map (el: "+${el}") config.services.dovecot2.sieve.extensions))
(mkChangedOptionModule [ "services" "dovecot2" "sieve" "globalExtensions" ] [ "services" "dovecot2" "pluginsSettings" "sieve_global_extensions" ]
(config: map (el: "+${el}") config.services.dovecot2.sieve.globalExtensions)) sieve = {
extensions = mkOption {
default = [];
description = "Deprecated in favour of {config}`services.dovecot2.pluginSettings.sieve_extensions` – Extra Sieve extensions to enable for user scripts";
example = [ "notify" "imapflags" "vnd.dovecot.filter" ];
type = types.listOf types.str;
};
globalExtensions = mkOption {
default = [];
example = [ "vnd.dovecot.environment" ];
description = "Deprecated in favour of {config}`services.dovecot2.pluginSettings.sieve_global_extensions` – Extra Sieve extensions to enable for global scripts";
type = types.listOf types.str;
};
}; |
}; | ||
|
||
sieve_pipe_bin_dir = mkOption { | ||
default = sievePipeBinScriptDirectory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default = sievePipeBinScriptDirectory; | |
default = null; |
This should be set as part of config
if sieve.pipeBins is non-empty so it doesn't get silently overwritten.
pluginSettings = (lib.mapAttrs (n: lib.mkDefault) ( | ||
sieveScriptSettings // imapSieveMailboxSettings | ||
)) // (lib.mapAttrs (n: lib.mkBefore) (filterAttrs (n: v: v != []) { | ||
sieve_global_extensions = ( | ||
optional (cfg.sieve.pipeBins != []) "+vnd.dovecot.pipe" | ||
); | ||
sieve_plugins = ( | ||
optional (cfg.imapsieve.mailbox != []) "sieve_imapsieve" | ||
++ optional (cfg.sieve.pipeBins != []) "sieve_extprograms" | ||
); | ||
})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pluginSettings = (lib.mapAttrs (n: lib.mkDefault) ( | |
sieveScriptSettings // imapSieveMailboxSettings | |
)) // (lib.mapAttrs (n: lib.mkBefore) (filterAttrs (n: v: v != []) { | |
sieve_global_extensions = ( | |
optional (cfg.sieve.pipeBins != []) "+vnd.dovecot.pipe" | |
); | |
sieve_plugins = ( | |
optional (cfg.imapsieve.mailbox != []) "sieve_imapsieve" | |
++ optional (cfg.sieve.pipeBins != []) "sieve_extprograms" | |
); | |
})); | |
pluginSettings = mkMerge [ | |
sieveScriptSettings | |
imapSieveMailboxSettings | |
(mkIf (cfg.sieve.pipeBins != []) { | |
sieve_plugins = [ "sieve_extprograms" ]; | |
sieve_global_extensions = [ "+vnd.dovecot.pipe" ]; | |
}) | |
(mkIf (cfg.imapsieve.mailbox != []) { | |
sieve_plugins = [ "sieve_imapsieve" ]; | |
}) | |
]; |
This is getting a bit messy. Also that mkDefault shouldn't be needed anymore since both sieveScriptSettings and imapSieveMailboxSettings will be empty until the user sets the corresponding options (and conflicts should error for these).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a cool trick! 👍
c139cd9
to
cc47995
Compare
Haven’t had time to test it yet sadly but I think I applied all the changes you requested. |
fbe85b1
to
c195934
Compare
Tested it now and appears to work! Turns out the |
escapeDovecotString = string: | ||
let | ||
escapedString = escape ["\\" "\""] string; | ||
in if match ''[[:space:]]*<.*|.*[[:space:]#"\].*|'' string != null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in if match ''[[:space:]]*<.*|.*[[:space:]#"\].*|'' string != null | |
in if match ''[[:space:]]*<.*|.*[[:space:]].*|'' string != null |
Actually, this regexp is invalid, I didn't check that before. Is this maybe what it should be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the regexp works just fine.
The following Nix configuration:
{
# …
services.dovecot2.pluginSettings = {
test_backslash = "\\"; # → "\\\\"
test_comment = "a#b"; # → "a#b"
test_empty = ""; # → ""
test_multiline = " a a \n b b \n c c "; # → " a a \\\n b b \\\n c c "
test_multiline_trailing = "a\nb\nc\n\n"; # → "a \\\nb \\\nc"
test_quote = "\""; # → "\\""
test_space = " "; # → " "
test_unquoted = "abc"; # → abc
};
}
Results, using the submitted regexp, in the following Dovecot configuration:
plugin {
test_backslash = "\\"
test_comment = "a#b"
test_empty = ""
test_multiline = " a a \
b b \
c c "
test_multiline_trailing = "a \
b \
c"
test_quote = "\""
test_space = " "
test_unquoted = abc
}
The condition if match ''[[:space:]]*<.*|.*[[:space:]#"\\].*|^$'' string != null
also appears to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I guess this hits NixOS/nix#1537. It fails to evaluate on my Mac.
error: invalid regular expression '[[:space:]]*<.*|.*[[:space:]#"\].*|'
The | needs to be escaped for it to work everywhere:
in if match ''[[:space:]]*<.*|.*[[:space:]#"\].*|'' string != null | |
in if match ''[[:space:]]*<.*\|.*[[:space:]#"\].*\|'' string != null |
edit: Oh wait, no, those are alternatives as in (a|b)
. Didn't know that worked without the parentheses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked the linked issue but the mentioned problem does not appear to be present inside a Darling (macOS on Linux emulator) container, so I’m unable to reproduce this macOS-only behaviour locally. The pattern is supposed to say “starts with <
(ignoring whitespace) OR contains at least one whitespace, #
, "
or \
character OR is empty” in any case.
I’ve updated the regexp to something that should hopefully work on macOS too – if it’s just the empty variant it dislikes. Please also check if the test vector I posted produces the same results: #286184 (comment) (there actually was a bug that single-lined strings containing only whitespace would also be trimmed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, looks like that works! The test cases give the same output here.
I thought maybe you could get the same behavior by overriding nix to use clangStdenv but apparently not. Well, doesn't matter now anyway.
c195934
to
3ce96b2
Compare
@RaitoBezarius @2xsaiko Sorry for bumping this, but can this be merged for 24.05 since it’d be a break change afterwards? |
I'm sorry, I can't do anything here since I can't merge PRs. Others who might be interested in reviewing this: @GaetanLepage @nlewo (since you recently touched dovecot in simple-nixos-mailserver, but feel free to ignore) Otherwise, try posting in https://discourse.nixos.org/t/prs-ready-for-review/3032. |
Since the last widespread breakage, I'm unfortunately a bit risk averse on large scale changes on Dovecot2, I'd appreciate more testing in this PR, maybe, including things that try to reproduce what SNM is trying to do. |
I submitted a NixOS Mailserver MR to test this MR:
Tests are currently failing because of:
@ntninja Could you provide the migration path? |
It should be |
@nlewo There used to be a migration path for these options initially, but I removed it after realizing that those options never have been in NixOS stable. (cf #286184 (comment)) You are attempting what RaitoBezarius asked for there, right? (I hadn’t heard of SNM before, these changes – and some – are for Modoboa, which I’ve privately packaged.) |
3ce96b2
to
ec36189
Compare
Draft: DO NOT MERGE: test NixOS/nixpkgs#286184 See merge request simple-nixos-mailserver/nixos-mailserver!324
…o plugin settings The added escaping functions cover the entire range of Dovecot supported config value constructs other than string-includes and variable references (neither of which has an equivalent in Nix) and are intended to be reusable for further structed configuration enhancements. Support for string-includes and variable references can be re-added (compared to the status quo where they happen to work due to lack of escaping) when they are needed.
Updated to include legacy handlers for inclusion in NixOS 24.11. |
… to `services.dovecot2.pluginSettings` Legacy handlers were added since these options did make it into NixOS stable now.
… `sieve_global_extensions` to the Dovecot plugin settings Setting these values to empty (the default) clears the defaults which causes Sieve script execution to become pretty useless. Also adds the ability to use `null` with any option to force the entire option to be omitted from the generated Dovecot configuration.
…ixOS/nix#1537) Also avoid trimming single-line string values unnecessarily.
Description of changes
The added escaping functions cover the entire range of Dovecot supported config value constructs other than string-includes and variable references (neither of which has an equivalent in Nix) and are intended to be reusable for further structed configuration enhancements. Support for string-includes and variable references can be re-added (compared to the status quo where they happen to work due to lack of escaping) when they are needed.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.