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 4cCtmw6yZSz30QC for ; Fri, 29 Aug 2025 09:50:40 +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 server-signature ECDSA (secp384r1 raw public key) server-digest SHA384 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mail01.haj.ipfire.org", Issuer "R13" (verified OK)) by mail02.haj.ipfire.org (Postfix) with ESMTPS id 4cCtmw1bkcz2xDp for ; Fri, 29 Aug 2025 09:50:40 +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 4cCtmt5j7Sz32r; Fri, 29 Aug 2025 09:50:38 +0000 (UTC) DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003ed25519; t=1756461038; 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=O7ASl/VHXur3GH5n0e8EbsOB/qugBeCFSWyfogwwI5o=; b=PhgGSzcyGfPinn5G7R++6tRxAxVeTOiP7DIgw6Q9lDV0EbNbuScWIJ6fouU9qHSQxEhv46 +VOVqMVKTCymdPCA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003rsa; t=1756461038; 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=O7ASl/VHXur3GH5n0e8EbsOB/qugBeCFSWyfogwwI5o=; b=R3UaeMb/gWdd+hJSPpHydFIQ5M4RqdhL88v2dQ9mKHJ5jJDhng6L+rLHt92x5+wz/p6yn+ YiT61IWus0uZeBfYeF01saUIpyv4Gin0f5x2mjv/d9TExl+izYqVwUpf9kvJSkos8Q4q6P iPnasVE44zXwG+bWalyu+S9ffBYFvswaAHh58RjSDU2gpQ+jb4GLCtrcCJzJnSS2A/KtpL p0IbUVQeozXn7j3Ler5rWtpQnhXjW2bgVF4ir4C3sZnJoBTH8Z3pASkJbpLZOANJbq5FAi PTc/+SvJTQWYo12tVSHtpmh74qqzyfx2K0KsA3ekdxpEGkJNxEj1rS5N7kqeIw== 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: Date: Fri, 29 Aug 2025 10:50:38 +0100 Cc: ddns@lists.ipfire.org Content-Transfer-Encoding: quoted-printable Message-Id: References: To: Chris Anton Hello Chris, 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. So let=E2=80=99s get into this... > On 29 Aug 2025, at 09:16, Chris Anton wrote: >=20 > From: faithinchaos21 = <45313722+faithinchaos21@users.noreply.github.com> I assume that you are faithinchaos21 and this code did not come from = someone else? > 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 To make it short, this patch is sadly very far away from what is = acceptable. Before we get into the actual implementation, there are many design = issues here that simply cannot be accepted into DDNS: * 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? * 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) * 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. > 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") > + 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. > + def update(self): > + import json, urllib.request, urllib.error 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. > + > + 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 A TTL of one second is never a good idea. > + > + headers =3D { > + "Authorization": "Bearer {0}".format(tok), > + "Content-Type": "application/json", > + "User-Agent": "IPFireDDNSUpdater/CFv4", > + } Since you are not using the internal request handler, you are creating = your own headers and a completely nonsensical user agent. Why? > + > + # --- 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 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. To just =E2=80=9Cguess=E2=80=9D the name of the zone is something that I = would not consider good style. > + > + 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}") Same as above, hardcoded defaults like the timeout. Spaghetti code. > + > + 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 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. > + > + # --- 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 Once again the same custom request logic. > + 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 I would really like to have support for Cloudflare in DDNS, but this is = sadly not the way. Please study my comments thoroughly and then lets come up with a plan = together how to implement this properly. Best, -Michael