[vpn] Add association state to fix dialog timeout issue. Fixes JB#59447 #34
Conversation
|
It might be even more streamlined to utilize all the states also in VPNs. Now the CONNECT state in VPNd is |
LaakkonenJussi
force-pushed
the
jb59447
branch
from
ee6c201
to
f5fcb37
Compare
LaakkonenJussi
changed the title
LaakkonenJussi
force-pushed
the
jb59447
branch
3 times, most recently
from
fd9001a
to
b48f341
Compare
LaakkonenJussi
changed the title
|
See also sailfishos/libconnman-qt#12 |
Any suggestions for the new state name? Would the |
Waiting for user sort of implicitly answers "why no timeout". But if association is technically more correct, that could be handled via comments too. And as those are already in place... trust your instincts ;-) |
LaakkonenJussi
force-pushed
the
jb59447
branch
from
9747d35
to
15120f2
Compare
There was a problem hiding this comment.
This is a really nice set of changes, and worked very nicely too (I could reproduce the issue before the change, and it was fixed afterwards). I just experienced one glitch, if the association+connecting time exceeded 120 seconds (see my comments in the code).
LaakkonenJussi
force-pushed
the
jb59447
branch
2 times, most recently
from
3c4de7b
to
9c23560
Compare
[agent] Cancel agent request on NoReply D-Bus error. JB#59447 Handle also the NoReply D-Bus error as this is commonly sent back when the timeout set for the request is exceeded. Canceling the request later becomes impossible as agent->pending will be set to NULL in agent_finalize_pending(). Thus, making later calls to connman_agent_cancel() to not to close down agent dialogs but instead they are piled up on top of each other.
[vpn-provider] Use association state for VPN agent input wait. JB#59447
Use the association state with VPNs to define that VPN is waiting for
input via agent. The same state is used for every service in connmand so
this change synchronizes the states in both.
Set the state to be identical to connmand side states by injecting this
into the VPN state machine before the connect state ("configuration"
state). This is then changed when the state is set to connect either
by getting a non-error reply from VPN agent or via VPN driver gets
connect state notify.
In this is association state the VPN indicates to connmand that the VPN
is requesting user input via agent and shouldn't be subject to connect
timeout checks. Having this additional state allows to obey the D-Bus VPN
agent query timeout value, instead of getting the dialog shut down at
connection timeout.
[vpn] Add association state before connect state. JB#59447 This changes the state machine by adding the VPN_STATE_ASSOCIATION to be entered right after connect() callback is called. This is needed in order to properly react with the user input dialog waiting on VPNs. CONNECT state is now set when the dialog is closed to indicate that user input is given and now the VPN really connects. When VPN notify() allback is called the connect state is enforced if the return value indicates so and the internal state is different. This is to accommodate the changes required and to operate as a fallback that the states of provider and driver are kept in sync. Handle also association state in vpn_notify() where the state reported by plugin can only be transitioned back to association from connect state. The initial state for the plugins is association so in that case there is no state change done. If this state is notified back in any other state it is logged as a warning with the plugin that caused the state transition attempt.
[vpn-agent] Do connect state transition after input dialog check. JB#59447 When the VPN requests input (credentials) via the VPN agent the vpn_agent_check_and_process_reply_error() does transition the state of the VPN provider to connect state when there is no error. This is done to facilitate the transition from the association state to connect state as each VPN should use this function to verify the D-Bus reply and, thus will be called after each reply.
[vpn] Add support for association state, add state getter. JB#59447 Support VPN wait user input state as the association state. Add support for "State" string into the get_property() driver callback.
[vpn] Check if connecting when setting state or disconnecting Add checking of connected and connecting state in cases when the state is being set and state transitions to disconnecting. This change avoids clearing the transport ident when VPN is waiting for input from VPN agent (association state).
[service] Explicit VPN connect timeout, ignore in VPN agent wait. JB#59447 Ignore the connect timeout autostarting when connecting a VPN service because initially the VPN is in association state in which the VPN is waiting for the VPN agent. Separate the starting of connect timeout into its own function __connman_service_start_connect_timeout() so provider.c can call it when it enters configuration state. When a VPN is waiting for user input it should not be affected by connect timeout as the connection is not yet attempted. This may happen if VPN resumes to association state when requiring the VPN agent for other, e.g., encrypted private key input after credential input.
[provider] Handle VPN configuration and association states. JB#59447 Set the association state when VPN is waiting for user input as an initial state after connecting the provider. Set the configuration state (as it is declaced to be the string to connect state in VPN) accordingly as well. Start VPN connect timeout in configuration state with restart option to ensure that the timeout begins from the last known configuration (connect) state.
[doc] Update VPN documentation for association state. JB#59447 Add brief descriptions of the association state. Add it to parameter descriptions as well.
LaakkonenJussi
force-pushed
the
jb59447
branch
from
9c23560
to
668391b
Compare
There was a problem hiding this comment.
I re-tested and it worked nicely for me. I get indefinite time for credential input, then the full two minutes for the connection stage. As discussed elsewhere, this limits the key decryption step to two minutes, but this is still a big improvement on the previous version, so that seems fine to me. The code looks good too. So LGTM. Very nice changes.
I hope that it is not indefinite but the 300s that is the default for agents? This can be set also as configuration option for vpnd if you need to do a verfication: And restart connman-vpn (or both). |
I probably just didn't wait for long enough while testing. I'll double check now. |
|
Yes, my mistake, you're correct: the dialogue resets after five minutes, so working just as you describe (and still approved). |
This fixes the VPN dialog timeout happening too early. The cause for it was
that connmand did connection timeout for every type of service after 120s
interval. The configuration value InputRequestTimeout for vpnd in
connman-vpn.conf was not, therefore, followed and the setting had no effect.
In order to fix the issue VPN connections now have association state in which
the connection timeout of connmand is not in effect. This state is the new
initial state for VPN connections to which they enter after getting connect
request. In this state VPNs request the credentials of sorts from the VPN
agent, which in turn requests missing content from user. The input request
timeout set in the settings is therefore followed as the VPN is not really yet
connected to the server and is not subject to connection timeout.
Every other service in ConnMan already implements Association state and prior
to this there was a possible confusion with VPNs having CONNECT state as
configuration state in vpnd but in ConnMan the state of the service would have
been set to association. These changes make the states to be equal both in
connmand and vpnd.