hwchen / keyring-rs

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

Add store password and get password blob functions for windows #172

Open zschreur opened 3 weeks ago

zschreur commented 3 weeks ago

Because of how the credentials are converted from u8 to u16 when stored on windows, it is not possible to store a blob of data that is greater than CRED_MAX_CREDENTIAL_BLOB_SIZE / 2.

This PR provides a workaround by creating a set_password_blob and get_password_blob function. These functions take and return a Vec<u8>. This allows a client to do something like:

pub fn store_cred(service: &str, user: &str, password: &Vec<u8>) -> Result<()> {
    match keyring::Entry::new(service, key)?
        .get_credential()
        .downcast_ref::<keyring::windows::WinCredential>()
    {
        Some(win_credential) => {
            win_credential.set_password_blob(password)
        },
       // ...

The code for getting & setting with a Vec<u8> was pulled out of set_password and get_password, and then those functions are now making use of the new blob functions.

brotskydotcom commented 3 weeks ago

Hi @zschreur, thanks for contributing this PR. Before I can carefully consider it, I need to understand what your design goals are and what motivates them. Can you please open a bug describing the issue you're trying to solve for? And then can you add some design notes to this PR explaining what solution you chose and why?

Here are some things to think about as you write the bug:

Here is something to think about as you write your design note:

Thanks again for contributing. I look forward to having more discussion about this.

zschreur commented 3 weeks ago

@brotskydotcom thank you for considering my PR. I have written up the issue with as much detail as I can here: https://github.com/hwchen/keyring-rs/issues/173.

The approach I took when designing this PR was to provide a fix for #173, while not bringing in a breaking change. This solution seemed to be the least invasive.

In my opinion I would have expected get_password and set_password to behave in the same way I have written get_password_blob and set_password_blob to work. So in that scenario, this PR does not actually solve the issue.

Keyring is meant to be interoperable with 3rd party use of the credential store, such as the Windows OS credential manager UI. Storing passwords as blobs would break that interoperability.

In regards to interoperability, the Windows OS Credential Manager UI does not allow for displaying the stored credential. You are only given the ability to delete or replace it. Because of this, I believe that storing the &str as its byte slice representation does not break interoperability.