Send connman mailing list submissions to
[email protected]
To subscribe or unsubscribe via email, send a message with subject or
body 'help' to
[email protected]
You can reach the person managing the list at
[email protected]
When replying, please edit your Subject line so it is more specific
than "Re: Contents of connman digest..."
Today's Topics:
1. Re: [PATCH 03/10] doc: Document VPN agent credential storage/retrieval
options
(Daniel Wagner)
2. Re: [PATCH 09/10] openvpn: Rewrite plugin to support VPN agent and
encrypted private keys
(Daniel Wagner)
3. Re: [PATCH 00/10] Rewrite of OpenVPN plugin, VPN agent additions and VPN
provider fixes
(Daniel Wagner)
4. Re: [PATCH 03/10] doc: Document VPN agent credential storage/retrieval
options
(Jussi Laakkonen)
5. Re: [PATCH 09/10] openvpn: Rewrite plugin to support VPN agent and
encrypted private keys
(Jussi Laakkonen)
----------------------------------------------------------------------
Date: Tue, 12 Nov 2019 09:05:10 +0100
From: Daniel Wagner <[email protected]>
Subject: Re: [PATCH 03/10] doc: Document VPN agent credential
storage/retrieval options
To: Jussi Laakkonen <[email protected]>
Cc: [email protected], David Llewellyn-Jones
<[email protected]>
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
Hi,
On Mon, Nov 11, 2019 at 04:01:47PM +0200, Jussi Laakkonen wrote:
> + boolean AllowStoreCredentials
> +
> + Indicates to the receiving UI whether the values
> + entered by the user can be stored for future use.
> + "Requirement" should be set to "control". A "Value"
> + of true indicates that the option to store the
> + credentials can be offered to the user, false
> + indicates that no such option should be presented.
> +
> + boolean AllowRetrieveCredentials
> +
> + Tells the receiving UI whether to attempt to retrieve
> + previously stored values. "Requirement" should be set
> + to "control". "Value" should be set to true if
> + previously stored values can be used, false otherwise.
> +
Maybe add some additiional information on the use case it in
vpn-overview.txt? I am sure there will be questions on how to use it,
or what the expected behavior of the UI should be.
> string VpnAgent.AuthFailure
>
> Informational field that can be used to indicate VPN
> @@ -122,8 +138,8 @@ Arguments string Type
> string Requirement
>
> Contains the requirement option. Valid values are
> - "mandatory", "optional", "alternate" or
> - "informational".
> + "mandatory", "optional", "alternate", "informational"
> + and "control".
>
> The "alternate" value specifies that this field can be
> returned as an alternative to another one.
> @@ -135,6 +151,11 @@ Arguments string Type
> is here only to provide an information so a value is
> attached to it.
>
> + A "control" argument is used to specify behaviour. The
> + effect will depend on the field name and value, but
> + control fields will not usually be presented directly
> + to the user, and are not expected to be returned.
> +
> array{string} Alternates
>
> Contains the list of alternate field names this
> @@ -174,3 +195,19 @@ Examples Requesting a username and password for L2TP
> network
> "Requirement" : "informational"
> } }
> ==> { "OpenConnect.Cookie" : "0123456@adfsf@asasdf" }
> +
> + Requesting a username and password but without allowing
> + the values entered by the user to be stored.
> +
> + RequestInput("/vpn2",
> + { "Username" : { "Type" : "string",
> + "Requirement" : "mandatory"
> + } }
> + { "Password" : { "Type" : "password",
> + "Requirement" : "mandatory"
> + } }
> + { "AllowStoreCredentials" : { "Type" :
> "boolean",
> + "Requirement" : "control",
> + "Value": false
> + } }
Just a nitpick, indent the "Value" to the same level as
"Requirement". Initially, I read it as nested value.
Thanks,
Daniel
------------------------------
Date: Tue, 12 Nov 2019 09:39:13 +0100
From: Daniel Wagner <[email protected]>
Subject: Re: [PATCH 09/10] openvpn: Rewrite plugin to support VPN
agent and encrypted private keys
To: Jussi Laakkonen <[email protected]>
Cc: [email protected], Matt Vogt <[email protected]>
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
Hi Jussi,
On Mon, Nov 11, 2019 at 04:01:53PM +0200, Jussi Laakkonen wrote:
> Co-authored-by: Matt Vogt <[email protected]>
> Co-authored-by: Slava Monich <[email protected]>
>
> This OpenVPN plugin rewrite contains numerous amount of fixes. Most
> importantly VPN agent is used to query credentials as well as the
> password for the encrypted private key.
>
> VPN agent support is done utilizing the management interface of OpenVPN.
> The management interface is opened at each connection attempt to get
> the potential requests for credentials, or encrypted private key
> password. OpenVPN process is started with the stored information and if
> there is some credential missing it will be queried via management
> interface.
>
> Each credential failure increases the authentication failed error
> counter in vpn-provider.c but does not indicate it as an error to be
> signaled. This is because the authentication failures are handled within
> the plugin->openvpn process and the openvpn process does not die in
> between. In case the credentials or the private key password is wrong
> OpenVPN requests them again via management channel. If the error would
> be signaled, connmand would have wrong indication of what is actually
> happening and would attempt to disconnect the VPN in question.
>
> The new VPN agent functionality is utilized to advise the VPN agent not
> to store the encrypted private key password. Encrypted private key
> password is kept in memory only, during the connman-vpnd lifetime. On
> some systems VPN agents may store the credentials into files and, thus
> it is imperative to not to save the encrypted private key password using
> the VPN agent as it is bad practice to have both encrypted file and its
> password stored on same storage space. Use of the
> vpn_agent_append_keep_credentials() is also needed to indicate VPN agent
> that the credentials should not be affected by the request to input
> encrypted private key password. It may be that some VPN agents would
> react to the storage and retrieval prevention values as the existing
> values should be removed.
>
> The private key password errors are not recorded as authentication
> errors but are handled internally within the plugin. The rationale is
> that since VPN agent is affected by the authentication errors and the
> VpnAgent.AuthFailure is sent in such case, and VPN agent is advised not
> to store the private key password, handling of the errors related to
> private key password should happen within the plugin. If the private key
> password stored in memory is wrong, it will be still attempted on first
> try but OpenVPN will requests new one via management interface after a
> failed attempt. The encrypted private key password failures are not
> reported by OpenVPN via management interface and following patch is
> required in order for the failures to be reported:
> https://sourceforge.net/p/openvpn/mailman/message/36789543/
This link will go stale. Would the plugin still work if the OpenVPN
client doesn't have this patch? Or could it made at least not hard
depending on it?
And now some nitpicks :)
> +static void ov_quote_credential(GString *line, const char *cred)
> +{
> + if (!line)
> + return;
> +
> + g_string_append_c(line, '"');
> +
> + while (*cred != '\0') {
> +
> + switch (*cred) {
> + case ' ':
> + case '"':
> + case '\\':
> + g_string_append_c(line, '\\');
> + break;
> + default:
> + break;
> + }
> +
> + g_string_append_c(line, *cred++);
> + }
> +
> + g_string_append_c(line, '"');
> +
> + return;
> +}
IIRC, newer gcc will warn about the 'return' here.
> +static int request_private_key_input(struct ov_private_data *data)
> +{
> + DBusMessage *message;
> + const char *path, *agent_sender, *agent_path;
> + const char *privatekeypass;
> + DBusMessageIter iter;
> + DBusMessageIter dict;
> + int err;
> + void *agent;
> +
> + /*
> + * First check if this is the second attempt to get the key within
> + * this connection. In such case there has been invalid Private Key
> + * Password and it must be reset, and queried from user.
> + */
> + if (data->failed_attempts_privatekey) {
> + vpn_provider_set_string_hide_value(data->provider,
> + "OpenVPN.PrivateKeyPassword", NULL);
> + } else {
> + /* If the encrypted Private key password is kept in memory and
> + * use it first. If authentication fails this is cleared,
> + * likewise it is when connman-vpnd is restarted.
> + */
> + privatekeypass = vpn_provider_get_string(data->provider,
> + "OpenVPN.PrivateKeyPassword");
> + if (privatekeypass) {
> + ov_return_private_key_password(data, privatekeypass);
> + goto out;
> + }
> + }
> +
> + agent = connman_agent_get_info(data->dbus_sender, &agent_sender,
> + &agent_path);
> + if (!agent || !agent_path)
> + return -ESRCH;
> +
> + message = dbus_message_new_method_call(agent_sender, agent_path,
> + VPN_AGENT_INTERFACE,
> + "RequestInput");
> + if (!message)
> + return -ENOMEM;
> +
> + dbus_message_iter_init_append(message, &iter);
> +
> + path = vpn_provider_get_path(data->provider);
> + dbus_message_iter_append_basic(&iter,
> + DBUS_TYPE_OBJECT_PATH, &path);
> +
> + connman_dbus_dict_open(&iter, &dict);
> +
> + connman_dbus_dict_append_dict(&dict, "OpenVPN.PrivateKeyPassword",
> + request_input_append_password, NULL);
> +
> + vpn_agent_append_host_and_name(&dict, data->provider);
> +
> + /* Do not allow to store or retrieve the encrypted Private Key pass */
> + vpn_agent_append_allow_credential_storage(&dict, false);
> + vpn_agent_append_allow_credential_retrieval(&dict, false);
> +
> + /*
> + * Indicate to keep credentials, the enc Private Key password should
> + * not affect the credential storing.
> + */
> + vpn_agent_append_keep_credentials(&dict, true);
> +
> + connman_dbus_dict_append_dict(&dict, "Enter Private Key password",
> + request_input_append_informational, NULL);
> +
> + connman_dbus_dict_close(&iter, &dict);
> +
> + err = connman_agent_queue_message(data->provider, message,
> + connman_timeout_input_request(),
> + request_input_private_key_reply, data, agent);
> +
> + if (err < 0 && err != -EBUSY) {
> + connman_error("error %d sending agent request", err);
> + dbus_message_unref(message);
> +
> + return err;
> + }
> +
> + dbus_message_unref(message);
> +
> +out:
> + return -EINPROGRESS;
> +}
> +
> +
One empty line too much.
> +static gboolean ov_management_handle_input(GIOChannel *source,
> + GIOCondition condition, gpointer user_data)
> +{
> + struct ov_private_data *data = user_data;
> + char *str = NULL;
> + int err = 0;
> + gboolean close = FALSE;
Just use plain normal bools, not the ones from Glib if the we don't have to.
> +
> + if ((condition & G_IO_IN) &&
> + g_io_channel_read_line(source, &str, NULL, NULL, NULL) ==
> + G_IO_STATUS_NORMAL) {
Hmm, I would prefer to have the g_io_channel_read_line() not part of
the if condition. Couldn't this function be organized slightly
different? In many places we use a 'goto err' to untangle this kind of
code. Just an idea.
> + str[strlen(str) - 1] = '\0';
> + connman_warn("openvpn request %s", str);
> +
> + if (g_str_has_prefix(str, ">PASSWORD:Need 'Auth'")) {
> + /*
> + * Request credentials from the user
> + */
> + err = request_credentials_input(data);
> + if (err != -EINPROGRESS)
> + close = TRUE;
> + } else if (g_str_has_prefix(str,
> + ">PASSWORD:Need 'Private Key'")) {
> + err = request_private_key_input(data);
> + if (err != -EINPROGRESS)
> + close = TRUE;
> + } else if (g_str_has_prefix(str,
> + ">PASSWORD:Verification Failed: 'Auth'")) {
> + /*
> + * Add error only, state change indication causes
> + * signal to be sent, which is not desired when
> + * OpenVPN is in interactive mode.
> + */
> + vpn_provider_add_error(data->provider,
> + VPN_PROVIDER_ERROR_AUTH_FAILED);
> + /*
> + * According to the OpenVPN manual about management interface
> + * https://openvpn.net/community-resources/management-interface/
> + * this should be received but it does not seem to be reported
> + * when decrypting private key fails. This requires a patch
> + * sent to upstream in order to work:
> + * https://sourceforge.net/p/openvpn/mailman/message/36789543/
> + */
This information will go stale. Maybe add this to our TODO file?
> + } else if (g_str_has_prefix(str, ">PASSWORD:Verification "
> + "Failed: 'Private Key'")) {
> + ++data->failed_attempts_privatekey;
data->failed_attempts_privatekey++;
> + }
> +
> + g_free(str);
> + } else if (condition & (G_IO_ERR | G_IO_HUP)) {
> + connman_warn("Management channel termination");
> + close = TRUE;
> + }
> +
> + if (close) {
> + close_management_interface(data);
> + }
Brackets not really needed.
> +
> + return TRUE;
> +}
> +
> +static int ov_management_connect_timer_cb(gpointer user_data)
> +{
> + struct ov_private_data *data = user_data;
> +
> + if (!data->mgmt_channel) {
> + int fd = socket(AF_UNIX, SOCK_STREAM, 0);
> + if (fd >= 0) {
> + struct sockaddr_un remote;
> + int err;
> +
> + memset(&remote, 0, sizeof(remote));
> + remote.sun_family = AF_UNIX;
> + g_strlcpy(remote.sun_path, data->mgmt_path,
> + sizeof(remote.sun_path));
> +
> + err = connect(fd, (struct sockaddr *)&remote,
> + sizeof(remote));
> + if (err == 0) {
> + data->mgmt_channel = g_io_channel_unix_new(fd);
> + data->mgmt_event_id =
> + g_io_add_watch(data->mgmt_channel,
> + G_IO_IN | G_IO_ERR | G_IO_HUP,
> + ov_management_handle_input,
> + data);
> +
> + connman_warn("Connected management socket");
> + data->mgmt_timer_id = 0;
> + return G_SOURCE_REMOVE;
> + }
> + close(fd);
> + }
> + }
> +
> + ++data->connect_attempts;
data->connect_attemps++;
> + if (data->connect_attempts > 30) {
> + connman_warn("Unable to connect management socket");
> + data->mgmt_timer_id = 0;
> + return G_SOURCE_REMOVE;
> }
>
> - err = setup_log_read(stdout_fd, stderr_fd);
> -done:
> - if (cb)
> - cb(provider, user_data, err);
> + return G_SOURCE_CONTINUE;
> +}
> +
> +static int ov_connect(struct vpn_provider *provider,
> + struct connman_task *task, const char *if_name,
> + vpn_provider_connect_cb_t cb, const char *dbus_sender,
> + void *user_data)
> +{
> + struct ov_private_data *data;
> +
> + data = g_try_new0(struct ov_private_data, 1);
> + if (!data)
> + return -ENOMEM;
> +
> + vpn_provider_set_plugin_data(provider, data);
> + data->provider = vpn_provider_ref(provider);
> + data->task = task;
> + data->dbus_sender = g_strdup(dbus_sender);
> + data->if_name = g_strdup(if_name);
> + data->cb = cb;
> + data->user_data = user_data;
> +
> + /*
> + * We need to use the management interface to provide
> + * the user credentials and password for decrypting private key.
> + */
> +
> + /* Set up the path for the management interface */
> + data->mgmt_path = g_strconcat("/tmp/connman-vpn-management-",
> + vpn_provider_get_ident(provider), NULL);
I don't think we should hardcode the /tmp path.
> + if (unlink(data->mgmt_path) != 0 && errno != ENOENT) {
> + connman_warn("Unable to unlink management socket %s: %d",
> + data->mgmt_path, errno);
> + }
> +
> + data->mgmt_timer_id = g_timeout_add(200,
> + ov_management_connect_timer_cb, data);
> +
> + task_append_config_data(provider, task);
> +
> + return run_connect(data, cb, user_data);
> +}
> +
> +static void ov_disconnect(struct vpn_provider *provider)
> +{
> + if (!provider)
> + return;
>
> - return err;
> + connman_agent_cancel(provider);
> }
>
> static int ov_device_flags(struct vpn_provider *provider)
> @@ -459,14 +1120,16 @@ static int ov_device_flags(struct vpn_provider
> *provider)
> }
>
> if (!g_str_equal(option, "tun")) {
> - connman_warn("bad OpenVPN.DeviceType value, falling back to
> tun");
> + connman_warn("bad OpenVPN.DeviceType value "
> + "falling back to tun");
> }
>
> return IFF_TUN;
> }
>
> static int ov_route_env_parse(struct vpn_provider *provider, const char *key,
> - int *family, unsigned long *idx, enum vpn_provider_route_type
> *type)
> + int *family, unsigned long *idx,
> + enum vpn_provider_route_type *type)
> {
> char *end;
> const char *start;
> @@ -492,6 +1155,7 @@ static int ov_route_env_parse(struct vpn_provider
> *provider, const char *key,
> static struct vpn_driver vpn_driver = {
> .notify = ov_notify,
> .connect = ov_connect,
> + .disconnect = ov_disconnect,
> .save = ov_save,
> .device_flags = ov_device_flags,
> .route_env_parse = ov_route_env_parse,
Overall, this is looks pretty good. I know understand your comment
about to much infrastructure code in the plugin. Indeed moving some of
this code into a library so other plugins can reuse it makes absolutely
sense.
Thanks,
Daniel
------------------------------
Date: Tue, 12 Nov 2019 09:42:57 +0100
From: Daniel Wagner <[email protected]>
Subject: Re: [PATCH 00/10] Rewrite of OpenVPN plugin, VPN agent
additions and VPN provider fixes
To: Jussi Laakkonen <[email protected]>
Cc: [email protected]
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
Hi Jussi,
On Mon, Nov 11, 2019 at 04:01:44PM +0200, Jussi Laakkonen wrote:
> This contains changes to three different components necessary for the OpenVPN
> plugin rewrite. This work is a combined effort of 4 authors.
>
> First, VPN agent is amended with three boolean control values in order to
> allow controlling of credential storing, retrieval and whether to keep the old
> credentials or not. The last one is meant to be used in situations where the
> second credential request is to be done and the second credentials are not to
> be stored, but the setting of AllowStoreCredentials to false should not affect
> to storing of the main credentials. Also, the parametrization in the generic
> requests of vpn-agent.c has been improved to support hiding of the password.
>
> Second, vpn-provider.c is amended to handle ENOENT when connection callback is
> called and to have a function to record an error without creating a signal out
> of it and changing state. This is the case when the authentication credentials
> are handled within the VPN process lifetime and it does not shutdown after the
> credentials are invalid but requests them via other means from plugin. In such
> case it is imperative just to record the error as otherwise signaling about
> the error while the VPN process is still running would have undesired effects.
>
> And last, VPN agent support was implemented to OpenVPN plugin. OpenVPN
> management interface is used to get credential as well as encrypted private
> key password requests. These requests are forwarded to VPN agent. Credential
> request is same as with other VPN plugins but is handled within the OpenVPN
> process lifetime, during which the credentials can be requested multiple times
> without restart. For this reason each authentication error is not need to be
> signaled but only recorded to inform VPN agent. The private key password
> request is also a different case as the private key password is not to be
> stored by the VPN agent. In this case VPN agent is requested not to store or
> retrieve the private key password but is instructed to keep the other (main)
> credentials.
Thanks a lot for taking the time and upstreaming this code, really
appreciated. Overall, there isn't much I can complain on it. A few
nitpicks that's all. I think if you could add some info to the
vpn-overview.txt file on how to use the API we should be good. If
anything falls apart we fix it up in the tree.
Thanks,
Daniel
------------------------------
Date: Tue, 12 Nov 2019 10:47:27 +0200
From: Jussi Laakkonen <[email protected]>
Subject: Re: [PATCH 03/10] doc: Document VPN agent credential
storage/retrieval options
To: [email protected]
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hi Daniel,
> Hi,
>
> Maybe add some additiional information on the use case it in
> vpn-overview.txt? I am sure there will be questions on how to use it,
> or what the expected behavior of the UI should be.
>
Sure, I'll write an example case there and add it into v2 of this patch.
I haven't actually even viewed that file before :)
>
> Just a nitpick, indent the "Value" to the same level as
> "Requirement". Initially, I read it as nested value.
>
Will do.
Cheers,
Jussi
------------------------------
Date: Tue, 12 Nov 2019 11:08:31 +0200
From: Jussi Laakkonen <[email protected]>
Subject: Re: [PATCH 09/10] openvpn: Rewrite plugin to support VPN
agent and encrypted private keys
To: [email protected]
Cc: Daniel Wagner <[email protected]>
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hi Daniel,
> Hi Jussi,
>
> On Mon, Nov 11, 2019 at 04:01:53PM +0200, Jussi Laakkonen wrote:
>
> This link will go stale. Would the plugin still work if the OpenVPN
> client doesn't have this patch? Or could it made at least not hard
> depending on it?
>
Plugin does work without the patch but it does not receive the error:
> >PASSWORD:Verification Failed: 'Private Key'
and just keeps trying with the invalid password. Maybe I just change the
link to
https://git.sailfishos.org/mer-core/openvpn/blob/4f4b4af116292a207416c8a990392e35a6fc41af/rpm/privatekey-passphrase-handling.diff
that should be more permanent.
> And now some nitpicks :)
>
Well, it can be also called a review. Thanks for commenting :)
>
> IIRC, newer gcc will warn about the 'return' here.
>
Will remove in v2.
>
> One empty line too much.
>
Will remove in v2.
>
> Just use plain normal bools, not the ones from Glib if the we don't
have to.
>
Sure, I'll change in v2.
>
> Hmm, I would prefer to have the g_io_channel_read_line() not part of
> the if condition. Couldn't this function be organized slightly
> different? In many places we use a 'goto err' to untangle this kind of
> code. Just an idea.
>
I'll restructure this a bit.
>
> This information will go stale. Maybe add this to our TODO file?
>
A permalink to the patch could suffice? I'm not sure if this is ConnMan
TODO as it does not involve ConnMan changes, but if you think it is I
can add that. Would README be a better place as there is a section for
OpenVPN (the long URL is a drag, though..)
>
> data->failed_attempts_privatekey++;
>
Yeah.. in v2.
>
> Brackets not really needed.
>
All parts aren't really looked that closely, will fix in v2.
>
> data->connect_attemps++;
Yeah.. in v2.
>
>
> I don't think we should hardcode the /tmp path.
>
Oh, that one. We haven't paid attention to that small thing. Yeah, it
should be defined via Makefile -DTMPDIR as the others are. Will fix in v2.
>
>
> Overall, this is looks pretty good. I know understand your comment
> about to much infrastructure code in the plugin. Indeed moving some of
> this code into a library so other plugins can reuse it makes absolutely
> sense.
>
Thanks. Yes, moving all that code from plugins into one library is a
task in the future (I hope not so distant one).
Cheers,
Jussi
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list -- [email protected]
To unsubscribe send an email to [email protected]
------------------------------
End of connman Digest, Vol 49, Issue 14
***************************************
