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

Adding crab hole #341598

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

NiklasVousten
Copy link

Description of changes

This change added crab-hole, a pi-hole clone written in Rust.
Additionally a service for crab-hole was added.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Copy link
Contributor

@misuzu misuzu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi there! Great work. I've added a couple of suggestions

description = "Crab-holes data directory.";
};

config = mkOption {
Copy link
Contributor

@misuzu misuzu Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks overly complicated to me.
Is there a reason it's not a regular RFC 42-style settings option?

Suggested change
config = mkOption {
settings = mkOption {

Comment on lines 194 to 232
warnings =
(
if (cfg.configFile != null) && (cfg.config != null) then
[
''
crab-hole: Config File and settings options are both set.
Config File will used instead!
''
]
else
[ ]
)
++ (
if (cfg.config.downstream == [ ]) then
[
''
crab-hole: Empty downstream specified. Server will not be accessible
''
]
else
[ ]
)
++ (
if (cfg.config.upstream.name_servers == [ ]) then
[
''
crab-hole: Empty upstream specified. Server will not be able to resolve
''
]
else
[ ]
);

assertions = [
{
assertion = (cfg.configFile != null) || (cfg.config != null);
message = "crab-hole: Need to set settings or config file";
}
];
Copy link
Contributor

@misuzu misuzu Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With RFC-42-style settings this is not required:

Suggested change
warnings =
(
if (cfg.configFile != null) && (cfg.config != null) then
[
''
crab-hole: Config File and settings options are both set.
Config File will used instead!
''
]
else
[ ]
)
++ (
if (cfg.config.downstream == [ ]) then
[
''
crab-hole: Empty downstream specified. Server will not be accessible
''
]
else
[ ]
)
++ (
if (cfg.config.upstream.name_servers == [ ]) then
[
''
crab-hole: Empty upstream specified. Server will not be able to resolve
''
]
else
[ ]
);
assertions = [
{
assertion = (cfg.configFile != null) || (cfg.config != null);
message = "crab-hole: Need to set settings or config file";
}
];
services.crab-hole.configFile = lib.mkDefault (settingsFormat.generate "crab-hole.toml" cfg.settings);
environment.etc."crab-hole.toml".source = cfg.configFile;


rustPlatform.buildRustPackage rec {
pname = "crab-hole";
version = "0.1.9";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
version = "0.1.9";
version = "0.1.9-unstable-2024-09-05";


meta.maintainers = [
lib.maintainers.NiklasVousten
];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A meta.doc would be great

pkgs,
...
}:
with lib;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an anti-pattern

Suggested change
with lib;

Comment on lines 40 to 41
type = types.nullOr (
types.submodule {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
type = types.nullOr (
types.submodule {
type = types.submodule {
freeformType = settingsFormat.type;

CRAB_HOLE_DIR = cfg.workDir;
};
serviceConfig = {
Type = "simple";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the default

Suggested change
Type = "simple";

Group = cfg.group;
WorkingDirectory = cfg.workDir;

ExecStart = ''${pkgs.crab-hole}/bin/crab-hole'';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ExecStart = ''${pkgs.crab-hole}/bin/crab-hole'';
ExecStart = lib.getExe cfg.package;

options = {
services.crab-hole = {
enable = mkEnableOption "Crab-hole Service";

This comment was marked as resolved.

Comment on lines 28 to 32
meta.platforms = with lib.platforms; [
linux
windows
darwin
];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
meta.platforms = with lib.platforms; [
linux
windows
darwin
];
platforms = lib.platforms.all;

Comment on lines 257 to 323
preStart =
let
settings =
{
blocklist = {
include_subdomains = cfg.config.blocklist.include_subdomains;
lists = lists.forEach cfg.config.blocklist.lists (
v: if isPath v then ("file://${cfg.workDir}/${filename v}") else v
);
allow_list = lists.forEach cfg.config.blocklist.allow_list (
v: if isPath v then ("file://${cfg.workDir}/${filename v}") else v
);
};
downstream = forEach cfg.config.downstream (
x:
if x.protocol == "udp" then
{
protocol = x.protocol;
listen = x.listen;
port = x.port;
}
else if (x.protocol == "https" && x.dns_hostname != null) then
x
else
{
protocol = x.protocol;
listen = x.listen;
port = x.port;
certificate = x.certificate;
key = x.key;
timeout = x.timeout;
}
);
upstream.name_servers = cfg.config.upstream.name_servers;
}
// (
if (cfg.config.api == null) then
{ }
else
{
api =
if (cfg.config.api.admin_key != null) then
cfg.config.api
else
{
port = cfg.config.api.port;
listen = cfg.config.api.listen;
show_doc = cfg.config.api.show_doc;
};
}
);

blockListFiles = builtins.filter (v: isPath v) cfg.config.blocklist.lists;
allowListFiles = builtins.filter (v: isPath v) cfg.config.blocklist.allow_list;

selectedConfig =
if cfg.configFile != null then cfg.configFile else (configFormat.generate "config.toml" settings);
in
''
cp -f '${selectedConfig}' '${cfg.workDir}/config.toml'
${builtins.concatStringsSep "\n" (
map (file: "cp -f '${file}' '${cfg.workDir}/${filename file}'") blockListFiles
)}
${builtins.concatStringsSep "\n" (
map (file: "cp -f '${file}' '${cfg.workDir}/${filename file}'") allowListFiles
)}
'';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
preStart =
let
settings =
{
blocklist = {
include_subdomains = cfg.config.blocklist.include_subdomains;
lists = lists.forEach cfg.config.blocklist.lists (
v: if isPath v then ("file://${cfg.workDir}/${filename v}") else v
);
allow_list = lists.forEach cfg.config.blocklist.allow_list (
v: if isPath v then ("file://${cfg.workDir}/${filename v}") else v
);
};
downstream = forEach cfg.config.downstream (
x:
if x.protocol == "udp" then
{
protocol = x.protocol;
listen = x.listen;
port = x.port;
}
else if (x.protocol == "https" && x.dns_hostname != null) then
x
else
{
protocol = x.protocol;
listen = x.listen;
port = x.port;
certificate = x.certificate;
key = x.key;
timeout = x.timeout;
}
);
upstream.name_servers = cfg.config.upstream.name_servers;
}
// (
if (cfg.config.api == null) then
{ }
else
{
api =
if (cfg.config.api.admin_key != null) then
cfg.config.api
else
{
port = cfg.config.api.port;
listen = cfg.config.api.listen;
show_doc = cfg.config.api.show_doc;
};
}
);
blockListFiles = builtins.filter (v: isPath v) cfg.config.blocklist.lists;
allowListFiles = builtins.filter (v: isPath v) cfg.config.blocklist.allow_list;
selectedConfig =
if cfg.configFile != null then cfg.configFile else (configFormat.generate "config.toml" settings);
in
''
cp -f '${selectedConfig}' '${cfg.workDir}/config.toml'
${builtins.concatStringsSep "\n" (
map (file: "cp -f '${file}' '${cfg.workDir}/${filename file}'") blockListFiles
)}
${builtins.concatStringsSep "\n" (
map (file: "cp -f '${file}' '${cfg.workDir}/${filename file}'") allowListFiles
)}
'';

environment = {
CRAB_HOLE_DIR = cfg.workDir;
};
serviceConfig = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
serviceConfig = {
restartTriggers = [ cfg.configFile ];
serviceConfig = {

Comment on lines 326 to 328
systemd.tmpfiles.rules = [
"d '${cfg.workDir}' 0750 ${cfg.user} ${cfg.group} - -"
];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be replaced by StateDirectory

Comment on lines 330 to 239
# Adding crab-hole user and group
users.users = optionalAttrs (cfg.user == "crab-hole") {
crab-hole = {
description = "Crab-hole service";
home = cfg.workDir;
group = cfg.group;
isSystemUser = true;
};
};

users.groups = optionalAttrs (cfg.group == "crab-hole") {
crab-hole = { };
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be replaced by DynamicUser

Comment on lines 344 to 346
environment.systemPackages = [
pkgs.crab-hole
];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really needed in PATH for everyone?

Suggested change
environment.systemPackages = [
pkgs.crab-hole
];

Type = "simple";
User = cfg.user;
Group = cfg.group;
WorkingDirectory = cfg.workDir;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
WorkingDirectory = cfg.workDir;
StateDirectory = "crab-hole";
WorkingDirectory = "/var/lib/crab-hole";

Comment on lines 244 to 245
User = cfg.user;
Group = cfg.group;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
User = cfg.user;
Group = cfg.group;
DynamicUser = true;

Comment on lines 239 to 241
environment = {
CRAB_HOLE_DIR = cfg.workDir;
};
Copy link
Contributor

@misuzu misuzu Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be better to just set the home directory.

Suggested change
environment = {
CRAB_HOLE_DIR = cfg.workDir;
};
environment.HOME = "/var/lib/crab-hole";

wantedBy = [ "multi-user.target" ];
after = [ "network-online.target" ];
wants = [ "network-online.target" ];
description = "Start the crab-hole dns server";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
description = "Start the crab-hole dns server";
description = "crab-hole dns server";

config = mkOption {
default = null;
description = "Crab-holes config";

Copy link
Contributor

@misuzu misuzu Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An example would be great. You can reuse upstream example here using https://pseitz.github.io/toml-to-json-online-converter/ and https://json-to-nix.pages.dev
It's better to make it smaller though

Suggested change
example = {
downstream = [
{
listen = "localhost";
port = 8080;
protocol = "udp";
}
{
listen = "[::]";
port = 8053;
protocol = "udp";
}
{
certificate = "dns.example.com.crt";
key = "dns.example.com.key";
listen = "[::]";
port = 8054;
protocol = "tls";
timeout_ms = 3000;
}
{
certificate = "dns.example.com.crt";
dns_hostname = "dns.example.com";
key = "dns.example.com.key";
listen = "[::]";
port = 8055;
protocol = "https";
timeout_ms = 3000;
}
{
certificate = "dns.example.com.crt";
dns_hostname = "dns.example.com";
key = "dns.example.com.key";
listen = "127.0.0.1";
port = 8055;
protocol = "quic";
timeout_ms = 3000;
}
];
api = {
admin_key = "1234";
listen = "127.0.0.1";
port = 8080;
show_doc = true;
};
blocklist = {
allow_list = [
"file:///allowed.txt"
];
include_subdomains = true;
lists = [
"https://raw.githubusercontent.com/StevenBlack/hosts/master/alternates/fakenews-gambling-porn/hosts"
"https://s3.amazonaws.com/lists.disconnect.me/simple_tracking.txt"
"file:///blocked.txt"
];
};
upstream = {
name_servers = [
{
protocol = "tls";
socket_addr = "[2606:4700:4700::1111]:853";
tls_dns_name = "1dot1dot1dot1.cloudflare-dns.com";
trust_nx_responses = false;
}
{
protocol = "tls";
socket_addr = "[2606:4700:4700::1001]:853";
tls_dns_name = "1dot1dot1dot1.cloudflare-dns.com";
trust_nx_responses = false;
}
{
protocol = "tls";
socket_addr = "1.1.1.1:853";
tls_dns_name = "1dot1dot1dot1.cloudflare-dns.com";
trust_nx_responses = false;
}
{
protocol = "tls";
socket_addr = "1.0.0.1:853";
tls_dns_name = "1dot1dot1dot1.cloudflare-dns.com";
trust_nx_responses = false;
}
];
options = {
validate = true;
};
};
};

ExecStart = ''${pkgs.crab-hole}/bin/crab-hole'';

AmbientCapabilities = "CAP_NET_BIND_SERVICE";
CapabilityBoundingSet = "CAP_NET_BIND_SERVICE";

This comment was marked as resolved.

@NiklasVousten
Copy link
Author

Thank you for your Feedback. This was very helpful!
I got inspired by some different existing configs, which then may have resulted in a bit of strange code. But your suggestions helped to clean this up a lot.

About some of the suggestions:

In my understanding, the following line ensures, that the service can only get the specified capabilities. This would improve security. So I think it is sensible to keep this line.
CapabilityBoundingSet = "CAP_NET_BIND_SERVICE";

Now on the prestart block.
The issue I had, where null values. For example, some values are not needed for the downstream, depending on the protocol. So udp will not need certificate. Thus I set the default to null, which then will be left out by the prestart block.

It can be solved with an apply for the downstream options, similar to the prestart.
One Issue i notices then, is missing required options like certificate for the quick protocol. It will fail at the build step and not on the validation.

Do you have a solution in mind for this case? Or can nudge me to a helpful resource?
I have tried enums once, but this failed (maybe I did not do it correctly).

@misuzu
Copy link
Contributor

misuzu commented Sep 16, 2024

Do you have a solution in mind for this case?

I'd need an example to give a useful advice, but I think just removing the offending options should fix the issue. It would still allow to set the options, but they won't be type-checked. On the other hand, you won't need to update the module when they change upstream. The nuclear solution would be to only leave freeformType = settingsFormat.type; in the submodule type

@NiklasVousten NiklasVousten force-pushed the adding-crab-hole branch 4 times, most recently from 3215570 to 6d03ac6 Compare September 22, 2024 14:36
@NiklasVousten
Copy link
Author

The freeformType = settingsFormat.type; was really helpful. This fixed the issue.

I guess a user then has to check if the service started correctly and fix the config if this failed.

It would be awesome if you could review the changes again @misuzu

I struggled briefly with getting the meta.doc to work, so a short check would be nice.

Copy link
Contributor

@misuzu misuzu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I've added a couple of suggestions

lib.maintainers.NiklasVousten
];
# Readme from upstream
meta.doc = pkgs.writeText "crab-hole.md" ''
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move the doc to its own file

Suggested change
meta.doc = pkgs.writeText "crab-hole.md" ''
meta.doc = ./crab-hole.md;

## Configuration: {#module-services-crab-hole-configuration}
Example config file using cloudflare as dot (dns-over-tls) upstream.
```toml
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to just leave a link to an example TOML file instead.
What would actually be useful here is a working configuration example of the crab-hole module, ready to be copy-pasted and customized.

Comment on lines 183 to 191
warnings =
if (cfg.settings.upstream.options != { }) && (cfg.settings.upstream.options.validate) then
[
''
Validate options will ONLY allow DNSSec domains. See https://github.com/LuckyTurtleDev/crab-hole/issues/29
''
]
else
[ ];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
warnings =
if (cfg.settings.upstream.options != { }) && (cfg.settings.upstream.options.validate) then
[
''
Validate options will ONLY allow DNSSec domains. See https://github.com/LuckyTurtleDev/crab-hole/issues/29
''
]
else
[ ];
warnings =
lib.optional (cfg.settings.upstream.options.validate or false)
''
Validate options will ONLY allow DNSSec domains. See https://github.com/LuckyTurtleDev/crab-hole/issues/29
'';


configFile = lib.mkOption {
type = lib.types.path;
description = "The config file of crab-hole. If files are added via url, make sure the service has access to them";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
description = "The config file of crab-hole. If files are added via url, make sure the service has access to them";
description = "The config file of crab-hole. If files are added via url, make sure the service has access to them. Setting this option will override any configuration applied by the settings option.";

@NiklasVousten
Copy link
Author

Documentation is now in a separate file.
I had to add the user and group again due to permission issues with certificates.

Should I add you as coauthor, because I used a lot of your suggestions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants