switch keychain Set() to update existing items instead of delete/add

[joemiller]

This switches the macos keychain's Set() implementation to use the go-keychain UpdateItem() API instead of doing a delete/re-add of the item.

Practically speaking this does not have much current impact to the library itself because it is not possible to retrieve this metadata through the current API, however, it sets the stage for improved metadata visibility like that being worked on in #29

example:

key                                      Created             LastModified
---------------------------------------- ------------------- -------------------
foo                                      2019-05-29 07:09:47 2019-05-29 07:27:21

[joemiller]

We nil out the ACL here before calling update. If we don't do this, macOS will prompt a number of times and say incorrectly that we are changing ownership and changing ACLs even if the access list is identical to the currently stored item.

[lox]

This seems like a super sensible idea! I wonder why we didn't do this initially? 🤔 I dimly recall there was a reason we deleted and re-added, I'll see if I can figure out what it was and if it's still relevant.

[joemiller]

@lox It might be due to the inclusion of an accesslist in the UpdateItem call. It seems to cause macos-keychain to throw several prompts with text such as "the wants to change ownership of an item" and "the wants to change permissions of an item". This occurs even if the accesslist is identical to previous calls to Set(). To avoid this we need to call UpdateItem without an accesslist attached.

So with the code as-is, I think there is a tradeoff. You wouldn't be able to change the ACL on subsequent Set() calls after an item is initially created. You would need to remove and re-add in that case. I had a look at some of apple's docs and the keybase/go-keychain lib and I don't think there is a way to query for the ACL implemented in the go-keychain lib right now. It does not look like we get ACLs returned with QueryItem(). I think apple has separate calls to query item ACLs.

Still thinking about this a bit, but some ideas so far:

1. look into extending keybase/keychain lib to support reading ACLs from Items, then update Set() api in this lib to first check the item's existing ACL before deciding whether to call UpdateItem() with or without an ACL attached.
2. Extend the API in this lib to include additional funcs.. maybe Update() that only updates content but not ACL. That might be confusing with Set() though.

[lox]

So with the code as-is, I think there is a tradeoff. You wouldn't be able to change the ACL on subsequent Set() calls after an item is initially created.

Have you tried this change with aws-vault to see if it has any impact?

[joemiller]

I have not, as I'm not an aws-vault user. I actually work mostly with google cloud.

…

On Fri, Jun 7, 2019 at 2:15 PM Lachlan Donald ***@***.***> wrote: So with the code as-is, I think there is a tradeoff. You wouldn't be able to change the ACL on subsequent Set() calls after an item is initially created. Have you tried this change with aws-vault to see if it has any impact? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#43?email_source=notifications&email_token=AAC4GA3R7RSJK63MNRV5GILPZLFWFA5CNFSM4HQNRZCKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXHAMRI#issuecomment-500041285>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC4GA6IB3ZTEOFJ4P4NTR3PZLFWFANCNFSM4HQNRZCA> .

[lox]

Ah, gotcha! I'll check it out then. Apologies, I assumed. I forget there are a lots of folks that use this dependency outside of aws-vault. That was after all why we decided to extract it 😅

[joemiller]

And thank you for extracting it =) I was halfway down the path of writing my own almost identical lib before stumbling upon this.

…

On Fri, Jun 7, 2019 at 4:35 PM Lachlan Donald ***@***.***> wrote: Ah, gotcha! I'll check it out then. Apologies, I assumed. I forget there are a lots of folks that use this dependency outside of aws-vault. That was after all why we decided to extract it 😅 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#43?email_source=notifications&email_token=AAC4GAYVB2QWCMOHLT7DJC3PZLWFDA5CNFSM4HQNRZCKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXHHPJA#issuecomment-500070308>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC4GA57D2WYLAHB72AM3XDPZLWFDANCNFSM4HQNRZCA> .

[mtibben]

I've tested this with aws-vault and setting is working as expected with new and existing keychain items