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
knock: init at 0.7.8 #74793
knock: init at 0.7.8 #74793
Conversation
is it easy to run as a standalone or should we expect some later module ? |
example = "7000,8000,9000"; | ||
}; | ||
|
||
one_time_sequences = mkOption { |
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.
Use camelCase for option names. Same for the others
example = 10; | ||
}; | ||
|
||
tcpflags = mkOption { |
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.
tcpFlags
default = null; | ||
type = nullOr str; | ||
description = '' | ||
Time to wait (in seconds) between Start_Command and Stop_Command. |
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.
Use <option>startCommand</option>
here to have it show up nicer in the manual. (and make sure to camelCase all such references to options too)
description = "Extra packages to add to PATH."; | ||
}; | ||
|
||
options = mkOption { |
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.
It's convention to not put such options into a separate attribute set. So I'd define options.services.knockd.interface
instead
options = mkOption { | ||
type = types.submodule { | ||
options = { | ||
useSyslog = mkEnableOption "logging messages through syslog()"; |
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.
Unless there is a good reason this should be configurable I think this can be dropped. The standard on NixOS is to log through journald. Similarly logfile
can be dropped.
serviceConfig = { | ||
ExecStart = "${pkgs.knock}/bin/knockd"; | ||
Restart = "always"; | ||
PIDFile = cfg.options.pidfile; |
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.
Also if possible make it not fork into the background, this is preferred in NixOS.
###### implementation | ||
|
||
config = mkIf cfg.enable { | ||
environment.systemPackages = with pkgs; [ knock ] ++ cfg.extraPackages; |
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.
Does this need to be added to systemPackages
? Unless there's a reason, this can be removed.
enable = mkEnableOption description; | ||
|
||
extraPackages = mkOption { | ||
default = with pkgs; [ iptables iproute ]; |
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.
Does the service work at all if these two packages aren't included here? If so, they should be set with path = [ iptables iproute ] ++ extraPackages
instead, and this should default to []
unitConfig.Documentation = "man:knockd(1)"; | ||
|
||
serviceConfig = { | ||
ExecStart = "${pkgs.knock}/bin/knockd"; |
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 runs as root like this which is not optimal. Try to make it use DynamicUser = true;
instead. Although this might not play well with the ability for the user to configure arbitrary commands.
port knock sequence. These port-hits need not be on open ports, since we | ||
use libpcap to sniff the raw interface traffic. | ||
''; | ||
homepage = "http://www.zeroflux.org/projects/knock"; |
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.
Has HTTPS support
@infinisil Thanks for an awesome review, I learned so much! I'll address all of the mentioned points whenever possible. |
Thank you for your contributions. This has been automatically marked as stale because it has had no activity for 180 days. If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity. Here are suggestions that might help resolve this more quickly:
|
Still important to me |
I marked this as stale due to inactivity. → More info |
If this is important to you please address the review feedback and solve the eval issue. Marking as draft. |
I marked this as stale due to inactivity. → More info |
Motivation for this change
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)This change is