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

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

Closed
wants to merge 1 commit into from

Conversation

zschreur
Copy link

@zschreur zschreur commented Jun 6, 2024

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
Copy link
Collaborator

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:

  • Are you trying to use keyring to write arbitrary binary data into credentials? That would seem to require changes on all platforms.
  • Are you trying to write long credential data? There are limitations in the length of the credential store on most platforms; why should Windows be treated differently?

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

  • 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.

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

@zschreur
Copy link
Author

zschreur commented Jun 9, 2024

@brotskydotcom thank you for considering my PR. I have written up the issue with as much detail as I can here: #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.

@brotskydotcom
Copy link
Collaborator

brotskydotcom commented Jul 6, 2024

Hey @zschreur, did you make any progress on reworking this PR to be generic a la #174? If so, I'd like to work it into the v3 final release, which is imminent.

@brotskydotcom
Copy link
Collaborator

I have written this functionality into the v3 release (now at rc.2), so I'm closing this PR.

@zschreur
Copy link
Author

zschreur commented Jul 8, 2024

I have written this functionality into the v3 release (now at rc.2), so I'm closing this PR.

Great thank you for letting me know.

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

Successfully merging this pull request may close these issues.

2 participants