| Summary: | OpenVPN: RW server doesn't start with /24 notation in subnet | ||
|---|---|---|---|
| Product: | IPFire | Reporter: | Michael Tremer <michael.tremer> |
| Component: | --- | Assignee: | Erik Kapfer <ummeegge> |
| Status: | ASSIGNED --- | QA Contact: | |
| Severity: | Minor Usability | ||
| Priority: | Will affect an average number of users | CC: | peter.mueller |
| Version: | 2 | ||
| Hardware: | unspecified | ||
| OS: | Unspecified | ||
| See Also: | https://bugzilla.ipfire.org/show_bug.cgi?id=12075 | ||
| Bug Depends on: | |||
| Bug Blocks: | 13633 | ||
| Attachments: |
Converts DDN or CIDR notation to OpenVPN configuration format
VER 2 - Converts DDN or CIDR notation to OpenVPN configuration format |
||
|
Description
Michael Tremer
2018-08-23 17:29:11 UTC
Hi Michael, pushed a patch for this one which you can find in here --> https://git.ipfire.org/?p=people/ummeegge/ipfire-2.x.git;a=commit;h=a15ba89b5c7b286365be324632c2074e28df1baa . Can deliver it also to the Mailinglist if we did potential optimization on it. Will try that also for N2N connections if a little more time is there and of course if you approved this one here. A question: In which order should we proceed further since we have now 3 patches for OpenVPN. 1) NCP --> https://lists.ipfire.org/pipermail/development/2018-August/004667.html 2) Compression --> https://bugzilla.ipfire.org/show_bug.cgi?id=11819 (mostly solved but with open question). 3) This one. Best, Erik On Fri, 2018-08-24 at 07:23 +0000, IPFire Bugzilla wrote: > Comment # 1 on bug 11823 from Erik Kapfer > Hi Michael, > pushed a patch for this one which you can find in here --> > https://git.ipfire.org/?p=people/ummeegge/ipfire-2.x.git;a=commit;h=a15ba89b5c7b286365be324632c2074e28df1baa > . Yes, please *always* submit them to the list if the patch is ready to be accepted. You can also send it as a RFC if you are unsure. > Can deliver it also to the Mailinglist if we did potential optimization on it. > Will try that also for N2N connections if a little more time is there and of > course if you approved this one here. Please change the patch that it uses the new functions from network- functions.pl. Those are more robust and should be shorter. > A question: In which order should we proceed further since we have now 3 > patches for OpenVPN. > > 1) NCP --> > https://lists.ipfire.org/pipermail/development/2018-August/004667.html > > 2) Compression --> https://bugzilla.ipfire.org/show_bug.cgi?id=11819 (mostly > solved but with open question). > > 3) This one. What do you mean by order? Those are three independent changes, aren't they? > > Best, > > Erik > You are receiving this mail because: > You reported the bug. (In reply to Michael Tremer from comment #2) > On Fri, 2018-08-24 at 07:23 +0000, IPFire Bugzilla wrote: > > Comment # 1 on bug 11823 from Erik Kapfer > > Hi Michael, > > pushed a patch for this one which you can find in here --> > > https://git.ipfire.org/?p=people/ummeegge/ipfire-2.x.git;a=commit;h=a15ba89b5c7b286365be324632c2074e28df1baa > > . > > Yes, please *always* submit them to the list if the patch is ready to > be accepted. You can also send it as a RFC if you are unsure. OK, will do that then. > > > Can deliver it also to the Mailinglist if we did potential optimization on it. > > Will try that also for N2N connections if a little more time is there and of > > course if you approved this one here. > > Please change the patch that it uses the new functions from network- > functions.pl. Those are more robust and should be shorter. Therefore i would need a hint from you cause this one is new for me. 1) I think in ovpnmain.cgi would than be the need to require 'network-functions.pl' first ? Cause currently it isn´t. 2) How can i use it then in this patch ? Might be great if can leave me a hint for this. > > > A question: In which order should we proceed further since we have now 3 > > patches for OpenVPN. > > > > 1) NCP --> > > https://lists.ipfire.org/pipermail/development/2018-August/004667.html > > > > 2) Compression --> https://bugzilla.ipfire.org/show_bug.cgi?id=11819 (mostly > > solved but with open question). > > > > 3) This one. > > What do you mean by order? Those are three independent changes, aren't > they? I meant which one i should deliver first, second, etc. . > > > > > Best, > > > > Erik On Fri, 2018-08-24 at 11:32 +0000, IPFire Bugzilla wrote: > > 1) I think in ovpnmain.cgi would than be the need to require > 'network-functions.pl' first ? Cause currently it isn´t. You can do it like this: https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=html/cgi-bin/fwhosts.cgi;h=576701ac662f7b878b0ab6760b88e2f045be126b;hb=HEAD#l29 > 2) How can i use it then in this patch ? You can then call the functions as &Network::function_name(); > Might be great if can leave me a hint for this. Let me know if you have any further questions :) > What do you mean by order? Those are three independent changes, aren't they? > > I meant which one i should deliver first, second, etc. . What ever is easiest. We should start with the security vulnerability first. (In reply to Michael Tremer from comment #4) > On Fri, 2018-08-24 at 11:32 +0000, IPFire Bugzilla wrote: > > > > 1) I think in ovpnmain.cgi would than be the need to require > > 'network-functions.pl' first ? Cause currently it isn´t. > > You can do it like this: > > > https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=html/cgi-bin/fwhosts.cgi; > h=576701ac662f7b878b0ab6760b88e2f045be126b;hb=HEAD#l29 OK, done. > > > 2) How can i use it then in this patch ? > > You can then call the functions as &Network::function_name(); Great. > > > Might be great if can leave me a hint for this. > > Let me know if you have any further questions :) Yes there are :D. I haven´t found a similar function like 'ipcidr2msk' from genral-functions.pl in the 'network-functions.pl' . Possibly i have tomatoes on my eyes 8-|. Also the 'sub NextIP' function might be great for the N2N ifconfig but same there, can´t find a similar function in 'network-functions.pl' ??? > > > What do you mean by order? Those are three independent changes, > aren't they? > > > > I meant which one i should deliver first, second, etc. . > > What ever is easiest. We should start with the security vulnerability > first. The easiest way might be to merge the already delivered NCP patch, would pull then the changes and would deliver then the compression patch (Have seen you already wrote there, will answer you soon). Best, Erik On Fri, 2018-08-24 at 12:49 +0000, IPFire Bugzilla wrote: > > > Might be great if can leave me a hint for this. > > > > Let me know if you have any further questions :) > Yes there are :D. I haven´t found a similar function like 'ipcidr2msk' from > genral-functions.pl in the 'network-functions.pl' . Possibly i have tomatoes > on > my eyes 8-|. > Also the 'sub NextIP' function might be great for the N2N ifconfig but same > there, can´t find a similar function in 'network-functions.pl' ??? There is &Network::convert_netmask2prefix(). Does that one work? > > > What do you mean by order? Those are three independent changes, > > aren't they? > > > > > > I meant which one i should deliver first, second, etc. . > > > > What ever is easiest. We should start with the security vulnerability > > first. > The easiest way might be to merge the already delivered NCP patch, would pull > then the changes and would deliver then the compression patch (Have seen you > already wrote there, will answer you soon). I guess we will have to do a few more things to get the NCP patch ready. (In reply to Michael Tremer from comment #6) > On Fri, 2018-08-24 at 12:49 +0000, IPFire Bugzilla wrote: > > > > Might be great if can leave me a hint for this. > > > > > > Let me know if you have any further questions :) > > Yes there are :D. I haven´t found a similar function like 'ipcidr2msk' from > > genral-functions.pl in the 'network-functions.pl' . Possibly i have tomatoes > > on > > my eyes 8-|. > > Also the 'sub NextIP' function might be great for the N2N ifconfig but same > > there, can´t find a similar function in 'network-functions.pl' ??? > > There is &Network::convert_netmask2prefix(). Does that one work? Have tested this one already print CONF "dh ${General::swroot}/ovpn/ca/$cgiparams{'DH_NAME'}\n"; if ($sovpnsettings{'DOVPN_SUBNET'} ne '') { @temp = split(/\n/,$vpnsettings{'DOVPN_SUBNET'}); foreach (@temp) { #@tempovpnsubnet = split("\/",&General::ipcidr2msk($_)); @tempovpnsubnet = split("\/",&Network::convert_netmask2prefix($_)); print CONF "server $tempovpnsubnet[0] $tempovpnsubnet[1]\n"; } } but it delivers no output. There is only server in the line. Am currently some steps further to get also the possiblity for CIDR notation for N2N. I need there the NextIP function --> https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=config/cfgroot/general-functions.pl;h=0577afe2809e62b3de113f96b3819ab04a43d917;hb=HEAD#l749 for the transport net (ifconfig) which works great from general-functions but if i use the 'find_next_ip_address' sub from network-functions --> https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=config/cfgroot/network-functions.pl;h=2902aabb04ac649d1260054039c4b43534e7ff4e;hb=HEAD#l253 it do not works either which i don´t understand since as far as i can see general-functions NextIP linked his function to network-functions. In ovpnmain.cgi are two more time '&General::ipcidr2msk' in usage, will genral-functions be dropped sometimes ? > > > > > What do you mean by order? Those are three independent changes, > > > aren't they? > > > > > > > > I meant which one i should deliver first, second, etc. . > > > > > > What ever is easiest. We should start with the security vulnerability > > > first. > > The easiest way might be to merge the already delivered NCP patch, would pull > > then the changes and would deliver then the compression patch (Have seen you > > already wrote there, will answer you soon). > > I guess we will have to do a few more things to get the NCP patch ready. OK, no problem. On Mon, 2018-08-27 at 13:46 +0000, IPFire Bugzilla wrote: > Comment # 7 on bug 11823 from Erik Kapfer > (In reply to Michael Tremer from comment #6) > > On Fri, 2018-08-24 at 12:49 +0000, IPFire Bugzilla wrote: > > > > > Might be great if can leave me a hint for this. > > > > > > > > Let me know if you have any further questions :) > > > Yes there are :D. I haven´t found a similar function like 'ipcidr2msk' > from > > > genral-functions.pl in the 'network-functions.pl' . Possibly i have > tomatoes > > > on > > > my eyes 8-|. > > > Also the 'sub NextIP' function might be great for the N2N ifconfig but > same > > > there, can´t find a similar function in 'network-functions.pl' ??? > > > > There is &Network::convert_netmask2prefix(). Does that one work? > > Have tested this one already > > print CONF "dh ${General::swroot}/ovpn/ca/$cgiparams{'DH_NAME'}\n"; > if ($sovpnsettings{'DOVPN_SUBNET'} ne '') { > @temp = split(/\n/,$vpnsettings{'DOVPN_SUBNET'}); > foreach (@temp) > { > #@tempovpnsubnet = split("\/",&General::ipcidr2msk($_)); > @tempovpnsubnet = > split("\/",&Network::convert_netmask2prefix($_)); > print CONF "server $tempovpnsubnet[0] $tempovpnsubnet[1]\n"; > } > } > > > but it delivers no output. There is only > > server > > in the line. The function returns undefined in case of invalid input. The function also only takes the netmask and converts that into a prefix. I guess you want it the other way around here. > Am currently some steps further to get also the possiblity for CIDR notation > for N2N. I need there the NextIP function --> Where do you need this? > https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=config/cfgroot/general-functions.pl;h=0577afe2809e62b3de113f96b3819ab04a43d917;hb=HEAD#l749 > for the transport net (ifconfig) which works great from general-functions but > if i use the 'find_next_ip_address' sub from network-functions --> > https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=config/cfgroot/network-functions.pl;h=2902aabb04ac649d1260054039c4b43534e7ff4e;hb=HEAD#l253 > it do not works either which i don´t understand since as far as i can see > general-functions NextIP linked his function to network-functions. > > In ovpnmain.cgi are two more time '&General::ipcidr2msk' in usage, will > genral-functions be dropped sometimes ? Yes, the plan is to drop those functions. They don't behave very well and sometimes return wrong output. The &Network::* functions are cleaner, simpler, have unit-tests and therefore should be used. https://bugzilla.ipfire.org/show_bug.cgi?id=11393 > > > > > > > > What do you mean by order? Those are three independent changes, > > > > aren't they? > > > > > > > > > > I meant which one i should deliver first, second, etc. . > > > > > > > > What ever is easiest. We should start with the security vulnerability > > > > first. > > > The easiest way might be to merge the already delivered NCP patch, would > pull > > > then the changes and would deliver then the compression patch (Have seen > you > > > already wrote there, will answer you soon). > > > > I guess we will have to do a few more things to get the NCP patch ready. > > OK, no problem. > You are receiving this mail because: > You reported the bug. Hi Michael, (In reply to Michael Tremer from comment #8) > On Mon, 2018-08-27 at 13:46 +0000, IPFire Bugzilla wrote: > > Comment # 7 on bug 11823 from Erik Kapfer > > (In reply to Michael Tremer from comment #6) > > > On Fri, 2018-08-24 at 12:49 +0000, IPFire Bugzilla wrote: > > > > > > Might be great if can leave me a hint for this. > > > > > > > > > > Let me know if you have any further questions :) > > > > Yes there are :D. I haven´t found a similar function like 'ipcidr2msk' > > from > > > > genral-functions.pl in the 'network-functions.pl' . Possibly i have > > tomatoes > > > > on > > > > my eyes 8-|. > > > > Also the 'sub NextIP' function might be great for the N2N ifconfig but > > same > > > > there, can´t find a similar function in 'network-functions.pl' ??? > > > > > > There is &Network::convert_netmask2prefix(). Does that one work? > > > > Have tested this one already > > > > print CONF "dh ${General::swroot}/ovpn/ca/$cgiparams{'DH_NAME'}\n"; > > if ($sovpnsettings{'DOVPN_SUBNET'} ne '') { > > @temp = split(/\n/,$vpnsettings{'DOVPN_SUBNET'}); > > foreach (@temp) > > { > > #@tempovpnsubnet = split("\/",&General::ipcidr2msk($_)); > > @tempovpnsubnet = > > split("\/",&Network::convert_netmask2prefix($_)); > > print CONF "server $tempovpnsubnet[0] $tempovpnsubnet[1]\n"; > > } > > } > > > > > > but it delivers no output. There is only > > > > server > > > > in the line. > > The function returns undefined in case of invalid input. > > The function also only takes the netmask and converts that into a prefix. I > guess you want it the other way around here. Exactly. How should we proceed there further ? Simply pasting this function into network-functions.pl won´t be that nice i guess. Do you have there a snipped which we can use ? > > > Am currently some steps further to get also the possiblity for CIDR notation > > for N2N. I need there the NextIP function --> > > Where do you need this? Since the last octetts of the ifconfig line for the IP addresses of the N2N subnet are hardcoded --> https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=html/cgi-bin/ovpnmain.cgi;h=976300fc72e77dd3e23e26bca243dc61faaa4606;hb=HEAD#l947 the whole subnet can not be used nor other addresses except 1 and 2 can be configured even the WUI provides it. I was able to solve this via the NextIP function. May this one is not so important but in addition to the CIDR changes also for N2N i was working also on this. Best, Erik (In reply to Erik Kapfer from comment #9) > Exactly. How should we proceed there further ? Simply pasting this function > into network-functions.pl won´t be that nice i guess. Do you have there a > snipped which we can use ? Just split the input into net address and subnet mask and then only pass the subnet mask into the function. > Since the last octetts of the ifconfig line for the IP addresses of the N2N > subnet are hardcoded --> > https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=html/cgi-bin/ovpnmain.cgi; > h=976300fc72e77dd3e23e26bca243dc61faaa4606;hb=HEAD#l947 the whole subnet can > not be used nor other addresses except 1 and 2 can be configured even the > WUI provides it. > I was able to solve this via the NextIP function. Definitely! Hi Michael, (In reply to Michael Tremer from comment #10) > (In reply to Erik Kapfer from comment #9) > > Exactly. How should we proceed there further ? Simply pasting this function > > into network-functions.pl won´t be that nice i guess. Do you have there a > > snipped which we can use ? > > Just split the input into net address and subnet mask and then only pass the > subnet mask into the function. Did that in tha way now: # Ovpn subnet calculate cidr2ddn if ($sovpnsettings{'DOVPN_SUBNET'} ne '') { my ($ip,$subnet); ($ip,$subnet=split(/\//,$vpnsettings{'DOVPN_SUBNET'})); $subnet = &Network::convert_prefix2netmask($subnet); print CONF "server $ip $subnet\n"; } Address from WUI is '10.123.123.0/24' Output in server.conf was 'server 192.0.0.0' It seems that this function from network-functions.pl have some other plans or i really did something wrong there which i do not see/understand. > > > Since the last octetts of the ifconfig line for the IP addresses of the N2N > > subnet are hardcoded --> > > https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=html/cgi-bin/ovpnmain.cgi; > > h=976300fc72e77dd3e23e26bca243dc61faaa4606;hb=HEAD#l947 the whole subnet can > > not be used nor other addresses except 1 and 2 can be configured even the > > WUI provides it. > > I was able to solve this via the NextIP function. > > Definitely! Did some testings a while ago and there it looked good. Best, Erik (In reply to Erik Kapfer from comment #11) > # Ovpn subnet calculate cidr2ddn > if ($sovpnsettings{'DOVPN_SUBNET'} ne '') { > my ($ip,$subnet); > ($ip,$subnet=split(/\//,$vpnsettings{'DOVPN_SUBNET'})); > $subnet = &Network::convert_prefix2netmask($subnet); > print CONF "server $ip $subnet\n"; > } > > Address from WUI is '10.123.123.0/24' > Output in server.conf was 'server 192.0.0.0' That cannot be the function. $ip is not even passed through it. So I guess there is some other problem here. Maybe try to print the value of all variables. OK, did that now and find a solution with network-functions.pl but i needed two checks since i haven´t found a function in network-functions.pl which was equal to ipcidr2msk in general-functions --> https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=config/cfgroot/general-functions.pl;h=0577afe2809e62b3de113f96b3819ab04a43d917;hb=HEAD#l762 . First step was to check if the WUI input delivers a prefix (cidr) notated subnet, if yes, i could pass the cidr to convert_prefix2netmask function. If the input was already in DDN notation the patch will just substitute '/' with ' ' to get a server IP DDN_Subnetmask . Changes can be found in here --> https://git.ipfire.org/?p=people/ummeegge/ipfire-2.x.git;a=blobdiff;f=html/cgi-bin/ovpnmain.cgi;h=b3d9b9d26774f58bb1ed3385b4b2c7481da3a91a;hp=976300fc72e77dd3e23e26bca243dc61faaa4606;hb=80470706f0e1b5e00d77d99c8ee565fcd051ffc4;hpb=ef9cc2e5d59924bb06d67dab2921876acca389c3 . May a little complicated but currently the only way which i could find by the usage of network-functions.pl instead of general-functions.pl. Best, Erik Actually I quite like that version now. It is explicit. It is clear what is going on. You could now make this a bit shorter even by having the print line after the if statement and then you don't need else any more. That is then short and sweet :) Please submit to the mailing list. Do we have any other places where the same problem could happen? (In reply to Michael Tremer from comment #14) > Actually I quite like that version now. It is explicit. It is clear what is > going on. > > You could now make this a bit shorter even by having the print line after > the if statement and then you don't need else any more. > > That is then short and sweet :) Please submit to the mailing list. This didn´t worked here. The DDN output works properly but the CIDR converting doesn´t. In this case DDN worked: server 10.101.252.0 255.255.252.0 CIDR doesn´t: server 10.101.252.0 22 > > Do we have any other places where the same problem could happen? Not a problem as i know but we can teach the N2N to accept also CIDRs for remote and local subnet configuration (currently only DDN), N2N do checks the input so it is different to the RW one which was clearly a bug. Best, Erik Hi Michael, have found an unexpected behavior with this patch. Please do not merge this, need to invest this further. Thanks. Erik What is the status of this bug? Created attachment 965 [details]
Converts DDN or CIDR notation to OpenVPN configuration format
Patch converts DDN or CIDR format into OpenVPN RW server.conf format. Patch set also network ip if not already correctly defined to prevent "Options error: --server directive network/netmask combination is invalid" .
@Erik: Thank you for your reply and working on this. Shall I submit the patch for you to the mailing list? Hello Peter, and thanks for your support. It might be great if you can do so. Have found some other things which i will list shortly and will also add another patch which includes some more changes. 1) Have added another check since the OpenVPN subnet do not allows more than 65536 clients so the subnet must be 255.255.0.0 (/16) or higher otherwise the following error message appears "Options error: --server directive netmask allows for too many host addresses (subnet must be 255.255.0.0 (/16) or higher)" and the server won´t start. This is related to this code --> https://github.com/OpenVPN/openvpn/blob/05271322e7b5f453fe9d85310e500d3460ac8ca4/src/openvpn/pool.h#L32 . 2) Have added network-functions.pl as required in ovpnmain.cgi. This worked strangely enough also without but better to add it since it is used to convert_prefix2netmask . 3) en.pl and de.pl needed according to 2) one more entry to give the user a useful hint. This has been added in the VER. 2 patch in the attachment. A question arises. The OpenVPN subnet entry in '/var/ipfire/ovpn/settings' changes from e.g. 10.12.0.0/255.255.0.0 to 10.12.0.0/16 so from DDN to CIDR. Have tested the firewall and it should handle the 'new' notation but am currently not sure if it does in general ? Have also found that wio.cgi uses also '$ovpnsettings{'DOVPN_SUBNET'}' in here --> https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=src/wio/wio.cgi;hb=65d5ec52ce288bdffd9e989581e3b638dc948210#l1574 but as far as i can see should 'IpInSubnet' handle this as well. May you or the list have thoughts on this, also some testings from other sides might be great. Thanks again for your offer. Best, Erik Created attachment 966 [details] VER 2 - Converts DDN or CIDR notation to OpenVPN configuration format VER 2 - Converts DDN or CIDR notation to OpenVPN configuration format Patch converts DDN or CIDR format into OpenVPN RW server.conf format. Patch set also network ip if not already correctly defined to prevent "Options error: --server directive network/netmask combination is invalid" . Another check has been added since the OpenVPN subnet do not allows more than 65536 clients so the subnet must be 255.255.0.0 (/16) or higher otherwise the following error message appears "Options error: --server directive netmask allows for too many host addresses (subnet must be 255.255.0.0 (/16) or higher)" and the server won´t start. This is related to this code --> https://github.com/OpenVPN/openvpn/blob/05271322e7b5f453fe9d85310e500d3460ac8ca4/src/openvpn/pool.h#L32 . en.pl and de.pl needed according one more entry to give the user a useful hint if he exceed the client limit. |