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 4cCvwt6fBpz30Q7 for ; Fri, 29 Aug 2025 10:42:38 +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 4cCvwq3lkGz2xQc for ; Fri, 29 Aug 2025 10:42: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 4cCvwp42T4z29; Fri, 29 Aug 2025 10:42:34 +0000 (UTC) DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003ed25519; t=1756464154; 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=N79vIHQAXGPRSn0G405XKphHEY7OMPUl2F1aWAK9CUw=; b=g0feL/BsRd2z6cc76NlYk5kAJlJYpwyKrqlr2W8NquzI1SGeQmvR+j8tKzQhqG7z3lsn3k IVEmcJRYmkNFP6Bg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003rsa; t=1756464154; 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=N79vIHQAXGPRSn0G405XKphHEY7OMPUl2F1aWAK9CUw=; b=ha++apw6ckbx2NBz65EP1x4yEoVCloNh9zKgcW1DrwF4e9HBt+/UXMDgWmCoI65TS2ZgbZ yLLXBMFu/vNtqztX8GQ3WUxtsVabT0rUtBA3zGFtBmAbkHMtmh+o3e/HZCozpAyRw8V/Ln Y5C4L7uxR1v7BECNrfzDc0BBcLbzhISTs+KynaQsmVwVpT/9W3bjaVfZLllBFU6Lli4DCv IijltuPYYxVCWcX56ROAoU4w6f0kOAd4DkQWnF+mD0cZA4BYHc2vtEBHVGYFwooxiXrSsr 6BPfSkl2G8tt+nkUC9b2j0W2r+QrHiuAKMBqFHgCbVOHbex6WJTEJXqf92ts/Q== 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: <411ca871-136c-4f2a-84ba-bce37a3a0caa@ipfire.org> Date: Fri, 29 Aug 2025 11:42:33 +0100 Cc: Stefan Schantl , "IPFire: Development-List" 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 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). 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'});