search

nm-l2tp / NetworkManager-l2tp

L2TP and L2TP/IPsec support for NetworkManager
GNU General Public License v2.0
486 stars 84 forks source link

better error message when failing to append "include ipsec.d/ipsec.nm-l2tp.secrets" #108

Closed teto closed 5 years ago

teto commented 5 years ago

Would it be possible to improve the error message "Could not open ..." at https://github.com/nm-l2tp/NetworkManager-l2tp/blob/master/src/nm-l2tp-service.c#L664 Even though it's something I had already found out ~16 months ago when fixing l2tp vpns for nixos, I forgot about it and wondered why networkmanger VPN connection: failed to connect: 'Could not open /etc/ipsec.secrets' when it was world readable. The thing is on nixos, these files are read-only. A message hinting at the fact NM-l2tp wanted to open it with write access would have helped me debug much faster the issue (also with what it wants to write in it: "failed to append [include xxxx] to /etc/ipsec.secrets"). I can now work around it.

teto commented 5 years ago

my fix was to add the full path to ipsec.secrets with include "/etc/ipsec.d/ipsec.nm-l2tp.secrets" but this doesn't pass the test at https://github.com/nm-l2tp/NetworkManager-l2tp/blob/master/src/nm-l2tp-service.c#L225. I am not sure the test can ever be bulletproof but having more feedback in the log with "VERBOSE" level as to why files are opened with write access would be valuable. And some day I should write a nixos test for it ... xd

dkosovic commented 5 years ago

Sounds reasonable regarding the error message out, will try and commit something when I get home tonight. Adding g_strerror(errno) output to the error message might also make is clearer it is a permission issue.

According to the following page, if the included filename doesn't start with a /, the directory containing the current ipsec.secrets file is prepended :

So seems a bit odd that the include directive doesn't work with a relative path. Just curious, did strongstrong produce an error as to which file it was trying to open?

teto commented 5 years ago

Sry i was not clear, the restriction about absolute paths is not a strongswan but a purely artificial one introduced in the nixos module (i might try to revert that behavior). Yet it would be nice if the nm-l2tp check could allow for an absolute path.

dkosovic commented 5 years ago

Commit https://github.com/nm-l2tp/NetworkManager-l2tp/commit/d490ba25b4a795e29dc776d96a1b25dc673e2d9c has updated code for the error output.

I've added a nixos workaround for the has_include_ipsec_secrets() function with commit https://github.com/nm-l2tp/NetworkManager-l2tp/commit/0c9db4925ba97462d2476802459005e472009fd4.

teto commented 5 years ago

thanks for the quick fixes. I am trying to package the master version and noticed:

some of the error messages:

checking whether EAP-TLS patch has been applied to pppd... no
configure: error: EAP-TLS patch has not been applied to pppd

Package nss was not found in the pkg-config search path.
Perhaps you should add the directory containing `nss.pc'
to the PKG_CONFIG_PATH environment variable
No package 'nss' found
configure: error: No package 'nss' found

it's all fixed but you might want to mention it in NEWS (and remove the old ChangeLog or move it to news ?).

dkosovic commented 5 years ago

Does NixOS allow packages that violate the GPLv2 license like the 1.7.0-dev release? I guess if you are not distributing binaries it is okay.

I'll be sure to update the NEWS file once OpenSSL 3.0.0 is released and I make a 1.8.0 release. The 1.7.0-dev entry will be removed from the NEWS file based on the upstream GNOME project convention for odd number developer releases.

I added the check on eap-tls patch after realising not all Linux distributions include it (was initially only testing with Fedora and Debian and their derivatives). With 1.8.0 I'm planning on if the eap-tls patch is not detected, it will disable User certificate support in the GUI by greying out the option. I figured with the 1.7.0-dev release it was more important to explicitly check the patch has been applied and then fail if it hasn't.

The EAP-TLS patch, OpenSSL and NSS are mentioned in the README.md file and 1.7.0-dev release notes: https://github.com/nm-l2tp/NetworkManager-l2tp/releases/tag/1.7.0-dev

I'm not exactly sure why the empty ChangeLog file was kept, but think it was to silence a lint like check or test.

dkosovic commented 5 years ago

Actually with 1.8.0 I'll probably add a --without-eap-tls-patch configure switch option, as the default behaviour of failing when the EAP-TLS patch is not detected is good to keep in order highlight that the patch is missing.

I'll probably remove the ChangeLog file once I start packaging 1.8.0 for a number of linux distros, I need to squash a number of other bogus lint warnings. I might as well put an override for ChangeLog if needed at the same time.

More details as to why it isn't possible to combine OpenSSL with GPL code from the NetworkManager mailing list :

It'll be a non-issue once the GPL compatible Apache License v2 license OpenSSL 3.0.0 is released, if the code is linked against OpenSSL 3.0.0 or later.

teto commented 5 years ago

thanks for the help. PR is here https://github.com/NixOS/nixpkgs/pull/59250 but it won't get merged, waiting for stable release instead. Thanks for the great support once again