Conversation
|
I've tested this (from the previous PR, but nothing substantial has changed since then) for both log processors and local API servers using systemd-nspawn containers on my home server, and it works well. The CrowdSec console is also able to see my setup. Legend:
For this setup, I only had to change the following in this crowdsec.nix file:
I'm a noob at self-hosting, so some of these issues might be just misconfigurations/stupid decisions from my side. |
|
@rharish101 Perhaps off-topic(?), but systemd credentials can be used to pass secrets into services. (I’m not able to directly test this PR at the moment, but I should be able to try it out when I get home.) |
(off-topic) Wow, yet another reason why systemd rocks. Thanks a lot! With this, my second point about the |
|
Reviewing now |
acuteaangle
left a comment
There was a problem hiding this comment.
I’m out of time to keep looking through this tonight.
I didn’t get a chance to set it up and run it locally tonight, so I may have missed an obvious lint/formatting issue somewhere.
dynamicUser will likely require changing a bunch of the directory constants.
systemd-confinement (source) is something we could also potentially look into. Not many nixpkgs modules use it currently, as it was incompatible with dynamicUser in the past.
I’m unfamiliar with the nixos tests, so I don’t know what sort of testing would be expected of a service like crowdsec. ‘internetworked intrusion prevention system’ feels like a tricky category of module to test, but is perhaps doable with some sort of container setup.
There was a problem hiding this comment.
Again, would be nice if we can use DynamicUser
There was a problem hiding this comment.
I found a different opinion from another active NixOS maintainer: https://c3d2.social/@sandro/114922075808595518
@SuperSandro2000 may I ask if your contra-arguments for including DynamicUser are significant enough to not use it or not?
There was a problem hiding this comment.
(I’m not the mentioned user, but I’d like to make my case for DynamicUser.)
why does every other new #NixOS module need to use DynamicUser
It is not compatible with easily providing sops secrets
systemd credentials may be used
and using LoadCredentials is just a pain in the ass.
fair enough.
In my opinion, the module should—if possible—abstract this.
forgejo does, as do nextcloud, paperless, lemmy, and many others.
it usually has negligible benefits.
Using dynamicUser means we don’t need to persist state in the form of a UID mapping, and is recommended for NixOS modules unless ‘problematic’. It may indeed be problematic for crowdsec—I haven’t yet had a chance to check what capabilities it needs to acquire logs. If it does work, I believe it is the best option.
There was a problem hiding this comment.
Would wrapProgram (full docs) from make(Binary)Wrapper be a simpler solution here?
There was a problem hiding this comment.
May I ask how that should look like?
cscli = pkgs.writeShellScriptBin "cscli" ''
wrapProgram $out/bin/cscli
''like this?
There was a problem hiding this comment.
Whoops, forgot I was looking at a module, not a package.
This feels like something that should be handled in the package.
This looks fine.
Of note: I believe sudo will drop most environment variables (by default); that may or may not be desirable.
I’d probably take after paperless and try to make it a little more robust,
sudo=exec
if [ "$USER" != "${cfg.user}" ]; then
${
if config.security.sudo.enable then
"sudo='exec ${config.security.wrapperDir}/sudo -u ${cfg.user}"
else
">&2 echo 'Aborting, cscli must be run as user `${cfg.user}`!'; exit 2"
}
fi
$sudo ${lib.getExe' cfg.package "cscli"} -c=${configFile} "$@"but most modules use the pattern exactly as already written, so I have no objections either way.
There was a problem hiding this comment.
Yeah, we can adjust cscli to have crowdsec in the path
There was a problem hiding this comment.
This feels like something that should be handled in the package.
So you mean that I should create a PR first to apply this to the package?
|
Typo in the pr title: crowsec - > crowdsec. |
Thanks! Fixed it |
fa7a49a to
7f8eb8e
Compare
TornaxO7
left a comment
There was a problem hiding this comment.
Wrote some responses to the review.
There was a problem hiding this comment.
Do you mean as the default username? I'd prefer to stick to crowdsec here since it's also more intuitive in my opinion.
There was a problem hiding this comment.
May I ask how that should look like?
cscli = pkgs.writeShellScriptBin "cscli" ''
wrapProgram $out/bin/cscli
''like this?
|
Now this is interesting. I did execute |
|
I haven’t found it yet, but it’s almost always trailing whitespace: the autoformatter doesn’t fix it, but the lint does complain about it. |
|
Hmm. Unable to reproduce locally. Cloning the PR and running |
|
Although I don't know why it's working now. |
|
nvm... |
|
Oh! I think maybe I see it. Line 509–510. I’m on a mobile device at the moment, so I can’t try it out. |
I assume you mean Using As @piegamesde said, the shell's pinned version was recently bumped, so your branch is likely using a different version of nixfmt to CI. Rebasing, then re-running treefmt should help. |
603d4e6 to
7a4dc4c
Compare
|
Yay, rebasing worked indeed. Thank you for the help :) |
|
I'm a bit unsure which reviews I should apply in which way. Any suggestions what I have to do to remove all "blockers"? |
|
Can anyone confirm that they have been able to successfully test this module? I’ve been trying to setup a minimal test case equivalent to the stock fail2ban rule (watch logs via journald, monitor for failed SSH logins, ban via iptables). However, I have not been successful so far. Additionally, doesn’t EDIT: |
Yup, here is my config for an SSH log processor (that doesn't act like a security engine): services.crowdsec = {
enable = true;
autoUpdateService = true;
name = "${config.networking.hostName}-sshd";
localConfig.acquisitions = [
{
source = "journalctl";
journalctl_filter = [ "_SYSTEMD_UNIT=sshd.service" ];
labels.type = "syslog";
use_time_machine = true;
}
];
hub.collections = [ "crowdsecurity/linux" ];
settings.lapi.credentialsFile = config.sops.secrets."crowdsec/sshd-creds".path;
};For the CrowdSec security engine, this is my config: # Add secrets using an environment file.
systemd.services.crowdsec.serviceConfig.EnvironmentFile = envFile;
services.crowdsec = {
enable = true;
autoUpdateService = true;
name = "${config.networking.hostName}-lapi";
# XXX: CrowdSec refuses to start unless some acquisitions are specified.
localConfig.acquisitions = [
{
source = "journalctl";
journalctl_filter = [ "_SYSTEMD_UNIT=ssh.service" ];
labels.type = "syslog";
}
];
settings.general = {
db_config = {
type = "postgres";
user = "crowdsec";
password = "\${DB_PASSWORD}";
db_name = "crowdsec";
host = constants.bridges.csec-pg.pg.ip4;
port = constants.ports.postgres;
};
api.server = {
enable = true;
listen_uri = "0.0.0.0:${toString constants.ports.crowdsec}";
console_path = "/var/lib/crowdsec/credentials/console.yaml";
online_client.credentials_path = "/var/lib/crowdsec/credentials/capi.yaml";
};
};
settings.lapi.credentialsFile = "/var/lib/crowdsec/credentials/lapi.yaml";
};
I wrote a custom module (just for my own use case) for a {
config,
lib,
pkgs,
...
}:
{
options.modules.crowdsec-bouncer = {
enable = lib.mkEnableOption "Enable CrowdSec firewall bouncer";
};
config =
let
package = pkgs.crowdsec-firewall-bouncer;
configFmt = pkgs.formats.yaml { };
configFile = configFmt.generate "crowdsec-firewall-bouncer.yaml" {
mode = "ipset";
log_mode = "stdout";
api_url = "\${API_URL}";
api_key = "\${API_KEY}";
};
ip4List = "crowdsec-blacklists";
ip6List = "crowdsec6-blacklists";
in
lib.mkIf config.modules.crowdsec-bouncer.enable {
networking.firewall.extraPackages = [ pkgs.ipset ];
networking.firewall.extraCommands = ''
ipset -exist create ${ip4List} hash:net timeout 3600
ipset -exist create ${ip6List} hash:net family inet6 timeout 3600
iptables -A INPUT -m set --match-set ${ip4List} src -j DROP
iptables -A FORWARD -m set --match-set ${ip4List} src -j DROP
ip6tables -A INPUT -m set --match-set ${ip6List} src -j DROP
ip6tables -A FORWARD -m set --match-set ${ip6List} src -j DROP
'';
environment.systemPackages = [
package
pkgs.ipset
];
sops.secrets."crowdsec/bouncer-env".restartUnits = [ "crowdsec-firewall-bouncer.service" ];
# Reference: https://github.com/crowdsecurity/cs-firewall-bouncer/blob/main/config/crowdsec-firewall-bouncer.service
systemd.services.crowdsec-firewall-bouncer = {
description = "The firewall bouncer for CrowdSec";
after = [
"syslog.target"
"network.target"
"remote-fs.target"
"nss-lookup.target"
"crowdsec.service"
];
wantedBy = [ "multi-user.target" ];
path = with pkgs; [
iptables
ipset
];
serviceConfig = {
Type = "notify";
ExecStart = [ "${lib.getExe package} -c ${configFile}" ];
ExecStartPre = [ "${lib.getExe package} -c ${configFile} -t" ];
ExecStartPost = [ "${lib.getExe' pkgs.coreutils-full "sleep"} 0.1" ];
Restart = "always";
RestartSec = 10;
LimitNOFILE = 65536;
KillMode = "mixed";
EnvironmentFile = config.sops.secrets."crowdsec/bouncer-env".path;
};
};
};
} |
|
I'm going to clean up the commit history if everything is fine. |
7b20158 to
cf632ee
Compare
There was a problem hiding this comment.
@acuteaangle I'm a bit unsure if I'm understanding the DynamicUser thingy correctly:
So I just need to apply DynamicUser = true, right?
Should I give the crowdsec-update-hub also the DynamicUser = true option? I don't see why I sholdn't.
|
Is there a clean way of handling the hub update branch? Right now, it's hard-coded to I updated nixpkgs, and CrowdSec is now at 1.6.11. This led to the service created by this module to stop working, since the CrowdSec hub doesn't have a v1.6.11 branch yet. Here's the link to an issue I opened upstream: crowdsecurity/hub#1444. The maintainer suggested I change the branch to |
I'd say it's fine to set it to Hm... maybe we can let the user set the version on their own and give it the default value |
|
(Just a reminder that I'm still open for discussion/feedback 👀 ) |
I agree, we could expose an option to set it, say with @TornaxO7 If you want to test with |
Co-authored-by: M0ustach3 <37956764+M0ustach3@users.noreply.github.com> Co-authored-by: Summer Tea <79724236+acuteaangle@users.noreply.github.com>
cf632ee to
f1e8e0f
Compare
|
What. Why did it got closed? |
|
Aw come on. |
|
There should be a button to reopen. |
|
I can't reopen it. Somehow I messed it really up. However, I've created a new PR: #437310 |
Looks like it was closed as part of you pushing? Maybe you force-pushed a history that had no diff against IIRC, GitHub usually closes PRs when the branch is deleted or has no diff with the base branch. And until the branch returns to a state where it has changes not in the base branch, it can't be re-opened either. |
|
Hm... probably because I had an outdated version on my main machine... |


"Continue-PR" of #387625.
Note: I'm not really experienced with nix but I can try to help to maintain this.
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.