Surprising feature on MacOS: empty service/account strings are wildcards

[cvermilion]

The underlying Security Framework functions take optional arguments for the service and account strings, but this bubbles up to keyring as plain &strs. Passing an empty string is then treated as a non-provided optional argument rather than an explicit empty value. So, for example:

    #[test]
    fn secret_wildcard() {
        let name = "test";
        let entry = keyring::Entry::new("my_service", name);
        entry.set_password("abc").unwrap();

        let entry = keyring::Entry::new("my_service", name);
        let pass = entry.get_password().unwrap();
        assert_eq!("abc", pass);

        // Can also access via empty string(s)!
        let entry = keyring::Entry::new("my_service", "");
        let pass = entry.get_password().unwrap();
        assert_eq!("abc", pass);

        // And can use the empty-string entry to overwrite the original!
        entry.set_password("def").unwrap();

        let entry = keyring::Entry::new("my_service", name);
        let pass = entry.get_password().unwrap();
        assert_eq!("def", pass);

        entry.delete_password().unwrap();
    }

I've only tested this on MacOS but I suspect the behavior is the same on iOS.

This makes some sense as a reflection of the underlying API, but just reading the docs for keyring, I think it's a very surprising behavior. I think this should either a) get clear documentation somewhere, b) be forbidden (just fail on empty values somewhere) or c) be reflected in the API by making the service/account params Options.

[brotskydotcom]

Great catch, @cvermilion, thanks! I'm inclined to simply refuse empty user names, since that could easily be made consistent behavior across all platforms, and it would conceal this Apple-specific "wildcard" feature (which can't be exposed by keyring in a cross-platform way. because it doesn't exist on Windows).

[cvermilion]

Yeah, I think that's a good plan. The "search if empty" feature sort of makes sense if you're familiar with the Apple APIs but otherwise is probably unexpected, so I don't think forbidding empty fields is a big loss. If someone wants this behavior on Mac/iOS they can use security_framework directly.

[brotskydotcom]

So looking at the code, there's really no way of putting a general fix in without changing the API or allowing the code to panic. This is because the signatures for Entry::new and friends don't allow for argument validation and graceful failure, so I can't easily refuse to allow credentials with empty service or user names. Here are some options I'm considering:

• Up the major version and allow Entry::new and friends to fail.
• Up the minor version and add new Entry::try_new and friends which fail (note this leaves the bug unfixed).
• Leave the version alone and just have all platform operations return a NoEntry error if there is an empty string in target, service, or username. Then document this special case in the docs.

I'm inclined to the latter, as this doesn't seem like it's worth altering the API for. @hwchen what do you think?

[cvermilion]

FWIW, I think that's a sensible approach, and #87 looks fine to me. Allowing Entry::new to fail might be worth considering for the next major version but fixing this particular edge case is probably not worth an API change on its own.

I'm not sure whether it's worth worrying about users who relied on this behavior -- it seems possible that someone who worked exclusively on Mac/iOS might have expected and used it. But given that it's undocumented and non-portable, I'd say it's probably ok to remove?

[brotskydotcom]

Thanks for the review and the thoughtful comments!

It's hard to believe anyone is actually relying on this on macOS/iOS, since it only works if the credential you're working with is either the only one with that service name or magically the "first" one the keyring decides to return.

My bigger concern is that someone might explicitly be using an empty username or service name in order to be compatible with a 3rd-party application. If so, they are going to be a bit surprised. But, since the behavior in that case is undocumented and varies across platforms, I feel OK about clarifying it. They can still access the existing functionality by using the new_with_credential entry point and creating the same platform-specific credential that would have been created before this change.

This discussion makes me think, however, that this should probably be a minor version release not a patch release, and I should add some explicit deprecation documentation to the release notes, as well as a warning that the next major release will probably do validation at Entry creation time.

[brotskydotcom]

@cvermilion @hwchen I've updated the PR #87 per my last comment. I'll merge later today or tomorrow unless someone objects.