-
-
Notifications
You must be signed in to change notification settings - Fork 189
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
Conversation
Signed-off-by: DL6ER <dl6er@dl6er.de>
…and remove obsolete variable thread_running[] (it is only ever set but never read) Signed-off-by: DL6ER <dl6er@dl6er.de>
…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 <dl6er@dl6er.de>
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 <dl6er@dl6er.de>
…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 <dl6er@dl6er.de>
…t accurate time from elsewhere - it isn't intuitive that the server cannot be started without the client Signed-off-by: DL6ER <dl6er@dl6er.de>
…ilable Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
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 <dl6er@dl6er.de>
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 <dl6er@dl6er.de>
@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 <dl6er@dl6er.de>
…tartup Signed-off-by: DL6ER <dl6er@dl6er.de>
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 |
…during startup" This reverts commit bfd242d.
…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 a037276. Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
…ing 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 <dl6er@dl6er.de>
…re. Use GCinterval instead of hard-coding one hour as interval Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Signed-off-by: DL6ER <dl6er@dl6er.de>
Conflicts have been resolved. |
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.
Maybe you can update the PR text because some if it is not valid anymore after the last commits.
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.If the time is updated by a large amount, restart FTL to ensure we load the correct history window.
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.