From 87dd091535736e824e2033e36ef0dc420ba9c316 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Sun, 16 Jun 2024 21:28:27 +0200 Subject: [PATCH 01/30] Fix crash caused by double free() corruption encountered with rev-server addresses with prefix lengths != {8,16,24,32} Signed-off-by: DL6ER --- src/config/dnsmasq_config.c | 6 ++++++ src/dnsmasq/option.c | 9 ++++----- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/config/dnsmasq_config.c b/src/config/dnsmasq_config.c index 8c2e26868..ad9fb93d4 100644 --- a/src/config/dnsmasq_config.c +++ b/src/config/dnsmasq_config.c @@ -723,6 +723,12 @@ bool __attribute__((const)) write_dnsmasq_config(struct config *conf, bool test_ { log_warn("New dnsmasq configuration is not valid (%s), config remains unchanged", errbuf); + if(debug_flags[DEBUG_ANY]) + { + log_debug(DEBUG_ANY, "Temporary dnsmasq config file left in place for debugging purposes"); + return false; + } + // Remove temporary config file if(remove(DNSMASQ_TEMP_CONF) != 0) { diff --git a/src/dnsmasq/option.c b/src/dnsmasq/option.c index 8e7377dc9..d075ad0aa 100644 --- a/src/dnsmasq/option.c +++ b/src/dnsmasq/option.c @@ -1193,10 +1193,10 @@ static char *domain_rev4(int from_file, char *server, struct in_addr *addr4, int return _("error"); } - if (sdetails.orig_hostinfo) - freeaddrinfo(sdetails.orig_hostinfo); } } + if (sdetails.orig_hostinfo) + freeaddrinfo(sdetails.orig_hostinfo); return NULL; } @@ -1280,11 +1280,10 @@ static char *domain_rev6(int from_file, char *server, struct in6_addr *addr6, in if (!add_update_server(flags, &serv_addr, &source_addr, interface, domain, NULL)) return _("error"); } - - if (sdetails.orig_hostinfo) - freeaddrinfo(sdetails.orig_hostinfo); } } + if (sdetails.orig_hostinfo) + freeaddrinfo(sdetails.orig_hostinfo); return NULL; } From 8e032db78c92747f5d84158988ef04df05a74a22 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Mon, 1 Jul 2024 16:19:38 +0200 Subject: [PATCH 02/30] Start NTP server only after first successful NTP synchronization Signed-off-by: DL6ER --- src/dnsmasq_interface.c | 5 ----- src/ntp/client.c | 15 +++++++++++++-- src/ntp/ntp.h | 2 +- src/ntp/server.c | 6 +++--- 4 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/dnsmasq_interface.c b/src/dnsmasq_interface.c index 72bb41cee..42caa431e 100644 --- a/src/dnsmasq_interface.c +++ b/src/dnsmasq_interface.c @@ -2912,17 +2912,12 @@ void FTL_fork_and_bind_sockets(struct passwd *ent_pw, bool dnsmasq_start) // so they will not listen to real-time signals handle_realtime_signals(); - // We will use the attributes object later to start all threads in - // detached mode pthread_attr_t attr; // Initialize thread attributes object with default attribute values // Do NOT detach threads as we want to join them during shutdown with a // fixed timeout to give them time to clean up and finish their work pthread_attr_init(&attr); - // Initialize NTP server - ntp_server_start(&attr); - // Start NTP sync thread ntp_start_sync_thread(&attr); diff --git a/src/ntp/client.c b/src/ntp/client.c index 532689a24..0b689a6f5 100644 --- a/src/ntp/client.c +++ b/src/ntp/client.c @@ -593,18 +593,28 @@ static void *ntp_client_thread(void *arg) // Run NTP client bool first_run = true; + bool ntp_server_started = false; while(!killed) { // Run NTP client - ntp_client(config.ntp.sync.server.v.s, true, false); + const bool success = ntp_client(config.ntp.sync.server.v.s, true, false); // Load queries from database after first NTP synchronization if(first_run) { load_queries_from_disk(); + first_run = false; } + if(success && !ntp_server_started) + { + // Initialize NTP server only after first NTP + // synchronization to ensure that the time is set + // correctly + ntp_server_started = ntp_server_start(); + } + // Intermediate cancellation-point BREAK_IF_KILLED(); @@ -625,6 +635,7 @@ bool ntp_start_sync_thread(pthread_attr_t *attr) strlen(config.ntp.sync.server.v.s) == 0 || config.ntp.sync.interval.v.ui == 0) { + log_info("NTP sync is disabled - NTP server will not be available"); load_queries_from_disk(); return false; } @@ -632,7 +643,7 @@ bool ntp_start_sync_thread(pthread_attr_t *attr) // Create thread if(pthread_create(&threads[NTP], attr, ntp_client_thread, NULL) != 0) { - log_err("Cannot create NTP client thread"); + log_err("Cannot create NTP client thread - NTP server will not be available"); load_queries_from_disk(); return false; } diff --git a/src/ntp/ntp.h b/src/ntp/ntp.h index 72445423e..7adbe8adc 100644 --- a/src/ntp/ntp.h +++ b/src/ntp/ntp.h @@ -27,7 +27,7 @@ uint64_t gettime64(void); void print_debug_time(const char *label, const uint32_t *u32p, const uint64_t ntp_time); // Start NTP server -bool ntp_server_start(pthread_attr_t *attr); +bool ntp_server_start(void); // Start NTP client bool ntp_client(const char *server, const bool settime, const bool print); diff --git a/src/ntp/server.c b/src/ntp/server.c index 652c95250..94a259f53 100644 --- a/src/ntp/server.c +++ b/src/ntp/server.c @@ -373,7 +373,7 @@ static void *ntp_bind_and_listen(void *param) } // Start the NTP server -bool ntp_server_start(pthread_attr_t *attr) +bool ntp_server_start(void) { // Spawn two pthreads, one for IPv4 and one for IPv6 @@ -382,7 +382,7 @@ bool ntp_server_start(pthread_attr_t *attr) { // Create a thread for the IPv4 NTP server pthread_t thread; - if (pthread_create(&thread, attr, ntp_bind_and_listen, (void *)0) != 0) + if (pthread_create(&thread, NULL, ntp_bind_and_listen, (void *)0) != 0) { log_ntp_message(true, true, "Cannot create NTP server thread for IPv4"); return false; @@ -394,7 +394,7 @@ bool ntp_server_start(pthread_attr_t *attr) { // Create a thread for the IPv6 NTP server pthread_t thread; - if (pthread_create(&thread, attr, ntp_bind_and_listen, (void *)1) != 0) + if (pthread_create(&thread, NULL, ntp_bind_and_listen, (void *)1) != 0) { log_ntp_message(true, true, "Cannot create NTP server thread for IPv6"); return false; From a04303ae2373ba5e68181e6341ab22669df393f2 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Mon, 1 Jul 2024 16:22:18 +0200 Subject: [PATCH 03/30] Ensure NTP servers are properly terminated when FTL is shutting down and remove obsolete variable thread_running[] (it is only ever set but never read) Signed-off-by: DL6ER --- src/database/database-thread.c | 2 -- src/enums.h | 4 +++- src/gc.c | 2 -- src/ntp/client.c | 8 +++----- src/ntp/server.c | 17 +++++++++++------ src/resolve.c | 3 --- src/signals.c | 5 +++-- src/signals.h | 1 - src/timers.c | 2 -- 9 files changed, 20 insertions(+), 24 deletions(-) diff --git a/src/database/database-thread.c b/src/database/database-thread.c index f8768902a..3a83e4252 100644 --- a/src/database/database-thread.c +++ b/src/database/database-thread.c @@ -83,7 +83,6 @@ static bool analyze_database(sqlite3 *db) void *DB_thread(void *val) { // Set thread name - thread_running[DB] = true; prctl(PR_SET_NAME, thread_names[DB], 0, 0, 0); // Save timestamp as we do not want to store immediately @@ -241,6 +240,5 @@ void *DB_thread(void *val) dbclose(&db); log_info("Terminating database thread"); - thread_running[DB] = false; return NULL; } diff --git a/src/enums.h b/src/enums.h index 2df1a0eb6..66ba915a0 100644 --- a/src/enums.h +++ b/src/enums.h @@ -250,7 +250,9 @@ enum thread_types { GC, DNSclient, TIMER, - NTP, + NTP_CLIENT, + NTP_SERVER4, + NTP_SERVER6, THREADS_MAX } __attribute__ ((packed)); diff --git a/src/gc.c b/src/gc.c index 3d4f0962c..4b6a42f52 100644 --- a/src/gc.c +++ b/src/gc.c @@ -481,7 +481,6 @@ static bool check_files_on_same_device(const char *path1, const char *path2) void *GC_thread(void *val) { // Set thread name - thread_running[GC] = true; prctl(PR_SET_NAME, thread_names[GC], 0, 0, 0); // Remember when we last ran the actions @@ -567,6 +566,5 @@ void *GC_thread(void *val) watch_config(false); log_info("Terminating GC thread"); - thread_running[GC] = false; return NULL; } diff --git a/src/ntp/client.c b/src/ntp/client.c index 0b689a6f5..066d31624 100644 --- a/src/ntp/client.c +++ b/src/ntp/client.c @@ -588,8 +588,7 @@ bool ntp_client(const char *server, const bool settime, const bool print) static void *ntp_client_thread(void *arg) { // Set thread name - thread_running[NTP] = true; - prctl(PR_SET_NAME, thread_names[NTP], 0, 0, 0); + prctl(PR_SET_NAME, thread_names[NTP_CLIENT], 0, 0, 0); // Run NTP client bool first_run = true; @@ -619,11 +618,10 @@ static void *ntp_client_thread(void *arg) BREAK_IF_KILLED(); // Sleep before retrying - thread_sleepms(NTP, 1000 * config.ntp.sync.interval.v.ui); + thread_sleepms(NTP_CLIENT, 1000 * config.ntp.sync.interval.v.ui); } log_info("Terminating NTP thread"); - thread_running[NTP] = false; return NULL; } @@ -641,7 +639,7 @@ bool ntp_start_sync_thread(pthread_attr_t *attr) } // Create thread - if(pthread_create(&threads[NTP], attr, ntp_client_thread, NULL) != 0) + if(pthread_create(&threads[NTP_CLIENT], attr, ntp_client_thread, NULL) != 0) { log_err("Cannot create NTP client thread - NTP server will not be available"); load_queries_from_disk(); diff --git a/src/ntp/server.c b/src/ntp/server.c index 94a259f53..ae9eed9c6 100644 --- a/src/ntp/server.c +++ b/src/ntp/server.c @@ -39,6 +39,10 @@ #include // log_ntp_message() #include "database/message-table.h" +// NTP_SERVER_IPV4,6 +#include "enums.h" +// threads +#include "signals.h" uint64_t ntp_last_sync = 0u; uint32_t ntp_root_delay = 0u; @@ -224,7 +228,7 @@ static bool ntp_reply(const int socket_fd, const struct sockaddr *saddr_p, const } // Process incoming NTP requests -static void request_process_loop(int fd, const char *ipstr, const int protocol) +static void request_process_loop(const int fd, const char *ipstr, const int protocol) { log_info("NTP server listening on %s:123 (%s)", ipstr, protocol == AF_INET ? "IPv4" : "IPv6"); while (true) @@ -280,9 +284,12 @@ static void request_process_loop(int fd, const char *ipstr, const int protocol) // Start the NTP server static void *ntp_bind_and_listen(void *param) { - const int protocol = param == 0 ? AF_INET : AF_INET6; + // Set thread name + const unsigned int thread_id = param == 0 ? NTP_SERVER4 : NTP_SERVER6; + prctl(PR_SET_NAME, thread_names[thread_id], 0, 0, 0); // Create a socket + const int protocol = param == 0 ? AF_INET : AF_INET6; errno = 0; const int s = socket(protocol, SOCK_DGRAM, IPPROTO_UDP); if(s == -1) @@ -301,8 +308,7 @@ static void *ntp_bind_and_listen(void *param) memset(ipstr, 0, sizeof(ipstr)); if(protocol == AF_INET) { - // IPv4 - set thread name - prctl(PR_SET_NAME, "NTP (IPv4)", 0, 0, 0); + // IPv4 NTP server // Prepare the bind address struct sockaddr_in bind_addr; @@ -327,8 +333,7 @@ static void *ntp_bind_and_listen(void *param) } else { - // IPv6 - set thread name - prctl(PR_SET_NAME, "NTP (IPv6)", 0, 0, 0); + // IPv6 NTP server // Set socket options to allow IPv6 only, otherwise it will bind // to both IPv4 and IPv6 and show IPv4 addresses as diff --git a/src/resolve.c b/src/resolve.c index 453e477e6..a99c93ac9 100644 --- a/src/resolve.c +++ b/src/resolve.c @@ -1053,14 +1053,12 @@ static void resolveUpstreams(const bool onlynew) void *DNSclient_thread(void *val) { // Set thread name - thread_running[DNSclient] = true; prctl(PR_SET_NAME, thread_names[DNSclient], 0, 0, 0); // Test struct sizes if(!check_struct_sizes()) { log_err("Struct sizes do not match expected sizes, aborting resolver thread"); - thread_running[DNSclient] = false; return NULL; } @@ -1124,6 +1122,5 @@ void *DNSclient_thread(void *val) } log_info("Terminating resolver thread"); - thread_running[DNSclient] = false; return NULL; } diff --git a/src/signals.c b/src/signals.c index 8e14cfe29..34f6b52c6 100644 --- a/src/signals.c +++ b/src/signals.c @@ -34,13 +34,14 @@ static time_t FTLstarttime = 0; volatile int exit_code = EXIT_SUCCESS; volatile sig_atomic_t thread_cancellable[THREADS_MAX] = { false }; -volatile sig_atomic_t thread_running[THREADS_MAX] = { false }; const char * const thread_names[THREADS_MAX] = { "database", "housekeeper", "dns-client", "timer", - "ntp-client" + "ntp-client", + "ntp-server4", + "ntp-server6", }; // Return the (null-terminated) name of the calling thread diff --git a/src/signals.h b/src/signals.h index 766648871..4e388dd90 100644 --- a/src/signals.h +++ b/src/signals.h @@ -29,7 +29,6 @@ extern volatile sig_atomic_t want_to_reimport_aliasclients; extern volatile sig_atomic_t want_to_reload_lists; extern volatile sig_atomic_t thread_cancellable[THREADS_MAX]; -extern volatile sig_atomic_t thread_running[THREADS_MAX]; extern const char * const thread_names[THREADS_MAX]; #define BREAK_IF_KILLED() { if(killed) break; } diff --git a/src/timers.c b/src/timers.c index 17b16704a..b841d3bed 100644 --- a/src/timers.c +++ b/src/timers.c @@ -84,7 +84,6 @@ void get_blockingmode_timer(double *delay, bool *target_status) void *timer(void *val) { // Set thread name - thread_running[GC] = true; prctl(PR_SET_NAME, thread_names[TIMER], 0, 0, 0); // Save timestamp as we do not want to store immediately @@ -110,7 +109,6 @@ void *timer(void *val) } log_info("Terminating timer thread"); - thread_running[GC] = false; return NULL; } From a0372769c17acdfb78b0a8834f1234dda32f6264 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Mon, 1 Jul 2024 16:40:45 +0200 Subject: [PATCH 04/30] DNSSEC signatures are only valid for specified time windows, and should be rejected outside those windows. This generates an interesting chicken-and-egg problem for machines which don't have a hardware real time clock. For these machines to determine the correct time typically requires use of NTP and therefore DNS, but validating DNS requires that the correct time is already known. Resolve this by setting dnssec-no-timecheck removing the time-window checks (but not other DNSSEC validation.) only until NTP sync finishes (or if we realize the user doesn't want it) We do not use the overloaded SIGINT (as dnsmasq) but SIGUSR7 to avoid killing the process when in debug mode (this is a fundamental drawback of the dnsmasq implementation) Signed-off-by: DL6ER --- src/config/dnsmasq_config.c | 6 +++++- src/dnsmasq/dnsmasq.c | 3 +++ src/ntp/client.c | 9 +++++++++ src/signals.c | 10 +++++++--- src/signals.h | 1 + 5 files changed, 25 insertions(+), 4 deletions(-) diff --git a/src/config/dnsmasq_config.c b/src/config/dnsmasq_config.c index 6b8cbc822..8fbd5fdf8 100644 --- a/src/config/dnsmasq_config.c +++ b/src/config/dnsmasq_config.c @@ -400,7 +400,11 @@ bool __attribute__((const)) write_dnsmasq_config(struct config *conf, bool test_ fputs("# 2017-02-02 root zone trust anchor\n", pihole_conf); fputs("trust-anchor=.,20326,8,2,E06D44B80B8F1D39A95C0B0D7C65D08458E880409BBC683457104237C7F8EC8D\n", pihole_conf); - fputs("\n", pihole_conf); + + // Prevent DNSSEC timestamp checks until either NTP synchronization has succeeded or + // the user has disabled the NTP client + fputs("# Do not check DNSSEC timestamps until NTP synchronization has succeeded\n", pihole_conf); + fputs("dnssec-no-timecheck\n\n", pihole_conf); } if(strlen(conf->dns.hostRecord.v.s) > 0) diff --git a/src/dnsmasq/dnsmasq.c b/src/dnsmasq/dnsmasq.c index 7990ed616..1a8170462 100644 --- a/src/dnsmasq/dnsmasq.c +++ b/src/dnsmasq/dnsmasq.c @@ -97,6 +97,7 @@ int main_dnsmasq (int argc, char **argv) sigaction(SIGUSR2, &sigact, NULL); sigaction(SIGHUP, &sigact, NULL); sigaction(SIGUSR6, &sigact, NULL); // Pi-hole modification + sigaction(SIGUSR7, &sigact, NULL); // Pi-hole modification sigaction(SIGALRM, &sigact, NULL); sigaction(SIGCHLD, &sigact, NULL); sigaction(SIGINT, &sigact, NULL); @@ -1358,6 +1359,8 @@ static void sig_handler(int sig) else event = EVENT_TIME; } + else if (sig == SIGUSR7) // Pi-hole modified + event = EVENT_TIME; else return; diff --git a/src/ntp/client.c b/src/ntp/client.c index 066d31624..a34d85040 100644 --- a/src/ntp/client.c +++ b/src/ntp/client.c @@ -603,6 +603,12 @@ static void *ntp_client_thread(void *arg) { load_queries_from_disk(); + // If this was the first run and NTP time synchronization was + // successful, we send SIGUSR7 to the embedded dnsmasq instance + // to signal time is now guaranteed to be correct + if(success) + kill(main_pid(), SIGUSR7); + first_run = false; } @@ -634,6 +640,9 @@ bool ntp_start_sync_thread(pthread_attr_t *attr) config.ntp.sync.interval.v.ui == 0) { log_info("NTP sync is disabled - NTP server will not be available"); + // Send SIGUSR7 to embedded dnsmasq instance to signal time is + // assumed to be correct + kill(main_pid(), SIGUSR7); load_queries_from_disk(); return false; } diff --git a/src/signals.c b/src/signals.c index 34f6b52c6..63270cc23 100644 --- a/src/signals.c +++ b/src/signals.c @@ -322,6 +322,11 @@ static void SIGRT_handler(int signum, siginfo_t *si, void *unused) // { // // Signal internally used to signal dnsmasq it has to stop // } + // else if(rtsig == 7) + // { + // // Signal internally used to signal dnsmasq it should do + // // DNSSEC timestamp checks + // } // Restore errno before returning back to previous context errno = _errno; @@ -448,9 +453,8 @@ void handle_realtime_signals(void) // Catch all real-time signals for(int signum = SIGRTMIN; signum <= SIGRTMAX; signum++) { - if(signum == SIGUSR6) - // Skip SIGUSR6 as it is used internally to signify - // dnsmasq to stop + if(signum == SIGUSR6 || signum == SIGUSR7) + // Skip SIGUSR6 as it is used internally by dnsmasq continue; struct sigaction SIGACTION = { 0 }; diff --git a/src/signals.h b/src/signals.h index 4e388dd90..cc5d8f1bc 100644 --- a/src/signals.h +++ b/src/signals.h @@ -13,6 +13,7 @@ #include "enums.h" #define SIGUSR6 (SIGRTMIN + 6) +#define SIGUSR7 (SIGRTMIN + 7) // defined in dnsmasq/dnsmasq.h extern volatile char FTL_terminate; From 4d32ffe13f8999fd6cda80060987985fe1aa87fc Mon Sep 17 00:00:00 2001 From: DL6ER Date: Mon, 1 Jul 2024 17:44:54 +0200 Subject: [PATCH 05/30] Run the NTP test later in the test suite to ensure the NTP server has been started Signed-off-by: DL6ER --- test/test_suite.bats | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/test/test_suite.bats b/test/test_suite.bats index d747e8dd6..3f7fa23cd 100644 --- a/test/test_suite.bats +++ b/test/test_suite.bats @@ -1353,12 +1353,6 @@ [[ ${lines[0]} == '{"error":{"key":"bad_request","message":"Config items set via environment variables cannot be changed via the API","hint":"misc.nice"},"took":'*'}' ]] } -@test "Check NTP server is broadcasting correct time" { - run bash -c './pihole-FTL ntp 127.0.0.1 --dry-run' - printf "%s\n" "${lines[@]}" - [[ $status == 0 ]] -} - # We cannot easily test IPv6 as it may not be available in docker (CI) @test "API domain search: Non-existing domain returns expected JSON" { @@ -1817,3 +1811,11 @@ printf "%s\n" "${lines[@]}" [[ ${lines[0]} == "3" ]] } + +@test "Check NTP server is broadcasting correct time" { + # Run this test at the very end of the test suite + # to ensure the NTP server has been started + run bash -c './pihole-FTL ntp 127.0.0.1' + printf "%s\n" "${lines[@]}" + [[ $status == 0 ]] +} From c846b21876741430c467eee3a9b8f9c10aab95c1 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Mon, 1 Jul 2024 20:22:03 +0200 Subject: [PATCH 06/30] Add new ntp.sync.active boolean to ease disabling of the NTP client. Also move all the RTC properties inside ntp.sync because this is where they apply and where RTC sync can be disabled Signed-off-by: DL6ER --- src/api/docs/content/specs/config.yaml | 29 +++++++++-------- src/config/config.c | 43 +++++++++++++++----------- src/config/config.h | 11 ++++--- src/ntp/client.c | 5 +-- src/ntp/rtc.c | 28 ++++++++--------- test/pihole.toml | 25 ++++++++------- 6 files changed, 78 insertions(+), 63 deletions(-) diff --git a/src/api/docs/content/specs/config.yaml b/src/api/docs/content/specs/config.yaml index af0edbfb6..cdab29c6c 100644 --- a/src/api/docs/content/specs/config.yaml +++ b/src/api/docs/content/specs/config.yaml @@ -348,21 +348,23 @@ components: sync: type: object properties: + active: + type: boolean server: type: string interval: type: integer count: type: integer - rtc: - type: object - properties: - set: - type: boolean - device: - type: string - utc: - type: boolean + rtc: + type: object + properties: + set: + type: boolean + device: + type: string + utc: + type: boolean resolver: type: object properties: @@ -708,13 +710,14 @@ components: active: true address: "" sync: + active: true server: "pool.ntp.org" interval: 3600 count: 8 - rtc: - set: true - device: "" - utc: true + rtc: + set: true + device: "" + utc: true resolver: resolveIPv4: true resolveIPv6: true diff --git a/src/config/config.c b/src/config/config.c index 8054e83d8..eee5a8c0a 100644 --- a/src/config/config.c +++ b/src/config/config.c @@ -814,6 +814,13 @@ void initConfig(struct config *conf) memset(&conf->ntp.ipv6.address.d.in6_addr, 0, sizeof(struct in6_addr)); conf->ntp.ipv6.address.c = validate_stub; // Only type-based checking + conf->ntp.sync.active.k = "ntp.sync.active"; + conf->ntp.sync.active.h = "Should FTL try to synchronize the system time with an upstream NTP server?"; + conf->ntp.sync.active.t = CONF_BOOL; + conf->ntp.sync.active.f = FLAG_RESTART_FTL; + conf->ntp.sync.active.d.b = true; + conf->ntp.sync.active.c = validate_stub; // Only type-based checking + conf->ntp.sync.server.k = "ntp.sync.server"; conf->ntp.sync.server.h = "NTP upstream server to sync with, e.g., \"pool.ntp.org\". Note that the NTP server should be located as close as possible to you in order to minimize the time offset possibly introduced by different routing paths."; conf->ntp.sync.server.a = cJSON_CreateStringReference("valid NTP upstream server"); @@ -833,24 +840,24 @@ void initConfig(struct config *conf) conf->ntp.sync.count.d.ui = 8; conf->ntp.sync.count.c = validate_stub; // Only type-based checking - conf->ntp.rtc.set.k = "ntp.rtc.set"; - conf->ntp.rtc.set.h = "Should FTL update a real-time clock (RTC) if available?"; - conf->ntp.rtc.set.t = CONF_BOOL; - conf->ntp.rtc.set.d.b = true; - conf->ntp.rtc.set.c = validate_stub; // Only type-based checking - - conf->ntp.rtc.device.k = "ntp.rtc.device"; - conf->ntp.rtc.device.h = "Path to the RTC device to update. Leave empty for auto-discovery"; - conf->ntp.rtc.device.a = cJSON_CreateStringReference("Path to the RTC device, e.g., \"/dev/rtc0\""); - conf->ntp.rtc.device.t = CONF_STRING; - conf->ntp.rtc.device.d.s = (char*)""; - conf->ntp.rtc.device.c = validate_stub; // Only type-based checking - - conf->ntp.rtc.utc.k = "ntp.rtc.utc"; - conf->ntp.rtc.utc.h = "Should the RTC be set to UTC?"; - conf->ntp.rtc.utc.t = CONF_BOOL; - conf->ntp.rtc.utc.d.b = true; - conf->ntp.rtc.utc.c = validate_stub; // Only type-based checking + conf->ntp.sync.rtc.set.k = "ntp.sync.rtc.set"; + conf->ntp.sync.rtc.set.h = "Should FTL update a real-time clock (RTC) if available?"; + conf->ntp.sync.rtc.set.t = CONF_BOOL; + conf->ntp.sync.rtc.set.d.b = true; + conf->ntp.sync.rtc.set.c = validate_stub; // Only type-based checking + + conf->ntp.sync.rtc.device.k = "ntp.sync.rtc.device"; + conf->ntp.sync.rtc.device.h = "Path to the RTC device to update. Leave empty for auto-discovery"; + conf->ntp.sync.rtc.device.a = cJSON_CreateStringReference("Path to the RTC device, e.g., \"/dev/rtc0\""); + conf->ntp.sync.rtc.device.t = CONF_STRING; + conf->ntp.sync.rtc.device.d.s = (char*)""; + conf->ntp.sync.rtc.device.c = validate_stub; // Only type-based checking + + conf->ntp.sync.rtc.utc.k = "ntp.sync.rtc.utc"; + conf->ntp.sync.rtc.utc.h = "Should the RTC be set to UTC?"; + conf->ntp.sync.rtc.utc.t = CONF_BOOL; + conf->ntp.sync.rtc.utc.d.b = true; + conf->ntp.sync.rtc.utc.c = validate_stub; // Only type-based checking // struct resolver diff --git a/src/config/config.h b/src/config/config.h index 069217d31..81d4a15f6 100644 --- a/src/config/config.h +++ b/src/config/config.h @@ -201,15 +201,16 @@ struct config { struct conf_item address; } ipv6; struct { + struct conf_item active; struct conf_item server; struct conf_item interval; struct conf_item count; + struct { + struct conf_item set; + struct conf_item device; + struct conf_item utc; + } rtc; } sync; - struct { - struct conf_item set; - struct conf_item device; - struct conf_item utc; - } rtc; } ntp; struct { diff --git a/src/ntp/client.c b/src/ntp/client.c index a34d85040..f5c9929a1 100644 --- a/src/ntp/client.c +++ b/src/ntp/client.c @@ -576,7 +576,7 @@ bool ntp_client(const char *server, const bool settime, const bool print) ntp_root_dispersion = D2FP(theta_stdev); // Finally, adjust RTC if configured - if(config.ntp.rtc.set.v.b) + if(config.ntp.sync.rtc.set.v.b) ntp_sync_rtc(); } @@ -635,7 +635,8 @@ static void *ntp_client_thread(void *arg) bool ntp_start_sync_thread(pthread_attr_t *attr) { // Return early if NTP client is disabled - if(config.ntp.sync.server.v.s == NULL || + if(config.ntp.sync.active.v.b == false || + config.ntp.sync.server.v.s == NULL || strlen(config.ntp.sync.server.v.s) == 0 || config.ntp.sync.interval.v.ui == 0) { diff --git a/src/ntp/rtc.c b/src/ntp/rtc.c index abeb457ef..3fe8f93b4 100644 --- a/src/ntp/rtc.c +++ b/src/ntp/rtc.c @@ -51,15 +51,15 @@ static int open_rtc(void) const gid_t gid = getgid(); // If the user has specified an RTC device, try to open it - if(config.ntp.rtc.device.v.s != NULL && - strlen(config.ntp.rtc.device.v.s) > 0) + if(config.ntp.sync.rtc.device.v.s != NULL && + strlen(config.ntp.sync.rtc.device.v.s) > 0) { // Open the RTC device - rtc_fd = open(config.ntp.rtc.device.v.s, O_RDONLY); + rtc_fd = open(config.ntp.sync.rtc.device.v.s, O_RDONLY); if (rtc_fd != -1) { log_debug(DEBUG_NTP, "Successfully opened RTC at \"%s\"", - config.ntp.rtc.device.v.s); + config.ntp.sync.rtc.device.v.s); return rtc_fd; } @@ -72,32 +72,32 @@ static int open_rtc(void) { // Get current owner of the device struct stat st = { 0 }; - if(stat(config.ntp.rtc.device.v.s, &st) == -1) + if(stat(config.ntp.sync.rtc.device.v.s, &st) == -1) { log_debug(DEBUG_NTP, "stat(\"%s\") failed: %s", - config.ntp.rtc.device.v.s, strerror(errno)); + config.ntp.sync.rtc.device.v.s, strerror(errno)); return -1; } - if(chown(config.ntp.rtc.device.v.s, uid, gid) == -1) + if(chown(config.ntp.sync.rtc.device.v.s, uid, gid) == -1) { log_debug(DEBUG_NTP, "chown(\"%s\", %u, %u) failed: %s", - config.ntp.rtc.device.v.s, uid, gid, strerror(errno)); + config.ntp.sync.rtc.device.v.s, uid, gid, strerror(errno)); return -1; } - rtc_fd = open(config.ntp.rtc.device.v.s, O_RDONLY); + rtc_fd = open(config.ntp.sync.rtc.device.v.s, O_RDONLY); if (rtc_fd != -1) { log_debug(DEBUG_NTP, "Successfully opened RTC at \"%s\"", - config.ntp.rtc.device.v.s); + config.ntp.sync.rtc.device.v.s); } // Chown the device back to the original owner - if(chown(config.ntp.rtc.device.v.s, st.st_uid, st.st_gid) == -1) + if(chown(config.ntp.sync.rtc.device.v.s, st.st_uid, st.st_gid) == -1) { log_debug(DEBUG_NTP, "chown(\"%s\", %u, %u) failed: %s", - config.ntp.rtc.device.v.s, st.st_uid, st.st_gid, strerror(errno)); + config.ntp.sync.rtc.device.v.s, st.st_uid, st.st_gid, strerror(errno)); return -1; } @@ -106,7 +106,7 @@ static int open_rtc(void) } log_debug(DEBUG_NTP, "Failed to open RTC at \"%s\": %s", - config.ntp.rtc.device.v.s, strerror(errno)); + config.ntp.sync.rtc.device.v.s, strerror(errno)); return -1; } @@ -255,7 +255,7 @@ bool ntp_sync_rtc(void) // Time to which we will set Hardware Clock, in broken down format struct tm new_time = { 0 }; const time_t newtime = time(NULL); - if(config.ntp.rtc.utc.v.b) + if(config.ntp.sync.rtc.utc.v.b) // UTC gmtime_r(&newtime, &new_time); else diff --git a/test/pihole.toml b/test/pihole.toml index 0b4332668..4dbe98898 100644 --- a/test/pihole.toml +++ b/test/pihole.toml @@ -481,6 +481,9 @@ address = "" [ntp.sync] + # Should FTL try to synchronize the system time with an upstream NTP server? + active = true + # NTP upstream server to sync with, e.g., "pool.ntp.org". Note that the NTP server # should be located as close as possible to you in order to minimize the time offset # possibly introduced by different routing paths. @@ -495,18 +498,18 @@ # Number of NTP syncs to perform and average before updating the system time count = 8 - [ntp.rtc] - # Should FTL update a real-time clock (RTC) if available? - set = true + [ntp.sync.rtc] + # Should FTL update a real-time clock (RTC) if available? + set = true - # Path to the RTC device to update. Leave empty for auto-discovery - # - # Possible values are: - # Path to the RTC device, e.g., "/dev/rtc0" - device = "" + # Path to the RTC device to update. Leave empty for auto-discovery + # + # Possible values are: + # Path to the RTC device, e.g., "/dev/rtc0" + device = "" - # Should the RTC be set to UTC? - utc = true + # Should the RTC be set to UTC? + utc = true [resolver] # Should FTL try to resolve IPv4 addresses to hostnames? @@ -1102,7 +1105,7 @@ all = true ### CHANGED, default = false # Configuration statistics: -# 148 total entries out of which 93 entries are default +# 149 total entries out of which 94 entries are default # --> 55 entries are modified # 2 entries are forced through environment: # - misc.nice From a3d2d469a5071a5dff4a4dc36df0941f3269c7b9 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Mon, 1 Jul 2024 20:23:35 +0200 Subject: [PATCH 07/30] Start NTP server also when NTP client is disabled - the system may get accurate time from elsewhere - it isn't intuitive that the server cannot be started without the client Signed-off-by: DL6ER --- src/ntp/client.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/ntp/client.c b/src/ntp/client.c index f5c9929a1..913fc2f86 100644 --- a/src/ntp/client.c +++ b/src/ntp/client.c @@ -640,19 +640,24 @@ bool ntp_start_sync_thread(pthread_attr_t *attr) strlen(config.ntp.sync.server.v.s) == 0 || config.ntp.sync.interval.v.ui == 0) { - log_info("NTP sync is disabled - NTP server will not be available"); + log_info("NTP sync is disabled"); // Send SIGUSR7 to embedded dnsmasq instance to signal time is // assumed to be correct kill(main_pid(), SIGUSR7); load_queries_from_disk(); + ntp_server_start(); return false; } // Create thread if(pthread_create(&threads[NTP_CLIENT], attr, ntp_client_thread, NULL) != 0) { - log_err("Cannot create NTP client thread - NTP server will not be available"); + log_err("Cannot create NTP client thread"); + // Send SIGUSR7 to embedded dnsmasq instance to signal time is + // assumed to be correct - at least we cannot synchronize it + kill(main_pid(), SIGUSR7); load_queries_from_disk(); + ntp_server_start(); return false; } From 4059586ed1d394616561c4b520df24d033b3dde2 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Mon, 1 Jul 2024 20:28:19 +0200 Subject: [PATCH 08/30] Do not even try to start NTP client thread if CAP_SYS_TIME is not available Signed-off-by: DL6ER --- src/ntp/client.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/src/ntp/client.c b/src/ntp/client.c index 913fc2f86..43cce286d 100644 --- a/src/ntp/client.c +++ b/src/ntp/client.c @@ -41,6 +41,8 @@ #include "database/message-table.h" // load_queries_from_disk() #include "database/query-table.h" +// check_capability() +#include "capabilities.h" struct ntp_sync { bool valid; @@ -649,12 +651,27 @@ bool ntp_start_sync_thread(pthread_attr_t *attr) return false; } + // Check if we have the ambient capabilities to set the system time. + // Without CAP_SYS_TIME, we cannot set the system time and the NTP + // client will not be able to synchronize the time so there is no point + // in starting the thread. + if(!check_capability(CAP_SYS_TIME)) + { + log_warn("Insufficient permissions to set system time, NTP client not available"); + // Send SIGUSR7 to embedded dnsmasq instance to signal time is + // assumed to be correct + kill(main_pid(), SIGUSR7); + load_queries_from_disk(); + ntp_server_start(); + return false; + } + // Create thread if(pthread_create(&threads[NTP_CLIENT], attr, ntp_client_thread, NULL) != 0) { log_err("Cannot create NTP client thread"); // Send SIGUSR7 to embedded dnsmasq instance to signal time is - // assumed to be correct - at least we cannot synchronize it + // assumed to be correct kill(main_pid(), SIGUSR7); load_queries_from_disk(); ntp_server_start(); From 0b82825b53dbc128f858c964a15237564e7c70ca Mon Sep 17 00:00:00 2001 From: DL6ER Date: Mon, 1 Jul 2024 20:52:57 +0200 Subject: [PATCH 09/30] The CI containers may not be able to set the host's time - this is okay Signed-off-by: DL6ER --- test/test_suite.bats | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_suite.bats b/test/test_suite.bats index 3f7fa23cd..c62c9c1d3 100644 --- a/test/test_suite.bats +++ b/test/test_suite.bats @@ -481,7 +481,7 @@ } @test "No WARNING messages in FTL.log (besides known warnings)" { - run bash -c 'grep "WARNING:" /var/log/pihole/FTL.log | grep -v -E "CAP_NET_ADMIN|CAP_NET_RAW|CAP_SYS_NICE|CAP_IPC_LOCK|CAP_CHOWN|CAP_NET_BIND_SERVICE|CAP_SYS_TIME|(Cannot set process priority)|FTLCONF_"' + run bash -c 'grep "WARNING:" /var/log/pihole/FTL.log | grep -v -E "CAP_NET_ADMIN|CAP_NET_RAW|CAP_SYS_NICE|CAP_IPC_LOCK|CAP_CHOWN|CAP_NET_BIND_SERVICE|CAP_SYS_TIME|(Cannot set process priority)|FTLCONF_|(Insufficient permissions to set system time, NTP client not available)"' printf "%s\n" "${lines[@]}" [[ "${lines[@]}" == "" ]] } From 4781c8d134d8c1383f8f00dad5dd0e66028e32bf Mon Sep 17 00:00:00 2001 From: DL6ER Date: Tue, 2 Jul 2024 03:46:26 +0200 Subject: [PATCH 10/30] Do not add errors encountered seen in CLI mode to the message table Signed-off-by: DL6ER --- src/database/message-table.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/database/message-table.c b/src/database/message-table.c index 031d7fb89..409d151b9 100644 --- a/src/database/message-table.c +++ b/src/database/message-table.c @@ -293,6 +293,10 @@ static int _add_message(const enum message_type type, static int _add_message(const enum message_type type, const char *message, const size_t count,...) { + // Log to database only if not in CLI mode + if(cli_mode) + return -1; + int rowid = -1; // Return early if database is known to be broken if(FTLDBerror()) @@ -1237,10 +1241,6 @@ void logg_regex_warning(const char *type, const char *warning, const int dbindex // Log to FTL.log log_warn("%s", buf); - // Log to database only if not in CLI mode - if(cli_mode) - return; - // Add to database add_message(REGEX_MESSAGE, regex, type, warning, dbindex); } From cdeb5e34272bcd6edaa78e1a8e85ebeb89a73aee Mon Sep 17 00:00:00 2001 From: DL6ER Date: Tue, 2 Jul 2024 03:48:12 +0200 Subject: [PATCH 11/30] Add which capability is missing in warnings (if applicable). Also reduce chown() code duplication by using a single function for all chown()-activities Signed-off-by: DL6ER --- src/config/toml_helper.c | 24 ++++-------------------- src/daemon.c | 33 ++++++++++++++++++++++++++------- src/dnsmasq_interface.c | 30 +++++++----------------------- src/files.c | 38 +++++++++++++++++++++----------------- src/files.h | 3 +++ src/ntp/client.c | 2 +- src/ntp/rtc.c | 12 ++++++++---- src/shmem.c | 11 +++++++---- test/test_suite.bats | 2 +- 9 files changed, 78 insertions(+), 77 deletions(-) diff --git a/src/config/toml_helper.c b/src/config/toml_helper.c index 26c6b1dd7..22b85dd06 100644 --- a/src/config/toml_helper.c +++ b/src/config/toml_helper.c @@ -23,6 +23,8 @@ #include // escape_json() #include "webserver/http-common.h" +// chown_pihole() +#include "files.h" // Open the TOML file for reading or writing FILE * __attribute((malloc)) __attribute((nonnull(1))) openFTLtoml(const char *mode, const unsigned int version) @@ -96,26 +98,8 @@ void closeFTLtoml(FILE *fp) // Chown file if we are root if(geteuid() == 0) - { - // Get UID and GID of user with name "pihole" - struct passwd *pwd = getpwnam("pihole"); - if(pwd == NULL) - { - log_warn("Cannot get UID and GID of user pihole: %s", strerror(errno)); - } - else - { - const uid_t pihole_uid = pwd->pw_uid; - const gid_t pihole_gid = pwd->pw_gid; - // Chown file to pihole user - if(chown(GLOBALTOMLPATH, pihole_uid, pihole_gid) != 0) - log_warn("Cannot chown "GLOBALTOMLPATH" to pihole:pihole (%u:%u): %s", - (unsigned int)pihole_uid, (unsigned int)pihole_gid, strerror(errno)); - else - log_debug(DEBUG_CONFIG, "Chown-ed "GLOBALTOMLPATH" to pihole:pihole (%u:%u)", - (unsigned int)pihole_uid, (unsigned int)pihole_gid); - } - } + chown_pihole(GLOBALTOMLPATH, NULL); + return; } diff --git a/src/daemon.c b/src/daemon.c index 5e7cf2dba..a85b0ed0b 100644 --- a/src/daemon.c +++ b/src/daemon.c @@ -330,13 +330,32 @@ void set_nice(void) // Set nice value const int ret = setpriority(which, pid, config.misc.nice.v.i); if(ret == -1) - // ERROR EPERM: The calling process attempted to increase its priority - // by supplying a negative value but has insufficient privileges. - // On Linux, the RLIMIT_NICE resource limit can be used to define a limit to - // which an unprivileged process's nice value can be raised. We are not - // affected by this limit when pihole-FTL is running with CAP_SYS_NICE - log_warn("Cannot set process priority to %d: %s. Process priority remains at %d", - config.misc.nice.v.i, strerror(errno), priority); + { + if(errno == EACCES || errno == EPERM) + { + // from man 2 setpriority: + // + // ERRORS + // [...] + // EACCES The caller attempted to set a lower nice value (i.e., a higher + // process priority), but did not have the required privilege (on + // Linux: did not have the CAP_SYS_NICE capability). + // + // EPERM A process was located, but its effective user ID did not match + // either the effective or the real user ID of the caller, and was + // not privileged (on Linux: did not have the CAP_SYS_NICE capabil‐ + // ity). + // [...] + log_warn("Insufficient permissions to set process priority to %d (CAP_SYS_NICE required), process priority remains at %d", + config.misc.nice.v.i, priority); + } + else + { + // Other error + log_warn("Cannot set process priority to %d: %s. Process priority remains at %d", + config.misc.nice.v.i, strerror(errno), priority); + } + } } } diff --git a/src/dnsmasq_interface.c b/src/dnsmasq_interface.c index 42caa431e..cce51b45a 100644 --- a/src/dnsmasq_interface.c +++ b/src/dnsmasq_interface.c @@ -2963,26 +2963,16 @@ void FTL_fork_and_bind_sockets(struct passwd *ent_pw, bool dnsmasq_start) // we're actually dropping root (user/group may be set to root) if(ent_pw != NULL && ent_pw->pw_uid != 0) { - log_info("FTL is going to drop from root to user %s (UID %u)", - ent_pw->pw_name, ent_pw->pw_uid); + log_info("FTL is going to drop from root to user pihole"); // Change ownership of shared memory objects chown_all_shmem(ent_pw); // Configured FTL log file - if(chown(config.files.log.ftl.v.s, ent_pw->pw_uid, ent_pw->pw_gid) == -1) - { - log_warn("Setting ownership (%u:%u) of %s failed: %s (%i)", - ent_pw->pw_uid, ent_pw->pw_gid, config.files.log.ftl.v.s, strerror(errno), errno); - } + chown_pihole(config.files.log.ftl.v.s, ent_pw); // Configured FTL database file - if(chown(config.files.database.v.s, ent_pw->pw_uid, ent_pw->pw_gid) == -1) - { - log_warn("Setting ownership (%u:%u) of %s failed: %s (%i)", - ent_pw->pw_uid, ent_pw->pw_gid, config.files.database.v.s, strerror(errno), errno); - - } + chown_pihole(config.files.database.v.s, ent_pw); // Check if auxiliary files exist and change ownership char *extrafile = calloc(strlen(config.files.database.v.s) + 5, sizeof(char)); @@ -2995,20 +2985,14 @@ void FTL_fork_and_bind_sockets(struct passwd *ent_pw, bool dnsmasq_start) // Check -wal file (write-ahead log) strcpy(extrafile, config.files.database.v.s); strcat(extrafile, "-wal"); - if(file_exists(extrafile) && chown(extrafile, ent_pw->pw_uid, ent_pw->pw_gid) == -1) - { - log_warn("Setting ownership (%u:%u) of %s failed: %s (%i)", - ent_pw->pw_uid, ent_pw->pw_gid, extrafile, strerror(errno), errno); - } + if(file_exists(extrafile)) + chown_pihole(extrafile, ent_pw); // Check -shm file (mmapped shared memory) strcpy(extrafile, config.files.database.v.s); strcat(extrafile, "-shm"); - if(file_exists(extrafile) && chown(extrafile, ent_pw->pw_uid, ent_pw->pw_gid) == -1) - { - log_warn("Setting ownership (%u:%u) of %s failed: %s (%i)", - ent_pw->pw_uid, ent_pw->pw_gid, extrafile, strerror(errno), errno); - } + if(file_exists(extrafile)) + chown_pihole(extrafile, ent_pw); // Free allocated memory free(extrafile); diff --git a/src/files.c b/src/files.c index cb5799fdf..8158f60a4 100644 --- a/src/files.c +++ b/src/files.c @@ -16,8 +16,6 @@ // opendir(), readdir() #include -// getpwuid() -#include // getgrgid() #include // NAME_MAX @@ -434,30 +432,36 @@ static int copy_file(const char *source, const char *destination) } // Change ownership of file to pihole user -static bool chown_pihole(const char *path) +bool chown_pihole(const char *path, struct passwd *pwd) { - // Get pihole user's uid and gid - struct passwd *pwd = getpwnam("pihole"); + // Get pihole user's UID and GID if not provided if(pwd == NULL) { - log_warn("chown_pihole(): Failed to get pihole user's uid: %s", strerror(errno)); - return false; - } - struct group *grp = getgrnam("pihole"); - if(grp == NULL) - { - log_warn("chown_pihole(): Failed to get pihole user's gid: %s", strerror(errno)); - return false; + pwd = getpwnam("pihole"); + if(pwd == NULL) + { + log_warn("chown_pihole(): Failed to get pihole user's UID/GID: %s", strerror(errno)); + return false; + } } + // Get group name + struct group *grp = getgrgid(pwd->pw_gid); + const char *grp_name = grp != NULL ? grp->gr_name : ""; + // Change ownership of file to pihole user - if(chown(path, pwd->pw_uid, grp->gr_gid) < 0) + if(chown(path, pwd->pw_uid, pwd->pw_gid) < 0) { - log_warn("chown_pihole(): Failed to change ownership of \"%s\" to %u:%u: %s", - path, pwd->pw_uid, grp->gr_gid, strerror(errno)); + log_warn("Failed to change ownership of \"%s\" to %s:%s (%u:%u): %s", + path, pwd->pw_name, grp_name, pwd->pw_uid, pwd->pw_gid, + errno == EPERM ? "Insufficient permissions (CAP_CHOWN required)" : strerror(errno)); + return false; } + log_debug(DEBUG_INOTIFY, "Changed ownership of \"%s\" to %s:%s (%u:%u)", + path, pwd->pw_name, grp_name, pwd->pw_uid, pwd->pw_gid); + return true; } @@ -533,7 +537,7 @@ void rotate_files(const char *path, char **first_file) } // Change ownership of file to pihole user - chown_pihole(new_path); + chown_pihole(new_path, NULL); } // Free memory diff --git a/src/files.h b/src/files.h index 742e8d9a5..fc10ad230 100644 --- a/src/files.h +++ b/src/files.h @@ -16,6 +16,8 @@ #include // SHA256_DIGEST_SIZE #include +// getpwuid() +#include #define MAX_ROTATIONS 15 #define BACKUP_DIR "/etc/pihole/config_backups" @@ -31,6 +33,7 @@ void ls_dir(const char* path); unsigned int get_path_usage(const char *path, char buffer[64]); struct mntent *get_filesystem_details(const char *path); bool directory_exists(const char *path); +bool chown_pihole(const char *path, struct passwd *pwd); void rotate_files(const char *path, char **first_file); bool files_different(const char *pathA, const char* pathB, unsigned int from); bool sha256sum(const char *path, uint8_t checksum[SHA256_DIGEST_SIZE]); diff --git a/src/ntp/client.c b/src/ntp/client.c index 43cce286d..2359d1522 100644 --- a/src/ntp/client.c +++ b/src/ntp/client.c @@ -657,7 +657,7 @@ bool ntp_start_sync_thread(pthread_attr_t *attr) // in starting the thread. if(!check_capability(CAP_SYS_TIME)) { - log_warn("Insufficient permissions to set system time, NTP client not available"); + log_warn("Insufficient permissions to set system time (CAP_SYS_TIME required), NTP client not available"); // Send SIGUSR7 to embedded dnsmasq instance to signal time is // assumed to be correct kill(main_pid(), SIGUSR7); diff --git a/src/ntp/rtc.c b/src/ntp/rtc.c index 3fe8f93b4..072327117 100644 --- a/src/ntp/rtc.c +++ b/src/ntp/rtc.c @@ -82,7 +82,8 @@ static int open_rtc(void) if(chown(config.ntp.sync.rtc.device.v.s, uid, gid) == -1) { log_debug(DEBUG_NTP, "chown(\"%s\", %u, %u) failed: %s", - config.ntp.sync.rtc.device.v.s, uid, gid, strerror(errno)); + config.ntp.sync.rtc.device.v.s, uid, gid, + errno == EPERM ? "Insufficient permissions (CAP_CHOWN required)" : strerror(errno)); return -1; } @@ -97,7 +98,8 @@ static int open_rtc(void) if(chown(config.ntp.sync.rtc.device.v.s, st.st_uid, st.st_gid) == -1) { log_debug(DEBUG_NTP, "chown(\"%s\", %u, %u) failed: %s", - config.ntp.sync.rtc.device.v.s, st.st_uid, st.st_gid, strerror(errno)); + config.ntp.sync.rtc.device.v.s, st.st_uid, st.st_gid, + errno == EPERM ? "Insufficient permissions (CAP_CHOWN required)" : strerror(errno)); return -1; } @@ -139,7 +141,8 @@ static int open_rtc(void) if(chown(rtc_devices[i], uid, gid) == -1) { log_debug(DEBUG_NTP, "chown(\"%s\", %u, %u) failed: %s", - rtc_devices[i], uid, gid, strerror(errno)); + rtc_devices[i], uid, gid, + errno == EPERM ? "Insufficient permissions (CAP_CHOWN required)" : strerror(errno)); return -1; } @@ -154,7 +157,8 @@ static int open_rtc(void) if(chown(rtc_devices[i], st.st_uid, st.st_gid) == -1) { log_debug(DEBUG_NTP, "chown(\"%s\", %u, %u) failed: %s", - rtc_devices[i], st.st_uid, st.st_gid, strerror(errno)); + rtc_devices[i], st.st_uid, st.st_gid, + errno == EPERM ? "Insufficient permissions (CAP_CHOWN required)" : strerror(errno)); return -1; } diff --git a/src/shmem.c b/src/shmem.c index 44f72eb8d..83e7da5c6 100644 --- a/src/shmem.c +++ b/src/shmem.c @@ -190,17 +190,20 @@ static bool chown_shmem(SharedMemory *sharedMemory, struct passwd *ent_pw) // Open shared memory object const int fd = shm_open(sharedMemory->name, O_RDWR, S_IRUSR | S_IWUSR); log_debug(DEBUG_SHMEM, "Changing %s (%d) to %u:%u", sharedMemory->name, fd, ent_pw->pw_uid, ent_pw->pw_gid); + if(fd == -1) { - log_crit("chown_shmem(): Failed to open shared memory object \"%s\": %s", + log_crit("Failed to open shared memory object \"%s\" for chown: %s", sharedMemory->name, strerror(errno)); exit(EXIT_FAILURE); } + if(fchown(fd, ent_pw->pw_uid, ent_pw->pw_gid) == -1) { - log_warn("chown_shmem(%d, %u, %u): failed for %s: %s (%d)", - fd, ent_pw->pw_uid, ent_pw->pw_gid, sharedMemory->name, - strerror(errno), errno); + log_crit("Failed to change ownership of shared memory object \"%s\": %s", + sharedMemory->name, + errno == EPERM ? "Insufficient permissions (CAP_CHOWN required)" : strerror(errno)); + return false; } diff --git a/test/test_suite.bats b/test/test_suite.bats index c62c9c1d3..2e0db7e50 100644 --- a/test/test_suite.bats +++ b/test/test_suite.bats @@ -481,7 +481,7 @@ } @test "No WARNING messages in FTL.log (besides known warnings)" { - run bash -c 'grep "WARNING:" /var/log/pihole/FTL.log | grep -v -E "CAP_NET_ADMIN|CAP_NET_RAW|CAP_SYS_NICE|CAP_IPC_LOCK|CAP_CHOWN|CAP_NET_BIND_SERVICE|CAP_SYS_TIME|(Cannot set process priority)|FTLCONF_|(Insufficient permissions to set system time, NTP client not available)"' + run bash -c 'grep "WARNING:" /var/log/pihole/FTL.log | grep -v -E "CAP_NET_ADMIN|CAP_NET_RAW|CAP_SYS_NICE|CAP_IPC_LOCK|CAP_CHOWN|CAP_NET_BIND_SERVICE|CAP_SYS_TIME|FTLCONF_"' printf "%s\n" "${lines[@]}" [[ "${lines[@]}" == "" ]] } From 1e251fbae859779203ba01c6cb8f3f3f4cad8145 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Fri, 5 Jul 2024 16:18:16 +0200 Subject: [PATCH 12/30] Remove hard-coded signal name from reloading string Signed-off-by: DL6ER --- src/dnsmasq_interface.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dnsmasq_interface.c b/src/dnsmasq_interface.c index cce51b45a..d74ce18d3 100644 --- a/src/dnsmasq_interface.c +++ b/src/dnsmasq_interface.c @@ -1803,7 +1803,7 @@ void FTL_dnsmasq_reload(void) // This function is called by the dnsmasq code on receive of SIGHUP // *before* clearing the cache and re-reading the lists if(reload++ > 0) - log_info("Received SIGHUP, flushing cache and re-reading config"); + log_info("Flushing cache and re-reading config"); // Gravity database updates // - (Re-)open gravity database connection From bfd242d6a8d5cda2b2b4226b88d83bbd86cf3e74 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Fri, 5 Jul 2024 16:45:45 +0200 Subject: [PATCH 13/30] Importmetatables already before delayed importing of queries during startup Signed-off-by: DL6ER --- src/database/query-table.c | 46 ++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 12 deletions(-) diff --git a/src/database/query-table.c b/src/database/query-table.c index 435f560a8..799811e56 100644 --- a/src/database/query-table.c +++ b/src/database/query-table.c @@ -41,6 +41,9 @@ static sqlite3_stmt **stmts[] = { &query_stmt, &forward_stmt, &addinfo_stmt }; +// Private prototypes +static bool import_linked_tables_from_disk(void); + // Return the maximum ID of the in-memory database unsigned long __attribute__((pure)) get_max_db_idx(void) { @@ -220,6 +223,9 @@ bool init_memory_database(void) return false; } + // Import linked-tables from disk database (domains, clients, ...) + import_linked_tables_from_disk(); + // Everything went well return true; } @@ -535,6 +541,24 @@ bool import_queries_from_disk(void) // Finalize statement sqlite3_finalize(stmt); + // End transaction + if((rc = sqlite3_exec(memdb, "END TRANSACTION", NULL, NULL, NULL)) != SQLITE_OK) + { + log_err("import_queries_from_disk(): Cannot end transaction: %s", sqlite3_errstr(rc)); + return false; + } + + // Get number of queries on disk before detaching + disk_db_num = get_number_of_queries_in_DB(memdb, "disk.query_storage"); + mem_db_num = get_number_of_queries_in_DB(memdb, "query_storage"); + + log_info("Imported %u queries from the on-disk database (it has %u rows)", mem_db_num, disk_db_num); + + return okay; +} + +static bool import_linked_tables_from_disk(void) +{ // Import linking tables and current AUTOINCREMENT values from the disk database const char *subtable_names[] = { "domain_by_id", @@ -551,6 +575,15 @@ bool import_queries_from_disk(void) "INSERT OR REPLACE INTO sqlite_sequence SELECT * FROM disk.sqlite_sequence" }; + // Begin transaction + int rc; + sqlite3 *memdb = get_memdb(); + if((rc = sqlite3_exec(memdb, "BEGIN TRANSACTION", NULL, NULL, NULL)) != SQLITE_OK) + { + log_err("import_queries_from_disk(): Cannot start transaction: %s", sqlite3_errstr(rc)); + return false; + } + // Import linking tables for(unsigned int i = 0; i < ArraySize(subtable_sql); i++) { @@ -567,13 +600,7 @@ bool import_queries_from_disk(void) return false; } - // Get number of queries on disk before detaching - disk_db_num = get_number_of_queries_in_DB(memdb, "disk.query_storage"); - mem_db_num = get_number_of_queries_in_DB(memdb, "query_storage"); - - log_info("Imported %u queries from the on-disk database (it has %u rows)", mem_db_num, disk_db_num); - - return okay; + return true; } // Export in-memory queries to disk - either due to periodic dumping (final = @@ -1370,11 +1397,6 @@ bool queries_to_database(void) log_debug(DEBUG_DATABASE, "Not storing query in database as there are none"); return true; } - if(!store_in_database) - { - log_debug(DEBUG_DATABASE, "Not storing query in database as this is disabled"); - return true; - } // Loop over recent queries and store new or changed ones in the // in-memory database From 93ca236ad9f04db3ab18d711672e20f7b4f1bd15 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Sat, 6 Jul 2024 21:18:36 +0200 Subject: [PATCH 14/30] Revert "Importmetatables already before delayed importing of queries during startup" This reverts commit bfd242d6a8d5cda2b2b4226b88d83bbd86cf3e74. --- src/database/query-table.c | 46 ++++++++++---------------------------- 1 file changed, 12 insertions(+), 34 deletions(-) diff --git a/src/database/query-table.c b/src/database/query-table.c index 799811e56..435f560a8 100644 --- a/src/database/query-table.c +++ b/src/database/query-table.c @@ -41,9 +41,6 @@ static sqlite3_stmt **stmts[] = { &query_stmt, &forward_stmt, &addinfo_stmt }; -// Private prototypes -static bool import_linked_tables_from_disk(void); - // Return the maximum ID of the in-memory database unsigned long __attribute__((pure)) get_max_db_idx(void) { @@ -223,9 +220,6 @@ bool init_memory_database(void) return false; } - // Import linked-tables from disk database (domains, clients, ...) - import_linked_tables_from_disk(); - // Everything went well return true; } @@ -541,24 +535,6 @@ bool import_queries_from_disk(void) // Finalize statement sqlite3_finalize(stmt); - // End transaction - if((rc = sqlite3_exec(memdb, "END TRANSACTION", NULL, NULL, NULL)) != SQLITE_OK) - { - log_err("import_queries_from_disk(): Cannot end transaction: %s", sqlite3_errstr(rc)); - return false; - } - - // Get number of queries on disk before detaching - disk_db_num = get_number_of_queries_in_DB(memdb, "disk.query_storage"); - mem_db_num = get_number_of_queries_in_DB(memdb, "query_storage"); - - log_info("Imported %u queries from the on-disk database (it has %u rows)", mem_db_num, disk_db_num); - - return okay; -} - -static bool import_linked_tables_from_disk(void) -{ // Import linking tables and current AUTOINCREMENT values from the disk database const char *subtable_names[] = { "domain_by_id", @@ -575,15 +551,6 @@ static bool import_linked_tables_from_disk(void) "INSERT OR REPLACE INTO sqlite_sequence SELECT * FROM disk.sqlite_sequence" }; - // Begin transaction - int rc; - sqlite3 *memdb = get_memdb(); - if((rc = sqlite3_exec(memdb, "BEGIN TRANSACTION", NULL, NULL, NULL)) != SQLITE_OK) - { - log_err("import_queries_from_disk(): Cannot start transaction: %s", sqlite3_errstr(rc)); - return false; - } - // Import linking tables for(unsigned int i = 0; i < ArraySize(subtable_sql); i++) { @@ -600,7 +567,13 @@ static bool import_linked_tables_from_disk(void) return false; } - return true; + // Get number of queries on disk before detaching + disk_db_num = get_number_of_queries_in_DB(memdb, "disk.query_storage"); + mem_db_num = get_number_of_queries_in_DB(memdb, "query_storage"); + + log_info("Imported %u queries from the on-disk database (it has %u rows)", mem_db_num, disk_db_num); + + return okay; } // Export in-memory queries to disk - either due to periodic dumping (final = @@ -1397,6 +1370,11 @@ bool queries_to_database(void) log_debug(DEBUG_DATABASE, "Not storing query in database as there are none"); return true; } + if(!store_in_database) + { + log_debug(DEBUG_DATABASE, "Not storing query in database as this is disabled"); + return true; + } // Loop over recent queries and store new or changed ones in the // in-memory database From f2f8c24fde0898f3a9ab26c5570b7aa9349842ad Mon Sep 17 00:00:00 2001 From: DL6ER Date: Sat, 6 Jul 2024 21:21:45 +0200 Subject: [PATCH 15/30] Revert "DNSSEC signatures are only valid for specified time windows, and should be rejected outside those windows. This generates an interesting chicken-and-egg problem for machines which don't have a hardware real time clock. For these machines to determine the correct time typically requires use of NTP and therefore DNS, but validating DNS requires that the correct time is already known. Resolve this by setting dnssec-no-timecheck removing the time-window checks (but not other DNSSEC validation.) only until NTP sync finishes (or if we realize the user doesn't want it)" This reverts commit a0372769c17acdfb78b0a8834f1234dda32f6264. Signed-off-by: DL6ER --- src/config/dnsmasq_config.c | 6 +----- src/dnsmasq/dnsmasq.c | 3 --- src/ntp/client.c | 11 +---------- src/signals.c | 10 +++------- src/signals.h | 1 - 5 files changed, 5 insertions(+), 26 deletions(-) diff --git a/src/config/dnsmasq_config.c b/src/config/dnsmasq_config.c index 8fbd5fdf8..6b8cbc822 100644 --- a/src/config/dnsmasq_config.c +++ b/src/config/dnsmasq_config.c @@ -400,11 +400,7 @@ bool __attribute__((const)) write_dnsmasq_config(struct config *conf, bool test_ fputs("# 2017-02-02 root zone trust anchor\n", pihole_conf); fputs("trust-anchor=.,20326,8,2,E06D44B80B8F1D39A95C0B0D7C65D08458E880409BBC683457104237C7F8EC8D\n", pihole_conf); - - // Prevent DNSSEC timestamp checks until either NTP synchronization has succeeded or - // the user has disabled the NTP client - fputs("# Do not check DNSSEC timestamps until NTP synchronization has succeeded\n", pihole_conf); - fputs("dnssec-no-timecheck\n\n", pihole_conf); + fputs("\n", pihole_conf); } if(strlen(conf->dns.hostRecord.v.s) > 0) diff --git a/src/dnsmasq/dnsmasq.c b/src/dnsmasq/dnsmasq.c index 1a8170462..7990ed616 100644 --- a/src/dnsmasq/dnsmasq.c +++ b/src/dnsmasq/dnsmasq.c @@ -97,7 +97,6 @@ int main_dnsmasq (int argc, char **argv) sigaction(SIGUSR2, &sigact, NULL); sigaction(SIGHUP, &sigact, NULL); sigaction(SIGUSR6, &sigact, NULL); // Pi-hole modification - sigaction(SIGUSR7, &sigact, NULL); // Pi-hole modification sigaction(SIGALRM, &sigact, NULL); sigaction(SIGCHLD, &sigact, NULL); sigaction(SIGINT, &sigact, NULL); @@ -1359,8 +1358,6 @@ static void sig_handler(int sig) else event = EVENT_TIME; } - else if (sig == SIGUSR7) // Pi-hole modified - event = EVENT_TIME; else return; diff --git a/src/ntp/client.c b/src/ntp/client.c index 2359d1522..aeb729ece 100644 --- a/src/ntp/client.c +++ b/src/ntp/client.c @@ -605,12 +605,6 @@ static void *ntp_client_thread(void *arg) { load_queries_from_disk(); - // If this was the first run and NTP time synchronization was - // successful, we send SIGUSR7 to the embedded dnsmasq instance - // to signal time is now guaranteed to be correct - if(success) - kill(main_pid(), SIGUSR7); - first_run = false; } @@ -642,10 +636,7 @@ bool ntp_start_sync_thread(pthread_attr_t *attr) strlen(config.ntp.sync.server.v.s) == 0 || config.ntp.sync.interval.v.ui == 0) { - log_info("NTP sync is disabled"); - // Send SIGUSR7 to embedded dnsmasq instance to signal time is - // assumed to be correct - kill(main_pid(), SIGUSR7); + log_info("NTP sync is disabled - NTP server will not be available"); load_queries_from_disk(); ntp_server_start(); return false; diff --git a/src/signals.c b/src/signals.c index 63270cc23..34f6b52c6 100644 --- a/src/signals.c +++ b/src/signals.c @@ -322,11 +322,6 @@ static void SIGRT_handler(int signum, siginfo_t *si, void *unused) // { // // Signal internally used to signal dnsmasq it has to stop // } - // else if(rtsig == 7) - // { - // // Signal internally used to signal dnsmasq it should do - // // DNSSEC timestamp checks - // } // Restore errno before returning back to previous context errno = _errno; @@ -453,8 +448,9 @@ void handle_realtime_signals(void) // Catch all real-time signals for(int signum = SIGRTMIN; signum <= SIGRTMAX; signum++) { - if(signum == SIGUSR6 || signum == SIGUSR7) - // Skip SIGUSR6 as it is used internally by dnsmasq + if(signum == SIGUSR6) + // Skip SIGUSR6 as it is used internally to signify + // dnsmasq to stop continue; struct sigaction SIGACTION = { 0 }; diff --git a/src/signals.h b/src/signals.h index cc5d8f1bc..4e388dd90 100644 --- a/src/signals.h +++ b/src/signals.h @@ -13,7 +13,6 @@ #include "enums.h" #define SIGUSR6 (SIGRTMIN + 6) -#define SIGUSR7 (SIGRTMIN + 7) // defined in dnsmasq/dnsmasq.h extern volatile char FTL_terminate; From 7f070e61982eef5963a16736c9423b41983c12f0 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Sat, 6 Jul 2024 21:25:16 +0200 Subject: [PATCH 16/30] Load queries during initialization of FTL Signed-off-by: DL6ER --- src/database/query-table.c | 7 ++++++- src/database/query-table.h | 1 - src/ntp/client.c | 23 ++--------------------- 3 files changed, 8 insertions(+), 23 deletions(-) diff --git a/src/database/query-table.c b/src/database/query-table.c index 435f560a8..a2a98a00c 100644 --- a/src/database/query-table.c +++ b/src/database/query-table.c @@ -41,6 +41,9 @@ static sqlite3_stmt **stmts[] = { &query_stmt, &forward_stmt, &addinfo_stmt }; +// Private prototypes +static void load_queries_from_disk(void); + // Return the maximum ID of the in-memory database unsigned long __attribute__((pure)) get_max_db_idx(void) { @@ -220,6 +223,8 @@ bool init_memory_database(void) return false; } + load_queries_from_disk(); + // Everything went well return true; } @@ -1635,7 +1640,7 @@ bool queries_to_database(void) return true; } -void load_queries_from_disk(void) +static void load_queries_from_disk(void) { // Compensate for possible jumps in time runGC(time(NULL), NULL, false); diff --git a/src/database/query-table.h b/src/database/query-table.h index df0a8dd08..1a5b8b5aa 100644 --- a/src/database/query-table.h +++ b/src/database/query-table.h @@ -119,7 +119,6 @@ bool add_additional_info_column(sqlite3 *db); void DB_read_queries(void); void update_disk_db_idx(void); bool queries_to_database(void); -void load_queries_from_disk(void); bool optimize_queries_table(sqlite3 *db); bool create_addinfo_table(sqlite3 *db); diff --git a/src/ntp/client.c b/src/ntp/client.c index aeb729ece..ac4c1eff6 100644 --- a/src/ntp/client.c +++ b/src/ntp/client.c @@ -39,10 +39,9 @@ #include // log_ntp_message() #include "database/message-table.h" -// load_queries_from_disk() -#include "database/query-table.h" // check_capability() #include "capabilities.h" + struct ntp_sync { bool valid; @@ -593,21 +592,12 @@ static void *ntp_client_thread(void *arg) prctl(PR_SET_NAME, thread_names[NTP_CLIENT], 0, 0, 0); // Run NTP client - bool first_run = true; bool ntp_server_started = false; while(!killed) { // Run NTP client const bool success = ntp_client(config.ntp.sync.server.v.s, true, false); - // Load queries from database after first NTP synchronization - if(first_run) - { - load_queries_from_disk(); - - first_run = false; - } - if(success && !ntp_server_started) { // Initialize NTP server only after first NTP @@ -636,8 +626,7 @@ bool ntp_start_sync_thread(pthread_attr_t *attr) strlen(config.ntp.sync.server.v.s) == 0 || config.ntp.sync.interval.v.ui == 0) { - log_info("NTP sync is disabled - NTP server will not be available"); - load_queries_from_disk(); + log_info("NTP sync is disabled"); ntp_server_start(); return false; } @@ -649,10 +638,6 @@ bool ntp_start_sync_thread(pthread_attr_t *attr) if(!check_capability(CAP_SYS_TIME)) { log_warn("Insufficient permissions to set system time (CAP_SYS_TIME required), NTP client not available"); - // Send SIGUSR7 to embedded dnsmasq instance to signal time is - // assumed to be correct - kill(main_pid(), SIGUSR7); - load_queries_from_disk(); ntp_server_start(); return false; } @@ -661,10 +646,6 @@ bool ntp_start_sync_thread(pthread_attr_t *attr) if(pthread_create(&threads[NTP_CLIENT], attr, ntp_client_thread, NULL) != 0) { log_err("Cannot create NTP client thread"); - // Send SIGUSR7 to embedded dnsmasq instance to signal time is - // assumed to be correct - kill(main_pid(), SIGUSR7); - load_queries_from_disk(); ntp_server_start(); return false; } From e07858fe92670aa76ea7f0973da2dc0f7850ab1c Mon Sep 17 00:00:00 2001 From: DL6ER Date: Sat, 6 Jul 2024 21:27:36 +0200 Subject: [PATCH 17/30] Restart FTL if system time has been updated by more than one hour using the internal NTP synchronization method. This ensures FTL can import the real most recent 24 hours data of history after a restart on a system lacking a real hardware clock Signed-off-by: DL6ER --- src/ntp/client.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/ntp/client.c b/src/ntp/client.c index ac4c1eff6..7b4cfb51d 100644 --- a/src/ntp/client.c +++ b/src/ntp/client.c @@ -593,11 +593,34 @@ static void *ntp_client_thread(void *arg) // Run NTP client bool ntp_server_started = false; + bool first_run = true; while(!killed) { + // Get time before NTP sync + const time_t before = time(NULL); + // Run NTP client const bool success = ntp_client(config.ntp.sync.server.v.s, true, false); + // Get time after NTP sync + const time_t after = time(NULL); + + // If the time was updated by more than one hour, restart FTL to + // import recent data. This is relevant when the system time was + // set to an incorrect value (e.g., due to a dead CMOS battery + // or overall missing RTC) and the time was off. + if(first_run && after - before > 3600) + { + log_info("System time was updated by more than one hour, restarting FTL to import recent data"); + // Set the restart flag to true + exit_code = RESTART_FTL_CODE; + // Send SIGTERM to FTL + kill(main_pid(), SIGTERM); + } + + // Set first run to false + first_run = false; + if(success && !ntp_server_started) { // Initialize NTP server only after first NTP From 3aa3e84ac6a1fb950640de5530a584fbb6656b2b Mon Sep 17 00:00:00 2001 From: DL6ER Date: Mon, 8 Jul 2024 18:39:52 +0200 Subject: [PATCH 18/30] Use abs(time_delta) to ensure we also restart if coming from the future. Use GCinterval instead of hard-coding one hour as interval Signed-off-by: DL6ER --- src/ntp/client.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/ntp/client.c b/src/ntp/client.c index 7b4cfb51d..bd8090f4d 100644 --- a/src/ntp/client.c +++ b/src/ntp/client.c @@ -605,13 +605,16 @@ static void *ntp_client_thread(void *arg) // Get time after NTP sync const time_t after = time(NULL); - // If the time was updated by more than one hour, restart FTL to - // import recent data. This is relevant when the system time was - // set to an incorrect value (e.g., due to a dead CMOS battery - // or overall missing RTC) and the time was off. - if(first_run && after - before > 3600) + // If the time was updated by more than a certain amount, + // restart FTL to import recent data. This is relevant when the + // system time was set to an incorrect value (e.g., due to a + // dead CMOS battery or overall missing RTC) and the time was + // off. + double time_delta; + if(first_run && (time_delta = fabs((double)after - before)) > GCinterval) { - log_info("System time was updated by more than one hour, restarting FTL to import recent data"); + log_info("System time was updated by %.1f seconds, restarting FTL to import recent data", + time_delta); // Set the restart flag to true exit_code = RESTART_FTL_CODE; // Send SIGTERM to FTL From 3e7bfd36d413870b0f8ce46cbf97617a414325c5 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Mon, 8 Jul 2024 20:26:41 +0200 Subject: [PATCH 19/30] Usw double time calculation Signed-off-by: DL6ER --- src/ntp/client.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/ntp/client.c b/src/ntp/client.c index bd8090f4d..91b4ec762 100644 --- a/src/ntp/client.c +++ b/src/ntp/client.c @@ -597,21 +597,21 @@ static void *ntp_client_thread(void *arg) while(!killed) { // Get time before NTP sync - const time_t before = time(NULL); + const double before = double_time(); // Run NTP client const bool success = ntp_client(config.ntp.sync.server.v.s, true, false); // Get time after NTP sync - const time_t after = time(NULL); + const double after = double_time(); // If the time was updated by more than a certain amount, // restart FTL to import recent data. This is relevant when the // system time was set to an incorrect value (e.g., due to a // dead CMOS battery or overall missing RTC) and the time was // off. - double time_delta; - if(first_run && (time_delta = fabs((double)after - before)) > GCinterval) + double time_delta = fabs(after - before); + if(first_run && time_delta > GCinterval) { log_info("System time was updated by %.1f seconds, restarting FTL to import recent data", time_delta); From 00ff114cabf091fbb108d77d57699ec6e2c0f72c Mon Sep 17 00:00:00 2001 From: DL6ER Date: Tue, 9 Jul 2024 18:58:55 +0200 Subject: [PATCH 20/30] Do not start threads in detached mode. Joining them may SEGFAULT with libmusl Signed-off-by: DL6ER --- src/dnsmasq_interface.c | 4 +--- src/tools/arp-scan.c | 2 -- src/tools/dhcp-discover.c | 2 -- 3 files changed, 1 insertion(+), 7 deletions(-) diff --git a/src/dnsmasq_interface.c b/src/dnsmasq_interface.c index 72bb41cee..c3803f469 100644 --- a/src/dnsmasq_interface.c +++ b/src/dnsmasq_interface.c @@ -2912,12 +2912,10 @@ void FTL_fork_and_bind_sockets(struct passwd *ent_pw, bool dnsmasq_start) // so they will not listen to real-time signals handle_realtime_signals(); - // We will use the attributes object later to start all threads in - // detached mode - pthread_attr_t attr; // Initialize thread attributes object with default attribute values // Do NOT detach threads as we want to join them during shutdown with a // fixed timeout to give them time to clean up and finish their work + pthread_attr_t attr; pthread_attr_init(&attr); // Initialize NTP server diff --git a/src/tools/arp-scan.c b/src/tools/arp-scan.c index c5ac6ec36..923625a3d 100644 --- a/src/tools/arp-scan.c +++ b/src/tools/arp-scan.c @@ -616,8 +616,6 @@ int run_arp_scan(const bool scan_all, const bool extreme_mode) pthread_attr_t attr; // Initialize thread attributes object with default attribute values pthread_attr_init(&attr); - // Set thread attributes to detached mode - pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED); struct ifaddrs *addrs, *tmp; getifaddrs(&addrs); diff --git a/src/tools/dhcp-discover.c b/src/tools/dhcp-discover.c index 045c8c147..c68a74f90 100644 --- a/src/tools/dhcp-discover.c +++ b/src/tools/dhcp-discover.c @@ -725,8 +725,6 @@ int run_dhcp_discover(void) pthread_attr_t attr; // Initialize thread attributes object with default attribute values pthread_attr_init(&attr); - // Set thread attributes to detached mode - pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED); // Create processing/printfing lock pthread_mutexattr_t lock_attr; From c91ac40bd2795d1041b4f7da5897dd4312b7bff1 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Sun, 14 Jul 2024 15:54:19 +0200 Subject: [PATCH 21/30] fail*.dnssec.works changed its configuration from being BOGUS to ABANDONNED and cannot be used any longer for BOGUS testing Signed-off-by: DL6ER --- test/test_suite.bats | 6 ------ 1 file changed, 6 deletions(-) diff --git a/test/test_suite.bats b/test/test_suite.bats index d747e8dd6..ed73f5576 100644 --- a/test/test_suite.bats +++ b/test/test_suite.bats @@ -401,12 +401,6 @@ [[ ${lines[@]} == *"status: NOERROR"* ]] } -@test "DNSSEC: BOGUS domain is rejected" { - run bash -c "dig A fail01.dnssec.works @127.0.0.1" - printf "%s\n" "${lines[@]}" - [[ ${lines[@]} == *"status: SERVFAIL"* ]] -} - @test "Special domain: NXDOMAIN is returned" { run bash -c "dig A mask.icloud.com @127.0.0.1" printf "%s\n" "${lines[@]}" From d39703b9a19b30bc5d70c64bf9d4b79b4b38197e Mon Sep 17 00:00:00 2001 From: DL6ER Date: Tue, 16 Jul 2024 11:05:03 +0200 Subject: [PATCH 22/30] Restart timer if set even if no blocking mode change was requested. This allows users to extend a temporary state, e.g. by running another "pihole disable 5m" shortly before the previous cacall reaches timeout. Previously, calling "pihoe disable 1m" would make the change permanent which seems undesirable Signed-off-by: DL6ER --- src/api/dns.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/api/dns.c b/src/api/dns.c index 10e906053..489ed7df5 100644 --- a/src/api/dns.c +++ b/src/api/dns.c @@ -100,7 +100,7 @@ static int set_blocking(struct ftl_conn *api) // The blocking status does not need to be changed // Delete a possibly running timer - set_blockingmode_timer(-1.0, true); + set_blockingmode_timer(timer, true); log_debug(DEBUG_API, "No change in blocking mode, resetting timer"); } From af7ea105eb0987aec4ebbe58189f063d93ea2ae6 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Tue, 25 Jun 2024 09:02:29 +0200 Subject: [PATCH 23/30] Implement actual TOP suggestions for the Query Log Signed-off-by: DL6ER --- src/api/queries.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/src/api/queries.c b/src/api/queries.c index eb16529fb..1728048fd 100644 --- a/src/api/queries.c +++ b/src/api/queries.c @@ -47,7 +47,7 @@ static int add_strings_to_array(struct ftl_conn *api, cJSON *array, const char * // Loop through returned rows int counter = 0; while((rc = sqlite3_step(stmt)) == SQLITE_ROW && - (max_count < 0 || ++counter < max_count)) + (max_count < 0 || ++counter <= max_count)) JSON_COPY_STR_TO_ARRAY(array, (const char*)sqlite3_column_text(stmt, 0)); // Acceptable return codes are either @@ -77,19 +77,24 @@ int api_queries_suggestions(struct ftl_conn *api) // Get domains cJSON *domain = JSON_NEW_ARRAY(); - rc = add_strings_to_array(api, domain, "SELECT domain FROM domain_by_id", count); + log_debug(DEBUG_API, "Reading top domains from database"); + rc = add_strings_to_array(api, domain, "WITH CTE AS (SELECT COUNT(*) cnt, domain FROM query_storage GROUP BY domain ORDER BY cnt DESC)"\ + "SELECT d.domain FROM CTE JOIN domain_by_id d ON CTE.domain = d.id", count); if(rc != 0) { log_err("Cannot read domains from database"); cJSON_Delete(domain); return rc; } + log_debug(DEBUG_API, "Read %d domains from database", cJSON_GetArraySize(domain)); // Get clients, both by IP and names // We have to call DISTINCT() here as multiple IPs can map to and name and // vice versa cJSON *client_ip = JSON_NEW_ARRAY(); - rc = add_strings_to_array(api, client_ip, "SELECT DISTINCT(ip) FROM client_by_id", count); + log_debug(DEBUG_API, "Reading top client IPs from database"); + rc = add_strings_to_array(api, client_ip, "WITH CTE AS (SELECT COUNT(*) cnt, client FROM query_storage GROUP BY client ORDER BY cnt DESC)"\ + "SELECT c.ip FROM CTE JOIN client_by_id c ON CTE.client = c.id", count); if(rc != 0) { log_err("Cannot read client IPs from database"); @@ -97,8 +102,12 @@ int api_queries_suggestions(struct ftl_conn *api) cJSON_Delete(client_ip); return rc; } + log_debug(DEBUG_API, "Read %d client IPs from database", cJSON_GetArraySize(client_ip)); + cJSON *client_name = JSON_NEW_ARRAY(); - rc = add_strings_to_array(api, client_name, "SELECT DISTINCT(name) FROM client_by_id", count); + log_debug(DEBUG_API, "Reading top client names from database"); + rc = add_strings_to_array(api, client_name, "WITH CTE AS (SELECT COUNT(*) cnt, client FROM query_storage GROUP BY client ORDER BY cnt DESC)"\ + "SELECT c.name FROM CTE JOIN client_by_id c ON CTE.client = c.id WHERE c.name IS NOT NULL", count); if(rc != 0) { log_err("Cannot read client names from database"); @@ -107,10 +116,13 @@ int api_queries_suggestions(struct ftl_conn *api) cJSON_Delete(client_name); return rc; } + log_debug(DEBUG_API, "Read %d client names from database", cJSON_GetArraySize(client_name)); // Get upstreams cJSON *upstream = JSON_NEW_ARRAY(); - rc = add_strings_to_array(api, upstream, "SELECT forward FROM forward_by_id", count); + log_debug(DEBUG_API, "Reading top upstreams from database"); + rc = add_strings_to_array(api, upstream, "WITH CTE AS (SELECT COUNT(*) cnt, forward FROM query_storage GROUP BY forward ORDER BY cnt DESC)"\ + "SELECT f.forward FROM CTE JOIN forward_by_id f ON CTE.forward = f.id", count); if(rc != 0) { log_err("Cannot read forward from database"); @@ -120,6 +132,7 @@ int api_queries_suggestions(struct ftl_conn *api) cJSON_Delete(upstream); return rc; } + log_debug(DEBUG_API, "Read %d upstreams from database", cJSON_GetArraySize(upstream)); // Get types cJSON *type = JSON_NEW_ARRAY(); From d01809a1a76a23ee2ff93993d9dc1f52e048db1c Mon Sep 17 00:00:00 2001 From: DL6ER Date: Tue, 25 Jun 2024 09:12:23 +0200 Subject: [PATCH 24/30] Sort only once for TOP clients Signed-off-by: DL6ER --- src/api/queries.c | 39 +++++++++++++++------------------------ 1 file changed, 15 insertions(+), 24 deletions(-) diff --git a/src/api/queries.c b/src/api/queries.c index 1728048fd..e7d71f687 100644 --- a/src/api/queries.c +++ b/src/api/queries.c @@ -22,7 +22,7 @@ // dbopen(false, ), dbclose() #include "database/common.h" -static int add_strings_to_array(struct ftl_conn *api, cJSON *array, const char *querystr, const int max_count) +static int add_strings_to_array(struct ftl_conn *api, cJSON *array1, cJSON *array2, const char *querystr, const int max_count) { sqlite3 *memdb = get_memdb(); @@ -48,7 +48,11 @@ static int add_strings_to_array(struct ftl_conn *api, cJSON *array, const char * int counter = 0; while((rc = sqlite3_step(stmt)) == SQLITE_ROW && (max_count < 0 || ++counter <= max_count)) - JSON_COPY_STR_TO_ARRAY(array, (const char*)sqlite3_column_text(stmt, 0)); + { + JSON_COPY_STR_TO_ARRAY(array1, (const char*)sqlite3_column_text(stmt, 0)); + if(array2 != NULL) + JSON_COPY_STR_TO_ARRAY(array2, (const char*)sqlite3_column_text(stmt, 1)); + } // Acceptable return codes are either // - SQLITE_DONE: We read all lines, or @@ -78,8 +82,8 @@ int api_queries_suggestions(struct ftl_conn *api) // Get domains cJSON *domain = JSON_NEW_ARRAY(); log_debug(DEBUG_API, "Reading top domains from database"); - rc = add_strings_to_array(api, domain, "WITH CTE AS (SELECT COUNT(*) cnt, domain FROM query_storage GROUP BY domain ORDER BY cnt DESC)"\ - "SELECT d.domain FROM CTE JOIN domain_by_id d ON CTE.domain = d.id", count); + rc = add_strings_to_array(api, domain, NULL, "WITH CTE AS (SELECT COUNT(*) cnt, domain FROM query_storage GROUP BY domain ORDER BY cnt DESC)"\ + "SELECT d.domain FROM CTE JOIN domain_by_id d ON CTE.domain = d.id", count); if(rc != 0) { log_err("Cannot read domains from database"); @@ -92,37 +96,24 @@ int api_queries_suggestions(struct ftl_conn *api) // We have to call DISTINCT() here as multiple IPs can map to and name and // vice versa cJSON *client_ip = JSON_NEW_ARRAY(); - log_debug(DEBUG_API, "Reading top client IPs from database"); - rc = add_strings_to_array(api, client_ip, "WITH CTE AS (SELECT COUNT(*) cnt, client FROM query_storage GROUP BY client ORDER BY cnt DESC)"\ - "SELECT c.ip FROM CTE JOIN client_by_id c ON CTE.client = c.id", count); - if(rc != 0) - { - log_err("Cannot read client IPs from database"); - cJSON_Delete(domain); - cJSON_Delete(client_ip); - return rc; - } - log_debug(DEBUG_API, "Read %d client IPs from database", cJSON_GetArraySize(client_ip)); - cJSON *client_name = JSON_NEW_ARRAY(); - log_debug(DEBUG_API, "Reading top client names from database"); - rc = add_strings_to_array(api, client_name, "WITH CTE AS (SELECT COUNT(*) cnt, client FROM query_storage GROUP BY client ORDER BY cnt DESC)"\ - "SELECT c.name FROM CTE JOIN client_by_id c ON CTE.client = c.id WHERE c.name IS NOT NULL", count); + log_debug(DEBUG_API, "Reading top client IPs and names from database"); + rc = add_strings_to_array(api, client_ip, client_name, "WITH CTE AS (SELECT COUNT(*) cnt, client FROM query_storage GROUP BY client ORDER BY cnt DESC)"\ + "SELECT c.ip,c.name FROM CTE JOIN client_by_id c ON CTE.client = c.id", count); if(rc != 0) { - log_err("Cannot read client names from database"); + log_err("Cannot read client IPs from database"); cJSON_Delete(domain); cJSON_Delete(client_ip); - cJSON_Delete(client_name); return rc; } - log_debug(DEBUG_API, "Read %d client names from database", cJSON_GetArraySize(client_name)); + log_debug(DEBUG_API, "Read %d client IPs and %d client names from database", cJSON_GetArraySize(client_ip), cJSON_GetArraySize(client_name)); // Get upstreams cJSON *upstream = JSON_NEW_ARRAY(); log_debug(DEBUG_API, "Reading top upstreams from database"); - rc = add_strings_to_array(api, upstream, "WITH CTE AS (SELECT COUNT(*) cnt, forward FROM query_storage GROUP BY forward ORDER BY cnt DESC)"\ - "SELECT f.forward FROM CTE JOIN forward_by_id f ON CTE.forward = f.id", count); + rc = add_strings_to_array(api, upstream, NULL, "WITH CTE AS (SELECT COUNT(*) cnt, forward FROM query_storage GROUP BY forward ORDER BY cnt DESC)"\ + "SELECT f.forward FROM CTE JOIN forward_by_id f ON CTE.forward = f.id", count); if(rc != 0) { log_err("Cannot read forward from database"); From 94dceae4198618f1ef023f51188174d604d2eaff Mon Sep 17 00:00:00 2001 From: DL6ER Date: Tue, 25 Jun 2024 09:14:42 +0200 Subject: [PATCH 25/30] Only add non-empty TOP replies Signed-off-by: DL6ER --- src/api/queries.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/api/queries.c b/src/api/queries.c index e7d71f687..7874c7d69 100644 --- a/src/api/queries.c +++ b/src/api/queries.c @@ -44,14 +44,23 @@ static int add_strings_to_array(struct ftl_conn *api, cJSON *array1, cJSON *arra sqlite3_errstr(rc)); } - // Loop through returned rows + // Loop through returned rows and add them to the array int counter = 0; while((rc = sqlite3_step(stmt)) == SQLITE_ROW && (max_count < 0 || ++counter <= max_count)) { - JSON_COPY_STR_TO_ARRAY(array1, (const char*)sqlite3_column_text(stmt, 0)); + const char *array1_str = (const char*)sqlite3_column_text(stmt, 0); + if(array1_str != NULL && array1_str[0] != '\0') + // Only add non-empty strings + JSON_COPY_STR_TO_ARRAY(array1, array1_str); if(array2 != NULL) - JSON_COPY_STR_TO_ARRAY(array2, (const char*)sqlite3_column_text(stmt, 1)); + { + // We have a second array to fill (second column in the query) + const char *array2_str = (const char*)sqlite3_column_text(stmt, 1); + if(array2_str != NULL && array2_str[0] != '\0') + // Only add non-empty strings + JSON_COPY_STR_TO_ARRAY(array2, array2_str); + } } // Acceptable return codes are either From 5d2d74a3e331a36d4547b018530f7f2dcdfc1ca3 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Tue, 25 Jun 2024 09:22:35 +0200 Subject: [PATCH 26/30] Fix LegacyKeyValueFormat warning during CI builds Signed-off-by: DL6ER --- .github/Dockerfile | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/Dockerfile b/.github/Dockerfile index 046e45f66..40d7a6141 100644 --- a/.github/Dockerfile +++ b/.github/Dockerfile @@ -5,13 +5,13 @@ WORKDIR /app COPY . /app ARG CI_ARCH="linux/amd64" -ENV CI_ARCH ${CI_ARCH} +ENV CI_ARCH=${CI_ARCH} ARG GIT_BRANCH="test" -ENV GIT_BRANCH ${GIT_BRANCH} +ENV GIT_BRANCH=${GIT_BRANCH} ARG GIT_TAG="test" -ENV GIT_TAG ${GIT_TAG} +ENV GIT_TAG=${GIT_TAG} ARG BUILD_OPTS="" -ENV BUILD_OPTS ${BUILD_OPTS} +ENV BUILD_OPTS=${BUILD_OPTS} # Build FTL # Remove possible old build files From bf1355c8ce43fcd7f94e522849fc05890279deda Mon Sep 17 00:00:00 2001 From: DL6ER Date: Tue, 16 Jul 2024 08:20:30 +0200 Subject: [PATCH 27/30] Increase speed of the /api/stats/* endpoints by reducing the use of shared memory locks. This is possible at the costs of a few bytes extra memory (at most a few KB) as we collect all data only once and don't need to get object poiters twice. Signed-off-by: DL6ER --- src/api/stats.c | 317 +++++++++++++++++++++++++++--------------------- 1 file changed, 176 insertions(+), 141 deletions(-) diff --git a/src/api/stats.c b/src/api/stats.c index b272e768d..09f71fc6f 100644 --- a/src/api/stats.c +++ b/src/api/stats.c @@ -14,8 +14,6 @@ #include "api/api.h" #include "shmem.h" #include "datastructure.h" -// read_setupVarsconf() -#include "config/setupVars.h" // logging routines #include "log.h" // config struct @@ -27,6 +25,17 @@ // sqrt() #include +struct top_entries { + int count; + unsigned int responses; + in_port_t port; + size_t namepos; + size_t ippos; + double rtime; + double rtuncertainty; + +}; + /* qsort comparison function (count field), sort ASC static int __attribute__((pure)) cmpasc(const void *a, const void *b) { @@ -55,6 +64,20 @@ int __attribute__((pure)) cmpdesc(const void *a, const void *b) return 0; } +// qsort subroutine, sort DESC +static int __attribute__((pure)) cmpdesc_te(const void *a, const void *b) +{ + const struct top_entries *elem1 = (struct top_entries*)a; + const struct top_entries *elem2 = (struct top_entries*)b; + + if (elem1->count > elem2->count) + return -1; + else if (elem1->count < elem2->count) + return 1; + else + return 0; +} + static int get_query_types_obj(struct ftl_conn *api, cJSON *types) { for(unsigned int i = TYPE_A; i < TYPE_MAX; i++) @@ -128,11 +151,14 @@ int api_stats_summary(struct ftl_conn *api) cJSON *gravity = JSON_NEW_OBJECT(); JSON_ADD_NUMBER_TO_OBJECT(gravity, "domains_being_blocked", counters->database.gravity); + // Unlock shared memory + unlock_shm(); + cJSON *json = JSON_NEW_OBJECT(); JSON_ADD_ITEM_TO_OBJECT(json, "queries", queries); JSON_ADD_ITEM_TO_OBJECT(json, "clients", clients); JSON_ADD_ITEM_TO_OBJECT(json, "gravity", gravity); - JSON_SEND_OBJECT_UNLOCK(json); + JSON_SEND_OBJECT(json); } int api_stats_top_domains(struct ftl_conn *api) @@ -151,18 +177,6 @@ int api_stats_top_domains(struct ftl_conn *api) JSON_SEND_OBJECT(json); } - // Lock shared memory - lock_shm(); - - // Allocate memory - const int domains = counters->domains; - int *temparray = calloc(2*domains, sizeof(int)); - if(temparray == NULL) - { - log_err("Memory allocation failed in %s()", __FUNCTION__); - return 0; - } - bool blocked = false; // Can be overwritten by query string int count = 10; // /api/stats/top_domains?blocked=true @@ -176,6 +190,26 @@ int api_stats_top_domains(struct ftl_conn *api) get_int_var(api->request->query_string, "count", &count); } + // Get domains which the user doesn't want to see + regex_t *regex_domains = NULL; + unsigned int N_regex_domains = 0; + compile_filter_regex(api, "webserver.api.excludeDomains", + config.webserver.api.excludeDomains.v.json, + ®ex_domains, &N_regex_domains); + + // Lock shared memory + lock_shm(); + + const int domains = counters->domains; + const int total_queries = counters->queries; + const int blocked_count = get_blocked_count(); + struct top_entries *top_domains = calloc(domains, sizeof(struct top_entries)); + if(top_domains == NULL) + { + log_err("Memory allocation failed in %s()", __FUNCTION__); + return 0; + } + unsigned int added_domains = 0u; for(int domainID = 0; domainID < domains; domainID++) { @@ -184,59 +218,41 @@ int api_stats_top_domains(struct ftl_conn *api) if(domain == NULL) continue; - // Add domain ID - temparray[2*added_domains + 0] = domainID; + const char *domain_name = getstr(domain->domainpos); + + // Hidden domain, probably due to privacy level. Skip this in the top lists + if(strcmp(domain_name, HIDDEN_DOMAIN) == 0) + continue; // Use either blocked or total count based on request string - temparray[2*added_domains + 1] = blocked ? domain->blockedcount : domain->count - domain->blockedcount; + top_domains[added_domains].count = blocked ? domain->blockedcount : domain->count - domain->blockedcount; + // Get domain name + top_domains[added_domains].namepos = domain->domainpos; + + // Increment counter added_domains++; } + // Unlock shared memory + unlock_shm(); + // Sort temporary array - qsort(temparray, added_domains, sizeof(int[2]), cmpdesc); + qsort(top_domains, added_domains, sizeof(*top_domains), cmpdesc_te); - // Get filter - const char* log_show = read_setupVarsconf("API_QUERY_LOG_SHOW"); - bool showpermitted = true, showblocked = true; - if(log_show != NULL) - { - if((strcmp(log_show, "permittedonly")) == 0) - showblocked = false; - else if((strcmp(log_show, "blockedonly")) == 0) - showpermitted = false; - else if((strcmp(log_show, "nothing")) == 0) - { - showpermitted = false; - showblocked = false; - } - } - clearSetupVarsArray(); + int n = 0; + cJSON *jtop_domains = JSON_NEW_ARRAY(); - // Get domains which the user doesn't want to see - regex_t *regex_domains = NULL; - unsigned int N_regex_domains = 0; - compile_filter_regex(api, "webserver.api.excludeDomains", - config.webserver.api.excludeDomains.v.json, - ®ex_domains, &N_regex_domains); + // Lock shared memory + lock_shm(); - int n = 0; - cJSON *top_domains = JSON_NEW_ARRAY(); for(unsigned int i = 0; i < added_domains; i++) { - // Get sorted index - const int domainID = temparray[2*i + 0]; - // Get domain pointer - const domainsData* domain = getDomain(domainID, true); - if(domain == NULL) + // Skip e.g. recycled domains + if(top_domains[i].namepos == 0) continue; - // Get domain name - const char *domain_name = getstr(domain->domainpos); - - // Hidden domain, probably due to privacy level. Skip this in the top lists - if(strcmp(domain_name, HIDDEN_DOMAIN) == 0) - continue; + const char *domain = getstr(top_domains[i].namepos); // Skip this client if there is a filter on it bool skip_domain = false; @@ -246,7 +262,7 @@ int api_stats_top_domains(struct ftl_conn *api) for(unsigned int j = 0; j < N_regex_domains; j++) { // Check if the domain matches the regex - if(regexec(®ex_domains[j], domain_name, 0, NULL, 0) == 0) + if(regexec(®ex_domains[j], domain, 0, NULL, 0) == 0) { // Domain matches skip_domain = true; @@ -258,30 +274,25 @@ int api_stats_top_domains(struct ftl_conn *api) if(skip_domain) continue; - int domain_count = -1; - if(blocked && showblocked && domain->blockedcount > 0) - { - domain_count = domain->blockedcount; - n++; - } - else if(!blocked && showpermitted && (domain->count - domain->blockedcount) > 0) - { - domain_count = domain->count - domain->blockedcount; - n++; - } - if(domain_count > -1) + if(top_domains[i].count > 0) { cJSON *domain_item = JSON_NEW_OBJECT(); - JSON_REF_STR_IN_OBJECT(domain_item, "domain", domain_name); - JSON_ADD_NUMBER_TO_OBJECT(domain_item, "count", domain_count); - JSON_ADD_ITEM_TO_ARRAY(top_domains, domain_item); + JSON_COPY_STR_TO_OBJECT(domain_item, "domain", domain); + JSON_ADD_NUMBER_TO_OBJECT(domain_item, "count", top_domains[i].count); + JSON_ADD_ITEM_TO_ARRAY(jtop_domains, domain_item); + n++; } // Only count entries that are actually sent and return when we have send enough data if(n >= count) break; } - free(temparray); + + // Unlock shared memory + unlock_shm(); + + // Free temporary array + free(top_domains); // Free regexes if(N_regex_domains > 0) @@ -295,13 +306,12 @@ int api_stats_top_domains(struct ftl_conn *api) } cJSON *json = JSON_NEW_OBJECT(); - JSON_ADD_ITEM_TO_OBJECT(json, "domains", top_domains); + JSON_ADD_ITEM_TO_OBJECT(json, "domains", jtop_domains); - const int blocked_count = get_blocked_count(); - JSON_ADD_NUMBER_TO_OBJECT(json, "total_queries", counters->queries); + JSON_ADD_NUMBER_TO_OBJECT(json, "total_queries", total_queries); JSON_ADD_NUMBER_TO_OBJECT(json, "blocked_queries", blocked_count); - JSON_SEND_OBJECT_UNLOCK(json); + JSON_SEND_OBJECT(json); } int api_stats_top_clients(struct ftl_conn *api) @@ -337,10 +347,12 @@ int api_stats_top_clients(struct ftl_conn *api) lock_shm(); int clients = counters->clients; - int *temparray = calloc(2*clients, sizeof(int)); - if(temparray == NULL) + const int total_queries = counters->queries; + const int blocked_count = get_blocked_count(); + struct top_entries *top_clients = calloc(clients, sizeof(struct top_entries)); + if(top_clients == NULL) { - log_err("Memory allocation failed in api_stats_top_clients()"); + log_err("Memory allocation failed in %s()", __FUNCTION__); return 0; } @@ -354,15 +366,26 @@ int api_stats_top_clients(struct ftl_conn *api) if(client == NULL || (!client->flags.aliasclient && client->aliasclient_id >= 0)) continue; - temparray[2*added_clients + 0] = clientID; + const char *client_ip = getstr(client->ippos); + // Hidden client, probably due to privacy level. Skip this in the top lists + if(strcmp(client_ip, HIDDEN_CLIENT) == 0) + continue; + // Use either blocked or total count based on request string - temparray[2*added_clients + 1] = blocked ? client->blockedcount : client->count; + top_clients[added_clients].count = blocked ? client->blockedcount : client->count; + + // Get client name and IP + top_clients[added_clients].ippos = client->ippos; + top_clients[added_clients].namepos = client->namepos; added_clients++; } + // Unlock shared memory + unlock_shm(); + // Sort temporary array - qsort(temparray, added_clients, sizeof(int[2]), cmpdesc); + qsort(top_clients, added_clients, sizeof(*top_clients), cmpdesc_te); // Get clients which the user doesn't want to see regex_t *regex_clients = NULL; @@ -372,24 +395,19 @@ int api_stats_top_clients(struct ftl_conn *api) ®ex_clients, &N_regex_clients); int n = 0; - cJSON *top_clients = JSON_NEW_ARRAY(); + cJSON *jtop_clients = JSON_NEW_ARRAY(); + + // Lock shared memory + lock_shm(); + for(unsigned int i = 0; i < added_clients; i++) { - // Get sorted indices and counter values (may be either total or blocked count) - const int clientID = temparray[2*i + 0]; - const int client_count = temparray[2*i + 1]; - // Get client pointer - const clientsData* client = getClient(clientID, true); - if(client == NULL) + // Skip e.g. recycled clients + if(top_clients[i].namepos == 0) continue; - // Get IP and host name of client - const char *client_ip = getstr(client->ippos); - const char *client_name = getstr(client->namepos); - - // Hidden client, probably due to privacy level. Skip this in the top lists - if(strcmp(client_ip, HIDDEN_CLIENT) == 0) - continue; + const char *client_ip = getstr(top_clients[i].ippos); + const char *client_name = getstr(top_clients[i].namepos); // Skip this client if there is a filter on it bool skip_client = false; @@ -419,21 +437,25 @@ int api_stats_top_clients(struct ftl_conn *api) // Return this client if the client made at least one query // within the most recent 24 hours - if(client_count > 0) + if(top_clients[i].count > 0) { cJSON *client_item = JSON_NEW_OBJECT(); - JSON_REF_STR_IN_OBJECT(client_item, "name", client_name); - JSON_REF_STR_IN_OBJECT(client_item, "ip", client_ip); - JSON_ADD_NUMBER_TO_OBJECT(client_item, "count", client_count); - JSON_ADD_ITEM_TO_ARRAY(top_clients, client_item); + JSON_COPY_STR_TO_OBJECT(client_item, "name", client_name); + JSON_COPY_STR_TO_OBJECT(client_item, "ip", client_ip); + JSON_ADD_NUMBER_TO_OBJECT(client_item, "count", top_clients[i].count); + JSON_ADD_ITEM_TO_ARRAY(jtop_clients, client_item); n++; } if(n == count) break; } + + // Unlock shared memory + unlock_shm(); + // Free temporary array - free(temparray); + free(top_clients); // Free regexes if(N_regex_clients > 0) @@ -447,20 +469,21 @@ int api_stats_top_clients(struct ftl_conn *api) } cJSON *json = JSON_NEW_OBJECT(); - JSON_ADD_ITEM_TO_OBJECT(json, "clients", top_clients); + JSON_ADD_ITEM_TO_OBJECT(json, "clients", jtop_clients); - const int blocked_count = get_blocked_count(); JSON_ADD_NUMBER_TO_OBJECT(json, "blocked_queries", blocked_count); - JSON_ADD_NUMBER_TO_OBJECT(json, "total_queries", counters->queries); - JSON_SEND_OBJECT_UNLOCK(json); + JSON_ADD_NUMBER_TO_OBJECT(json, "total_queries", total_queries); + JSON_SEND_OBJECT(json); } int api_stats_upstreams(struct ftl_conn *api) { const int upstreams = counters->upstreams; - int *temparray = calloc(2*upstreams, sizeof(int)); - if(temparray == NULL) + const int forwarded_count = get_forwarded_count(); + const int total_queries = counters->queries; + struct top_entries *top_upstreams = calloc(upstreams, sizeof(struct top_entries)); + if(top_upstreams == NULL) { log_err("Memory allocation failed in api_stats_upstreams()"); return 0; @@ -477,22 +500,34 @@ int api_stats_upstreams(struct ftl_conn *api) if(upstream == NULL) continue; - temparray[2*added_upstreams + 0] = upstreamID; - temparray[2*added_upstreams + 1] = upstream->count; + top_upstreams[added_upstreams].count = upstream->count; + top_upstreams[added_upstreams].ippos = upstream->ippos; + top_upstreams[added_upstreams].namepos = upstream->namepos; + top_upstreams[added_upstreams].port = upstream->port; + top_upstreams[added_upstreams].responses = upstream->responses; + top_upstreams[added_upstreams].rtime = upstream->rtime; + top_upstreams[added_upstreams].rtuncertainty = upstream->rtuncertainty; added_upstreams++; } + // Unlock shared memory + unlock_shm(); + // Sort temporary array in descending order - qsort(temparray, upstreams, sizeof(int[2]), cmpdesc); + qsort(top_upstreams, added_upstreams, sizeof(*top_upstreams), cmpdesc); // Loop over available forward destinations - cJSON *top_upstreams = JSON_NEW_ARRAY(); + cJSON *jtop_upstreams = JSON_NEW_ARRAY(); + + // Lock shared memory + lock_shm(); + for(int i = -2; i < (int)added_upstreams; i++) { int count = 0; const char* ip, *name; - int port = -1; + in_port_t port = -1; double responsetime = 0.0, uncertainty = 0.0; if(i == -2) @@ -512,33 +547,22 @@ int api_stats_upstreams(struct ftl_conn *api) else { // Regular upstream destination - // Get sorted indices - const int upstreamID = temparray[2*i + 0]; - - // Get upstream pointer - const upstreamsData *upstream = getUpstream(upstreamID, true); - if(upstream == NULL) - continue; - - // Get IP and host name of upstream destination if available - ip = getstr(upstream->ippos); - name = getstr(upstream->namepos); - port = upstream->port; - - // Get percentage - count = upstream->count; + ip = getstr(top_upstreams[i].ippos); + name = getstr(top_upstreams[i].namepos); + port = top_upstreams[i].port; + count = top_upstreams[i].count; // Compute average response time and uncertainty (unit: seconds) - if(upstream->responses > 0) + if(top_upstreams[i].responses > 0) { // Simple average of the response times - responsetime = upstream->rtime / upstream->responses; + responsetime = top_upstreams[i].rtime / top_upstreams[i].responses; } - if(upstream->responses > 1) + if(top_upstreams[i].responses > 1) { // The actual value will be somewhere in a neighborhood around the mean value. // This neighborhood of values is the uncertainty in the mean. - uncertainty = sqrt(upstream->rtuncertainty / upstream->responses / (upstream->responses-1)); + uncertainty = sqrt(top_upstreams[i].rtuncertainty / top_upstreams[i].responses / (top_upstreams[i].responses-1)); } } @@ -548,31 +572,36 @@ int api_stats_upstreams(struct ftl_conn *api) if(count > 0 || i < 0) { cJSON *upstream = JSON_NEW_OBJECT(); - JSON_REF_STR_IN_OBJECT(upstream, "ip", ip); - JSON_REF_STR_IN_OBJECT(upstream, "name", name); + JSON_COPY_STR_TO_OBJECT(upstream, "ip", ip); + JSON_COPY_STR_TO_OBJECT(upstream, "name", name); JSON_ADD_NUMBER_TO_OBJECT(upstream, "port", port); JSON_ADD_NUMBER_TO_OBJECT(upstream, "count", count); cJSON *statistics = JSON_NEW_OBJECT(); JSON_ADD_NUMBER_TO_OBJECT(statistics, "response", responsetime); JSON_ADD_NUMBER_TO_OBJECT(statistics, "variance", uncertainty); JSON_ADD_ITEM_TO_OBJECT(upstream, "statistics", statistics); - JSON_ADD_ITEM_TO_ARRAY(top_upstreams, upstream); + JSON_ADD_ITEM_TO_ARRAY(jtop_upstreams, upstream); } } + // Unlock shared memory + unlock_shm(); + // Free temporary array - free(temparray); + free(top_upstreams); cJSON *json = JSON_NEW_OBJECT(); - JSON_ADD_ITEM_TO_OBJECT(json, "upstreams", top_upstreams); - const int forwarded_count = get_forwarded_count(); + JSON_ADD_ITEM_TO_OBJECT(json, "upstreams", jtop_upstreams); + JSON_ADD_NUMBER_TO_OBJECT(json, "forwarded_queries", forwarded_count); - JSON_ADD_NUMBER_TO_OBJECT(json, "total_queries", counters->queries); - JSON_SEND_OBJECT_UNLOCK(json); + JSON_ADD_NUMBER_TO_OBJECT(json, "total_queries", total_queries); + + JSON_SEND_OBJECT(json); } int api_stats_query_types(struct ftl_conn *api) { + // Lock shared memory lock_shm(); cJSON *types = JSON_NEW_OBJECT(); @@ -583,11 +612,14 @@ int api_stats_query_types(struct ftl_conn *api) return ret; } + // Unlock shared memory + unlock_shm(); + cJSON *json = JSON_NEW_OBJECT(); JSON_ADD_ITEM_TO_OBJECT(json, "types", types); // Send response - JSON_SEND_OBJECT_UNLOCK(json); + JSON_SEND_OBJECT(json); } int api_stats_recentblocked(struct ftl_conn *api) @@ -641,7 +673,10 @@ int api_stats_recentblocked(struct ftl_conn *api) break; } + // Unlock shared memory + unlock_shm(); + cJSON *json = JSON_NEW_OBJECT(); JSON_ADD_ITEM_TO_OBJECT(json, "blocked", blocked); - JSON_SEND_OBJECT_UNLOCK(json); + JSON_SEND_OBJECT(json); } From 5fdfeb9ef62a4923c0db35b97f4b479f9dd62abd Mon Sep 17 00:00:00 2001 From: DL6ER Date: Tue, 16 Jul 2024 09:45:59 +0200 Subject: [PATCH 28/30] Add blocked domains in Query Log suggestions Signed-off-by: DL6ER --- src/api/api.h | 6 + src/api/queries.c | 65 ++++------- src/api/stats.c | 222 +++++++++++++++++++++++------------- src/webserver/json_macros.h | 25 ++++ 4 files changed, 198 insertions(+), 120 deletions(-) diff --git a/src/api/api.h b/src/api/api.h index e8e57964b..e9db54097 100644 --- a/src/api/api.h +++ b/src/api/api.h @@ -33,6 +33,12 @@ int api_stats_upstreams(struct ftl_conn *api); int api_stats_top_domains(struct ftl_conn *api); int api_stats_top_clients(struct ftl_conn *api); int api_stats_recentblocked(struct ftl_conn *api); +cJSON *get_top_domains(struct ftl_conn *api, const int count, + const bool blocked, const bool domains_only); +cJSON *get_top_clients(struct ftl_conn *api, const int count, + const bool blocked, const bool clients_only, + const bool names_only); +cJSON *get_top_upstreams(struct ftl_conn *api, const bool upstreams_only); // History methods int api_history(struct ftl_conn *api); diff --git a/src/api/queries.c b/src/api/queries.c index 7874c7d69..774094380 100644 --- a/src/api/queries.c +++ b/src/api/queries.c @@ -22,6 +22,7 @@ // dbopen(false, ), dbclose() #include "database/common.h" +#if 0 static int add_strings_to_array(struct ftl_conn *api, cJSON *array1, cJSON *array2, const char *querystr, const int max_count) { @@ -80,60 +81,44 @@ static int add_strings_to_array(struct ftl_conn *api, cJSON *array1, cJSON *arra return 0; } +#endif int api_queries_suggestions(struct ftl_conn *api) { - int rc; // Does the user request a custom number of records to be included? int count = 30; get_int_var(api->request->query_string, "count", &count); // Get domains - cJSON *domain = JSON_NEW_ARRAY(); - log_debug(DEBUG_API, "Reading top domains from database"); - rc = add_strings_to_array(api, domain, NULL, "WITH CTE AS (SELECT COUNT(*) cnt, domain FROM query_storage GROUP BY domain ORDER BY cnt DESC)"\ - "SELECT d.domain FROM CTE JOIN domain_by_id d ON CTE.domain = d.id", count); - if(rc != 0) + cJSON *domain = get_top_domains(api, count, false, true); + cJSON *blocked = get_top_domains(api, count, true, true); + // Add domains from both arrays, avoiding duplicates + cJSON *entry = NULL; + cJSON_ArrayForEach(entry, blocked) { - log_err("Cannot read domains from database"); - cJSON_Delete(domain); - return rc; + // Check if the domain is already in the list + bool found = false; + cJSON *entry2 = NULL; + cJSON_ArrayForEach(entry2, domain) + { + if(strcmp(cJSON_GetStringValue(entry), cJSON_GetStringValue(entry2)) == 0) + { + found = true; + break; + } + } + if(!found) + JSON_ADD_ITEM_TO_ARRAY(domain, cJSON_Duplicate(entry, true)); } - log_debug(DEBUG_API, "Read %d domains from database", cJSON_GetArraySize(domain)); + // Free the blocked list + cJSON_Delete(blocked); // Get clients, both by IP and names - // We have to call DISTINCT() here as multiple IPs can map to and name and - // vice versa - cJSON *client_ip = JSON_NEW_ARRAY(); - cJSON *client_name = JSON_NEW_ARRAY(); - log_debug(DEBUG_API, "Reading top client IPs and names from database"); - rc = add_strings_to_array(api, client_ip, client_name, "WITH CTE AS (SELECT COUNT(*) cnt, client FROM query_storage GROUP BY client ORDER BY cnt DESC)"\ - "SELECT c.ip,c.name FROM CTE JOIN client_by_id c ON CTE.client = c.id", count); - if(rc != 0) - { - log_err("Cannot read client IPs from database"); - cJSON_Delete(domain); - cJSON_Delete(client_ip); - return rc; - } - log_debug(DEBUG_API, "Read %d client IPs and %d client names from database", cJSON_GetArraySize(client_ip), cJSON_GetArraySize(client_name)); + cJSON *client_ip = get_top_clients(api, count, false, true, false); + cJSON *client_name = get_top_clients(api, count, false, true, true); // Get upstreams - cJSON *upstream = JSON_NEW_ARRAY(); - log_debug(DEBUG_API, "Reading top upstreams from database"); - rc = add_strings_to_array(api, upstream, NULL, "WITH CTE AS (SELECT COUNT(*) cnt, forward FROM query_storage GROUP BY forward ORDER BY cnt DESC)"\ - "SELECT f.forward FROM CTE JOIN forward_by_id f ON CTE.forward = f.id", count); - if(rc != 0) - { - log_err("Cannot read forward from database"); - cJSON_Delete(domain); - cJSON_Delete(client_ip); - cJSON_Delete(client_name); - cJSON_Delete(upstream); - return rc; - } - log_debug(DEBUG_API, "Read %d upstreams from database", cJSON_GetArraySize(upstream)); - + cJSON *upstream = get_top_upstreams(api, true); // Get types cJSON *type = JSON_NEW_ARRAY(); queriesData query = { 0 }; diff --git a/src/api/stats.c b/src/api/stats.c index 09f71fc6f..18227e3c8 100644 --- a/src/api/stats.c +++ b/src/api/stats.c @@ -161,7 +161,8 @@ int api_stats_summary(struct ftl_conn *api) JSON_SEND_OBJECT(json); } -int api_stats_top_domains(struct ftl_conn *api) +cJSON *get_top_domains(struct ftl_conn *api, const int count, + const bool blocked, const bool domains_only) { // Exit before processing any data if requested via config setting if(config.misc.privacylevel.v.privacy_level >= PRIVACY_HIDE_DOMAINS) @@ -171,23 +172,14 @@ int api_stats_top_domains(struct ftl_conn *api) // Minimum structure is // {"top_domains":[]} - cJSON *json = JSON_NEW_OBJECT(); - cJSON *top_domains = JSON_NEW_ARRAY(); - JSON_ADD_ITEM_TO_OBJECT(json, "top_domains", top_domains); - JSON_SEND_OBJECT(json); - } - - bool blocked = false; // Can be overwritten by query string - int count = 10; - // /api/stats/top_domains?blocked=true - if(api->request->query_string != NULL) - { - // Should blocked domains be shown? - get_bool_var(api->request->query_string, "blocked", &blocked); - - // Does the user request a non-default number of replies? - // Note: We do not accept zero query requests here - get_int_var(api->request->query_string, "count", &count); + if(domains_only) + return cJSON_CreateArray(); + + cJSON *json = cJSON_CreateObject(); + cJSON_AddItemToObject(json, "domains", cJSON_CreateArray()); + cJSON_AddNumberToObject(json, "total_queries", -1); + cJSON_AddNumberToObject(json, "blocked_queries", -1); + return json; } // Get domains which the user doesn't want to see @@ -207,7 +199,7 @@ int api_stats_top_domains(struct ftl_conn *api) if(top_domains == NULL) { log_err("Memory allocation failed in %s()", __FUNCTION__); - return 0; + return NULL; } unsigned int added_domains = 0u; @@ -241,7 +233,7 @@ int api_stats_top_domains(struct ftl_conn *api) qsort(top_domains, added_domains, sizeof(*top_domains), cmpdesc_te); int n = 0; - cJSON *jtop_domains = JSON_NEW_ARRAY(); + cJSON *jtop_domains = cJSON_CreateArray(); // Lock shared memory lock_shm(); @@ -271,20 +263,23 @@ int api_stats_top_domains(struct ftl_conn *api) } } - if(skip_domain) + if(skip_domain || top_domains[i].count < 1) continue; - if(top_domains[i].count > 0) + if(domains_only) { - cJSON *domain_item = JSON_NEW_OBJECT(); - JSON_COPY_STR_TO_OBJECT(domain_item, "domain", domain); - JSON_ADD_NUMBER_TO_OBJECT(domain_item, "count", top_domains[i].count); - JSON_ADD_ITEM_TO_ARRAY(jtop_domains, domain_item); - n++; + cJSON_AddStringToArray(jtop_domains, domain); + } + else + { + cJSON *domain_item = cJSON_CreateObject(); + cJSON_AddStringToObject(domain_item, "domain", domain); + cJSON_AddNumberToObject(domain_item, "count", top_domains[i].count); + cJSON_AddItemToArray(jtop_domains, domain_item); } // Only count entries that are actually sent and return when we have send enough data - if(n >= count) + if(++n >= count) break; } @@ -305,19 +300,43 @@ int api_stats_top_domains(struct ftl_conn *api) free(regex_domains); } - cJSON *json = JSON_NEW_OBJECT(); - JSON_ADD_ITEM_TO_OBJECT(json, "domains", jtop_domains); - - JSON_ADD_NUMBER_TO_OBJECT(json, "total_queries", total_queries); - JSON_ADD_NUMBER_TO_OBJECT(json, "blocked_queries", blocked_count); + if(domains_only) + { + // Return the array of domains only + return jtop_domains; + } - JSON_SEND_OBJECT(json); + // else: Build and return full object + cJSON *json = cJSON_CreateObject(); + cJSON_AddItemToObject(json, "domains", jtop_domains); + cJSON_AddNumberToObject(json, "total_queries", total_queries); + cJSON_AddNumberToObject(json, "blocked_queries", blocked_count); + return json; } -int api_stats_top_clients(struct ftl_conn *api) +int api_stats_top_domains(struct ftl_conn *api) { + bool blocked = false; // Can be overwritten by query string int count = 10; + // /api/stats/top_domains?blocked=true + if(api->request->query_string != NULL) + { + // Should blocked domains be shown? + get_bool_var(api->request->query_string, "blocked", &blocked); + // Does the user request a non-default number of replies? + // Note: We do not accept zero query requests here + get_int_var(api->request->query_string, "count", &count); + } + + cJSON *json = get_top_domains(api, count, blocked, false); + JSON_SEND_OBJECT(json); +} + +cJSON *get_top_clients(struct ftl_conn *api, const int count, + const bool blocked, const bool clients_only, + const bool names_only) +{ // Exit before processing any data if requested via config setting if(config.misc.privacylevel.v.privacy_level >= PRIVACY_HIDE_DOMAINS_CLIENTS) { @@ -326,21 +345,14 @@ int api_stats_top_clients(struct ftl_conn *api) // Minimum structure is // {"top_clients":[]} - cJSON *json = JSON_NEW_OBJECT(); - cJSON *top_clients = JSON_NEW_ARRAY(); - JSON_ADD_ITEM_TO_OBJECT(json, "top_clients", top_clients); - JSON_SEND_OBJECT(json); - } - - bool blocked = false; // /api/stats/top_clients?blocked=true - if(api->request->query_string != NULL) - { - // Should blocked clients be shown? - get_bool_var(api->request->query_string, "blocked", &blocked); - - // Does the user request a non-default number of replies? - // Note: We do not accept zero query requests here - get_int_var(api->request->query_string, "count", &count); + if(clients_only) + return cJSON_CreateArray(); + + cJSON *json = cJSON_CreateObject(); + cJSON_AddItemToObject(json, "clients", cJSON_CreateArray()); + cJSON_AddNumberToObject(json, "total_queries", -1); + cJSON_AddNumberToObject(json, "blocked_queries", -1); + return json; } // Lock shared memory @@ -432,22 +444,29 @@ int api_stats_top_clients(struct ftl_conn *api) } } - if(skip_client) + if(skip_client || top_clients[i].count < 1) continue; - // Return this client if the client made at least one query - // within the most recent 24 hours - if(top_clients[i].count > 0) + if(clients_only) { - cJSON *client_item = JSON_NEW_OBJECT(); - JSON_COPY_STR_TO_OBJECT(client_item, "name", client_name); - JSON_COPY_STR_TO_OBJECT(client_item, "ip", client_ip); - JSON_ADD_NUMBER_TO_OBJECT(client_item, "count", top_clients[i].count); - JSON_ADD_ITEM_TO_ARRAY(jtop_clients, client_item); - n++; + if(names_only) + { + if(strlen(client_name) > 0) + cJSON_AddStringToArray(jtop_clients, client_name); + } + else + cJSON_AddStringToArray(jtop_clients, client_ip); + } + else + { + cJSON *client_item = cJSON_CreateObject(); + cJSON_AddStringToObject(client_item, "name", client_name); + cJSON_AddStringToObject(client_item, "ip", client_ip); + cJSON_AddNumberToObject(client_item, "count", top_clients[i].count); + cJSON_AddItemToArray(jtop_clients, client_item); } - if(n == count) + if(++n == count) break; } @@ -468,16 +487,40 @@ int api_stats_top_clients(struct ftl_conn *api) free(regex_clients); } - cJSON *json = JSON_NEW_OBJECT(); - JSON_ADD_ITEM_TO_OBJECT(json, "clients", jtop_clients); + if(clients_only) + { + // Return the array of clients only + return jtop_clients; + } - JSON_ADD_NUMBER_TO_OBJECT(json, "blocked_queries", blocked_count); - JSON_ADD_NUMBER_TO_OBJECT(json, "total_queries", total_queries); - JSON_SEND_OBJECT(json); + // else: Build and return full object + cJSON *json = cJSON_CreateObject(); + cJSON_AddItemToObject(json, "clients", jtop_clients); + cJSON_AddNumberToObject(json, "total_queries", total_queries); + cJSON_AddNumberToObject(json, "blocked_queries", blocked_count); + return json; } +int api_stats_top_clients(struct ftl_conn *api) +{ + bool blocked = false; // Can be overwritten by query string + int count = 10; + // /api/stats/top_clients?blocked=true + if(api->request->query_string != NULL) + { + // Should blocked clients be shown? + get_bool_var(api->request->query_string, "blocked", &blocked); + + // Does the user request a non-default number of replies? + // Note: We do not accept zero query requests here + get_int_var(api->request->query_string, "count", &count); + } + + cJSON *json = get_top_clients(api, count, blocked, false, false); + JSON_SEND_OBJECT(json); +} -int api_stats_upstreams(struct ftl_conn *api) +cJSON *get_top_upstreams(struct ftl_conn *api, const bool upstreams_only) { const int upstreams = counters->upstreams; const int forwarded_count = get_forwarded_count(); @@ -569,18 +612,25 @@ int api_stats_upstreams(struct ftl_conn *api) // Send data: // - always if i < 0 (special upstreams: blocklist and cache) // - only if there are any queries for all others (i > 0) - if(count > 0 || i < 0) + if(count < 1 && i >= 0) + continue; + + if(upstreams_only) + { + cJSON_AddStringToArray(jtop_upstreams, name); + } + else { cJSON *upstream = JSON_NEW_OBJECT(); - JSON_COPY_STR_TO_OBJECT(upstream, "ip", ip); - JSON_COPY_STR_TO_OBJECT(upstream, "name", name); - JSON_ADD_NUMBER_TO_OBJECT(upstream, "port", port); - JSON_ADD_NUMBER_TO_OBJECT(upstream, "count", count); + cJSON_AddStringToObject(upstream, "ip", ip); + cJSON_AddStringToObject(upstream, "name", name); + cJSON_AddNumberToObject(upstream, "port", port); + cJSON_AddNumberToObject(upstream, "count", count); cJSON *statistics = JSON_NEW_OBJECT(); - JSON_ADD_NUMBER_TO_OBJECT(statistics, "response", responsetime); - JSON_ADD_NUMBER_TO_OBJECT(statistics, "variance", uncertainty); - JSON_ADD_ITEM_TO_OBJECT(upstream, "statistics", statistics); - JSON_ADD_ITEM_TO_ARRAY(jtop_upstreams, upstream); + cJSON_AddNumberToObject(statistics, "response", responsetime); + cJSON_AddNumberToObject(statistics, "variance", uncertainty); + cJSON_AddItemToObject(upstream, "statistics", statistics); + cJSON_AddItemToArray(jtop_upstreams, upstream); } } @@ -590,12 +640,24 @@ int api_stats_upstreams(struct ftl_conn *api) // Free temporary array free(top_upstreams); - cJSON *json = JSON_NEW_OBJECT(); - JSON_ADD_ITEM_TO_OBJECT(json, "upstreams", jtop_upstreams); + if(upstreams_only) + { + // Return the array of upstreams only + return jtop_upstreams; + } + + // else: Build and return full object + cJSON *json = cJSON_CreateObject(); + cJSON_AddItemToObject(json, "upstreams", jtop_upstreams); + cJSON_AddNumberToObject(json, "total_queries", total_queries); + cJSON_AddNumberToObject(json, "forwarded_queries", forwarded_count); - JSON_ADD_NUMBER_TO_OBJECT(json, "forwarded_queries", forwarded_count); - JSON_ADD_NUMBER_TO_OBJECT(json, "total_queries", total_queries); + return json; +} +int api_stats_upstreams(struct ftl_conn *api) +{ + cJSON *json = get_top_upstreams(api, false); JSON_SEND_OBJECT(json); } diff --git a/src/webserver/json_macros.h b/src/webserver/json_macros.h index 1ad9b09dd..5961c3ac0 100644 --- a/src/webserver/json_macros.h +++ b/src/webserver/json_macros.h @@ -57,6 +57,29 @@ cJSON_AddItemToObject(object, key, string_item); \ }) +// Hand over allocated string to cJSON - it will thereafter take care of freeing +// it when the cJSON object is deleted +#define JSON_GIVE_STR_TO_OBJECT(object, key, string)({ \ + cJSON *string_item = NULL; \ + if(string != NULL) \ + { \ + string_item = cJSON_CreateStringReference((const char*)(string)); \ + string_item->type &= ~cJSON_IsReference; \ + } \ + else \ + { \ + string_item = cJSON_CreateNull(); \ + } \ + if(string_item == NULL) \ + { \ + cJSON_Delete(object); \ + send_http_internal_error(api); \ + log_err("JSON_GIVE_STR_TO_OBJECT FAILED (key: \"%s\", string: \"%s\")!", key, string); \ + return 500; \ + } \ + cJSON_AddItemToObject(object, key, string_item); \ +}) + #define JSON_ADD_NUMBER_TO_OBJECT(object, key, num)({ \ const double number = num; \ if(cJSON_AddNumberToObject(object, key, number) == NULL) \ @@ -260,3 +283,5 @@ cJSON *elem = cJSON_GetObjectItemCaseSensitive(obj, key); \ elem != NULL ? cJSON_IsTrue(elem) : false; \ }) + +#define cJSON_AddStringToArray(array, string) cJSON_AddItemToArray(array, cJSON_CreateString(string)) From 0a971d39b86cdaa3eedbcfabac5ff791c7232ae4 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Thu, 18 Jul 2024 16:55:17 +0200 Subject: [PATCH 29/30] Remove duplicates from client name suggestions Signed-off-by: DL6ER --- src/api/queries.c | 3 +++ src/webserver/http-common.c | 43 +++++++++++++++++++++++++++++++++++++ src/webserver/http-common.h | 1 + test/pdns/setup.sh | 6 ++++++ 4 files changed, 53 insertions(+) diff --git a/src/api/queries.c b/src/api/queries.c index 774094380..bc8dac1d1 100644 --- a/src/api/queries.c +++ b/src/api/queries.c @@ -117,6 +117,9 @@ int api_queries_suggestions(struct ftl_conn *api) cJSON *client_ip = get_top_clients(api, count, false, true, false); cJSON *client_name = get_top_clients(api, count, false, true, true); + // Delete duplicate entries from client_name + cJSON_unique_array(client_name); + // Get upstreams cJSON *upstream = get_top_upstreams(api, true); // Get types diff --git a/src/webserver/http-common.c b/src/webserver/http-common.c index 2154f0f61..75e687ba4 100644 --- a/src/webserver/http-common.c +++ b/src/webserver/http-common.c @@ -662,3 +662,46 @@ char *__attribute__((malloc)) escape_json(const char *string) // Return the JSON escaped string return namep; } + +// Remove duplicates from a cJSON array +// This function uses the less efficient cJSON_GetArraySize() function compared +// to cJSON_ArrayForEach() as we are going to modify the array in-place while +// iterating over it +void cJSON_unique_array(cJSON *array) +{ + // Check if the array is an array + if(!cJSON_IsArray(array)) + return; + + for(int oi = 0; oi < cJSON_GetArraySize(array); oi++) + { + // Get the outer item + cJSON *outer_item = cJSON_GetArrayItem(array, oi); + // Check if the item is a string + if (!cJSON_IsString(outer_item)) + continue; + + // Check for duplicates in the remainder of the array + for(int ii = oi + 1; ii < cJSON_GetArraySize(array); ii++) + { + // Get the inner item + cJSON *inner_item = cJSON_GetArrayItem(array, ii); + // Check if the inner item is a string + if (!cJSON_IsString(inner_item)) + continue; + + // Compare the two strings + if(strcmp(outer_item->valuestring, inner_item->valuestring) == 0) + { + // Remove the duplicate item, this is safe as we are + // at least one item ahead of the outer item + cJSON_DeleteItemFromArray(array, ii); + // Compensate for removed item (the for loop + // will increment ii for the next step, thus + // we need to decrement it here) + ii--; + continue; + } + } + } +} diff --git a/src/webserver/http-common.h b/src/webserver/http-common.h index d8bfe1afa..bde500e56 100644 --- a/src/webserver/http-common.h +++ b/src/webserver/http-common.h @@ -103,5 +103,6 @@ char * __attribute__((malloc)) escape_html(const char *string); int check_json_payload(struct ftl_conn *api); int parse_groupIDs(struct ftl_conn *api, tablerow *table, cJSON *row); char * __attribute__((malloc)) escape_json(const char *string); +void cJSON_unique_array(cJSON *array); #endif // HTTP_H diff --git a/test/pdns/setup.sh b/test/pdns/setup.sh index 715d5571a..7a860fef2 100644 --- a/test/pdns/setup.sh +++ b/test/pdns/setup.sh @@ -124,6 +124,12 @@ pdnsutil add-record arpa. 2.1.168.192.in-addr PTR a.ftl. pdnsutil add-record arpa. 1.0.c.1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.8.e.f.ip6 PTR ftl. pdnsutil add-record arpa. 2.0.c.1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.8.e.f.ip6 PTR aaaa.ftl. +# Add DNSSEC zone +pdnsutil create-zone dnssec ns1.ftl + +# Create trust anchor +pdnsutil add-zone-key dnssec KSK active + # Calculates the ‘ordername’ and ‘auth’ fields for all zones so they comply with # DNSSEC settings. Can be used to fix up migrated data. Can always safely be # run, it does no harm. From e128d866bf96d5529f0361998b3553b854d8b928 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Thu, 18 Jul 2024 17:55:25 +0200 Subject: [PATCH 30/30] Undo unintended test change Signed-off-by: DL6ER --- test/pdns/setup.sh | 6 ------ 1 file changed, 6 deletions(-) diff --git a/test/pdns/setup.sh b/test/pdns/setup.sh index 7a860fef2..715d5571a 100644 --- a/test/pdns/setup.sh +++ b/test/pdns/setup.sh @@ -124,12 +124,6 @@ pdnsutil add-record arpa. 2.1.168.192.in-addr PTR a.ftl. pdnsutil add-record arpa. 1.0.c.1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.8.e.f.ip6 PTR ftl. pdnsutil add-record arpa. 2.0.c.1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.8.e.f.ip6 PTR aaaa.ftl. -# Add DNSSEC zone -pdnsutil create-zone dnssec ns1.ftl - -# Create trust anchor -pdnsutil add-zone-key dnssec KSK active - # Calculates the ‘ordername’ and ‘auth’ fields for all zones so they comply with # DNSSEC settings. Can be used to fix up migrated data. Can always safely be # run, it does no harm.