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 4cCvXh3sh3z30Pj for ; Fri, 29 Aug 2025 10:25: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 4cCvXg5Rf7z2xQc for ; Fri, 29 Aug 2025 10:25:07 +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 4cCvXf53CjzY; Fri, 29 Aug 2025 10:25:06 +0000 (UTC) DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003ed25519; t=1756463106; 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=qbs2MbAWxsBLBoMrYWO0opB3Cohg8QucERUc7pPpZUM=; b=dy5WIxWRcHTZxVmjr12D6jiFQjLgrosa76HQFo4VjK+oj4aXd24IOk+vHRIGe6cpx0I2Ft D6D1ZsrwUTpe6fCg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003rsa; t=1756463106; 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=qbs2MbAWxsBLBoMrYWO0opB3Cohg8QucERUc7pPpZUM=; b=N2CiuelMpSDQoJCLG7t/se6f5UxSY6OMS5DmrEc3puFAVlU9yBNRJ6abzZFeOCeb02OwJn BI5kmOVl3cONhPfP6aH7M+XdWhHUzqOOG+hiDT5Uoozo3TlZNoA6vBM1MSNT47kd1+lODk NpzbR3fT5zz/cRkzdzil+oMIMpA66+IW2W6Hu+i777zSxxBioJMMTaXcOTUYVGGewKrQsf XEmGFRH2LsUXDq6b8QK7zfm6EAKV7pTfkFgX6WmztV5dMK0uPxErvxKgvaTNMle6+FtSh7 Eqe8F5lpdRbgVvq0Xtver039pGPDupzYjxK8tqa4RNt6vEATvrNi359Qz98NHQ== 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: <2139f7-68b17f00-3-48fa3400@237050521> Date: Fri, 29 Aug 2025 11:25:05 +0100 Cc: Chris Anton , ddns@lists.ipfire.org Content-Transfer-Encoding: quoted-printable Message-Id: <32702E44-5264-4D75-8645-F297E4AF920B@ipfire.org> References: <2139f7-68b17f00-3-48fa3400@237050521> To: Bernhard Beroun Hello Bernhard, No you are not hijacking. 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 Your patch looks much more like it because it is using and extending the = code of DDNS. However, it also only implements IPv4. Does Cloudflare not support IPv6 = for DNS at all? -Michael > 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