Usually secrets must be loaded since calling Update() on a connection requires the whole package of settings.  I recall it was done this way to ensure that you could selectively clear secrets by simply not including them in the dict when calling Update().  Otherwise NM wouldn't know which secrets to keep and which to clear.

However, it seems unlikely that anyone would want to clear all secrets at the same time and then build them back up.  So a potential fix for this would be:

1) if *no* secrets are sent in the Update() call, leave secrets alone, and don't call SaveSecrets() in any of the agents.

2) if it really is useful to clear all secrets at the same time, then we could add a ClearSecrets() call to the Connection object to do that (secured appropriately of course), but I'm not sure this is necessary.

Duplicates bug #727558

Created attachment 282086 [details] [review]
Suggested fix

I'm wondering if this makes sense?

Comment on attachment 282086 [details] [review]
Suggested fix

The patch shouldn't be specific to ifcfg-rh. It should happen in nm-settings-connection, when it's updating the connection with the new values (update_auth_cb()). It needs to merge in the existing secrets if there are no secrets in the new connection.

Branch jk/bgo728920-update-connection-secrets contains fixes for the issue.

The cli (last) commit adds an option for showing secrets in nmcli. But it should be enhanced to include other missing secret-type properties yet, because majority of them is missing at the moment. Though, we can leave it out from this patch set, for later enhancements of nmcli secrets handling in order not to block this bug.

I have one question about GetSecrets(). Have an empty setting_name parameter (returning secrets for the whole connection) ever worked?

Now it is blocked by
»·······/* Make sure the request actually requests something we can return */
»·······if (!nm_connection_get_setting_by_name (NM_CONNECTION (self), setting_name)) {
»·······»·······g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_SETTING,
»·······»·······             "%s.%d - Connection didn't have requested setting '%s'.",
»·······»·······             __FILE__, __LINE__, setting_name);
»·······»·······return 0;
»·······}
in nm_settings_connection_get_secrets(). And before there was just a FIXME comment /* FIXME: if setting_name is empty, return all secrets */.

This commit might be related: d8cbecec8bfc20968e1c9364b36d09faeab93f2.

(In reply to comment #5)
> Branch jk/bgo728920-update-connection-secrets contains fixes for the issue.
> 
> The cli (last) commit adds an option for showing secrets in nmcli. But it
> should be enhanced to include other missing secret-type properties yet, because
> majority of them is missing at the moment. Though, we can leave it out from
> this patch set, for later enhancements of nmcli secrets handling in order not
> to block this bug.
> 
> I have one question about GetSecrets(). Have an empty setting_name parameter
> (returning secrets for the whole connection) ever worked?
> 
> Now it is blocked by
> »·······/* Make sure the request actually requests something we can return */
> »·······if (!nm_connection_get_setting_by_name (NM_CONNECTION (self),
> setting_name)) {
> »·······»·······g_set_error (error, NM_SETTINGS_ERROR,
> NM_SETTINGS_ERROR_INVALID_SETTING,
> »·······»·······             "%s.%d - Connection didn't have requested setting
> '%s'.",
> »·······»·······             __FILE__, __LINE__, setting_name);
> »·······»·······return 0;
> »·······}
> in nm_settings_connection_get_secrets(). And before there was just a FIXME
> comment /* FIXME: if setting_name is empty, return all secrets */.
> 
> This commit might be related: d8cbecec8bfc20968e1c9364b36d09faeab93f2.

I'm not sure it has, actually.  The D-Bus API docs state that "if the setting_name is empty all secrets will be returned" but I'm not sure we've actually ever used that feature in either nm-applet, nm-ce, or even gnome shell.  But I believe the intention of the API docs is correct, we should fix this.  It could actually simplify code in the editor too.

> ifcfg-rh: fix reading HWADDR_BLACKLIST

>+transform_hwddr_blacklist (const char *blacklist)

typo: "hwaddr"

"iter-shift" needs spaces around the "-". And that logic is complicated enough that it would be good to have a test case for that (invalid hwaddrs) too.


> libnm-core: allow G_TYPE_STRV to be NULL

This should already be fixed now in master by

  6c6cec0 libnm-core: fix getting/setting 0-length array properties


> cli: add '--show-secrets' option for 'nmcli connection show'

Not sure I like this... It seems like it makes it too easy for bad guys to get them. :)

(In reply to comment #7)
> > ifcfg-rh: fix reading HWADDR_BLACKLIST
> 
> >+transform_hwddr_blacklist (const char *blacklist)
> 
> typo: "hwaddr"
> 
> "iter-shift" needs spaces around the "-". And that logic is complicated enough
> that it would be good to have a test case for that (invalid hwaddrs) too.
> 
It has been in master already, actually. Fixed now by 
7966b6c ifcfg-rh: fix typo in function name and enhance testcase

> 
> > libnm-core: allow G_TYPE_STRV to be NULL
> 
> This should already be fixed now in master by
> 
>   6c6cec0 libnm-core: fix getting/setting 0-length array properties
>
Yeah, rebased to master.
 
> 
> > cli: add '--show-secrets' option for 'nmcli connection show'
> 
> Not sure I like this... It seems like it makes it too easy for bad guys to get
> them. :)
Hmm, bad guys are bad :(. But, don't let them limit good guys :)

The branch is updated due to recent master changes and rebased to it.

I just merged danw/libnm-api to master, rebased jk/bgo728920-update-connection-secrets onto it (dropping "libnm: add nm_remote_connection_get_secrets_sync()") and pushed a fixup on top of it (to deal with the slightly-different synchronous get_secrets API from danw/libnm-api).

Anyway, I'm fine with the code, but would like a second opinion on whether the --show-secrets flag is a good thing or not... (I'm not super opposed to it, just semi-concerned.)

(In reply to comment #6)
> (In reply to comment #5)
> > I have one question about GetSecrets(). Have an empty setting_name parameter
> > (returning secrets for the whole connection) ever worked?
> > 
> > Now it is blocked by
> > »·······/* Make sure the request actually requests something we can return */
> > »·······if (!nm_connection_get_setting_by_name (NM_CONNECTION (self),
> > setting_name)) {
> > »·······»·······g_set_error (error, NM_SETTINGS_ERROR,
> > NM_SETTINGS_ERROR_INVALID_SETTING,
> > »·······»·······             "%s.%d - Connection didn't have requested setting
> > '%s'.",
> > »·······»·······             __FILE__, __LINE__, setting_name);
> > »·······»·······return 0;
> > »·······}
> > in nm_settings_connection_get_secrets(). And before there was just a FIXME
> > comment /* FIXME: if setting_name is empty, return all secrets */.
> > 
> > This commit might be related: d8cbecec8bfc20968e1c9364b36d09faeab93f2.
> 
> I'm not sure it has, actually.  The D-Bus API docs state that "if the
> setting_name is empty all secrets will be returned" but I'm not sure we've
> actually ever used that feature in either nm-applet, nm-ce, or even gnome
> shell.  But I believe the intention of the API docs is correct, we should fix
> this.  It could actually simplify code in the editor too.

I have created bug 737418 for that.


(In reply to comment #9)
> I just merged danw/libnm-api to master, rebased
> jk/bgo728920-update-connection-secrets onto it (dropping "libnm: add
> nm_remote_connection_get_secrets_sync()") and pushed a fixup on top of it (to
> deal with the slightly-different synchronous get_secrets API from
> danw/libnm-api).
> 
> Anyway, I'm fine with the code, but would like a second opinion on whether the
> --show-secrets flag is a good thing or not... (I'm not super opposed to it,
> just semi-concerned.)

The code is not strictly related to this bug. So I have created new bug 737415 to continue on the topic there.


Update() and ClearSecrets() part merged to master:
df230fc merge: secret handling enhancements (bgo #728920)
502eb2d core: add ClearSecrets() D-Bus call
54a5f45 settings: do not clear secrets on Update() without any secrets (bgo #728920)

*** Bug 727558 has been marked as a duplicate of this bug. ***