fcheung / keychain

Bindings for the OS X keychain
MIT License
65 stars 15 forks source link

Preparing to Keychain Item Import #4

Closed legal90 closed 9 years ago

legal90 commented 9 years ago

I've tried to implement import of Keychain Items from both the row File and String instances. I'm very new to this stuff and I haven't managed to allow trusting to all applications yet.

@fcheung Please, review this code and give recommendations what I have to change here. I would be grateful if you could help me with that.

Currently it is working for me this way:

irb(main):005:0> require 'keychain'
=> true
irb(main):006:0> kc = Keychain.create('/tmp/test.keychain', 'secretpass')
=> <SecKeychain 0x7fb39bda0810: /private/tmp/test.keychain>
irb(main):007:0> kc.unlock!('secretpass')
=> nil
irb(main):008:0> kc.import(File.open('/path/to/some.key'), ['/usr/bin/security', '/usr/bin/codesign'])
=> nil
> Keychain::Scope.new(Sec::Classes::KEY, kc).all
=> [<SecKey 0x7fb39e910590 [0x7fff7968bed0]>]
fcheung commented 9 years ago

I left some comments, but overall looks sane. I'd split up a little though (e.g. a method to create trusted apps, a method to create an access) to improve readability & for future reuse. (And obviously some spec coverage would be nice!)

legal90 commented 9 years ago

@fcheung Thanks a lot for recommendations! I've rebased my changeset, so no it looks better.

But I found a bug with duplicated item. If we try to import the same item twice(or more), then we will get an exception...

...
irb(main):005:0> item = kc.import(key, ['/usr/bin/security', '/usr/bin/codesign']).first
=> <SecKey 0x7fe885309230 [0x7fff7968bed0]>
irb(main):006:0> item = kc.import(key, ['/usr/bin/security', '/usr/bin/codesign']).first
Keychain::DuplicateItemError: The specified item already exists in the keychain. (-25299)
    from /path/to/ruby-keychain/lib/keychain/sec.rb:181:in `check_osstatus'
    from /path/to/ruby-keychain/lib/keychain/keychain.rb:149:in `import'
    from (irb):6
    from /Users/username/.rbenv/versions/2.1.0/bin/irb:11:in `<main>'

But item has been imported with some strange new label:

irb(main):007:0> Keychain::Scope.new(Sec::Classes::KEY, kc).all.each {|k| puts k.attributes}
{:can_sign=>true, :key_class=>"1", :can_encrypt=>true, :can_decrypt=>true, :can_wrap=>true, :key_type=>"42", :can_unwrap=>true, :can_verify=>true, :is_permanent=>true, :can_derive=>true, :application_label=>"\x91T\x13o\xB4\x90\xEB\xB2h\x8Ej\xD7\xB0\x1D\xD9\x0E\x18\x8F\xD9\xB3", :effective_key_size=>2048, :size_in_bites=>2048, :label=>"Imported Private Key"}
{:can_sign=>true, :key_class=>"1", :can_encrypt=>true, :can_decrypt=>true, :can_wrap=>true, :key_type=>"42", :can_unwrap=>true, :can_verify=>true, :is_permanent=>true, :can_derive=>true, :application_label=>"DC22122C04816637", :effective_key_size=>2048, :size_in_bites=>2048, :label=>"DC22122C04816637"}
=> [<SecKey 0x7fe885309230 [0x7fff7968bed0]>, <SecKey 0x7fe885305480 [0x7fff7968bed0]>]

I've double checked the lib documentation, but I don't see that I do something wrong. May be, it is a bug of "Security" library?

fcheung commented 9 years ago

That is odd. That inspect output looks like you've got multiple ruby objects wrapping the same pointer (0x7fff7968bed0) which is odd. If you look at the keychain with the keychain access app do you see the duplicate entries?

legal90 commented 9 years ago

When I look at the Kaychain Access GUI, I see only the one item labeled "Imported Private Key". But if I delete this item, than all clones appear there too. In the example above it will be an item with label "DC22122C04816637"

fcheung commented 9 years ago

Hmm. I don't really know much about this area of the keychain API, so not sure what is happening

legal90 commented 9 years ago

I've noticed the same behavior with security import (on OS X 10.10.3):

$ security import /path/to/some.key -k /tmp/test.keychain
1 key imported.

$ security import /path/to/some.key -k /tmp/test.keychain
security: SecKeychainItemImport: The specified item already exists in the keychain.

$ security import /path/to/some.key -k /tmp/test.keychain
security: SecKeychainItemImport: The specified item already exists in the keychain.

$ security dump-keychain /tmp/test.keychain
keychain: "/private/tmp/test.keychain"
class: 0x00000010
attributes:
    0x00000000 <uint32>=0x00000010
    0x00000001 <blob>="Imported Private Key"
    0x00000002 <blob>=<NULL>
<...>   
keychain: "/private/tmp/test.keychain"
class: 0x00000010
attributes:
    0x00000000 <uint32>=0x00000010
    0x00000001 <blob>="D9FF37113EB731A4"
    0x00000002 <blob>=<NULL>
<...>
keychain: "/private/tmp/test.keychain"
class: 0x00000010
attributes:
    0x00000000 <uint32>=0x00000010
    0x00000001 <blob>="20047CA1D93A9DFC"
    0x00000002 <blob>=<NULL>
<...> 
fcheung commented 9 years ago

In that case maybe it's just what the api does & we can ignore that problem

fcheung commented 9 years ago

Anyway, looks good to me now, will merge when you're ready