Keybase patches + Updated README

[lox]

In the spirit of getting upstream back to the canonical home for this package, this PR includes the changes that @akalin and @gabriel have made:

• Use generic keychain items rather than internet passwords
• Fix memory leaks and various bugs
• Simplify keychainError type
• Add a GetAllAccountNames() function

Thoughts on merging this in and then iterating somewhat on the API after some discussion?

[bgentry]

Thanks for putting this together. I will review it soon.

Looks like a small conflict (probably due to just-merged Solaris build
flag). Can you rebase on master and force push?

I'm more than willing to review and merge any PRs, including this one, but
I'm not sure it's ok to merge other authors' work into my license without
their express consent. @akalin @gabriel can you give some sort of
indication as to your stance on this?

On Thursday, September 3, 2015, Lachlan Donald notifications@github.com
wrote:

In the spirit of getting upstream back to the canonical home for this
package, this PR includes the changes that @akalin
https://github.com/akalin and @gabriel https://github.com/gabriel
have made:

• Use generic keychain items rather than internet passwords
• Fix memory leaks and various bugs
• Simplify keychainError type
• Add a GetAllAccountNames() function

Thoughts on merging this in and then iterating somewhat on the API after

some discussion?

You can view, comment on, or merge this pull request online at:

#6
Commit Summary

• Audit for memory bugs and leaks, and fix found issues
• Use generic password keychain functions instead of internet passwords
• Simplify keychainError type
• Add new GetAllAccountNames() function
• Replace ReplaceOrAddGenericPassword with RemoveAndAddGenericPassword
• Using SecItemAdd to support access groups. Changing password data to
  []byte to avoid unnecessary encoding.
• Fixing a couple leaks from last commit
• Revert recent Keychain-related changes.
• Create keychain item with access for an additional application.
• Password is []byte. Using SecAddItem. TrustedApplications.
• Fixes from review
• Merge pull request Thank you! #2 from keybase/appaccess
• Updated README with example of usage

File Changes

• M README.md
  https://github.com/bgentry/go-osxkeychain/pull/6/files#diff-0 (26)
• M osxkeychain.go
  https://github.com/bgentry/go-osxkeychain/pull/6/files#diff-1 (632)
• D osxkeychain.h
  https://github.com/bgentry/go-osxkeychain/pull/6/files#diff-2 (2)
• M osxkeychain_test.go
  https://github.com/bgentry/go-osxkeychain/pull/6/files#diff-3 (278)

Patch Links:

• https://github.com/bgentry/go-osxkeychain/pull/6.patch
• https://github.com/bgentry/go-osxkeychain/pull/6.diff

—
Reply to this email directly or view it on GitHub
#6.

[akalin-keybase]

This (and the MIT license) looks good to me!

[bgentry]

I've started looking at this, but it's impossible to merge in its current state due to conflicts. I'll need to spend some time reconciling that unless you can do so.

A couple of other things I noticed on the first pass:

• exported functions and constants no longer have documentation with this PR. To make the library follow Go best practices and be usable by e.g. godoc, it should follow standard conventions by documenting anything that's exported.
• should we really drop the InternetPassword functions as this PR does? Or should they be kept alongside the GenericPassword functions?

[lox]

Been thinking about this one a fair bit. Perhaps we should just iterate on #7 and come up with an API that works for both GenericPassword and InternetPassword?

Do you have a specific use for InternetPassword @bgentry?

[bgentry]

@lox I started using InternetPassword because it seemed simpler at the time (given how much I was struggling to get anything working here). I don't know whether I should just be using GenericPassword instead.

In general, I would prefer that this library map closely to the underlying API primitives in the Keychain Services APIs. We shouldn't try to offer a higher level abstraction that makes it harder to map from Keychain concepts and docs to this package. If there's a good opportunity to build such an abstraction to simplify usage for those who don't necessarily care about the underlying details, I'd prefer that be built alongside the basic building blocks or in another package.

I still think most of the stuff in here is good, though, and will spend some time trying to reconcile the two forks.

[akalin-keybase]

internet passwords are pretty different (have more options) from generic passwords, I think. If no one is actually using the internet password code, though, I'd prefer just ripping it out. We can always add it back later when it's needed!

I'll try and add docs for exported stuff (in the keybase fork).

[akalin-keybase]

Okay, added docs and fixed golint warnings.

[lox]

@akalin-keybase might be worth you opening a new PR directly from the keybase master?