Re: [PATCH] OpenVPN: Delete RRD dir if connection is deleted

[Michael Tremer <michael.tremer@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 6354 bytes --]

Hi,

So where are we on this issue?

Is the patch ready to be accepted?

How do we delete the files that should already have been deleted?

-Michael

> On 11 Apr 2020, at 13:52, ummeegge <ummeegge(a)ipfire.org> wrote:
> 
> Hi Michael,
> 
> Am Samstag, den 11.04.2020, 13:24 +0100 schrieb Michael Tremer:
>> Hi,
>> 
>>> On 11 Apr 2020, at 12:59, ummeegge <ummeegge(a)ipfire.org> wrote:
>>> 
>>> Hi Michael,
>>> 
>>> Am Samstag, den 11.04.2020, 11:46 +0100 schrieb Michael Tremer:
>>>> Hi,
>>>> 
>>>> This is a good find.
>>>> 
>>>> Did you have a connection that had a space in the common name?
>>>> Potentially it is that.
>>> 
>>> No, the connections doesn´t have spaces. 
>>> 
>>>> 
>>>> Changing the code to use the common name should be trivial. Maybe
>>>> just try printing the path it is trying to delete. Are the files
>>>> maybe not accessible by “nobody”?
>>> 
>>> They are pretty much all root:root . If i change the permissions to
>>> nobody:nobdy i can delete all of them (by deleting X509) via a
>>> 
>>> @@ -1288,6 +1277,9 @@
>>>    while ($file = glob("${General::swroot}/ovpn/n2nconf/*")) {
>>> 	system ("rm -rf $file");
>>>    }
>>> +    while ($file = glob("/var/log/rrd/collectd/localhost/openvpn-
>>> *")) {
>>> +    	system ("rm -rf $file");
>>> +    }
>>> 
>>> which would spare this code -->
>>> 
> https://git.ipfire.org/?p=ipfire-2.x.git;a=commitdiff;h=e1297cbb7659618c526fdc1ab07e97f57f55fd78
>>> . Haven´t checked that yet for the deletion of only one
>>> connection...
>> 
>> If they belong to root, the web UI won’t have permissions to delete
>> them.
> Have changed the permissions via chown -R and tried to delete then via
> single connection but also via X509 deletion (deleting all) with no
> luck.
> Nevertheless, the RRD creation should chown then openvpn-* directories 
> too which it currently do not.
> 
>> 
>> That is something we will have to handle in openvpnctrl then.
> Yes.
> 
>> 
>>> Might it be possible that openvpnctrl handles there something
>>> incorrect ?
>> 
>> Is there any code to handle it? And if so, why is the CGI calling
>> “rm”?
> It is held in the already existing coding style -->
> https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=html/cgi-bin/ovpnmain.cgi;h=e76a688fe7dcda0b77bf716eb2538342cd775b00;hb=refs/heads/core142#l1231
> which should prevent the rmdir/unlink part for every connection i think.
> 
> 
> Best,
> 
> 
> Erik
> 
> 
>> 
>> 
>> -Michael
>> 
>>> 
>>> Best,
>>> 
>>> 
>>> Erik
>>> 
>>>> 
>>>> -Michael
>>>> 
>>>>> On 11 Apr 2020, at 09:06, ummeegge <ummeegge(a)ipfire.org> wrote:
>>>>> 
>>>>> Hi all,
>>>>> this patch does only works if the common name is the same then
>>>>> the
>>>>> connection name. Have encountered that the rrd creation for
>>>>> OpenVPN
>>>>> uses the common name of the certificate not the connection name
>>>>> -->
>>>>> 
>>>>> # root @ ipfire-server in /var/log/rrd/collectd/localhost
>>>>> [8:34:50] 
>>>>> $ ls
>>>>> cpu-0      disk-loop0                 iptables-filter-
>>>>> PSCAN  processes-charon    processes-spamd
>>>>> cpu-1      disk-
>>>>> sda                   load                   processes-
>>>>> java      processes-squid
>>>>> cpu-
>>>>> 2      entropy                    memory                 proces
>>>>> ses-
>>>>> mpd       processes-squidguard
>>>>> cpu-3      interface                  openvpn-
>>>>> rwonecert      processes-nmbd      processes-sshd
>>>>> cpufreq    iptables-filter-NEWNOTSYN  openvpn-
>>>>> rwtwocert      processes-openvpn   sensors-coretemp-isa-0000
>>>>> disk-dm-0  iptables-filter-
>>>>> POLICYFWD  ping                   processes-qemu      sensors-
>>>>> f71869-isa-0290
>>>>> disk-dm-1  iptables-filter-
>>>>> POLICYIN   processes              processes-rtorrent  swap
>>>>> disk-dm-2  iptables-filter-POLICYOUT  processes-
>>>>> asterisk     processes-smbd
>>>>> 
>>>>> $ cat /var/ipfire/ovpn/ovpnconfig 
>>>>> 1,on,rwonename,rwonecert,host,cert,,,,,,,,,,,,,,,,,,,,,,,,,,,,d
>>>>> ynam
>>>>> ic
>>>>> 2,on,rwtwoname,rwtwocert,host,cert,,,,,,,,,,,,,,,,,,,,,,,,,,,,d
>>>>> ynam
>>>>> ic,,,,,,,,,,,
>>>>> 
>>>>> strangely enough if i set the element index to [2] it doesn´t
>>>>> work.
>>>>> Currently not sure why that´s happen.
>>>>> 
>>>>> It is better to revert this patch.
>>>>> 
>>>>> Best,
>>>>> 
>>>>> Erik
>>>>> 
>>>>> Am Samstag, den 28.03.2020, 10:45 +0100 schrieb ummeegge:
>>>>>> Hi Peter,
>>>>>> 
>>>>>> Am Samstag, den 28.03.2020, 09:25 +0000 schrieb Peter Müller:
>>>>>>> Reviewed-by: Peter Müller <peter.mueller(a)ipfire.org>
>>>>>>> 
>>>>>>> In my opinion, this fixes #11713.
>>>>>> 
>>>>>> Haven´t seen that one, yes i think so.
>>>>>> Have found another one in here --> 
>>>>>> 
>>>>> 
>>>>> 
>>> 
>>> 
> https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=html/cgi-bin/ovpnmain.cgi;h=e76a688fe7dcda0b77bf716eb2538342cd775b00;hb=HEAD#l1224
>>>>>> which can not be solved in this way. Need to have another
>>>>>> look
>>>>>> into
>>>>>> this.
>>>>>> Will send a separate patch then for "delete all RRDs if X509
>>>>>> is
>>>>>> deleted".
>>>>>> 
>>>>>> Need a little more time.
>>>>>> 
>>>>>> Best,
>>>>>> 
>>>>>> Erik
>>>>>> 
>>>>>>> 
>>>>>>>> Signed-off-by: Erik Kapfer <ummeegge(a)ipfire.org>
>>>>>>>> ---
>>>>>>>> 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 ce9524df7..00ecd77a0 100644
>>>>>>>> --- a/html/cgi-bin/ovpnmain.cgi
>>>>>>>> +++ b/html/cgi-bin/ovpnmain.cgi
>>>>>>>> @@ -2513,7 +2513,7 @@ else
>>>>>>>> # CCD end
>>>>>>>> 		# Update collectd configuration and delete all
>>>>>>>> RRD
>>>>>>>> files of the removed connection
>>>>>>>> 		&writecollectdconf();
>>>>>>>> -		system ("/usr/local/bin/openvpnctrl -drrd
>>>>>>>> $confighash{$cgiparams{'KEY'}}[1]");
>>>>>>>> +		system ('/usr/local/bin/openvpnctrl', '-drrd',
>>>>>>>> $confighash{$cgiparams{'KEY'}}[1]);
>>>>>>>> 
>>>>>>>> 		delete $confighash{$cgiparams{'KEY'}};
>>>>>>>> 		my $temp2 = `/usr/bin/openssl ca -gencrl -out
>>>>>>>> ${General::swroot}/ovpn/crls/cacrl.pem -config
>>>>>>>> ${General::swroot}/ovpn/openssl/ovpn.cnf`;