[vpn] Connecting fixes, fix VPNC connect reply and improve logging. Fixes JB#55105 #36
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
LaakkonenJussi
force-pushed
the
jb55105
branch
from
5b6dded
to
b2402f4
Compare
LaakkonenJussi
changed the title
Thaodan
approved these changes
Dec 27, 2022
LaakkonenJussi
force-pushed
the
jb55105
branch
from
2b9ed38
to
f271df9
Compare
LaakkonenJussi
changed the title
LaakkonenJussi
force-pushed
the
jb55105
branch
2 times, most recently
from
d076c06
to
9d2ae82
Compare
[vpnc] Log unhandled errors. JB#55105 Add logging of unhandled errors to help in error resolving.
[vpnc] Ensure that vpnc_connect_done() is called at shutdown. JB#55105 The vpnc_connect_done() is called only once, at success or at failure. Adding this call makes sure that it will be called when the server returns invalid response, configuration is incompatible with the server or vpnc cannot be run on the system (e.g., vpnc is ran as regular user that has no permission to create raw sockets). By doing so the D-Bus LimitsExceeded can be avoided as every pending request, even in case of failures, will get a response.
[provider] Reply with EALREADY if connecting a VPN in progress. JB#55105 It is clearer to report back to the caller that the VPN is being already set to connect to indicate the clear difference between "connection set to in progress" vs. "we are already connecting this".
[service] Handle provider EALREADY replies if connecting a VPN. JB#55105 The provider.c now returns EALREADY when trying to connect a VPN that is already being set to connect. In case of VPN autoconnection this should be dealt similarly to EINPROGRESS as it indicates we could have a VPN left to connect as the result of connection is not yet known. Also cleanup the service/provider connect reply code handling. By reacting to EALREADY the connect_timeout() is being set only once for a connection attempt.
LaakkonenJussi
force-pushed
the
jb55105
branch
from
9d2ae82
to
1c37ff5
Compare
[vpnc] Use natt as the default NAT Traversal Mode. JB#55105
For example, on the man-pages of Debian the following is stated on
--natt-mode option:
--natt-mode <natt/none/force-natt/cisco-udp>
Which NAT-Traversal Method to use:
• natt -- NAT-T as defined in RFC3947
• none -- disable use of any NAT-T method
• force-natt -- always use NAT-T encapsulation even without presence of a NAT device (useful if the OS captures all ESP traffic)
• cisco-udp -- Cisco proprietary UDP encapsulation, commonly over Port 10000
Note: cisco-tcp encapsulation is not yet supported
Default: natt
conf-variable: NAT Traversal Mode <natt/none/force-natt/cisco-udp>
Thus, the default here should be also "natt". This avoids creating
IPPROTO_ESP socket that is a raw socket and cannot be used as a regular
user.
LaakkonenJussi
force-pushed
the
jb55105
branch
from
89df2af
to
d27c8f9
Compare
|
Got quite a bit more stuff since my earlier approve. Not that familiar with the details and don't have vpnc to test with, but can't say there's anything wrong either so guess this is still approved from my side. |
llewelld
reviewed
Jan 12, 2023
There was a problem hiding this comment.
The changes look sensible, just one point that I wasn't sure about.
I'm also not able to test this unfortunately. Are there any publicly accessible VPNC servers I could try with? Otherwise, it looks sensible enough, I would be comfortable to approve it anyway.
llewelld
approved these changes
Jan 18, 2023
LaakkonenJussi
changed the title
LaakkonenJussi
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add logging of unhandled errors with VPNC to help in error resolving.
Fix the replying to the connect request when VPNC connection abruptly fails
because of server/client misconfiguration. This avoids the D-Bus LimitsExceeded
error when VPNC keeps trying and failing by always replying to the connection
request.
Use natt as default NAT Traversal mode. This is the default according to man
pages.
Use EALREADY in provider.c connect reply for VPNs that were already connecting.
This helps in differentiate between the two cases: VPN was just connected or
has been trying to connect and waiting for vpnd reply. Also connect_timeout()
is set only once per connection attempt.