r-lib / keyring

:closed_lock_with_key: Access the system credential store from R
https://keyring.r-lib.org/
Other
196 stars 28 forks source link

Why does wincred backend use keyring:service:username format? #84

Closed MichaelJW closed 3 years ago

MichaelJW commented 4 years ago

I've noticed that, while e.g. the Python keyring library stores credentials in Windows Credential Manager with a credential name that simply matches the service, this R library uses a credential name of the format keyring:service:username (or simply :service:username if no keyring is specified).

This can make it confusing when trying to access the same credentials with different libraries (or when using the native Credential Manager app on Windows); the workaround I've been using is to save credentials with the :service:username format in Credential Manager, and then, if using the Python library, to specify this same format when attempting to read credentials, rather than just specifying the service name.

But I'm curious why this library insists on that format in b_wincred_target() - is it for compatibility with the other backends, or because it uses the old Win32 API perhaps? Thanks in advance.

gaborcsardi commented 4 years ago

It is because keyring provides (mostly) the same API on all OSes, and wincred does not support users natively. Many tools do this, e.g. the git credential helper uses the username:url format.

I agree that sometimes you just want basic access to the credential store, so we can certainly add this.

I was thinking of creating a new oskeyring package that would just have the raw access and keyring could depend on that. Because keyring already has a bunch of dependencies, and it would be nice to have oskeyring as a zero dependency low level package.

gaborcsardi commented 4 years ago

But we can also add a function to keyring that does it, as a start.

MichaelJW commented 4 years ago

Cool - thank you! Much appreciated. And cheers for the quick response.

gaborcsardi commented 4 years ago

Do you want to submit a PR for a function that gives raw access? No pressure, just an opportunity.

MichaelJW commented 4 years ago

Absolutely, I'll try to submit something this week.

MichaelJW commented 4 years ago

@gaborcsardi Before I put the PR together, could I get your opinion on the way I'm thinking of structuring this please?

New functions in backend-wincred: b_wincred_set_native() (and b_wincred_set_with_value_native()) which take service, username, and password, check whether the WCM already has a credential stored with the same service name and warn that it will be overwritten with the new username if so, and store the credential with the given service - no b_wincred_target() transformation. No support for multiple keyrings.

Similarly, a new function b_wincred_get_native(), which takes service (and optionally username) and returns the password; if the supplied username does not match that of the credential, it gives a warning or an error.

At the API level: key_get_native(), key_set_native(), and key_set_with_value_native(), which take keyring, service, username (and password for the set functions) and - if the backend is wincred - call these new b_wincred_*_native() functions, giving a warning or error if keyring is set to anything other than NULL or "".

For other backends these key_*_native() functions would use whatever the backends natively support - e.g. since backend-macos already supports keyrings and users natively, the key_*_native() functions could just call the existing b_macos_get() and b_macos_set() functions.

Alternatively, native could be a flag in the key_*() functions, so key_get(service, username, keyring, native=TRUE) , if using the wincred backend, would call b_wincred_get_native().

gaborcsardi commented 4 years ago

I think this would be completely separate from the backends. We would just have wincred_get() etc.

MichaelJW commented 4 years ago

Here's the PR with the new functions. I added a wincred_get_username() function that is not in the common API since I've found that would be helpful in my own usage of WCM; I also added a check in wincred_get_key() that will throw an error if a username is supplied but the username does not match the one stored in the credential.

I have seen #85 but not added the encoding options you suggest at this point - wanted to make sure this was all OK first. I also have not added macos_ functions as I do not have a Mac to test them on.

Cheers

gaborcsardi commented 3 years ago

You can now use the oskeyring package to this I believe, so I am going to close this now. Please let me know if you still have problems. Thanks!