Allow Updating Keys and Certificates in a KeyChain#9
Allow Updating Keys and Certificates in a KeyChain#9cfis wants to merge 23 commits intofcheung:masterfrom
Conversation
…ored in a key chain. It does this by moving access to SecItemUpdate to the Sec::Base class. This was inspired by the section “Adding, Removing, and Working With Keys and Certificates” in the Key Services Programming Guide which mentions SecItemAdd, SecItemUpdate and SecItemCopyMatching as base functions that should be used on keys and certificates stored in the KeyChain. This patch does make it possible for example to change a key’s name as stored in the keychain (my particular use case).
…for certificate start and finish times.
…ptions in Scope class.
|
Any chance this could get merged? |
fcheung
left a comment
fcheung
left a comment
There was a problem hiding this comment.
Hi,
thanks for this! Looks 99% good, just a few small comments that i've left - mostly memory related. I've also fixed the travis build on master - if you could merge master again & see if get gets travis green on your branch than that would be super.
| access_buffer = FFI::MemoryPointer.new(:pointer) | ||
| status = Sec.SecKeychainItemCopyAccess(self, access_buffer) | ||
| Sec.check_osstatus status | ||
| Access.new(access_buffer.read_pointer) |
There was a problem hiding this comment.
Do you need a release_on_gc here (to eventually release the result of SecKeychainItemCopyAccess?)
| access_buffer = FFI::MemoryPointer.new(:pointer) | ||
| status = Sec.SecAccessCreate(description.to_cf, trusted_apps.to_cf, access_buffer) | ||
| Sec.check_osstatus status | ||
| self.new(access_buffer.read_pointer) |
| unless applications_ref.read_pointer.null? | ||
| applications_cf = CF::Base.typecast(applications_ref.read_pointer).release_on_gc | ||
| @applications = applications_cf.to_ruby | ||
| applications_cf.release |
There was a problem hiding this comment.
I think this is releaseing applications_ref a second time
| self | ||
| else | ||
| pointer = Sec.SecKeyCopyPublicKey(self) | ||
| self.class.new(pointer) |
| @conditions.each do |key, value| | ||
| key_cf = inverse_attributes[key] || INVERSE_ATTR_MAP[key] | ||
| if key_cf.nil? | ||
| raise "Unknown search key: #{key}. Type: #{@kind}. Please look at the class's ATTR_MAP constant for accepted keys" |
There was a problem hiding this comment.
raise ArgumentError rather than runtime error for these?
| end | ||
|
|
||
| # Removes the item from the associated keychain | ||
| def delete |
There was a problem hiding this comment.
I think this only makes sense if some of the things that inherit from Sec::Base cease to do so (Access, TrustedApplication, ACL, maybe more), since those aren't (If I understand correctly) keychain items
| query = build_refresh_query | ||
| status = Sec.SecItemCopyMatching(query, result) | ||
| Sec.check_osstatus(status) | ||
| cf_dict = CF::Base.typecast(result.read_pointer) |
There was a problem hiding this comment.
I think this is another missing release (although it looks like this was in the original code too)
2 participants
The goal of this patch is to enable updating keys and certificates stored in a key chain. It does this by moving access to SecItemUpdate to the Sec::Base class. This was inspired by the section “Adding, Removing, and Working With Keys and Certificates” in the Key Services Programming Guide which mentions SecItemAdd, SecItemUpdate and SecItemCopyMatching as base functions that should be used on keys and certificates stored in the KeyChain. This patch does make it possible for example to change a key’s name as stored in the keychain (my particular use case).