Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

luci-app-libreswan: Add LuCI for Libreswan #5932

Closed
wants to merge 1 commit into from

Conversation

jempatel
Copy link
Contributor

@jempatel jempatel commented Aug 14, 2022

A new app for Libreswan IPSec with LuCI Support.

We also need openwrt/packages#19079
We also need openwrt/packages#19233

Screenshots from the new Luci App Libreswan:

Overview:
OpenWrt-Overview-LuCI

Globals:
OpenWrt-IPSec-Globals-LuCI

Proposals:
OpenWrt-IPSec-Proposals-LuCI

Proposals Edit:
OpenWrt-IPSec-Proposals-Edit-LuCI

Tunnels:
OpenWrt-IPSec-Tunnels-LuCI

Tunnels (General):
OpenWrt-IPSec-Tunnels-General-LuCI

Tunnels (Authentication):
OpenWrt-IPSec-Tunnels-Authentication-LuCI

Tunnels (Interface):
OpenWrt-IPSec-Tunnels-Interface-LuCI

Tunnels (Advanced):
OpenWrt-IPSec-Tunnels-Advanced-LuCI

@jempatel jempatel force-pushed the luci-app-libreswan branch 3 times, most recently from 1c0359e to 96d1dca Compare August 17, 2022 16:47
@jempatel jempatel force-pushed the luci-app-libreswan branch 4 times, most recently from f51d3b1 to 4e851a5 Compare August 31, 2022 09:38
@systemcrash
Copy link
Contributor

Looks good, logical. Problems with 0.0.0.0/0 masks - IPv4 only. The GUI assumes that the necessary kernel modules are installed (and loaded). Have you tested in the absence of a (configured for use) crypto module?

It's a start. Handling certs would be a good improvement, but increases complexity, ofc.

@jempatel
Copy link
Contributor Author

jempatel commented Sep 3, 2022

Looks good, logical. Problems with 0.0.0.0/0 masks - IPv4 only. The GUI assumes that the necessary kernel modules are installed (and loaded). Have you tested in the absence of a (configured for use) crypto module?

It's a start. Handling certs would be a good improvement, but increases complexity, ofc.

Did not get you with "Problems with 0.0.0.0/0 masks - IPv4 only",

  • Remote/Local Subnets are allowed with datatype ipaddr, so it should allow ipv4 and ipv6 both.

The GUI assumes that the necessary kernel modules are installed (and loaded).

  • App is installed with LUCI_DEPENDS libreswan and libreswan has all module dependencies handled, I also have checked other apps, but could not find any app that might be checking for the module is loaded or not, Can you pls point me any reference of what you meant here?

@systemcrash
Copy link
Contributor

Just a thought, but given that libreswan and strongswan are virtually identical, can this GUI be dual purpose and used for both? I actually don't know what needs to be done in the package descriptions and/or permissions files, but maybe @jow has a tip here.

@lucize
Copy link
Contributor

lucize commented Dec 25, 2022

wanted to test with latest, does it need some updates ?

image

@jempatel
Copy link
Contributor Author

wanted to test with latest, does it need some updates ?

image

I have rebased the current branch with the latest master and Its working fine at my end. Can you pls check again?

@lucize
Copy link
Contributor

lucize commented Jan 27, 2023

so I used the today master and still the same error on this page, I also cleared the config file and it's the same for me

image

@jempatel
Copy link
Contributor Author

jempatel commented Jan 28, 2023

so I used the today master and still the same error on this page, I also cleared the config file and it's the same for me

image

I got it, there was actually a syntax error, and the fix is pushed. Weird, It was not reporting in private/incognito mode at my end.

@lucize
Copy link
Contributor

lucize commented Jan 29, 2023

great stuff, it works

image

Sun Jan 29 16:08:44 2023 authpriv.warn pluto[8684]: "forti/1x1" #2: initiator established Child SA using #1; IPsec tunnel [0.0.0.0-255.255.255.255:0-65535 0] -> [0.0.0.0-255.255.255.255:0-65535 0] {ESPinUDP=>0x6b4ab02b <0xf4081b5d xfrm=AES_CBC_256-HMAC_SHA2_256_128 NATD=x.x.x.x:4500 DPD=active}

@lucize
Copy link
Contributor

lucize commented Jan 29, 2023

Tested-by: Lucian [email protected]

o.optional = false;

o = s.taboption('authentication', form.Value, 'psk', _('Preshare Key'));
o.datatype = 'and(string, minlenght(8))'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minlength(8)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected


o = s.taboption('authentication', form.ListValue, 'authby', _('Auth Method'));
o.default = 'secret'
o.value('secret', 'Preshare Key');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preshared Key

Also, please add never and null types. Useful to some.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

o.value('sha512', _('SHA512'));

o = s.taboption('general', form.MultiValue, 'encryption_algorithm', _('Encryption Method'));
o.default = '3des';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change this default, and mark 3des with a *, and a GUI note explaining it as unsafe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

o = s.taboption('general', form.MultiValue, 'encryption_algorithm', _('Encryption Method'));
o.default = '3des';
o.value('3des', _('3DES'))
o.value('aes', _('AES'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mark AES as default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated


o = s.taboption('general', form.MultiValue, 'hash_algorithm', _('Hash Algorithm'));
o.default = 'md5';
o.value('md5', _('MD5'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mark this hash with a *:
o.value('md5', _('MD5*'));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

o = s.taboption('general', form.MultiValue, 'hash_algorithm', _('Hash Algorithm'));
o.default = 'md5';
o.value('md5', _('MD5'));
o.value('sha1', _('SHA1'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mark this hash with a *:
o.value('sha1', _('SHA1*'));

  • Move to a better, more collision resistant hash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated


o = s.taboption('advanced', form.Value, 'ikelifetime', _('IKE Life Time'));
o.datatype = 'uinteger';
o.default = 10800;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add multiple values to choose from (and hint in readable text the amount of hours it represents), and permit the user to enter their own values.

If the current libreswan version is >=4.2, then the default is 8h.

I do not think that this should be an unsigned integer...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestions: 1h, 2h, 4h, 8h, 12h, 16, 24h, custom...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

o.modalonly = true;
o.optional = false;

o = s.taboption('authentication', form.Value, 'psk', _('Preshare Key'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a depends where authentication = secret - future iterations may add other auth methods.

for (var i = 0; i < netDevs.length; i++) {
var addrs = netDevs[i].getIPAddrs();
for (var j = 0; j < addrs.length; j++) {
var subnet = calculateNetwork(addrs[j].split('/')[0], addrs[j].split('/')[1]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested what happens here with an IPv6?

Copy link
Contributor Author

@jempatel jempatel Oct 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This app version does not support IPv6, getIPAddrs only returns IPv4 addresses.

        /**                                                                          
         * Query all IPv4 addresses of the logical interface.                        
         *                                                                           
         * @returns {string[]}                                                      
         * Returns an array of IPv4 addresses in CIDR notation which have been
         * registered by the protocol handler. The order of the resulting array
         * follows the order of the addresses in `ubus` runtime information.
         */
        getIPAddrs: function() {

@zcracker
Copy link

zcracker commented Jun 5, 2023

Why can't it be merged, this app is very much needed

@sjkhsl
Copy link

sjkhsl commented Jun 13, 2023

Error after latest main build

RPCError
RPC call to uci/get failed with ubus code 4: Resource not found
  at ClassConstructor.handleCallReply (https://192.168.79.128/luci-static/resources/rpc.js?v=git-23.156.69953-fa775ee:15:3)

@lucize
Copy link
Contributor

lucize commented Oct 2, 2023

tested it today on master and I don't have that issue, all seems to work

@systemcrash
Copy link
Contributor

@jempatel can you address my comments, and let me know when you're done.

@jempatel
Copy link
Contributor Author

@jempatel can you address my comments, and let me know when you're done?

Sure, Let me address the comments and test changes.

@systemcrash
Copy link
Contributor

Sure, Let me address the comments and test changes.

If you did something, nothing has changed here...

@jempatel
Copy link
Contributor Author

Sure, Let me address the comments and test changes.

If you did something, nothing has changed here...

I've recently rebased the master branches of both luci and packages feeds in order to synchronize all the latest changes. Now I am testing my local changes with fresh firmware build and separate package installation on installed firmware. Once the testing confirms that everything is functioning correctly, I will re-request for review.

@jempatel
Copy link
Contributor Author

This PR depends on
openwrt/packages#19233

@jempatel
Copy link
Contributor Author

Sure, Let me address the comments and test changes.

If you did something, nothing has changed here...

@systemcrash all the comments are addressed and PR in packages is also merged now. If everything is fine, we can merge this as well

@systemcrash
Copy link
Contributor

Almost, you have the right values there, but they should be made translation (i18n) friendly.

e.g.

o.value('secret', 'Shared Secret');

should be

o.value('secret', _('Shared Secret'));

o.value('sha384', _('SHA384'));
o.value('sha512', _('SHA512'));

o = s.taboption('general', form.MultiValue, 'encryption_algorithm', _('Encryption Method'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update to e.g.:

		o = s.taboption('general', form.MultiValue, 'encryption_algorithm', _('Encryption Method'), _('* = unsafe'));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated


o = s.taboption('general', form.MultiValue, 'encryption_algorithm', _('Encryption Method'));
o.default = 'aes';
o.value('3des', _('3DES* [Unsafe]'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just 3DES*

Copy link
Contributor

@systemcrash systemcrash Nov 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With ofc

		o = s.taboption('general', form.MultiValue, 'encryption_algorithm', _('Encryption Method'), _('* = unsafe'));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

o.value('aes256', _('AES256'));
o.value('camellia_cbc', _('CAMELLIA_CBC'));

o = s.taboption('general', form.MultiValue, 'dh_group', _('DH Group'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comment _('* = unsafe. See <a href="%s">RFC8247</a>.').format('https://www.rfc-editor.org/rfc/rfc8247#section-2.4')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

o.value('dh19', _('DH Group 19'));
o.value('dh20', _('DH Group 20'));
o.value('dh21', _('DH Group 21'));
o.value('dh22', _('DH Group 22'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mark dh22 as DH Group 22*

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated


o = s.taboption('general', form.MultiValue, 'dh_group', _('DH Group'));
o.default = 'modp1536';
o.value('modp1536', _('DH Group 5'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mark with *

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with ofc

		o = s.taboption('general', form.MultiValue, 'dh_group', _('DH Group'), _('* = unsafe'));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

o.default = '9m';
o.value('5m', '5m');
o.value('9m', '9m');
o.value('15m', '15h');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 🧐

@systemcrash
Copy link
Contributor

@jempatel please fix these most recent additions

@jempatel
Copy link
Contributor Author

jempatel commented Nov 9, 2023

@jempatel please fix these most recent additions

Done

@jempatel jempatel requested a review from systemcrash November 9, 2023 06:41
o.value('camellia_cbc', _('CAMELLIA_CBC'));

o = s.taboption('general', form.MultiValue, 'dh_group', _('DH Group'),
('* = %s, See <a href="%s">RFC8247</a>.').format(_('Unsafe'), 'https://www.rfc-editor.org/rfc/rfc8247#section-2.4'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be sure that translatable sections start with _.

Copy link
Contributor Author

@jempatel jempatel Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its already handled inside the format function except string "See"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps just put See also in with Unsafe, e.g. _('Unsafe, See')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

LuCI Support for IPSec VPN (Libreswan)

Signed-off-by: Jaymin Patel <[email protected]>
@systemcrash
Copy link
Contributor

Ok good. @jow- @hnyman any other reviewers? I'm largely satisfied.

@jow-
Copy link
Contributor

jow- commented Nov 10, 2023

I held back merging this as the underlying packages PR were open for a long time, but it seems they finally got merged last week, so let's go ahead! We can still continue polishing this in subsequent PRs if the need arises.

@jempatel - thanks a lot for your effort and patience.

@jow-
Copy link
Contributor

jow- commented Nov 10, 2023

Merged via 9f65244

@jow- jow- closed this Nov 10, 2023
@systemcrash
Copy link
Contributor

@jempatel looks like you missed the 15m -> 15h thing I flagged in the review.

@jempatel
Copy link
Contributor Author

@jempatel looks like you missed the 15m -> 15h thing I flagged in the review.

Ahh, Yup I missed that. Looks like someone had already updated and merged that to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants