hwchen / keyring-rs

Cross-platform library and utility to manage passwords
Apache License 2.0
497 stars 53 forks source link

How to handle `Ambiguous`? #201

Closed ReactorScram closed 3 months ago

ReactorScram commented 3 months ago

From https://github.com/firezone/firezone/issues/6175

I got into a weird situation, my dev VM is returning the Ambiguous error for both get_password and set_password on the current 3.x version.

This doesn't happen when I use the same code on my test VM, so I believe it's a problem with my secret service state in the dev VM, not with my code.

But I'm not sure how I would fix this if it happened on a production system. If delete_credential can also return Ambiguous, does that mean there is no way to repair this incorrect state using only keyring-rs itself? (aside from some workaround like picking a new key name and saving it to disk)

I'm also not sure how I got into this state. I don't run many programs on the dev VM, though I have run multiple builds of the Firezone Client, including builds with the old 2.x keyring-rs library.

So I am hesitant to fix the VM without knowing how to replicate or even fix the issue.

Thoughts?

brotskydotcom commented 3 months ago

Hi @ReactorScram, thanks much for reporting this, because I've always wondered whether anyone has ever actually gotten an Ambiguous error in production use :)! The error came about because of changes in the way v1 and v2 work:

Hearing that it is only your dev machine that is exhibiting this problem does not surprise me. Here's my suggestion on how to proceed if you want to use only the keyring to debug and fix the situation:

Alternatively, and possibly preferentially, I would suggest that you use a UI-based secret service client (typically this is included in your system settings UI) to find the ambiguous credentials and look at their attributes. My guess above may be correct, but it may also be the case that they are both v2/v3 credentials and they are sitting in different collections (one of which used to be the default but then had its collection file renamed to something other than "login"). If that's the case, you will probably have to use the UI tool to fix the issue; the keyring's cross-platform interface doesn't have the juice to deal with platform-specific issues like this (or, frankly, with ambiguity arising from the use of credential managers with other attribute conventions).

Please let me know if this helps, and what you find! As I said at the beginning, I really want to know!

ReactorScram commented 3 months ago

Okay! So I've run into a couple strange things already:

image
brotskydotcom commented 3 months ago

Wow! Great detective work! You've already found at least one bug... in the CLI! :) Looking at these lines:

if args.user.is_empty() || args.user.eq_ignore_ascii_case("<logged-in username>") {
    args.user = whoami::username()
}

I can see that the CLI is not letting you replicate the API call that firezone is making: notice the additional user@ in the output of the cli. I will fix that right away. In the meantime, in your fork, can you just remove the arg.user.is_empty() || from that line and rebuild; that should at least allow you to repro the issue and get the CLI to show you the ambiguous credentials.

Since firezone is using a target, keyring will be creating a secret-service collection named for the target and storing the credential there. I don't have any experience either with the KWallet or the KDEWalletManager, so I'm not exactly sure how one can use the manager UI to see non-default collections created by the secret service. Looking at this page on the ArchWiki, it seems like looking at the ~/.local/share/kwalletd folder might be useful in figuring out whether KWallet is implementing that collection by creating a different wallet completely, and whether there's a way to get the wallet manager to look at that wallet. Also, per this page, the wallet manager should be able to open a sidebar that displays wallets other than the default. Maybe that will help you to locate the ambiguous credentials?

Please let me know what you find out; I really appreciate you digging into this!

ReactorScram commented 3 months ago

that should at least allow you to repro the issue and get the CLI to show you the ambiguous credentials.

I removed that entire if-statement but now I'm just getting the same output as Firezone:

More than one credential found for '[dev.firezone.client/token]@': [Any { .. }, Any { .. }]

I'm not sure how it's an Any. It should be a trait object implementing Credential, and I should be able to downcast it, right? I don't know enough about runtime typing in Rust to figure it out. But it's not giving me any new debug info. :/

Also I somehow got KWallet and gnome-keyring installed side-by-side, so I used secret-tool search --all target dev.firezone.client/token to search for secrets:

[/org/freedesktop/secrets/collection/dev_2efirezone_2eclient_5ftoken/17]
label = keyring-rs v3.0.4 for target 'dev.firezone.client/token', service '', user ''
secret = [REDACTED]
created = 2024-07-30 15:40:25
modified = 2024-07-30 15:40:25
schema = org.freedesktop.Secret.Generic
attribute.application = rust-keyring
attribute.service = 
attribute.target = dev.firezone.client/token
attribute.username =

But I only see one secret there.

brotskydotcom commented 3 months ago

Also I somehow got KWallet and gnome-keyring installed side-by-side, so I used secret-tool search --all target dev.firezone.client/token to search for secrets

Using the target as the search attribute is not reliable for finding the duplicate credential, because the duplicate probably doesn't have a target attribute. Try searching against an attribute that you know is identical (and non-empty) between the two, such as the application attribute (always rust-keyring for this crate). So, for example, try secret-tool search --all application rust-keyring to see if that digs up the other one as well.

I'm not sure how it's an Any. It should be a trait object implementing Credential, and I should be able to downcast it, right? I don't know enough about runtime typing in Rust to figure it out.

Wow! Another bug you've found!! The CLI should be down-casting the Any to the correct platform-specific credential type so that it can be debug-printed (which would also provide clients an example of how to do that)! I will fix that right now also!

ReactorScram commented 3 months ago

Okay now I'm seeing this from secret-tool search --all --unlock application rust-keyring

[/org/freedesktop/secrets/collection/dev_2efirezone_2eclient_5ftoken/17]
label = keyring-rs v3.0.4 for target 'dev.firezone.client/token', service '', user ''
secret = [REDACTED]
created = 2024-07-30 15:40:25
modified = 2024-07-30 15:40:25
schema = org.freedesktop.Secret.Generic
attribute.application = rust-keyring
attribute.service = 
attribute.target = dev.firezone.client/token
attribute.username = 

[/org/freedesktop/secrets/collection/dev_2efirezone_2eclient_5ftest_5fBYKPFT6P_5ftoken/1]
label = keyring-rs v3.0.3 for target 'dev.firezone.client/test_BYKPFT6P/token', service '', user ''
secret = asdf
created = 2024-07-22 21:18:49
modified = 2024-07-22 21:18:49
schema = org.freedesktop.Secret.Generic
attribute.application = rust-keyring
attribute.service = 
attribute.target = dev.firezone.client/test_BYKPFT6P/token
attribute.username = 

And noticing a couple more oddities:

brotskydotcom commented 3 months ago

Hmmmm. Two credentials that have explicit target attributes should not be seen as ambiguous. Yes, keyring does its initial search for credentials by using just the username and service attributes, but then the results of that search are further filtered by the target attribute (if it exists), so if you are specifying an explicit target in the get_password call you should not be getting both credentials back. So I can see two possibilities:

  1. There's a bug in the "explicit target" filtering happening in keyring. I will do some testing right now to check on that.

  2. There's yet another credential in your dev keyring somewhere that has no target attribute, an empty username attribute, and an empty service attribute. To test this, try doing secret-tool search --all --unlock service "" and secret-tool search --all --unlock username "", and see if one of them turns up another credential.

I'm fascinated to hear what you find! Thanks again for pushing on this!

ReactorScram commented 3 months ago

I tried those commands and it's just returning the same two secrets. I don't see any obvious bugs in the keyring target filtering code either.

Very strange.

brotskydotcom commented 3 months ago

OK, we obviously need the CLI to tell us what it's finding, in detail. I'm working on that, stay tuned.

brotskydotcom commented 3 months ago

OK, please upgrade to v3.1.0 and try the CLI again to see which credentials it thinks are ambiguous. I'm very interested! I'm going to leave this question open until we figure out what's going on with your dev machine.

ReactorScram commented 3 months ago

Huh, well it's not saying ambiguous anymore.

cargo run --features crypto-rust,sync-secret-service,windows-native --release --example keyring-cli -- -v --target "dev.firezone.client/token" --service "" --user "" password returns:

[REDACTED STRING #1]
Password for '[dev.firezone.client/token]@' is '[REDACTED STRING#1]'

I checked out the main branch of Firezone, which had been replicating this, and it no longer replicates, it logs in just fine.

Does keyring 3.1 do anything that could possibly repair this on-disk, or did my dev VM just fix itself overnight at random? 🤔

brotskydotcom commented 3 months ago

I believe your dev VM just fixed itself overnight. Did you log out or reboot or take an update or do anything else that might have caused the secret service to be restarted? Given what you reported yesterday about it sometimes showing you labels and sometimes not, I suspect there may have been cached data that was corrupt and coming back as partial results, and that's why you were getting the Ambiguous error. It's even possible that calls made through the secret-tool kicked it enough that it flushed the corrupt data.

I'm going to close this issue. While I'm happy things are working for you, I'm kind of bummed we didn't get to try out the shiny new diagnostics it caused me to build :). Ah, well, I'll have to be satisfied with my manufactured tests...

Thanks again for reporting this and working it with me. It's a delight for me to have the opportunity for a give and take with a client dev! It's exactly the kind of thing open source makes possible and "my real job" never did!

ReactorScram commented 3 months ago

I think I did reboot it, the UTM VMs hang for no reason every now and then, I think it hanged yesterday.

It shows an uptime of 1 day, 1:45 which I guess means 1h45m, so I must have rebooted it yesterday before my last comment where secret-tool was returning something odd. I thought I had replicated the issue in Firezone after that but maybe not.

Yeah, on the Firezone issue I'll chalk it up to "My VM is bad please re-open if this happens to anyone else"

ReactorScram commented 2 months ago

It may still just be my dev VM randomly corrupting its own state, but it's happening again. This is at a time when the VM's DNS is also broken, so unrelated proof that something is wrong in my VM.

I restarted the VM hoping to clear any in-memory state that's breaking the DNS and secret service, but even after restarting I'm still getting this in Firezone

(Had to screenshot because copy-paste quit working in UTM as I was making this post)

image

So I doubt it's any fault of keyring but I thought I'd mention it for the record.

As you can see from the error string, we haven't updated to the new keyring. I'll do that in a dev branch and see what happens. Assuming ofc that my VM can find github.com and crates.io...

ReactorScram commented 2 months ago

okay well the new keyring isn't on crates and Cargo is telling me that some underlying Windows dep clashes with chrono anyway 🤷‍♀️

I'll come back to this

brotskydotcom commented 2 months ago

So sorry about the failure to publish 3.1 to crates.io: that's now fixed.

On the Windows side, I took a dependabot-suggested update to the latest Microsoft-supported windows crate as part of 3.1. Let me know if that introduces some conflict; I am willing to roll it back.

brotskydotcom commented 2 months ago

By the way, if you can get 3.1 working on your dev machine, I would love to see the error output from 3.1 (which will show us the credential details).

ReactorScram commented 2 months ago

Yeah 3.1 does work standalone.

Output of cargo run --features crypto-rust,sync-secret-service,windows-native --example keyring-cli -- --target "dev.firezone.client/token" -v -s "" -u "" password is

(Pretty-printed by hand, secrets didn't appear to be included)

More than one credential found for '[dev.firezone.client/token]@': [
    SsCredential { 
        attributes: {
            "gkr:compat:hashed:username": "d41d8cd98f00b204e9800998ecf8427e", 
            "gkr:compat:hashed:service": "d41d8cd98f00b204e9800998ecf8427e", 
            "xdg:schema": "org.freedesktop.Secret.Generic", 
            "gkr:compat:hashed:target": "17d58222bb2f0367a7a27a97ae3e5382", 
            "gkr:compat:hashed:application": "79d05a70f3e1b0a4365c7912ee036a95"
        }, 
        label: "", 
        target: None 
    }, 
    SsCredential { 
        attributes: {
            "xdg:schema": "org.freedesktop.Secret.Generic", 
            "gkr:compat:hashed:target": "a6e156c65c6804d9bdb95f480310ea36", 
            "gkr:compat:hashed:application": "79d05a70f3e1b0a4365c7912ee036a95", 
            "gkr:compat:hashed:service": "d41d8cd98f00b204e9800998ecf8427e", 
            "gkr:compat:hashed:username": "d41d8cd98f00b204e9800998ecf8427e"
        }, 
        label: "",
        target: None
    }
]

Here they are in sorted order:

So how are they ambiguous if the target actually does vary?

brotskydotcom commented 2 months ago

OK, now we are getting somewhere!

  1. Yes, you're right, the secrets are intentionally omitted. They aren't actually fetched in the ambiguous case.
  2. The fact that there's no label on either of those indicates that neither of them was actually written by keyring. Even in v1 keyring always put a label on every credential it created. In fact no secret service client is ever supposed to create an entry with no label: the spec requires a label (although I don't recall whether it can be empty).
  3. Keyring is showing you that neither of those credentials has a target. None of those attributes prefixed with gkr:compat:hashed: mean anything to keyring.
  4. I've done some research and figured out that those attributes are related to this fixed bug in the Gnome keyring code. They were first reported in this bug.

Here's my current thinking about what's going on:

brotskydotcom commented 2 months ago

OK, with a bunch more research, I think I know exactly what's happening on your dev machine. Please see the comments on #204 for a reproduced example. I am thinking about the right way to handle this problem.