Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

3.2.0 is broken with secret service on Linux? #207

Closed
VorpalBlade opened this issue Aug 30, 2024 · 5 comments · Fixed by #209
Closed

3.2.0 is broken with secret service on Linux? #207

VorpalBlade opened this issue Aug 30, 2024 · 5 comments · Fixed by #209
Assignees
Labels

Comments

@VorpalBlade
Copy link

VorpalBlade commented Aug 30, 2024

This seems to affect both sync and async secret service backends in 3.2.0.

With 3.1.0 I can in chezmoi_modify_manager (actually in ini-merge, which is what uses keyring, the former is the CLI program) query secret service on Linux. With 3.2.0 I get errors like:

[chezmoi_modify_manager ERROR] Keyring query: service=ini_processor user=prusa_mk39_apikey
[chezmoi_modify_manager ERROR] Keyring lookup error: No matching entry found in secure storage

which would come from https://github.com/VorpalBlade/ini-merge/blob/0abcabd5712f40b691805ee47f8c07479ce342dc/src/merge/mutations/transforms.rs#L371

I don't have a minimal reproducer (I could try making one if you need it).

In my documentation I suggest users store their entries using:

secret-tool lookup --label="Your label" service your_service_name username your_user_name

This seems to no longer work in the new version, which means it is a major semver break in my opinion.

If this was an intentional break the semver of this crate should have been updated (4.0) rather than staying at 3.x, and in that case the correct action would be to yank 3.2 and re-release it as 4.0

VorpalBlade added a commit to VorpalBlade/ini-merge that referenced this issue Aug 30, 2024
@brotskydotcom brotskydotcom self-assigned this Aug 31, 2024
@brotskydotcom
Copy link
Collaborator

Hi @VorpalBlade - very sorry about the problems you are seeing.

It looks like you are running into the (intentional) break introduced in keyring 3.2 with respect to the ability to read 3rd party secret-service credentials (as well as v1 keyring credentials): keyring 3.2 requires the target attribute to be set on credentials (and it must be set to "default" if the credential is in the login store). This change was introduced to fix #204; it is documented there and in the README for 3.2.

I understand why you see this as a SerVer break. I am willing to treat it that way (yank 3.2 and re-release it as 4.0) if necessary, but first I am going to take a day or two in order to:

  1. try to find a way to "reconnect" keyring with "missing target" credentials (that is, repair the break), and
  2. warn clients who are dependent on the fix for Keyring doesn't know how to disambiguate elements from locked collections. #204 that they will need to upgrade to 4.0 to get the fix.

@brotskydotcom
Copy link
Collaborator

@ReactorScram, I wanted to give you a heads-up about this bug, since firezone is dependent on the fix for #204.

@VorpalBlade
Copy link
Author

Hm looking at your example cli code, it is unfortunate to need platform specific code. I do target all the major platforms and I don't really want to have to have different code for those, as that means I don't get to test the code myself (I only use Linux).

Additionally, I haven't found a good way to test the keyring parts in end-to-end integration tests (especially not cross platform). I'm not a fan of mocking the credential store, since that presumably wouldn't have discovered this issue anyway.

One solution would be to not recommend using secret-tool going forward, but instead add a built in command to let users set this (this is probably better for non-Linux users too, since presumably they don't have secret-tool, hadn't thought about that before). So that is something I probably need to do regardless.

Still, there are downsides:

  • This would be a breaking change for my command.
  • My command that is meant to run extremely quick (10-20 ms if using keyring, <10 ms otherwise), and where "the keyring can't be accessed, because I'm logged in over ssh" is not a fatal error, which complicates the logic significantly with now two layers for fallback.

@VorpalBlade
Copy link
Author

Also, (and I'm sure this is an oversight, or I am missing something) it looks like your example uses use keyring::{secret_service::SsCredential, Entry, Result}; but that doesn't exist according to the documentation? Maybe the docs are just not being built with the correct feature flags?

@brotskydotcom
Copy link
Collaborator

OK, this bug is fixed in the newly-released version 3.2.1. Keyring, when given an entry with no target, will now find secret-service credentials in the default collection that have no target attribute but have service and username attributes that match the entry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants