Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2441653002:
Always unlock all libsecret items in Password Manager and OSCrypt (Closed)
|
Created:
4 years, 2 months ago by cfroussios Modified:
4 years, 1 month ago Reviewers:
vasilii CC:
chromium-reviews, gcasto+watchlist_chromium.org, vabr+watchlistpasswordmanager_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUnlock all libsecret items in Password Manager and OSCrypt
The various libsecret methods have different contracts w.r.t. unlocking
keyring items. Of the currently used methods, only secret_service_store
will trigger unlocking as necessary. As a result, operations fail in
various ways until someone performs the first store operation.
With the CL, the libsecret native backend of Password Manager is set to explicitly
request unlocking of items on every call. OSCrypt avoids using
secret_service_lookup, which apparently ignores locked items.
BUG=657828, 631171
Committed: https://crrev.com/51e9b147713a497adcb228ae729a799872a35237
Cr-Commit-Position: refs/heads/master@{#427067}
Patch Set 1 #Patch Set 2 : Fix for os_crypt #Patch Set 3 : leak #Patch Set 4 : Error handling #
Total comments: 25
Patch Set 5 : fun with errors #Patch Set 6 : fix #
Total comments: 2
Patch Set 7 : format #
Messages
Total messages: 45 (32 generated)
The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Unlock all libsecret items in Password Manager and OSCrypt The various libsecret methods have different contracts w.r.t. unlocking keyring items. In the way they are used now, only store calls have the power to trigger unlocking the keyring. As a result, operations fail in various ways until someone performs the first store operation. BUG=657828,631171 ========== to ========== Unlock all libsecret items in Password Manager and OSCrypt The various libsecret methods have different contracts w.r.t. unlocking keyring items. Of the currently used methods, only secret_service_store will trigger unlocking as necessary. As a result, operations fail in various ways until someone performs the first store operation. The libsecret native backend of Password Manager is set to explicitly request unlocking of items on every call. OSCrypt avoids using secret_service_lookup, which apparently ignores locked items. BUG=657828,631171 ==========
Description was changed from ========== Unlock all libsecret items in Password Manager and OSCrypt The various libsecret methods have different contracts w.r.t. unlocking keyring items. Of the currently used methods, only secret_service_store will trigger unlocking as necessary. As a result, operations fail in various ways until someone performs the first store operation. The libsecret native backend of Password Manager is set to explicitly request unlocking of items on every call. OSCrypt avoids using secret_service_lookup, which apparently ignores locked items. BUG=657828,631171 ========== to ========== Unlock all libsecret items in Password Manager and OSCrypt The various libsecret methods have different contracts w.r.t. unlocking keyring items. Of the currently used methods, only secret_service_store will trigger unlocking as necessary. As a result, operations fail in various ways until someone performs the first store operation. With the CL, the libsecret native backend of Password Manager is set to explicitly request unlocking of items on every call. OSCrypt avoids using secret_service_lookup, which apparently ignores locked items. BUG=657828,631171 ==========
cfroussios@chromium.org changed reviewers: + vasilii@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... File components/os_crypt/key_storage_libsecret.cc (right): https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... components/os_crypt/key_storage_libsecret.cc:41: LOG(ERROR) << "OSCrypt found more than one encryption keys."; Seems like you use VLOG everywhere? https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... components/os_crypt/key_storage_libsecret.cc:44: g_list_free(secret_items); Doesn't it destroy SecretItem* too? https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... components/os_crypt/key_storage_libsecret.cc:80: if (error) { if (error) again? https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... components/os_crypt/key_storage_libsecret.cc:111: if (error || !search_results) { Why the identical logic above checks just |error|. Can it be that both error and search_results aren't empty? https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... components/os_crypt/key_storage_libsecret.cc:112: g_error_free(error); is it ok to free 0? https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... File components/os_crypt/key_storage_libsecret_unittest.cc (right): https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... components/os_crypt/key_storage_libsecret_unittest.cc:136: return result; Setting |error|? https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... components/os_crypt/key_storage_libsecret_unittest.cc:169: (decltype(&::secret_item_get_secret))mock_secret_item_get_secret; What is the reason to change the type of the function? It doesn't look safe. https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/lib... File components/os_crypt/libsecret_util_linux.cc (right): https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/lib... components/os_crypt/libsecret_util_linux.cc:104: static_cast<SecretSearchFlags>(SECRET_SEARCH_ALL | SECRET_SEARCH_UNLOCK), Will it unlock if nothing is found? What actually happens when the Keyring is locked? Can you read metadata at least?
The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #7 (id:120001) has been deleted
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... File components/os_crypt/key_storage_libsecret.cc (right): https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... components/os_crypt/key_storage_libsecret.cc:41: LOG(ERROR) << "OSCrypt found more than one encryption keys."; On 2016/10/21 14:10:07, vasilii wrote: > Seems like you use VLOG everywhere? Done. https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... components/os_crypt/key_storage_libsecret.cc:44: g_list_free(secret_items); On 2016/10/21 14:10:07, vasilii wrote: > Doesn't it destroy SecretItem* too? There is no method to free a SecretItem. https://developer.gnome.org/libsecret/unstable/SecretItem.html (delete means deleting it from the keyring) I'm not 100% sure what they do, so let's assume it does. https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... components/os_crypt/key_storage_libsecret.cc:80: if (error) { On 2016/10/21 14:10:07, vasilii wrote: > if (error) again? Done. https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... components/os_crypt/key_storage_libsecret.cc:111: if (error || !search_results) { On 2016/10/21 14:10:07, vasilii wrote: > Why the identical logic above checks just |error|. Can it be that both error and > search_results aren't empty? I intended to flatten the nulls in ToSingleSecret() and forgot to finish it. Done. https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... components/os_crypt/key_storage_libsecret.cc:112: g_error_free(error); On 2016/10/21 14:10:07, vasilii wrote: > is it ok to free 0? They check that it's not null. https://github.com/GNOME/glib/blob/master/glib/gerror.c#L487 But reading deeper I now see that they may print an error if it is null. https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... File components/os_crypt/key_storage_libsecret_unittest.cc (right): https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... components/os_crypt/key_storage_libsecret_unittest.cc:136: return result; On 2016/10/21 14:10:07, vasilii wrote: > Setting |error|? Done. https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... components/os_crypt/key_storage_libsecret_unittest.cc:169: (decltype(&::secret_item_get_secret))mock_secret_item_get_secret; On 2016/10/21 14:10:07, vasilii wrote: > What is the reason to change the type of the function? It doesn't look safe. The mock method gets a MockSecretItem and returns a MockSecretValue. These are not compatible with the real types. Using the real classes would be more code. The entire scheme for testing keyring and libsecret is a bit hacky. It should probably be reworked sometime after the native backends are deprecated. https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/lib... File components/os_crypt/libsecret_util_linux.cc (right): https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/lib... components/os_crypt/libsecret_util_linux.cc:104: static_cast<SecretSearchFlags>(SECRET_SEARCH_ALL | SECRET_SEARCH_UNLOCK), On 2016/10/21 14:10:07, vasilii wrote: > Will it unlock if nothing is found? > What actually happens when the Keyring is locked? Can you read metadata at > least? Search will always return the SecretItems, which individually may or may not be locked. Metadata isn't protected. In this particular call unlocking is redundant, because we're expecting no results. I added this so that I can make it a rule that we always unlock the stuff that we use. Tests now expect this flag without caring what we're fetching. Is there a case where we only need the metadata? I didn't see it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... File components/os_crypt/key_storage_libsecret.cc (right): https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... components/os_crypt/key_storage_libsecret.cc:44: g_list_free(secret_items); On 2016/10/21 17:56:35, cfroussios wrote: > On 2016/10/21 14:10:07, vasilii wrote: > > Doesn't it destroy SecretItem* too? > > There is no method to free a SecretItem. > https://developer.gnome.org/libsecret/unstable/SecretItem.html > (delete means deleting it from the keyring) > > I'm not 100% sure what they do, so let's assume it does. The concern was that secret_item_get_secret(secret_item) reads from the uninitialized memory. https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... File components/os_crypt/key_storage_libsecret_unittest.cc (right): https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... components/os_crypt/key_storage_libsecret_unittest.cc:169: (decltype(&::secret_item_get_secret))mock_secret_item_get_secret; On 2016/10/21 17:56:36, cfroussios wrote: > On 2016/10/21 14:10:07, vasilii wrote: > > What is the reason to change the type of the function? It doesn't look safe. > > The mock method gets a MockSecretItem and returns a MockSecretValue. These are > not compatible with the real types. Using the real classes would be more code. > > The entire scheme for testing keyring and libsecret is a bit hacky. It should > probably be reworked sometime after the native backends are deprecated. I see only one place where secret_item_get_secret is called (NativeBackendLibsecret::ConvertFormList). If it's called after this assignment then the binary may crash. What happens if you just assign nullptr in line 168? https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/lib... File components/os_crypt/libsecret_util_linux.cc (right): https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/lib... components/os_crypt/libsecret_util_linux.cc:104: static_cast<SecretSearchFlags>(SECRET_SEARCH_ALL | SECRET_SEARCH_UNLOCK), On 2016/10/21 17:56:36, cfroussios wrote: > On 2016/10/21 14:10:07, vasilii wrote: > > Will it unlock if nothing is found? > > What actually happens when the Keyring is locked? Can you read metadata at > > least? > > Search will always return the SecretItems, which individually may or may not be > locked. Metadata isn't protected. > > In this particular call unlocking is redundant, because we're expecting no > results. I added this so that I can make it a rule that we always unlock the > stuff that we use. Tests now expect this flag without caring what we're > fetching. > > Is there a case where we only need the metadata? I didn't see it. This method will never unlock anything, we put SECRET_SEARCH_UNLOCK only for consistency. Is it correct?
https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... File components/os_crypt/key_storage_libsecret.cc (right): https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... components/os_crypt/key_storage_libsecret.cc:44: g_list_free(secret_items); On 2016/10/24 10:41:14, vasilii wrote: > On 2016/10/21 17:56:35, cfroussios wrote: > > On 2016/10/21 14:10:07, vasilii wrote: > > > Doesn't it destroy SecretItem* too? > > > > There is no method to free a SecretItem. > > https://developer.gnome.org/libsecret/unstable/SecretItem.html > > (delete means deleting it from the keyring) > > > > I'm not 100% sure what they do, so let's assume it does. > > The concern was that secret_item_get_secret(secret_item) reads from the > uninitialized memory. Yes, I forgot to complete my point that if doesn't need deleting, then it might be a pointer to a static container. Anyway, this is not the assumption anymore. https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... File components/os_crypt/key_storage_libsecret_unittest.cc (right): https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... components/os_crypt/key_storage_libsecret_unittest.cc:169: (decltype(&::secret_item_get_secret))mock_secret_item_get_secret; On 2016/10/24 10:41:14, vasilii wrote: > On 2016/10/21 17:56:36, cfroussios wrote: > > On 2016/10/21 14:10:07, vasilii wrote: > > > What is the reason to change the type of the function? It doesn't look safe. > > > > The mock method gets a MockSecretItem and returns a MockSecretValue. These are > > not compatible with the real types. Using the real classes would be more code. > > > > The entire scheme for testing keyring and libsecret is a bit hacky. It should > > probably be reworked sometime after the native backends are deprecated. > > I see only one place where secret_item_get_secret is called > (NativeBackendLibsecret::ConvertFormList). If it's called after this assignment > then the binary may crash. What happens if you just assign nullptr in line 168? NativeBackendLibsecret::ConvertFormList is never supposed to be called from OSCrypt tests. ToSingleSecret() is where KeyStorageLibsecret calls secret_item_get_secret. The tests will crash if the function pointer is null. https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/lib... File components/os_crypt/libsecret_util_linux.cc (right): https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/lib... components/os_crypt/libsecret_util_linux.cc:104: static_cast<SecretSearchFlags>(SECRET_SEARCH_ALL | SECRET_SEARCH_UNLOCK), On 2016/10/24 10:41:14, vasilii wrote: > On 2016/10/21 17:56:36, cfroussios wrote: > > On 2016/10/21 14:10:07, vasilii wrote: > > > Will it unlock if nothing is found? > > > What actually happens when the Keyring is locked? Can you read metadata at > > > least? > > > > Search will always return the SecretItems, which individually may or may not > be > > locked. Metadata isn't protected. > > > > In this particular call unlocking is redundant, because we're expecting no > > results. I added this so that I can make it a rule that we always unlock the > > stuff that we use. Tests now expect this flag without caring what we're > > fetching. > > > > Is there a case where we only need the metadata? I didn't see it. > > This method will never unlock anything, we put SECRET_SEARCH_UNLOCK only for > consistency. Is it correct? Yes, it's only for consistency. I don't expect any effect here.
https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... File components/os_crypt/key_storage_libsecret_unittest.cc (right): https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... components/os_crypt/key_storage_libsecret_unittest.cc:169: (decltype(&::secret_item_get_secret))mock_secret_item_get_secret; On 2016/10/24 11:25:55, cfroussios wrote: > On 2016/10/24 10:41:14, vasilii wrote: > > On 2016/10/21 17:56:36, cfroussios wrote: > > > On 2016/10/21 14:10:07, vasilii wrote: > > > > What is the reason to change the type of the function? It doesn't look > safe. > > > > > > The mock method gets a MockSecretItem and returns a MockSecretValue. These > are > > > not compatible with the real types. Using the real classes would be more > code. > > > > > > The entire scheme for testing keyring and libsecret is a bit hacky. It > should > > > probably be reworked sometime after the native backends are deprecated. > > > > I see only one place where secret_item_get_secret is called > > (NativeBackendLibsecret::ConvertFormList). If it's called after this > assignment > > then the binary may crash. What happens if you just assign nullptr in line > 168? > > NativeBackendLibsecret::ConvertFormList is never supposed to be called from > OSCrypt tests. > > ToSingleSecret() is where KeyStorageLibsecret calls secret_item_get_secret. The > tests will crash if the function pointer is null. Here is incomplete list of why this is wrong: - (c++ 5.2.10) The effect of calling a function through a pointer to a function type (8.3.5) that is not the same as the type used in the definition of the function is undefined. - Converting T1* to T2* and back is unspecified if alignment of T1 and T2 are unknown. - C-style cast is prohibited. reinterpret_cast is allowed when you know what you are doing. Why didn't you just derive from SecretValue and SecretItem? https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/lib... File components/os_crypt/libsecret_util_linux.cc (right): https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/lib... components/os_crypt/libsecret_util_linux.cc:104: static_cast<SecretSearchFlags>(SECRET_SEARCH_ALL | SECRET_SEARCH_UNLOCK), On 2016/10/24 11:25:55, cfroussios wrote: > On 2016/10/24 10:41:14, vasilii wrote: > > On 2016/10/21 17:56:36, cfroussios wrote: > > > On 2016/10/21 14:10:07, vasilii wrote: > > > > Will it unlock if nothing is found? > > > > What actually happens when the Keyring is locked? Can you read metadata at > > > > least? > > > > > > Search will always return the SecretItems, which individually may or may not > > be > > > locked. Metadata isn't protected. > > > > > > In this particular call unlocking is redundant, because we're expecting no > > > results. I added this so that I can make it a rule that we always unlock the > > > stuff that we use. Tests now expect this flag without caring what we're > > > fetching. > > > > > > Is there a case where we only need the metadata? I didn't see it. > > > > This method will never unlock anything, we put SECRET_SEARCH_UNLOCK only for > > consistency. Is it correct? > > Yes, it's only for consistency. I don't expect any effect here. Acknowledged.
https://codereview.chromium.org/2441653002/diff/140001/components/os_crypt/ke... File components/os_crypt/key_storage_libsecret.cc (right): https://codereview.chromium.org/2441653002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.cc:67: VLOG(1) << "Libsecret lookup failed."; indent 2 spaces.
The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... File components/os_crypt/key_storage_libsecret_unittest.cc (right): https://codereview.chromium.org/2441653002/diff/60001/components/os_crypt/key... components/os_crypt/key_storage_libsecret_unittest.cc:169: (decltype(&::secret_item_get_secret))mock_secret_item_get_secret; On 2016/10/24 12:39:35, vasilii wrote: > On 2016/10/24 11:25:55, cfroussios wrote: > > On 2016/10/24 10:41:14, vasilii wrote: > > > On 2016/10/21 17:56:36, cfroussios wrote: > > > > On 2016/10/21 14:10:07, vasilii wrote: > > > > > What is the reason to change the type of the function? It doesn't look > > safe. > > > > > > > > The mock method gets a MockSecretItem and returns a MockSecretValue. These > > are > > > > not compatible with the real types. Using the real classes would be more > > code. > > > > > > > > The entire scheme for testing keyring and libsecret is a bit hacky. It > > should > > > > probably be reworked sometime after the native backends are deprecated. > > > > > > I see only one place where secret_item_get_secret is called > > > (NativeBackendLibsecret::ConvertFormList). If it's called after this > > assignment > > > then the binary may crash. What happens if you just assign nullptr in line > > 168? > > > > NativeBackendLibsecret::ConvertFormList is never supposed to be called from > > OSCrypt tests. > > > > ToSingleSecret() is where KeyStorageLibsecret calls secret_item_get_secret. > The > > tests will crash if the function pointer is null. > > Here is incomplete list of why this is wrong: > - (c++ 5.2.10) The effect of calling a function through a pointer to a function > type (8.3.5) that is not the same as the type used in the definition of the > function is undefined. > - Converting T1* to T2* and back is unspecified if alignment of T1 and T2 are > unknown. > - C-style cast is prohibited. reinterpret_cast is allowed when you know what you > are doing. > > Why didn't you just derive from SecretValue and SecretItem? I copied the scheme from native_backend_libsecret_unittest.cc. If I understand correctly, your concerns apply to the entire scheme, and not just this particular method. Addressing the concerns would require reworking the entire file, if not also the respective keyring tests and password manager tests. Could I start a bug, instead of fixing them in this CL? I will try to merge this into previous versions and I would like this to be focused on the bug I'm fixing. https://codereview.chromium.org/2441653002/diff/140001/components/os_crypt/ke... File components/os_crypt/key_storage_libsecret.cc (right): https://codereview.chromium.org/2441653002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.cc:67: VLOG(1) << "Libsecret lookup failed."; On 2016/10/24 12:56:00, vasilii wrote: > indent 2 spaces. Done.
LGTM with the promise that the pattern is fixed later.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/24 13:35:12, vasilii wrote: > LGTM with the promise that the pattern is fixed later. Thanks! I added it to my queue crbug.com/658723
The CQ bit was checked by cfroussios@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Unlock all libsecret items in Password Manager and OSCrypt The various libsecret methods have different contracts w.r.t. unlocking keyring items. Of the currently used methods, only secret_service_store will trigger unlocking as necessary. As a result, operations fail in various ways until someone performs the first store operation. With the CL, the libsecret native backend of Password Manager is set to explicitly request unlocking of items on every call. OSCrypt avoids using secret_service_lookup, which apparently ignores locked items. BUG=657828,631171 ========== to ========== Unlock all libsecret items in Password Manager and OSCrypt The various libsecret methods have different contracts w.r.t. unlocking keyring items. Of the currently used methods, only secret_service_store will trigger unlocking as necessary. As a result, operations fail in various ways until someone performs the first store operation. With the CL, the libsecret native backend of Password Manager is set to explicitly request unlocking of items on every call. OSCrypt avoids using secret_service_lookup, which apparently ignores locked items. BUG=657828,631171 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Unlock all libsecret items in Password Manager and OSCrypt The various libsecret methods have different contracts w.r.t. unlocking keyring items. Of the currently used methods, only secret_service_store will trigger unlocking as necessary. As a result, operations fail in various ways until someone performs the first store operation. With the CL, the libsecret native backend of Password Manager is set to explicitly request unlocking of items on every call. OSCrypt avoids using secret_service_lookup, which apparently ignores locked items. BUG=657828,631171 ========== to ========== Unlock all libsecret items in Password Manager and OSCrypt The various libsecret methods have different contracts w.r.t. unlocking keyring items. Of the currently used methods, only secret_service_store will trigger unlocking as necessary. As a result, operations fail in various ways until someone performs the first store operation. With the CL, the libsecret native backend of Password Manager is set to explicitly request unlocking of items on every call. OSCrypt avoids using secret_service_lookup, which apparently ignores locked items. BUG=657828,631171 Committed: https://crrev.com/51e9b147713a497adcb228ae729a799872a35237 Cr-Commit-Position: refs/heads/master@{#427067} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/51e9b147713a497adcb228ae729a799872a35237 Cr-Commit-Position: refs/heads/master@{#427067} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
