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 4cFF4D3ftRz30VZ for ; Sun, 31 Aug 2025 14:38:36 +0000 (UTC) Received: from mail01.ipfire.org (mail01.haj.ipfire.org [172.28.1.202]) (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 4cFF4C5LCbz2yrq for ; Sun, 31 Aug 2025 14:38:35 +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 4cFF4B3Rjkz2W3; Sun, 31 Aug 2025 14:38:34 +0000 (UTC) DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003ed25519; t=1756651114; 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=p7qEclU4Q9JxJd1f0Z42V6qfXWcHjgScak2F7M1K1FQ=; b=0mu9BR5qh6LlqoGFLBsAjRVekXamxPArR7Ys4iip4M37SyKxdzcc/x6qcZeatnHwiXZf61 65IfsneVM0QbkUDA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003rsa; t=1756651114; 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=p7qEclU4Q9JxJd1f0Z42V6qfXWcHjgScak2F7M1K1FQ=; b=vLtvQY0POX1SRiR5yez2FfB0uvbARae0yQ3FKmdX7A7q+De9Xu4Aio0BFwIv22Ssfr38Lt 0zrHq3Sy+j1B2fVoEvT1Jl2xprub9cLEwG8H5DCscTuz/zDbF4RucQcwdjE6bMW8Csv2LP mpnr1GI93hddtUgc0p8NSrcfYOJqvtl//5dPngP1uI3ccq3bo0weJgWmRrVG6qk3RyAt9A D2/7/n93fW22rQtshNnvQARab3ys7zDTlM9ptHe/wxN64aRcXIDgSwEhkz4S8RlheGlopT Xx15PLwmUdpfZn6NiVcMbyv3To7WQkybuET3muFycFB6bfJR1G7kHpi+rMTBUw== 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: <2fd1aa-68b1ec80-2b-633f1b80@145901099> Date: Sun, 31 Aug 2025 15:38:33 +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> To: Bernhard Beroun Hello, 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. I have studied your patch a little and I have taken some changes from it = that should go into the core: https://git.ipfire.org/?p=3Dddns.git;a=3Dshortlog;h=3Drefs/heads/json 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. 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: = https://developers.cloudflare.com/api/resources/dns/subresources/records/m= ethods/update/ 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. -Michael > 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