Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify how and when UQDN are forwarded with conditional forwarding #1873

Merged
merged 2 commits into from
Sep 11, 2021

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Sep 9, 2021

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes, and have included unit tests where possible.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)

What does this PR aim to accomplish?:

Clarify how and when UQDN are forwarded with conditional forwarding

How does this PR accomplish the above?:

Add more description in the corresponding sections. This is the result of https://discourse.pi-hole.net/t/also-forward-unqualified-host-names-creates-dnssec-loop/49208

What documentation changes (if any) are needed to support this PR?:

This is a documentation change.

@DL6ER DL6ER added this to the v5.6 milestone Sep 9, 2021
@DL6ER DL6ER requested review from yubiuser, Bucking-Horn and a team September 9, 2021 19:05
settings.php Outdated Show resolved Hide resolved
Co-authored-by: XhmikosR <[email protected]>
@yubiuser
Copy link
Member

yubiuser commented Sep 10, 2021

If Conditional Fowarding is enabled, unticking this box may cause a partial DNS loop under certain circumstances (e.g. if a client would send TLD DNSSEC queries).

I probably would reframe this sentence. Reason: default on a fresh installation is having all boxes unticked. But this sentence assumes it was already ticked. Maybe something like

If Conditional Fowarding is also enabled, ticking this box can prevent a potential partial DNS loop under certain circumstances (e.g. if a client would send TLD DNSSEC queries).


Additionally, I would add some hint for the advanced uses (and maybe for us to remember :-) That the option only applies to A and AAAA queries. This could already be in the header ("Never forward non-FQDNs A and AAAAqueries")

@DL6ER
Copy link
Member Author

DL6ER commented Sep 10, 2021

Reason: default on a fresh installation is having all boxes unticked

I disagree.

On fresh installs, the option will not be set at all in setupVars.conf which ticks the option (see line 133):
https://github.com/pi-hole/AdminLTE/blob/fb9bd561fdf936891370d85b32e899dff1dbf9d4/settings.php#L126-L134

Same for the other box (Never forward reverse lookups for private IP ranges):
https://github.com/pi-hole/AdminLTE/blob/fb9bd561fdf936891370d85b32e899dff1dbf9d4/settings.php#L136-L144

@PromoFaux PromoFaux merged commit 87beebd into release/v5.6 Sep 11, 2021
@PromoFaux PromoFaux deleted the tweak/FQDN_UQDN_DNS branch September 11, 2021 19:59
@pralor-bot
Copy link

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

https://discourse.pi-hole.net/t/pi-hole-ftl-v5-9-web-v5-6-and-core-v5-4-released/49544/1

@yubiuser
Copy link
Member

yubiuser commented Sep 12, 2021

I disagree.

You are right, but something is wrong here. The boxes are ticked in the web interface and also the template of 01-pihole.conf contains domain-needed and bogus-priv

https://github.com/pi-hole/pi-hole/blob/2673c2c0720fb3871a4c3cae6a3f38f9ccede24e/advanced/01-pihole.conf#L24-L28

But I tried it two times now on a fresh install and without further modifications both options are removed from the /etc/dnsmasq.d/01-pihole.conf (they are still there in the local repo clone in /etc/.pihole/)

Bildschirmfoto zu 2021-09-12 22-41-16

I'm not sure why the lines are removed, because the install script does only replace the @placeholders
https://github.com/pi-hole/pi-hole/blob/4736e03108763cc2d5659f48d8a1e8a64d9b2608/automated%20install/basic-install.sh#L1370-L1402

@yubiuser
Copy link
Member

yubiuser commented Sep 12, 2021

I think the reason is here:
finalExports calls ProcessDNSSettings from webpage.sh

https://github.com/pi-hole/pi-hole/blob/4736e03108763cc2d5659f48d8a1e8a64d9b2608/automated%20install/basic-install.sh#L1934

https://github.com/pi-hole/pi-hole/blob/2673c2c0720fb3871a4c3cae6a3f38f9ccede24e/advanced/Scripts/webpage.sh#L147

Which deletes domain-needed and bogus.priv only to add them again if {DNS_BOGUS_PRIV} and {DNS_FQDN_REQUIRED} are true.

https://github.com/pi-hole/pi-hole/blob/2673c2c0720fb3871a4c3cae6a3f38f9ccede24e/advanced/Scripts/webpage.sh#L170-L178

As long we don't call ProcessDNSSettings with those arguments, they will always be deleted.


ADD

An easy solution would be to add DNS_FQDN_REQUIRED=true and DNS_BOGUS_PRIV=true to the exported variables here (exported to setupVars.conf)
https://github.com/pi-hole/pi-hole/blob/4736e03108763cc2d5659f48d8a1e8a64d9b2608/automated%20install/basic-install.sh#L1912-L1923

Sourcing setupVars.conf is the first step in ProcessDNSSettings
https://github.com/pi-hole/pi-hole/blob/2673c2c0720fb3871a4c3cae6a3f38f9ccede24e/advanced/Scripts/webpage.sh#L148

I can file a PR tomorrow...

@DL6ER
Copy link
Member Author

DL6ER commented Sep 13, 2021

Thanks for your analysis. This definitely needs fixing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants