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

Improve DHCP handling #1731

Merged
merged 7 commits into from
Nov 16, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions src/api/config.c
Original file line number Diff line number Diff line change
Expand Up @@ -693,8 +693,23 @@ static int api_config_patch(struct ftl_conn *api)
const char *response = getJSONvalue(new_item, elem, &newconf);
if(response != NULL)
{
log_err("/api/config: %s invalid: %s", new_item->k, response);
continue;
char *hint = calloc(strlen(new_item->k) + strlen(response) + 3, sizeof(char));
if(hint == NULL)
{
free_config(&newconf);
return send_json_error(api, 500,
"internal_error",
"Failed to allocate memory for hint",
NULL);
}
strcpy(hint, new_item->k);
strcat(hint, ": ");
strcat(hint, response);
free_config(&newconf);
return send_json_error_free(api, 400,
"bad_request",
"Config item is invalid",
hint, true);
}

// Get pointer to memory location of this conf_item (global)
Expand Down
11 changes: 11 additions & 0 deletions src/api/docs/content/specs/config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -268,8 +268,10 @@ components:
type: boolean
IPv4:
type: string
x-format: ipv4
IPv6:
type: string
x-format: ipv6
blocking:
type: object
properties:
Expand All @@ -279,8 +281,10 @@ components:
type: boolean
IPv4:
type: string
x-format: ipv4
IPv6:
type: string
x-format: ipv6
rateLimit:
type: object
properties:
Expand All @@ -295,10 +299,16 @@ components:
type: boolean
start:
type: string
x-format: ipv4
end:
type: string
x-format: ipv4
router:
type: string
x-format: ipv4
netmask:
type: string
x-format: ipv4
domain:
type: string
leaseTime:
Expand Down Expand Up @@ -627,6 +637,7 @@ components:
end: "192.168.0.250"
router: "192.168.0.1"
domain: "lan"
netmask: "0.0.0.0"
leaseTime: "24h"
ipv6: true
rapidCommit: true
Expand Down
19 changes: 13 additions & 6 deletions src/config/config.c
Original file line number Diff line number Diff line change
Expand Up @@ -687,23 +687,23 @@ void initConfig(struct config *conf)
conf->dhcp.start.k = "dhcp.start";
conf->dhcp.start.h = "Start address of the DHCP address pool";
conf->dhcp.start.a = cJSON_CreateStringReference("<ip-addr>, e.g., \"192.168.0.10\"");
conf->dhcp.start.t = CONF_STRING;
conf->dhcp.start.t = CONF_STRUCT_IN_ADDR;
conf->dhcp.start.f = FLAG_RESTART_FTL;
conf->dhcp.start.d.s = (char*)"";
memset(&conf->dhcp.start.d.in_addr, 0, sizeof(struct in_addr));

conf->dhcp.end.k = "dhcp.end";
conf->dhcp.end.h = "End address of the DHCP address pool";
conf->dhcp.end.a = cJSON_CreateStringReference("<ip-addr>, e.g., \"192.168.0.250\"");
conf->dhcp.end.t = CONF_STRING;
conf->dhcp.end.t = CONF_STRUCT_IN_ADDR;
conf->dhcp.end.f = FLAG_RESTART_FTL;
conf->dhcp.end.d.s = (char*)"";
memset(&conf->dhcp.end.d.in_addr, 0, sizeof(struct in_addr));

conf->dhcp.router.k = "dhcp.router";
conf->dhcp.router.h = "Address of the gateway to be used (typically the address of your router in a home installation)";
conf->dhcp.router.a = cJSON_CreateStringReference("<ip-addr>, e.g., \"192.168.0.1\"");
conf->dhcp.router.t = CONF_STRING;
conf->dhcp.router.t = CONF_STRUCT_IN_ADDR;
conf->dhcp.router.f = FLAG_RESTART_FTL;
conf->dhcp.router.d.s = (char*)"";
memset(&conf->dhcp.router.d.in_addr, 0, sizeof(struct in_addr));

conf->dhcp.domain.k = "dhcp.domain";
conf->dhcp.domain.h = "The DNS domain used by your Pi-hole";
Expand All @@ -712,6 +712,13 @@ void initConfig(struct config *conf)
conf->dhcp.domain.f = FLAG_RESTART_FTL | FLAG_ADVANCED_SETTING;
conf->dhcp.domain.d.s = (char*)"lan";

conf->dhcp.netmask.k = "dhcp.netmask";
conf->dhcp.netmask.h = "The netmask used by your Pi-hole. For directly connected networks (i.e., networks on which the machine running Pi-hole has an interface) the netmask is optional and may be set to \"0.0.0.0\": it will then be determined from the interface configuration itself. For networks which receive DHCP service via a relay agent, we cannot determine the netmask itself, so it should explicitly be specified, otherwise Pi-hole guesses based on the class (A, B or C) of the network address.";
conf->dhcp.netmask.a = cJSON_CreateStringReference("<any valid netmask>, e.g., \"255.255.255.0\" or \"0.0.0.0\" for auto-discovery");
conf->dhcp.netmask.t = CONF_STRUCT_IN_ADDR;
conf->dhcp.netmask.f = FLAG_RESTART_FTL | FLAG_ADVANCED_SETTING;
memset(&conf->dhcp.netmask.d.in_addr, 0, sizeof(struct in_addr));

conf->dhcp.leaseTime.k = "dhcp.leaseTime";
conf->dhcp.leaseTime.h = "If the lease time is given, then leases will be given for that length of time. If not given, the default lease time is one hour for IPv4 and one day for IPv6.";
conf->dhcp.leaseTime.a = cJSON_CreateStringReference("The lease time can be in seconds, or minutes (e.g., \"45m\") or hours (e.g., \"1h\") or days (like \"2d\") or even weeks (\"1w\"). You may also use \"infinite\" as string but be aware of the drawbacks");
Expand Down
3 changes: 2 additions & 1 deletion src/config/config.h
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ enum conf_type {
#define FLAG_PSEUDO_ITEM (1 << 2)
#define FLAG_INVALIDATE_SESSIONS (1 << 3)
#define FLAG_WRITE_ONLY (1 << 4)
#define FLAG_ENV_VAR (1 << 5)
#define FLAG_ENV_VAR (1 << 5)

struct conf_item {
const char *k; // item Key
Expand Down Expand Up @@ -178,6 +178,7 @@ struct config {
struct conf_item end;
struct conf_item router;
struct conf_item domain;
struct conf_item netmask;
struct conf_item leaseTime;
struct conf_item ipv6;
struct conf_item rapidCommit;
Expand Down
25 changes: 19 additions & 6 deletions src/config/dnsmasq_config.c
Original file line number Diff line number Diff line change
Expand Up @@ -419,12 +419,25 @@ bool __attribute__((const)) write_dnsmasq_config(struct config *conf, bool test_
fputs("# DHCP server setting\n", pihole_conf);
fputs("dhcp-authoritative\n", pihole_conf);
fputs("dhcp-leasefile="DHCPLEASESFILE"\n", pihole_conf);
fprintf(pihole_conf, "dhcp-range=%s,%s,%s\n",
conf->dhcp.start.v.s,
conf->dhcp.end.v.s,
conf->dhcp.leaseTime.v.s);
fprintf(pihole_conf, "dhcp-option=option:router,%s\n",
conf->dhcp.router.v.s);
char start[INET_ADDRSTRLEN] = { 0 },
end[INET_ADDRSTRLEN] = { 0 },
router[INET_ADDRSTRLEN] = { 0 };
inet_ntop(AF_INET, &conf->dhcp.start.v.in_addr, start, INET_ADDRSTRLEN);
inet_ntop(AF_INET, &conf->dhcp.end.v.in_addr, end, INET_ADDRSTRLEN);
inet_ntop(AF_INET, &conf->dhcp.router.v.in_addr, router, INET_ADDRSTRLEN);
fprintf(pihole_conf, "dhcp-range=%s,%s", start, end);
// Net mask is optional, only add if it is not 0.0.0.0
const struct in_addr inaddr_empty = {0};
if(memcmp(&conf->dhcp.netmask.v.in_addr, &inaddr_empty, sizeof(inaddr_empty)) != 0)
{
char netmask[INET_ADDRSTRLEN] = { 0 };
inet_ntop(AF_INET, &conf->dhcp.netmask.v.in_addr, netmask, INET_ADDRSTRLEN);
fprintf(pihole_conf, ",%s", netmask);
}
// Lease time is optional, only add it if it is set
if(strlen(conf->dhcp.leaseTime.v.s) > 0)
fprintf(pihole_conf, ",%s", conf->dhcp.leaseTime.v.s);
fprintf(pihole_conf, "\ndhcp-option=option:router,%s\n", router);

if(conf->dhcp.rapidCommit.v.b)
fputs("dhcp-rapid-commit\n", pihole_conf);
Expand Down
54 changes: 43 additions & 11 deletions test/api/libs/responseVerifyer.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
# Please see LICENSE file for your rights under this license.

import io
import pprint
import ipaddress
import random
import zipfile
from libs.openAPI import openApi
Expand Down Expand Up @@ -221,15 +221,47 @@ def verify_teleporter_zip(self, teleporter_archive: bytes):
return self.errors


# Check if a string is a valid IPv4 address
def valid_ipv4(self, addr: str) -> bool:
octets = addr.split(".") # type: list[str]
if len(octets) != 4:
return False
for octet in octets:
if not octet.isdigit():
return False
if int(octet) < 0 or int(octet) > 255:
return False
return True


# Check if a string is a valid IPv6 address
def valid_ipv6(self, addr: str) -> bool:
# Split the address into parts
parts = addr.split(":") # type: list[str]
# Check if the address is a valid IPv6 address
if len(parts) != 8:
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to use shortened IPv6 addresses here like 2001:db8::1234:5678?
Will this code work in this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for your comment, something got lost here. As you can see, I added import ipaddress above and actually have also used it for the check but this got somehow lost. I will add it back in

return False


# Verify a single property's type
def verify_type(self, prop_type: any, yaml_type: str, yaml_nullable: bool):
def verify_type(self, prop: any, yaml_type: str, yaml_nullable: bool, yaml_format: str = None):
# Get the type of the property
prop_type = type(prop)
# None is an acceptable reply when this is specified in the API specs
if prop_type is type(None) and yaml_nullable:
return True
# Check if the type is correct using the YAML_TYPES translation table
if yaml_type not in self.YAML_TYPES:
self.errors.append("Property type \"" + yaml_type + "\" is not valid in OpenAPI specs")
return False
if yaml_format is not None:
# Check if the format is correct
if yaml_format == "ipv4" and not type(ipaddress.ip_address(prop)) is ipaddress.IPv4Address:
self.errors.append("Property \"" + str(prop) + "\" is not a valid IPv4 address")
return False
elif yaml_format == "ipv6" and not type(ipaddress.ip_address(prop)) is ipaddress.IPv6Address:
self.errors.append("Property \"" + str(prop) + "\" is not a valid IPv6 address")
return False
return prop_type in self.YAML_TYPES[yaml_type]


Expand Down Expand Up @@ -306,17 +338,19 @@ def verify_property(self, YAMLprops: dict, YAMLexamples: dict, FTLprops: dict, p
# if not defined as string, integer, etc.)
yaml_nullable = 'nullable' in YAMLprop and YAMLprop['nullable'] == True

# Get format of this property (if defined)
yaml_format = YAMLprop['format'] if 'format' in YAMLprop else YAMLprop['x-format'] if 'x-format' in YAMLprop else None

# Add this property to the YAML response
self.YAMLresponse[flat_path] = []

# Check type of YAML example (if defined)
if 'example' in YAMLprop:
example_type = type(YAMLprop['example'])
# Check if the type of the example matches the
# type we defined in the API specs
self.YAMLresponse[flat_path].append(YAMLprop['example'])
if not self.verify_type(example_type, yaml_type, yaml_nullable):
self.errors.append(f"API example ({str(example_type)}) does not match defined type ({yaml_type}) in {flat_path} (nullable: " + ("True" if yaml_nullable else "False") + ")")
if not self.verify_type(YAMLprop['example'], yaml_type, yaml_nullable, yaml_format):
self.errors.append(f"API example ({str(type(YAMLprop['example']))}) does not match defined type ({yaml_type}) in {flat_path} (nullable: " + ("True" if yaml_nullable else "False") + ")")
return False

# Check type of externally defined YAML examples (next to schema)
Expand All @@ -340,16 +374,14 @@ def verify_property(self, YAMLprops: dict, YAMLexamples: dict, FTLprops: dict, p
if skip_this:
continue
# Check if the type of the example matches the type we defined in the API specs
example_type = type(example)
self.YAMLresponse[flat_path].append(example)
if not self.verify_type(example_type, yaml_type, yaml_nullable):
self.errors.append(f"API example ({str(example_type)}) does not match defined type ({yaml_type}) in {flat_path} (nullable: " + ("True" if yaml_nullable else "False") + ")")
if not self.verify_type(example, yaml_type, yaml_nullable, yaml_format):
self.errors.append(f"API example ({str(type(example))}) does not match defined type ({yaml_type}) in {flat_path} (nullable: " + ("True" if yaml_nullable else "False") + ")")
return False

# Compare type of FTL's reply against what we defined in the API specs
ftl_type = type(FTLprop)
if not self.verify_type(ftl_type, yaml_type, yaml_nullable):
self.errors.append(f"FTL's reply ({str(ftl_type)}) does not match defined type ({yaml_type}) in {flat_path}")
if not self.verify_type(FTLprop, yaml_type, yaml_nullable, yaml_format):
self.errors.append(f"FTL's reply ({str(type(FTLprop))}) does not match defined type ({yaml_type}) in {flat_path}")
return False
return all_okay

Expand Down
11 changes: 11 additions & 0 deletions test/pihole.toml
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,17 @@
# <any valid domain>
domain = "lan"

# The netmask used by your Pi-hole. For directly connected networks (i.e., networks on
# which the machine running Pi-hole has an interface) the netmask is optional and may
# be set to "0.0.0.0": it will then be determined from the interface configuration
# itself. For networks which receive DHCP service via a relay agent, we cannot
# determine the netmask itself, so it should explicitly be specified, otherwise
# Pi-hole guesses based on the class (A, B or C) of the network address.
#
# Possible values are:
# <any valid netmask>, e.g., "255.255.255.0" or "0.0.0.0" for auto-discovery
netmask = "0.0.0.0"

# If the lease time is given, then leases will be given for that length of time. If not
# given, the default lease time is one hour for IPv4 and one day for IPv6.
#
Expand Down