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/sshguard: fix syslog identifiers and pid file #54495

Merged
merged 1 commit into from Jan 29, 2019

Conversation

Projects
None yet
4 participants
@peterhoeg
Copy link
Member

commented Jan 23, 2019

Motivation for this change
  1. Some syslog identifiers contain brackets - without enclosing quotes, that will break
  2. sshguard runs in the foreground, so if we give the PIDFile directive to systemd, it thinks the service should double-fork making service restarts when deploying with nixops fail.
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Cc @sargon

@sargon

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2019

Looks good to me.

@Infinisil
Copy link
Contributor

left a comment

The commits should also have the "nixos/sshguard:" prefix, instead of just "sshguard:"

Show resolved Hide resolved nixos/modules/services/security/sshguard.nix Outdated

@Mic92 Mic92 changed the title sshguard (nixos): fix syslog identifiers and pid file nixos/sshguard: fix syslog identifiers and pid file Jan 24, 2019

@Infinisil
Copy link
Contributor

left a comment

Since you're doing some cleanups (very nice!), I'm suggesting 2 more

Also, commit message needs to be changed (something like "nixos/sshguard: fix syslog ids, no more pid file, cleanups")

Show resolved Hide resolved nixos/modules/services/security/sshguard.nix Outdated
unitConfig.Documentation = "man:sshguard(8)";

serviceConfig = {
Type = "simple";

This comment has been minimized.

Copy link
@Infinisil

Infinisil Jan 24, 2019

Contributor

"simple" is the default, so this can be removed

This comment has been minimized.

Copy link
@peterhoeg

peterhoeg Jan 26, 2019

Author Member

I left this in on purpose, as systemd will be moving towards Type = "exec"; in one of the upcoming versions which will eventually become the default.

This comment has been minimized.

Copy link
@Infinisil

Infinisil Jan 26, 2019

Contributor

Is there a reason "exec" wouldn't work though? I doubt they'll make a change that breaks every such service.

This comment has been minimized.

Copy link
@peterhoeg

peterhoeg Jan 28, 2019

Author Member

There probably is no reason that exec shouldn't work so this is just to make things explicit. I'm fully with you - there is normally no point to specify what's already the default but in this case, I think it makes sense to be explicit.

This comment has been minimized.

Copy link
@Infinisil

Infinisil Jan 28, 2019

Contributor

I don't really agree, we shouldn't make changes already for a future version that only potentially (with a low chance) could break it.

But I don't want to hold this PR back just because of this

@peterhoeg peterhoeg force-pushed the peterhoeg:f/sshguard branch 2 times, most recently from 767a6f4 to 3281964 Jan 26, 2019

nixos/sshguard: fix syslog ids, no more pid file, cleanups
1. Allow syslog identifiers with special characters
2. Do not write a pid file as we are running in foreground anyway
3. Clean up the module for readability

Without this, when deploying using nixops, restarting sshguard would make
nixops show an error about restarting the service although the service is
actually being restarted.

@peterhoeg peterhoeg force-pushed the peterhoeg:f/sshguard branch from 3281964 to ee472e4 Jan 28, 2019

###### implementation

config = mkIf cfg.enable {

environment.systemPackages = [ pkgs.sshguard pkgs.iptables pkgs.ipset ];

This comment has been minimized.

Copy link
@Infinisil

Infinisil Jan 28, 2019

Contributor

Is sshguard useless on the command line? Is that the reason for the removal of this?

This comment has been minimized.

Copy link
@peterhoeg

peterhoeg Jan 29, 2019

Author Member

Correct, you don't interact with sshguard. You could argue that ipset should be there in case you want to manually inspect the ipset that sshguard manipulates but for all normal uses, neither of these are needed.

@Infinisil Infinisil merged commit f73df18 into NixOS:master Jan 29, 2019

9 checks passed

grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
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

@peterhoeg peterhoeg deleted the peterhoeg:f/sshguard branch Jan 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.