-
-
Notifications
You must be signed in to change notification settings - Fork 187
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve NTP implementation #2004
base: development-v6
Are you sure you want to change the base?
Conversation
Signed-off-by: DL6ER <[email protected]>
…and remove obsolete variable thread_running[] (it is only ever set but never read) Signed-off-by: DL6ER <[email protected]>
…ld 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 <[email protected]>
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 |
TODO
|
… been started Signed-off-by: DL6ER <[email protected]>
…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 <[email protected]>
…t accurate time from elsewhere - it isn't intuitive that the server cannot be started without the client Signed-off-by: DL6ER <[email protected]>
…ilable Signed-off-by: DL6ER <[email protected]>
Signed-off-by: DL6ER <[email protected]>
Without
👍 Is there some value in including a hint that Similarly
Maybe something here about
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it try to open the database if it errored before?
nanopi@nanopi:~$ pihole-FTL ntp de.ntp.org
Using NTP server: de.ntp.org
Error NTP client: Cannot resolve NTP server address: Name has no usable address "de.ntp.org"
Cannot open database in read-write mode
Signed-off-by: DL6ER <[email protected]>
Because it wants to store this in the |
…uce chown() code duplication by using a single function for all chown()-activities Signed-off-by: DL6ER <[email protected]>
@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.
There are already options to disable it, even when it is not a single one:
will disable all NTP components inside Pi-hole. I don't think we really need one option. |
Well awesome then! ^^
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 ^^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What signal are you sending to dnsmasq
to signal correct time? You created a custom SIGUSR7
which is interpreted as SIGUP
2024-07-04 21:06:30.699 CEST [670283/T670448] INFO: -> DNS cache records: 4020
2024-07-04 21:06:30.699 CEST [670283/T670448] INFO: -> Known forward destinations: 2
2024-07-04 21:06:30.700 CEST [670283/T670464] INFO: NTP server listening on 0.0.0.0:123 (IPv4)
2024-07-04 21:06:30.700 CEST [670283/T670465] INFO: NTP server listening on :::123 (IPv6)
2024-07-04 21:06:30.703 CEST [670283M] INFO: Received SIGHUP, flushing cache and re-reading config
2024-07-04 21:06:30.704 CEST [670283M] INFO: Blocking status is enabled
dnsmasq
config says, one should use SIGINT
I noticed that queries that happen after dnsmasq
start before starting DNSSEC validation are not logged to the query database. Basically everything between
Jul 4 21:18:55 dnsmasq[670676]: DNSSEC validation enabled
Jul 4 21:18:55 dnsmasq[670676]: DNSSEC signature timestamps not checked until receipt of SIGINT
....
Jul 4 21:19:03 dnsmasq[670676]: now checking DNSSEC signature timestamps
Is this unavoidable?
How can a DNSSEC validation be secure before the DNSSEC timestamp can be checked?
Jul 4 21:18:59 dnsmasq[670676]: reply rsa2048-sha256.ippacket.stream is DNSKEY keytag 46436, algo 8
Jul 4 21:18:59 dnsmasq[670676]: validation result is SECURE
Jul 4 21:18:59 dnsmasq[670676]: reply sigok.ippacket.stream is <CNAME>
Jul 4 21:18:59 dnsmasq[670676]: reply sigok.rsa2048-sha256.ippacket.stream is 195.201.14.36
....
Jul 4 21:19:03 dnsmasq[670676]: now checking DNSSEC signature timestamps
Signed-off-by: DL6ER <[email protected]>
…tartup Signed-off-by: DL6ER <[email protected]>
Yes. The new signal was introduced because The logged message contains the
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 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).
Because |
I fear it did not work. The query at
Only part of the query has been captured |
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:
git rebase
)Checklist:
developmental
branch.