Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Proposal: Support buffers for binary keychain data #464

Open
Dru89 opened this issue Jun 25, 2022 · 0 comments
Open

Proposal: Support buffers for binary keychain data #464

Dru89 opened this issue Jun 25, 2022 · 0 comments

Comments

@Dru89
Copy link

Dru89 commented Jun 25, 2022

Summary

It would be great if the APIs in this library supported more than just "UTF-8" based string passwords from the library. Sometimes, at least on the macOS keychain, there are instances where the data stored might be binary instead of a simple UTF-8 string, and converting it to UTF-8 actually breaks the data structure. I've outlined what this change could look like at the bottom of this file, and provided a small commit sample of what the C++ changes would look like for a single function.

Motivation

I've come across several use cases in macOS Keychain where the "Password" data that gets stored is not actually UTF-8, but some kind of binary. Usually something created with NSKeyedArchiver. There was actually another request (#130) that mentioned binary plist support wasn't working because the output was UTF-8.

Allowing a Buffer type to be used for passwords would add better support for those use cases.

Describe alternatives you've considered

There aren't really any alternatives that would involve using this library at least. Outside of this library, I think the remaining options are either writing a similar C++ binding that added support for buffers, or outputting to the shell and doing something like:

child_process.exec('security find-generic-password -s "service"', {encoding: 'buffer'}, (err, stdout, stderr) => {
  // do something with stdout as buffer here.
});

Additional context

I'm imagining this as a non-breaking change to the existing APIs that are exported. Something like:

// Each of these is just defined here as an overload so that TypeScript smartly returns the right thing.
// But also I'm handwaving this a bit as I type it into GitHub Issues. 😅 
getPassword(service: string, account: string, opts?: {encoding: BufferEncoding}): Promise<string | null>;
getPassword(service: string, account: string, opts: {encoding: 'buffer'}): Promise<Buffer | null>;

type Credentials<T> = { account: string, password: T };
findCredentials(service: string, opts?: {encoding: BufferEncoding}): Promise<Array<Credentials<string>>>;
findCredentials(service: string, opts: {encoding: 'buffer'}): Promise<Array<Credentials<Buffer>>>;

findPassword(service: string, opts?: {encoding: BufferEncoding}): Promise<string | null>;
findPassword(service: string, opts: {encoding: 'buffer'}): Promise<Buffer | null>;

// This function just gets updated to take either a string or a Buffer
setPassword(service: string, account: string, password: string | Buffer): Promise<void>;

// This obviously wouldn't change.
deletePassword(service: string, account: string): Promise<boolean>;

To keep the C++ API simple in this change, I would just always return a buffer from the C++ bindings. And then convert that buffer to a string, if necessary, on the node side.

I wanted to understand a bit of what the level of effort for this would look like, so I tried updating findCredentials to support buffered output. (This one felt like the hardest one to support, since it could find and return multiple passwords.)

You can find my commit here: Dru89@8d661ef. If this work seems valuable at all, I'd be happy to put up the PR for it. But I also understand that this would be a relatively sizable update to the existing API, so I understand if it's not something this project finds valuable.

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

No branches or pull requests

1 participant