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 4cD4541Kmbz30Mg for ; Fri, 29 Aug 2025 16:50:16 +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 4cD4505j17z2xMw for ; Fri, 29 Aug 2025 16:50:12 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange x25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by mail01.ipfire.org (Postfix) with ESMTPSA id 4cD44z68wkz5n; Fri, 29 Aug 2025 16:50:11 +0000 (UTC) DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003ed25519; t=1756486212; 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=/0ufgd8SXBLGm71o5BBuLozlRrGS64EhFzKes2d6oP4=; b=pjEPhVoGr/9kpjMkIIJf5/Cint1FiXpWzCTXWJEdEutviVyI+CPWQJZM180QpcstmajSJP vPsUR1y5o4c5S/AA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003rsa; t=1756486212; 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=/0ufgd8SXBLGm71o5BBuLozlRrGS64EhFzKes2d6oP4=; b=LW3Q/7Eg83gXTBtaMGIFxeUNcwWufEX4alNEhNSG/Tw8IhiM+vw7r1W0U94whQEy2CNoYU QbH9O05Pp6gvolpRE7BNEAP7cnn2Uzc9Xe5GkOPQ5yPDU3bEddySfZ2gEku6/zCfFWm690 bSXjErlyXaz9SfuiRSki/NJzLlzGPwbE5nx/4H4blVaAqLUnj2MaKuc+U4qvL69Wg6mMP3 HhHfGp/10q/7BmI/G6cAUAmfbuWx0hqlzTcfwLe5cOtTgeU1KyLUKdGkXf5z5QpP7OXm18 0X841Qkhia58Q+BSt3536rLzJaFkTWHJov5MK53vdtB+ikHtmMBsVfF0i6bXUQ== Message-ID: Date: Fri, 29 Aug 2025 18:50:07 +0200 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 To: Michael Tremer 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> Content-Language: en-GB Cc: "IPFire: Development-List" , Stefan Schantl From: Adolf Belka In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi Michael, 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=ipfire-2.x.git;a=blob;f=html/cgi-bin/ovpnmain.cgi;hb=198025111e37a80944dbab9ddd57967945e27949#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’t 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=ipfire-2.x.git;a=commitdiff;h=351113e21eecd730b33a2d73c1bb74eff9fcb845 > > 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). Have tested this out with the latest CU197 build. Everything worked as I expected it to. I started with a working client in CU196. I then updated to CU197 and the update worked as expected and the connection worked. I then restored a client connection that had previously been created in CU197 and that also worked fine. I then did a restore of the CU196 version and the connection also worked. 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. 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. 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. Regards, Adolf. > > All the best, > -Michael > >> On 27 Aug 2025, at 12:55, Adolf Belka wrote: >> >> Hi Michael, >> >> On 26/08/2025 20:02, Adolf Belka wrote: >>> Hi Michael, >>> On 26/08/2025 12:45, Michael Tremer wrote: >>>> Hello Adolf, >>>> >>>> Hmm, maybe Stefan’s patch is not providing the full solution. >>>> >>>> I created a new function some time ago which is called “set_defaults” 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. >>>> >>>> I understand why Stefan is turning of that initialisation, but this does seem to create some more consequences. >>>> >>>> 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='' and if yes then it writes ncp-disable into server.conf and leaves DATACIPHERS='' 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='' 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. >> >> 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. >> >> Doing this and then removing the backup.pl modification I made worked for all restores of old backups that were still using ncp-disable. >> >> I also tested out doing restores of backups with the new config with DATACIPHERS and those also all worked, giving the expected entries. >> >> In all cases this was with Stefan's patch in place. >> >> 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. >> >> However, I am not certain if the changes I have made are the best way to deal with this issue or not. >> >> 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. >> >> Open for feedback. >> >> Regards, >> >> Adolf. >> >>>> >>>> Maybe we need to rethink how we can make set_defaults() work so that we don’t have to add more and more hacks?! >>> If this can be done then that would probably be a good idea. >>> Regards, >>> Adolf. >>>> >>>> Best, >>>> -Michael >>>> >>>>> On 25 Aug 2025, at 09:51, Adolf Belka wrote: >>>>> >>>>> Hi Stefan, >>>>> >>>>> 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. >>>>> >>>>> 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. >>>>> >>>>> 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=AES-256-GCM|AES-128-GCM|CHACHA20-POLY1305 into the settings file. >>>>> >>>>> 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. >>>>> >>>>> 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. >>>>> >>>>> Regards, >>>>> >>>>> Adolf. >>>>> >>>>> >>>>>> 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. >>>>>>> >>>>>>> Signed-off-by: Stefan Schantl >>>>>>> --- >>>>>>> html/cgi-bin/ovpnmain.cgi | 2 +- >>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>> >>>>>>> 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=""; >>>>>>> "MAX_CLIENTS" => 100, >>>>>>> "MSSFIX" => "off", >>>>>>> "TLSAUTH" => "on", >>>>>>> -}); >>>>>>> +}) unless (%vpnsettings); >>>>>>> # Load CGI parameters >>>>>>> &Header::getcgihash(\%cgiparams, {'wantfile' => 1, 'filevar' => 'FH'}); > > >