Improve NTP implementation

[DL6ER]

What does this implement/fix?

Start NTP server thread(s) only after the first successful NTP time sync. This ensures the time we offer is correct. As a by-product this also gives some time for alternative NTP servers (e.g., crony) to finish their startup so we don't take the port before they can do it. Also, disable DNSSEC time-windows validation until first sync succeeded to avoid DNSSEC-related issues during NTP sync.

---

Related issue or feature (if applicable): N/A

Pull request in docs with documentation (if applicable): N/A

---

By submitting this pull request, I confirm the following:

1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
2. I have commented my proposed changes within the code.
3. I am willing to help maintain this change if there are issues with it later.
4. It is compatible with the EUPL 1.2 license
5. I have squashed any insignificant commits. (git rebase)

Checklist:

• [x] The code change is tested and works locally.
• [x] I based my code and PRs against the repositories developmental branch.
• [x] I signed off all commits. Pi-hole enforces the DCO for all contributions
• [x] I signed all my commits. Pi-hole requires signatures to verify authorship
• [x] I have read the above and my PR is ready for review.

[pralor-bot]

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/ntp-feature-request-for-pihole-ftl-v6/70991/2

[DL6ER]

TODO

• Run NTP test later in the CI test suite to give it time to properly start after initial time synchronization
• do not start NTP client if CAP_SET_TIME is not available
• add dedicated boolean to ease disabling of NTP client
• docs: Ignore SIGUSR7

[PromoFaux]

Without SYS_TIME on a container:

pihole  | 2024-07-01 20:32:06.427 BST [243M] WARNING: Insufficient permissions to set system time, NTP client not available

👍

Is there some value in including a hint that SYS_TIME is unavailable in this warning message?

Similarly

Maybe something here about SYS_NICE ``` pihole | 2024-07-01 20:32:33.023 BST [244M] WARNING: Cannot set process priority to -10: Permission denied. Process priority remains at 0 ```

[DL6ER]

Why does it try to open the database if it errored before?

Because it wants to store this in the message table. But it does make sense to never store in the message table when running interactively. Pushed now.

[Gontier-Julien]

@DL6ER With this PR would this mean that it won't throw a warning anymore when there already is an ntp server on the server/system? Also i don't know/think it already exist so correct me if i'm wrong as always, but there isn't an single option to disable the ntp client & server currently no? As i already have an ntp server that also sync the clock of the server it self there isn't much of a need for pi-hole to act either as a server or client then, so maybe as single option could be added to disable it?

[DL6ER]

With this PR would this mean that it won't throw a warning anymore when there already is an ntp server on the server/system?

Well, it'd warn to the log file that the port is already taken and, hence, Pi-hole won't start its own NTP server. But you can safely ignore this warning if you know you have a local NTP server. There will be no warning added to the Pi-hole diagnosis system.

---

an single option to disable the ntp client & server

There are already options to disable it, even when it is not a single one:

ntp.ipv4.active = false
ntp.ipv6.active = false
ntp.sync.active = false

will disable all NTP components inside Pi-hole. I don't think we really need one option.

[Gontier-Julien]

Well, it'd warn to the log file that the port is already taken and, hence, Pi-hole won't start its own NTP server. But you can safely ignore this warning if you know you have a local NTP server. There will be no warning added to the Pi-hole diagnosis system.

Well awesome then! ^^

There are already options to disable it, even when it is not a single one:

ntp.ipv4.active = false
ntp.ipv6.active = false
ntp.sync.active = false

will disable all NTP components inside Pi-hole. I don't think we really need one option.

That what i've done currently, but it fine by me too tbh, i was think so it simple for anyone, but i guess that if someone already have a ntp server then it should be easy

Alright then, thank ^^

[DL6ER]

What signal are you sending to dnsmasq to signal correct time? You created a custom SIGUSR7 which is interpreted as SIGUP

[...]

dnsmasq config says, one should use SIGINT

Yes. The new signal was introduced because SIGINT has a different effect when starting dnsmasq in debug mode (pihole-FTL -d): it terminates the program. This is also mentioned in man dnsmasq but it doesn't seem useful here. Hence , I introduced a new signal that does the exact same and reuses the same code except going the exit() path.

The logged message contains the SIGHUP a fixed string which is now outdated. It could already have be triggered with SIGINT before as well so it wasn't necessarily accurate anyway - removed with the last commit.

---

I noticed that queries that happen after dnsmasq start before starting DNSSEC validation are not logged to the query database.

[...]

Is this unavoidable?

Sorry, I see I have not mentioned this before. It is hard to avoid. The issue is that we can only add queries after the database importing from disk or we will not have the domain, client, and forward IDs as we have not imported these at this point.

edit Turns out it helps to step back and look at things later again - it is actually less hard, I just had to arrange some code to disentangle database initialization. We have now two parts, one running during database initialization (importing all the dependencies for new queries) and another one importing the actual queries which can be delayed until we know we have a proper time (or will never get one).

---

How can a DNSSEC validation be secure before the DNSSEC timestamp can be checked?

Because dnsmasq still performs all the checks, it will not just consider signature expired or signature not yet valid as errors. The cache is flushed once time is ready to ensure all domains are re-checked and we don't trust an SECURE which was not time-checked.

[yubiuser]

I fear it did not work. The query at 23:03:10 has not been logged to the database.

nanopi@nanopi:~$ cat /var/log/pihole/pihole.log | grep DNSSEC
Jul  5 23:03:08 dnsmasq[684108]: DNSSEC validation enabled
Jul  5 23:03:08 dnsmasq[684108]: DNSSEC signature timestamps not checked until receipt of SIGINT
...
Jul  5 23:03:15 dnsmasq[684108]: now checking DNSSEC signature timestamps

nanopi@nanopi:~$ cat /var/log/pihole/pihole.log | grep sigok
Jul  5 23:03:10 dnsmasq[684108]: query[A] sigok.ippacket.stream from 10.0.1.202
Jul  5 23:03:10 dnsmasq[684108]: forwarded sigok.ippacket.stream to 8.8.8.8
Jul  5 23:03:10 dnsmasq[684108]: reply sigok.ippacket.stream is <CNAME>
Jul  5 23:03:10 dnsmasq[684108]: reply sigok.rsa2048-sha256.ippacket.stream is 195.201.14.36

nanopi@nanopi:~$ pihole-FTL sqlite3 -h  /etc/pihole/pihole-FTL.db "Select * from queries where domain is 'sigok.ippacket.stream'"
id       timestamp         type  status  domain                 client     forward     additional_info  reply_type  reply_time            dnssec  list_id
-------  ----------------  ----  ------  ---------------------  ---------  ----------  ---------------  ----------  --------------------  ------  -------
2930152  1716923761.61009  1     2       sigok.ippacket.stream  10.0.1.24  8.8.8.8#53  (null)           3           0.220105886459351     1       (null) 
2930156  1716923761.41762  1     2       sigok.ippacket.stream  10.0.1.24  8.8.8.8#53  (null)           12          0.181222915649414     5       (null) 
2930240  1716923869.86778  1     2       sigok.ippacket.stream  10.0.1.24  8.8.8.8#53  (null)           12          0.207485675811768     5       (null) 
2930250  1716923870.08774  1     2       sigok.ippacket.stream  10.0.1.24  8.8.8.8#53  (null)           3           0.401464700698853     1       (null) 
3569511  1720119147.28513  1     2       sigok.ippacket.stream  10.0.1.59  8.8.8.8#53  (null)           3           0.428323030471802     1       (null) 
3569513  1720119148.74269  1     3       sigok.ippacket.stream  10.0.1.59  (null)      (null)           3           0.000569820404052734  1       (null) 
3570094  1720119705.92446  1     2       sigok.ippacket.stream  10.0.1.59  8.8.8.8#53  (null)           3           0.266446113586426     1       (null) 
3570098  1720119710.19207  1     3       sigok.ippacket.stream  10.0.1.59  (null)      (null)           3           0.000896692276000977  1       (null) 

Only part of the query has been captured