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 4cDBXs6LFQz30Qf for ; Fri, 29 Aug 2025 21:41:17 +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 4cDBXp3Kbkz2xP7 for ; Fri, 29 Aug 2025 21:41:14 +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 4cDBXn4knqz4MZ; Fri, 29 Aug 2025 21:41:13 +0000 (UTC) DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003ed25519; t=1756503673; 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=oAg7SAAuDSYCxYIAbgTZ81z5taTTXVDVCKOufZrxwmk=; b=O1iYVNn+AsR5D815u6JefPvqfpm/A62OZmYQwREOZ5CfzvtOuUTtZWbp1hWTZK/iGXPPJ+ 0ieV4i/N1WDiFmBg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003rsa; t=1756503673; 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=oAg7SAAuDSYCxYIAbgTZ81z5taTTXVDVCKOufZrxwmk=; b=p7lJAQFeeL8e97k2yvwIqKg3MHe6ADRpQiLdpDBiD6yWdi8dSZeZCYQESd4pvKtxPpU0lI PCFT47gklF6eB9NlHpTc1jgcIRs4yazfYPMR8BtXGJUEsnq8B/e7iLztiVlnfErxdgjPGp shZhzDoJfuM4BFGuzwcI4+R5NXnG7VLMq7p7BzZkJNNNBVKgCVluc7YvTfjQXBuwcS+kkE /inSb1Q3gyHEmYvTj9oBRj0N9Rc0MupfMedsjQfY1aiqLHMUTSRM9xwKYCIJM+wmg8x7I/ wUG7sndBtoxQUFPD9w/tgyxaBX//RuKldYAYdzntBXKt5Y72e0D5lbgjXMa1NA== 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] ovpnmain.cgi: Apply default settings when neccessary From: Michael Tremer In-Reply-To: Date: Fri, 29 Aug 2025 22:41:12 +0100 Cc: "IPFire: Development-List" , Stefan Schantl Content-Transfer-Encoding: quoted-printable Message-Id: References: <118761f0-24cd-4a62-b064-8d87dffc6b89@ipfire.org> <20250819183916.5083-1-stefan.schantl@ipfire.org> <68d888b8-e445-4555-bd29-e9d604adece6@ipfire.org> <179609FF-E0E3-4FCB-85F9-474C3566A224@ipfire.org> <3e6b6533-1a23-4e38-b126-a5072d8ff93a@ipfire.org> <411ca871-136c-4f2a-84ba-bce37a3a0caa@ipfire.org> To: Adolf Belka Perfect. I am happy that we found a solution for this. > On 29 Aug 2025, at 17:50, Adolf Belka wrote: >=20 > Hi Michael, >=20 > On 29/08/2025 12:42, Michael Tremer wrote: >> Hello Adolf, >> Thank you for looking at this in detail. I was a little bit confused = first, but I think I am now on the same page: >> * The issue is that we write ncp-disable into the configuration file = if no data ciphers have been selected. >> This should have worked fine in OpenVPN 2.5 (for which this code was = written), but since ncp-disable was now removed in OpenVPN 2.6, we can = no longer do this. >> Normally we would never land here, because there would always be a = default for DATACIPHERS set in = https://git.ipfire.org/?p=3Dipfire-2.x.git;a=3Dblob;f=3Dhtml/cgi-bin/ovpnm= ain.cgi;hb=3D198025111e37a80944dbab9ddd57967945e27949#l117 >> Since we have now modified this, we can no longer rely on any of = these defaults being set. That is a real shame. >> But I don=E2=80=99t see how we can fix this now at all. The problem = lies in browsers not setting us any values if a checkbox has not been = selected. There are a thousand hacks how to achieve that and they are = all opening up another large list of new problems. So I am not very keen = on doing this. >> I am proposing this as a solution now: >> = https://git.ipfire.org/?p=3Dipfire-2.x.git;a=3Dcommitdiff;h=3D351113e21eec= d730b33a2d73c1bb74eff9fcb845 >> It is not very pretty, but it works reliably without any Javascript = stunts or other hacks in the browser. We just need to make ensure that = we are extending it whenever we are adding more checkboxes. >> Please test and let me know if this resolves the issue(s). >=20 > Have tested this out with the latest CU197 build. >=20 > Everything worked as I expected it to. >=20 > I started with a working client in CU196. >=20 > I then updated to CU197 and the update worked as expected and the = connection worked. >=20 > I then restored a client connection that had previously been created = in CU197 and that also worked fine. >=20 > I then did a restore of the CU196 version and the connection also = worked. >=20 > I then modified a connection to use different ciphers in the = data-ciphers table and backed that up. I then restored one of the other = backups with the standard default data-ciphers. Then I restored the = backup with the modified data-ciphers and those were correctly restored. >=20 > So in terms of my standard client connections from CU196, everything = is working as expected both after an update to CU197 or by restoring a = CU196 based backup into CU197. >=20 > While working on various things today I have found another thing that = needs to be resolved when doing a restore of a client with multiple = routes to be pushed but I will do a separate email on that. >=20 > Regards, >=20 > Adolf. >=20 >> All the best, >> -Michael >>> On 27 Aug 2025, at 12:55, Adolf Belka = wrote: >>>=20 >>> Hi Michael, >>>=20 >>> On 26/08/2025 20:02, Adolf Belka wrote: >>>> Hi Michael, >>>> On 26/08/2025 12:45, Michael Tremer wrote: >>>>> Hello Adolf, >>>>>=20 >>>>> Hmm, maybe Stefan=E2=80=99s patch is not providing the full = solution. >>>>>=20 >>>>> I created a new function some time ago which is called = =E2=80=9Cset_defaults=E2=80=9D and the idea is that it would populate = any fields that have not been initialized. That way, if we add anything = new, there should always be a good default. >>>>>=20 >>>>> I understand why Stefan is turning of that initialisation, but = this does seem to create some more consequences. >>>>>=20 >>>>> We should never have any server configurations left with = ncp-disable, because that will not work any more. I thought regenerating = the configuration files through the CGI should take care of this. >>>> If Stefan's patch is not present then yes it takes care of it for = old restores with ncp-disable still in the server.conf file. >>>> If Stefan's patch is present then the default settings don't get = used as the restore already has the settings values defined. However the = old settings file has no DATACIPHERS entry in it and in ovpnmain.cgi = there is still a section of code that checks if DATACIPHERS=3D'' and if = yes then it writes ncp-disable into server.conf and leaves = DATACIPHERS=3D'' and this causes the openvpn-rw daemon to fail to start. >>>> That code is in lines 294 to 298. I suspect those lines were not = intended to be still in there. >>>> However this means that when an old (non ncp) restore is done then = ncp-disable is written into the server.conf file. >>>> Maybe if that section of code checks if DATACIPHERS=3D'' and if it = is then enters the default values for DATACIPHERS then maybe my "fixes" = in backup.pl and update.sh might not be needed. I could have a look at = that. I have a little bit of time now to work on IPFire as my nephew = and his family are now going back home so I can do some testing out of = those scenarios. >>>=20 >>> I modified the section of code to remove the ncp-disable entry and = if the DATACIPHERS value is empty then I am entering the default = DATACIPHERS set. >>>=20 >>> Doing this and then removing the backup.pl modification I made = worked for all restores of old backups that were still using = ncp-disable. >>>=20 >>> I also tested out doing restores of backups with the new config with = DATACIPHERS and those also all worked, giving the expected entries. >>>=20 >>> In all cases this was with Stefan's patch in place. >>>=20 >>> Based on this then the changes I made previously to backup.pl and = update.sh would no longer be needed and everything would be dealt with = in ovpnmain.cgi. >>>=20 >>> However, I am not certain if the changes I have made are the best = way to deal with this issue or not. >>>=20 >>> So I will submit a patch with the changes as an RFC patch so it can = be confirmed as the right way or if this would be considered a hack = approach. >>>=20 >>> Open for feedback. >>>=20 >>> Regards, >>>=20 >>> Adolf. >>>=20 >>>>>=20 >>>>> Maybe we need to rethink how we can make set_defaults() work so = that we don=E2=80=99t have to add more and more hacks?! >>>> If this can be done then that would probably be a good idea. >>>> Regards, >>>> Adolf. >>>>>=20 >>>>> Best, >>>>> -Michael >>>>>=20 >>>>>> On 25 Aug 2025, at 09:51, Adolf Belka = wrote: >>>>>>=20 >>>>>> Hi Stefan, >>>>>>=20 >>>>>> On 23/08/2025 14:55, Adolf Belka wrote: >>>>>>> Hi Stefan, >>>>>>> I tried out the CU197 Testing update with this patch in place. = It works fine for a new install, where there is no existing settings = file but for updates or when a restore from an old backup is being done = then a settings file already exists and then the default settings are = not applied and this results in the settings file having no CIPHERS = entry but having a fallback DCIPHER entry. >>>>>>> In the update where the OpenVPN RW server is stopped before = updating and started again afterwards this causes the server to fail to = start as there is no CIPHER entry. When a restore from backup is done = then the same thing happens with no CIPHERS entry, just a DCIPHER one = but as the server is running when the restore is done, it stays running = with the old settings but if the Save button is pressed then it Stops = because the settings file now has no CIPHERS entry. >>>>>>> Not sure how to fix this at the moment. Maybe it needs to be if = the settings file exists and it contains a CIPHERS entry but I am not = sure that is the right approach or not. >>>>>>=20 >>>>>> Figured out how to fix this. Your patch in ovpnmain.cgi stays the = same. I just needed to add in some additional lines into backup.pl and = the CU197 update.sh file. >>>>>>=20 >>>>>> I just check if ncp-disable is present in server.conf and if it = is then delete it and then add in the default = DATACIPHERS=3DAES-256-GCM|AES-128-GCM|CHACHA20-POLY1305 into the = settings file. >>>>>>=20 >>>>>> If ncp-disable is in the server.conf then the restore is from = prior to openvpn-2.6 and there will be no DATACIPHERS entry. >>>>>>=20 >>>>>> I have tested this out with the backup.pl changes and your patch = in place and everything works correctly again. I will submit patches for = these additional changes. >>>>>>=20 >>>>>> Regards, >>>>>>=20 >>>>>> Adolf. >>>>>>=20 >>>>>>=20 >>>>>>> Regards, >>>>>>> Adolf. >>>>>>> On 19/08/2025 20:39, Stefan Schantl wrote: >>>>>>>> Only apply the default settings in case nothing has been = configured yet, >>>>>>>> otherwise existing settings may get overwritten. >>>>>>>>=20 >>>>>>>> Signed-off-by: Stefan Schantl >>>>>>>> --- >>>>>>>> html/cgi-bin/ovpnmain.cgi | 2 +- >>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>>>=20 >>>>>>>> diff --git a/html/cgi-bin/ovpnmain.cgi = b/html/cgi-bin/ovpnmain.cgi >>>>>>>> index 83f9fdc02..a2f95dc9a 100644 >>>>>>>> --- a/html/cgi-bin/ovpnmain.cgi >>>>>>>> +++ b/html/cgi-bin/ovpnmain.cgi >>>>>>>> @@ -132,7 +132,7 @@ my $col=3D""; >>>>>>>> "MAX_CLIENTS" =3D> 100, >>>>>>>> "MSSFIX" =3D> "off", >>>>>>>> "TLSAUTH" =3D> "on", >>>>>>>> -}); >>>>>>>> +}) unless (%vpnsettings); >>>>>>>> # Load CGI parameters >>>>>>>> &Header::getcgihash(\%cgiparams, {'wantfile' =3D> 1, = 'filevar' =3D> 'FH'});