Add 'edit' action to Custom DNS endpoint #2379
Conversation
martinbutt
force-pushed
the
devel
branch
2 times, most recently
from
8038224
to
3baed89
Compare
Signed-off-by: Martin Butt <git@martinbutt.com>
|
I'm a bit confused on this use case, can you help clarify?
You're reporting your local RFC1918 address? Why would you do that since those addresses are not routeable or accessible outsied the lan?
Yes, that's what DDNS is used for, though there really isn't a "record" kept, the A record is updated when the DDNS client detects a change.
If it's RFC1918 then it doesn't expose anything about the network.
This is where I'm confused, why are private range addresses being used as DDNS values? |
There are cases where giving RFC1918 addresses a DNS resolvable hostname is useful. For example, you might have an IP camera. Instead of plugging in the camera, scanning your network for the new device, making note of the IP address and using that IP address whenever you want to view the feed. You can use the DDNS option the camera has to just view the feed (while inside the network, of course!) via camera.myhouse.com. This example extends to any devices that you want to connect to within your network where you don't want to manually scan for and keep track of the IP.
Let's say now that the DNS records for myhouse.com returns:
Even though these are RFC1918 IPs, it leaks information.
Correct. And as I am already running a DNS server within my network, i.e. Pi Hole, it would be great if I could use its local DNS feature to store my internal hostnames within my network, but currently the endpoints can only
|
This sounds like a great situation to use Pi-hole's DHCP server and give a static lease to that device/MAC. Edit: Not so that you always have that IP address known but because you can assign a hostname to that static lease that would become an A record.
I don't really see that as information that would be useful to anyone. I might be missing the potential attack surface that is exposed however. Seems like the core issue is using DDNS with a public A record and private IP addresses.
That's really what DHCP would be great at. You could use the router as the DHCP server and store the leases there and then have Pi-hole conditionally forward queries for your local domain to the router. Editing a record isn't how DNS is designed to operate. If you want to modify records then delete the old one and create a new one. This PR only has modifications for API so it is missing changes to the web interface to actually implement the API changes. That being said, remove/add new record is the workflow to use instead of modify existing. The DDNS client doesn't need to know the prior IP address, it only needs to know the A record value you want to update and then remove that record followed by adding a new value. Editing a record needs to create a new value anyways to make sure the TTL counters are not left in an unknown state. |
| function editCustomDNSEntry($ip = '', $domain = '', $reload = '', $json = true) | ||
| { | ||
| try { | ||
| $ip = !empty($_REQUEST['ip']) ? trim($_REQUEST['ip']) : $ip; | ||
| $domain = !empty($_REQUEST['domain']) ? trim($_REQUEST['domain']) : $domain; | ||
| $reload = !empty($_REQUEST['reload']) ? trim($_REQUEST['reload']) : $reload; | ||
|
|
||
| if (empty($ip)) { | ||
| return returnError('IP must be set', $json); | ||
| } | ||
|
|
||
| $ipType = get_ip_type($ip); | ||
|
|
||
| if (!$ipType) { | ||
| return returnError('IP must be valid', $json); | ||
| } | ||
|
|
||
| if (empty($domain)) { | ||
| return returnError('Domain must be set', $json); | ||
| } | ||
|
|
||
| if (!validDomain($domain)) { | ||
| return returnError('Domain must be valid', $json); | ||
| } | ||
|
|
||
| $existingEntries = getCustomDNSEntries(); | ||
|
|
||
| foreach ($existingEntries as $entry) { | ||
| if ($entry->domain == $domain) { | ||
| $old_ip = $entry->ip; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (isset($old_ip)) { | ||
| pihole_execute('-a removecustomdns '.$old_ip.' '.$domain); | ||
| } | ||
|
|
||
| pihole_execute('-a addcustomdns '.$ip.' '.$domain.' '.$reload); | ||
|
|
||
| return returnSuccess('', $json); | ||
| } catch (\Exception $ex) { | ||
| return returnError($ex->getMessage(), $json); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this is all logic that should be in what ever client that would be calling this endpoint.
The client should be doing the lifting of deciding if the record needs to be updated/modified by checking the existing records existence and then checking if the record needs updating. It would also be up to the client to decide if any existing records should be deleted since DNS itself has no requirement for an A record value to be unique.
I guess a simple wrapper update endpoint that is basically syntactic sugar for delete and then add could work but I like the idea of API endpoints doing one specific job and being extremely unambiguous with it's function.
Solely my views of course.
There was a problem hiding this comment.
This is basically:
// Pseudo-code. Not an actual code:
function editCustomDNSEntry($ip = '', $domain = '', $reload = '', $json = true) {
deleteCustomDNSEntry($ip, $domain); // this function currently can't receive parameters
addCustomDNSEntry($ip, $domain, $reload, $json);
}The only thing needed is to allow deleteCustomDNSEntry() to receive parameters (ip and domain from the entry to be deleted).
There was a problem hiding this comment.
Do you even need the IP address? Just the record should be enough.
There was a problem hiding this comment.
@rdwebdesign pretty much, except the current delete endpoint needs you to pass in the current IP address of the entry. As @dschaper points out, the IP is not actually needed, as you can only have one entry per hostname, so the hostname alone should be enough.
I can submit a separate PR to refactor the delete endpoint and remove the need for the IP to be sent. I think that's a good change regardless of the rest. What do you think?
Even if I make that change, it's still two API calls to "edit" a record. Ideally we would want one endpoint per CRUD operation, no? The only reason this is a discussion at the API level is because the underlying pihole CLI doesn't support edits. Maybe I should focus my efforts in adding the edit option there and keep the API a one-to-one mapping to the CLI.
There was a problem hiding this comment.
Actually, this API was created mostly to be used by the web interface.
In my opinion, the introduced change is not being used by the web interface at all. No pages are using use the new feature.
Ideally we would want one endpoint per CRUD operation, no?
I understand your idea, but this doesn't matter. These API calls use pihole to delete/save the values.
Your new code calls pihole 2 times:
pihole_execute('-a removecustomdns '.$old_ip.' '.$domain) and
pihole_execute('-a addcustomdns '.$ip.' '.$domain.' '.$reload)
This is exactly like calling the functions we already have (deleteCustomDNSEntry() followed by addCustomDNSEntry()).
This new function just uses a slightly different "path", but it calls the same code from pihole.
In the end pihole code needs 2 calls because it has only 2 functions to handle Custom DNS entries.
There is no "edit" or "update" function there. It will just remove the record and write the new one.
|
I've submitted a separate PR over to the CLI that would allow this to be simplified. pi-hole/pi-hole#4969 |
What i don't get here is why you don't just assign a static ip lease to your camera then use piholes local dns feature to setup a static dns entry pointing to that ip no ddns needed since the ip of the device won't change and if done right even if the device gets disconnected for a longer period of time it would not be an issue since the static lease should still make sure to reserve the ip for said device until it reconnects. |
Signed-off-by: Martin Butt git@martinbutt.com
Thank you for your contribution to the Pi-hole Community!
Please read the comments below to help us consider your Pull Request.
We are all volunteers and completing the process outlined will help us review your commits quicker.
Please make sure you
This PR adds an 'edit' action to the custom DNS endpoint. The reason for this is to allow existing Dynamic DNS clients the ability to update the custom DNS records.
Example use case
An individual has several devices on their local network. They have a Dynamic DNS client (Inadyn, ddclient, etc) installed on the devices to report their local IP to an external service so they can SSH to them easily. Currently, these DNS records become part of the public DNS records. The addresses for example could be 192.168.0.101-105. There are two downsides to having them listed as public DNS records, a) this exposes information about the individual's local network, and b) these addresses serve no purpose outside of the individual's local network. Pi Hole's Custom DNS entries are a solution for this, but the current implementation is not easy for Dynamic DNS clients to work with.
Before this commit the Dynamic DNS client would need to a) know the old IP address registered to the hostname to delete it, which is not something the current popular Dynamic DNS clients have easy access to, and b) make two API calls (one to delete and one to add the new entry). Most popular Dynamic DNS clients only have the option to hit one endpoint.
I have extended the API to add a new 'edit' action, which can look up and delete the previous hostname entry (if it exists) and add the new one.
If there is documentation on the current API, please let me know where it is and I'll update it.
By submitting this pull request, I confirm the following:
git rebase)