Replace keychain with go keychain #130
Conversation
keyring/keychain.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| keychainAccessGroup = "ACE1234DEF.com.99designs.aws-vault" |
There was a problem hiding this comment.
I don't fully understand what this is, but looks like it maps to kSecAttrAccessGroup which doesn't sound necessary in this use-case, and ACE1234DEF looks suspiciously like sample data.
Access groups are used to share keychain items among two or more apps.
It's being passed to gokeychain.NewGenericPassword() which calls SetAccessGroup() which calls k.SetString(AccessGroupKey, ag) which treats empty string as unset. So perhaps we should just pass an empty string to gokeychain.NewGenericPassword()? Or is this actually doing something?
There was a problem hiding this comment.
Yup, agreed. It's for sharing items between a family of apps, which we don't need.
| return &keychain{ | ||
| path: filepath.Join(home, "/Library/Keychains/"+name+".keychain"), |
There was a problem hiding this comment.
This is one change that probably needs testing. The new code uses simply name +".keychain", vs the old one that references the full path. It works fine on my machine which is 10.12.6.
keyring/keychain.go
Outdated
| newItem.SetLabel(item.Label) | ||
| newItem.SetDescription(item.Description) | ||
| newItem.SetData(item.Data) | ||
| newItem.SetSynchronizable(gokeychain.SynchronizableNo) |
There was a problem hiding this comment.
I like that this is more specific. I wonder what the old default was.
keyring/keychain.go
Outdated
| } | ||
|
|
||
| return err | ||
| return gokeychain.AddItem(item) |
There was a problem hiding this comment.
I'm getting a compile error?
$ go build github.com/99designs/aws-vault
# github.com/99designs/aws-vault/keyring
keyring/keychain.go:85: cannot use item (type Item) as type keychain.Item in argument to keychain.AddItem
|
All looks good to me |
|
We really need some sort of CI for this project. |
|
Actually, I have spotted a difference in behaviour. When creating a keychain, v3.7.1 prompts for a password. This version does not. Is that intentional? |
|
Nope, not intentional. |
|
Good spotting. |
keyring/keychain.go
Outdated
| if count > 0 { | ||
| a = make([]C.CFTypeRef, count) | ||
| C.CFArrayGetValues(cfArray, C.CFRange{0, count}, (*unsafe.Pointer)(&a[0])) | ||
| if k.passphrase != "" { |
|
Verified this works. |
|
Hey, I was just going through documenting/tidying stuff in the keyring extraction I did. Looks like keybase/go-keychain#13 never merged. I'm wondering if there's a bug hidden somewhere and it's no longer doing what we think it does. |
|
Nah, those folks just take a while to respond sometimes. |
I've been meaning to do this for ages, but finally got around to putting in the PR's to https://github.com/keybase/go-keychain that will let us entirely replace our long-ago-forked custom keychain interaction code. So much joy in this diff.
keybase/go-keychain#12
keybase/go-keychain#13