From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.haj.ipfire.org (localhost [IPv6:::1]) by mail02.haj.ipfire.org (Postfix) with ESMTP id 4cGSSD67cxz30Vd for ; Tue, 02 Sep 2025 14:15:08 +0000 (UTC) Received: from mail01.ipfire.org (mail01.haj.ipfire.org [IPv6:2001:678:b28::25]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange x25519) (Client CN "mail01.haj.ipfire.org", Issuer "R13" (verified OK)) by mail02.haj.ipfire.org (Postfix) with ESMTPS id 4cGSSD0nBvz2xFx for ; Tue, 02 Sep 2025 14:15:08 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail01.ipfire.org (Postfix) with ESMTPSA id 4cGSSB3hQ0z6p; Tue, 02 Sep 2025 14:15:06 +0000 (UTC) DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003ed25519; t=1756822507; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=H3IHrggn5MPV00+lLAfrh1BBrTe6sfpDaqCc3x6GYc4=; b=ez76L4alJs9+63+XiTINOdDef95lYHG970Tc6DoIA7tFgVW93D+hZ0zamp3ZulgGjd+rpy /CVR9rxMo5FSo5Ag== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003rsa; t=1756822507; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=H3IHrggn5MPV00+lLAfrh1BBrTe6sfpDaqCc3x6GYc4=; b=Y4ga//0bu0nr83iCwgSHzGlwYkw3BYQ+8ZN/ZB1LkY7CHSpg+mFHI7xl+B+2+Ya84PLbo/ VFIoQJtmF3ISNEIFsM7mjiPQehXcKvlyvHyDG9qAENlCPtgnuKMkM5x/KjmBYOIf2yAT2j 4Q5SPWoGtTpj2TONaJY303Wzmw0l9ddKfTKch73EJTJIFSlJfTvUsVB6cqnLw3QD5l/G+e 8WN8jYYlel7BhBAHS8nC5ITBgnRszDvyZHX0LNjs9Ee5WEhZBFs4Iaa8xMB28QmZkXAeXH dH6KjFAnpeto89PTRiRuWsoqGfD6WFxEO7dd+BlW3QBiq/c+SwlerzRtWlgKpA== Content-Type: text/plain; charset=utf-8 Precedence: list List-Id: List-Subscribe: , List-Unsubscribe: , List-Post: List-Help: Sender: Mail-Followup-To: Mime-Version: 1.0 Subject: Re: [PATCH] ddns: add Cloudflare (v4) provider using API token From: Michael Tremer In-Reply-To: <13f39e-68b60000-1-59b1d300@94062361> Date: Tue, 2 Sep 2025 15:15:05 +0100 Cc: Chris Anton , ddns@lists.ipfire.org Content-Transfer-Encoding: quoted-printable Message-Id: References: <2139f7-68b17f00-3-48fa3400@237050521> <32702E44-5264-4D75-8645-F297E4AF920B@ipfire.org> <2fd1aa-68b1ec80-2b-633f1b80@145901099> <13f39e-68b60000-1-59b1d300@94062361> To: Bernhard Beroun Hello Bernhard, > On 1 Sep 2025, at 21:19, Bernhard Beroun = wrote: >=20 > Hello Michael, >=20 > thanks for adding the changes to the core - I'll adapt the changeset = to make use of it. I pushed these into a separate branch, so please feel free to test them = well. I did not really have a good way to test the code much because = nothing is really using it, yet. > What I do not fully understand though is, what the actual benefit of = using the PUT = (https://developers.cloudflare.com/api/resources/dns/subresources/records/= methods/update/) endpoint compared to the PATCH = (https://developers.cloudflare.com/api/resources/dns/subresources/records/= methods/edit/) endpoint, which currently using, is.=20 >=20 > As far as I've seen, both the PUT and the PATCH endpoint require a = zone_id and a dns_record_id, which both need to be fetched upfront. To = me both endpoints seems to be pretty much equal in terms of necessary = roundtrips? Or am I missing something (obvious) here? Yes, I agree that this is not entirely obvious what the difference is. I understand from the PATCH method, that the records have to exist = before and they can be updated. Hence there is some extra code required = to create the records if they did not exist before. That just requires = more roundtrips, more code, and therefore more things that could go = wrong. I am understanding the PUT method has something very similar, but it = does not have any prerequisites. The records don=E2=80=99t have to exist = and if they do, that does not matter to us (except CNAME which is = mentioned in the documentation). However, you are absolutely correct, = that it would require some kind of record ID. That makes very little = sense for us. But=E2=80=A6 maybe I should have explained myself better in my previous = email=E2=80=A6 I think this might all come together in this API call: = https://developers.cloudflare.com/api/resources/dns/subresources/records/m= ethods/batch/ We can batch multiple things together and there is the option to = =E2=80=9CPUT=E2=80=9D some new records in there. Exactly what we would = need. We just send the new records, don=E2=80=99t have to consider = anything beforehand, and simply let the API update the zone. That way, we would have only one call (outside fetching that stupid zone = ID) and therefore we only have to do error handling once. Either the = update was successful or it was not. That seems to be a much nicer and = cleaner approach. Since you have access to the API, can you confirm that my view of this = is current with a couple of cURL calls maybe? -Michael >=20 > Thanks a lot! >=20 > Cheers > Bernhard >=20 > Am Sonntag, August 31, 2025 16:38 CEST, schrieb Michael Tremer = : >=20 > =20 >>=20 >> Hello, >>=20 >> Well, some of the providers are actually not (too) well tested. That = is because I simply don=E2=80=99t have an account with all of them. So I = don=E2=80=99t feel bad to add IPv6 here without having it tested, = because if it does not work, we won=E2=80=99t loose anything. >>=20 >> I have studied your patch a little and I have taken some changes from = it that should go into the core: >>=20 >> https://git.ipfire.org/?p=3Dddns.git;a=3Dshortlog;h=3Drefs/heads/json >>=20 >> This is basically that the request function can process JSON better = without having to care about all the encoding/decoding stuff in the = provider handler. I think that will make the code much more readable. >>=20 >> Then, it concerns me that we will have to send so many requests. That = does not sound very efficient to me. And maybe we don=E2=80=99t have to = do this, because there is another option: >>=20 >> = https://developers.cloudflare.com/api/resources/dns/subresources/records/m= ethods/update/ >>=20 >> That way, we can just send the update as a single PUT operation and = replace whatever has been there before. That should work. We are even = able to send an A and AAAA record in one go. The only annoying part that = remains is fetching the ID of the zone. I am not sure whether ddns = should have the ability to cache data like this. >>=20 >> -Michael >>=20 >> > On 29 Aug 2025, at 19:08, Bernhard Beroun = wrote: >> >=20 >> > Hello Michael, >> >=20 >> > as far as I know Cloudflare also supports IPv6 for DNS, but since I = do not use any IPv6 in my home network, I haven't looked into that any = further. I also didn't want to add anything I can't thoroughly test = myself, therefore I decided to only add IPv4. I was hoping that it's = possible to get the IPv4 part into the codebase and let others build = upon that and add the missing IPv6 support. But I also totally = understand if that's something you don't want to do. >> >=20 >> > Cheers, >> > Bernhard >> >=20 >> >=20 >> > Am Freitag, August 29, 2025 12:25 CEST, schrieb Michael Tremer = : >> >=20 >> >=20 >> >>=20 >> >> Hello Bernhard, >> >>=20 >> >> No you are not hijacking. >> >>=20 >> >> I don=E2=80=99t know why your patch did not receive any feedback. = It must have fallen off the radar. I am not even the maintainer for = DDNS=E2=80=A6 >> >>=20 >> >> Your patch looks much more like it because it is using and = extending the code of DDNS. >> >>=20 >> >> However, it also only implements IPv4. Does Cloudflare not support = IPv6 for DNS at all? >> >>=20 >> >> -Michael >> >>=20 >> >> > On 29 Aug 2025, at 11:20, Bernhard Beroun = wrote: >> >> >=20 >> >> > Hi everybody, >> >> > I don't want to hijack this mail thread here, but just wanted to = let you know that I've also submitted a patch a few months ago. See = https://lists.ipfire.org/ddns/19993045-2374-4537-96a2-5e2b1900a750@lifting= coder.com/T/#u >> >> > Unfortunately I never received any feedback, so I assumed = Cloudflare support wasn't desired. Again, sorry for hijacking this email = chain here, just wanted to quickly drop it here, since it matches the = topic. >> >> > Have a nice day, >> >> > Bernhard >> >> >=20 >> >> > Am Freitag, August 29, 2025 11:50 CEST, schrieb Michael Tremer = : >> >> >=20 >> >> >=20 >> >> >>=20 >> >> >> Hello Chris, >> >> >>=20 >> >> >> You don=E2=80=99t need to submit any patches more than once. We = will get back to you as soon as there is time. >> >> >>=20 >> >> >> So let=E2=80=99s get into this... >> >> >>=20 >> >> >> > On 29 Aug 2025, at 09:16, Chris Anton = wrote: >> >> >> >=20 >> >> >> > From: faithinchaos21 = <45313722+faithinchaos21@users.noreply.github.com> >> >> >>=20 >> >> >> I assume that you are faithinchaos21 and this code did not come = from someone else? >> >> >>=20 >> >> >> > Date: Wed, 27 Aug 2025 01:22:46 -0500 >> >> >> > Subject: [PATCH] ddns: add Cloudflare (v4) provider using API = token >> >> >> > MIME-Version: 1.0 >> >> >> > Content-Type: text/plain; charset=3DUTF-8 >> >> >> > Content-Transfer-Encoding: 8bit >> >> >> >=20 >> >> >> > This adds a provider =E2=80=9Ccloudflare.com-v4=E2=80=9D that = updates an A record >> >> >> > via Cloudflare=E2=80=99s v4 API using a Bearer token. The = token is accepted >> >> >> > from either =E2=80=98token=E2=80=99 or legacy =E2=80=98password= =E2=80=99 for UI compatibility. >> >> >> >=20 >> >> >> > Tested on IPFire 2.29 / Core 196: >> >> >> > - no-op if A already matches WAN IP >> >> >> > - successful update when WAN IP changes >> >> >> > - logs include CFv4 breadcrumbs for troubleshooting >> >> >>=20 >> >> >> To make it short, this patch is sadly very far away from what = is acceptable. >> >> >>=20 >> >> >> Before we get into the actual implementation, there are many = design issues here that simply cannot be accepted into DDNS: >> >> >>=20 >> >> >> * This patch only adds support for IPv4. As far as I am aware, = Cloudflare is capable of IPv6, too. Why would this be limited to IPv4 = only? >> >> >>=20 >> >> >> * You are not using the scaffolding and tools that DDNS is = providing. A lot of code is being written with plain Python and throwing = away all the features that DDNS provides (catching Exceptions, proxy = support and many, many more=E2=80=A6) >> >> >>=20 >> >> >> * This all looks very AI-generated to me and is implemented on = a green field. You are even importing all the modules that you need and = ignore everything else from DDNS. Why not use this snippet as a = standalone script then? There was no consideration for what code existed = already. >> >> >>=20 >> >> >> > Signed-off-by: Chris Anton >> >> >> > --- >> >> >> > 563f089d0820bd61ad4aecac248d5cc1f2adfc81 >> >> >> > src/ddns/providers.py | 121 = ++++++++++++++++++++++++++++++++++++++++++ >> >> >> > 1 file changed, 121 insertions(+) >> >> >> >=20 >> >> >> > diff --git a/src/ddns/providers.py b/src/ddns/providers.py >> >> >> > index 59f9665..df0f3a9 100644 >> >> >> > --- a/src/ddns/providers.py >> >> >> > +++ b/src/ddns/providers.py >> >> >> > @@ -341,6 +341,127 @@ def have_address(self, proto): >> >> >> >=20 >> >> >> > return False >> >> >> >=20 >> >> >> > +class DDNSProviderCloudflareV4(DDNSProvider): >> >> >> > + """ >> >> >> > + Cloudflare v4 API using a Bearer Token. >> >> >> > + Put the API Token in the 'token' OR 'password' field of the = DDNS entry. >> >> >> > + Optional in ddns.conf: >> >> >> > + proxied =3D false|true (default false; keep false for = WireGuard) >> >> >> > + ttl =3D 1|60|120... (default 1 =3D 'automatic') >> >> >> > + """ >> >> >> > + handle =3D "cloudflare.com-v4" >> >> >> > + name =3D "Cloudflare (v4)" >> >> >> > + website =3D "https://www.cloudflare.com/" >> >> >> > + protocols =3D ("ipv4",) >> >> >> > + supports_token_auth =3D True >> >> >> > + holdoff_failure_days =3D 0 >> >> >> > + >> >> >> > + def _bool(self, key, default=3DFalse): >> >> >> > + v =3D str(self.get(key, default)).strip().lower() >> >> >> > + return v in ("1", "true", "yes", "on") >> >> >> > + >> >> >>=20 >> >> >> This is a good example of a function which totally goes against = the coding style of the rest of the code base. It uses str(), chains a = lot of functions to gather to make the code look shorter, but very = difficult to read. >> >> >>=20 >> >> >> > + def update(self): >> >> >> > + import json, urllib.request, urllib.error >> >> >>=20 >> >> >> Just no. We don=E2=80=99t import anything further down the = line. DDNS provides a toolkit of what to use and you should stay within = it. If some functionally is missing, DDNS=E2=80=99 functionality should = be extended so that other providers can re-use the same well-maintained = and tested code base. >> >> >>=20 >> >> >> > + >> >> >> > + tok =3D self.get("token") or self.get("password") >> >> >> > + if not tok: >> >> >> > + raise DDNSConfigurationError("API Token (password/token) >> >> >> > is missing.") >> >> >> > + >> >> >> > + proxied =3D self._bool("proxied", False) >> >> >> > + try: >> >> >> > + ttl =3D int(self.get("ttl", 1)) >> >> >> > + except Exception: >> >> >> > + ttl =3D 1 >> >> >>=20 >> >> >> A TTL of one second is never a good idea. >> >> >>=20 >> >> >> > + >> >> >> > + headers =3D { >> >> >> > + "Authorization": "Bearer {0}".format(tok), >> >> >> > + "Content-Type": "application/json", >> >> >> > + "User-Agent": "IPFireDDNSUpdater/CFv4", >> >> >> > + } >> >> >>=20 >> >> >> Since you are not using the internal request handler, you are = creating your own headers and a completely nonsensical user agent. Why? >> >> >>=20 >> >> >> > + >> >> >> > + # --- find zone --- >> >> >> > + parts =3D self.hostname.split(".") >> >> >> > + if len(parts) < 2: >> >> >> > + raise DDNSRequestError("Hostname '{0}' is not a valid >> >> >> > domain.".format(self.hostname)) >> >> >> > + >> >> >> > + zone_id =3D None >> >> >> > + zone_name =3D None >> >> >> > + for i in range(len(parts) - 1): >> >> >> > + candidate =3D ".".join(parts[i:]) >> >> >> > + url =3D >> >> >> > = f"https://api.cloudflare.com/client/v4/zones?name=3D{candidate}" >> >> >> > + try: >> >> >> > + req =3D urllib.request.Request(url, headers=3Dheaders, >> >> >> > method=3D"GET") >> >> >> > + with urllib.request.urlopen(req, timeout=3D20) as r: >> >> >> > + data =3D json.loads(r.read().decode()) >> >> >> > + except Exception as e: >> >> >> > + raise DDNSUpdateError(f"Failed to query Cloudflare >> >> >> > zones API: {e}") >> >> >> > + >> >> >> > + if data.get("success") and data.get("result"): >> >> >> > + zone_id =3D data["result"][0]["id"] >> >> >> > + zone_name =3D candidate >> >> >> > + break >> >> >>=20 >> >> >> It is not acceptable to build anything custom that sends a = request. You are removing all other kinds of features that I have = mentioned above. >> >> >>=20 >> >> >> To just =E2=80=9Cguess=E2=80=9D the name of the zone is = something that I would not consider good style. >> >> >>=20 >> >> >> > + >> >> >> > + if not zone_id: >> >> >> > + raise DDNSRequestError(f"Could not find a Cloudflare Zone >> >> >> > for '{self.hostname}'.") >> >> >> > + >> >> >> > + logger.info("CFv4: zone=3D%s id=3D%s", zone_name, zone_id) >> >> >> > + >> >> >> > + # --- get record --- >> >> >> > + rec_url =3D >> >> >> > = f"https://api.cloudflare.com/client/v4/zones/{zone_id}/dns_records?type=3D= A&name=3D{self.hostname}" >> >> >> > + try: >> >> >> > + req =3D urllib.request.Request(rec_url, headers=3Dheaders, >> >> >> > method=3D"GET") >> >> >> > + with urllib.request.urlopen(req, timeout=3D20) as r: >> >> >> > + rec_data =3D json.loads(r.read().decode()) >> >> >> > + except Exception as e: >> >> >> > + raise DDNSUpdateError(f"Failed to query Cloudflare DNS >> >> >> > records API: {e}") >> >> >> > + >> >> >> > + if not rec_data.get("success"): >> >> >> > + errs =3D rec_data.get("errors") or [] >> >> >> > + if any("Authentication error" in (e.get("message", "") or >> >> >> > "") for e in errs): >> >> >> > + raise DDNSAuthenticationError("Invalid API Token.") >> >> >> > + raise DDNSUpdateError(f"Cloudflare API error finding >> >> >> > record: {errs}") >> >> >>=20 >> >> >> Same as above, hardcoded defaults like the timeout. Spaghetti = code. >> >> >>=20 >> >> >> > + >> >> >> > + results =3D rec_data.get("result") or [] >> >> >> > + if not results: >> >> >> > + raise DDNSRequestError(f"No A record found for >> >> >> > '{self.hostname}' in zone '{zone_name}'.") >> >> >> > + >> >> >> > + record_id =3D results[0]["id"] >> >> >> > + stored_ip =3D results[0]["content"] >> >> >> > + logger.info("CFv4: record_id=3D%s stored_ip=3D%s", = record_id, stored_ip) >> >> >> > + >> >> >> > + # --- compare IPs --- >> >> >> > + current_ip =3D self.get_address("ipv4") >> >> >> > + logger.info("CFv4: current_ip=3D%s vs stored_ip=3D%s", >> >> >> > current_ip, stored_ip) >> >> >> > + if current_ip =3D=3D stored_ip: >> >> >> > + logger.info("CFv4: no update needed") >> >> >> > + return >> >> >>=20 >> >> >> Why is this done using the API at all? We have functionality to = use DNS for this which creates a lot less load than performing several = API requests every couple of minutes. >> >> >>=20 >> >> >> > + >> >> >> > + # --- update --- >> >> >> > + upd_url =3D >> >> >> > = f"https://api.cloudflare.com/client/v4/zones/{zone_id}/dns_records/{record= _id}" >> >> >> > + payload =3D { >> >> >> > + "type": "A", >> >> >> > + "name": self.hostname, >> >> >> > + "content": current_ip, >> >> >> > + "ttl": ttl, >> >> >> > + "proxied": proxied, >> >> >> > + } >> >> >> > + logger.info("CFv4: updating %s -> %s (proxied=3D%s = ttl=3D%s)", >> >> >> > self.hostname, current_ip, proxied, ttl) >> >> >> > + >> >> >> > + try: >> >> >> > + req =3D urllib.request.Request( >> >> >> > + upd_url, data=3Djson.dumps(payload).encode(), >> >> >> > headers=3Dheaders, method=3D"PUT" >> >> >> > + ) >> >> >> > + with urllib.request.urlopen(req, timeout=3D20) as r: >> >> >> > + upd =3D json.loads(r.read().decode()) >> >> >> > + except Exception as e: >> >> >> > + raise DDNSUpdateError(f"Failed to send update request to >> >> >> > Cloudflare: {e}=E2=80=9D) >> >> >> >=20 >> >> >>=20 >> >> >> Once again the same custom request logic. >> >> >>=20 >> >> >> > + if not upd.get("success"): >> >> >> > + raise DDNSUpdateError(f"Cloudflare API error on update: >> >> >> > {upd.get('errors')}") >> >> >> > + >> >> >> > + logger.info("CFv4: update ok for %s -> %s", self.hostname, = current_ip) >> >> >> > + return >> >> >> > + >> >> >> >=20 >> >> >> > class DDNSProtocolDynDNS2(object): >> >> >> > """ >> >> >> >=20 >> >> >>=20 >> >> >> I would really like to have support for Cloudflare in DDNS, but = this is sadly not the way. >> >> >>=20 >> >> >> Please study my comments thoroughly and then lets come up with = a plan together how to implement this properly. >> >> >>=20 >> >> >> Best, >> >> >> -Michael >> >> >>=20 >> >> >>=20 >> >> >>=20 >> >> >=20 >> >> >=20 >> >> >=20 >> >> >=20 >> >>=20 >> >>=20 >> >>=20 >> >=20 >> >=20 >> >=20 >> >=20 >>=20 >> =20 >=20 >=20 >=20 > =20