Loading pkcs12 der formatted bytes fails on macOS about rust-native-tls HOT 28 CLOSED

I just pushed a test showing this issue here:

https://github.com/bluejekyll/rust-native-tls/tree/bfry/working-pk12

the working test:

https://github.com/bluejekyll/rust-native-tls/blob/bfry/working-pk12/src/test.rs#L180

the broken test (calls into the new logic):

https://github.com/bluejekyll/rust-native-tls/blob/bfry/working-pk12/src/test.rs#L189

both use a newly generated cert each time, the first one passes every time for me.

from rust-native-tls.

The broken test fails for me as well.

from rust-native-tls.

it's possible that for some reason the cert that's being generated isn't good, though I use that logic elsewhere and it works in those places...

from rust-native-tls.

It seems that removing the keychain from the options is what causes the breakage. This could be a bug in rust-security-framework, the OS, or perhaps the cert isn't good (although I don't know why it would work when a keychain is specified). I haven't experienced any real world issues myself, but I haven't been using OpenSSL generated certs either.

from rust-native-tls.

The C-API's documentation:

/*!
@function SecPKCS12Import
@abstract Imports the contents of a PKCS12 formatted blob.
@param pkcs12_data The PKCS12 data to be imported.
@param options A dictionary containing import options. A kSecImportExportPassphrase entry is required at minimum. Only password-based PKCS12 blobs are currently supported.
@param items On return, an array containing a dictionary for every item extracted. Use kSecImportItem constants to access specific elements of these dictionaries. Your code must CFRelease the array when it is no longer needed.
@Result errSecSuccess in case of success. errSecDecode means either the blob can't be read or it is malformed.
errSecAuthFailed means an incorrect password was supplied, or data in the container is damaged.
*/
OSStatus SecPKCS12Import(CFDataRef pkcs12_data, CFDictionaryRef options, CFArrayRef * __nonnull CF_RETURNS_RETAINED items);

from rust-native-tls.

Yeah, I was looking at that... there's nothing obviously wrong.

from rust-native-tls.

Well, fortunately this part of the OS is open source.

from rust-native-tls.

https://opensource.apple.com/source/Security/Security-57740.1.18/OSX/libsecurity_keychain/lib/SecImportExport.c.auto.html

from rust-native-tls.

Notably, it uses the default keychain if none is specified, which isn't really what we want :/

from rust-native-tls.

The comment about a keychain being required also contradicts the documentation.

from rust-native-tls.

but it doesn't return that, does it? we're getting nothing back at the moment. It's bizarre that it works with the temp keychain but not on your new method. That's really odd.

And yes, that comment does seem to state that a keychain is required.

from rust-native-tls.

So I just checked my default keychain and all the certs I've been reading for the past few months have been added to it 🤔. Perhaps there's something special about the default keychain that's rejecting the certs.

Actually, see #19. This may be the same issue.

Anyway, it's not desirable to store these in any keychain, so I'm going to confirm whether calling SecItemImport directly without specifying a keychain actually works; the documentation suggests it should but who knows with Apple. If it does, then I will go patch rust-security-framework to do the right thing.

from rust-native-tls.

Ok, here's a new piece of evidence... changing the name of the cert each test run causes this test to pass. It appears that Apple is "remembering" the password. Which means auto-generated certs in tests maybe inappropriate for Apple? And that can be validated by looking in the default keychain.

I'm guessing this wasn't a problem for the temporary keychain, because it would be deleted after every test.

from rust-native-tls.

Ok, I added a random serial number to the test... this resolves the issue with my tests. Not sure if there is more than this that we'd want to fix. I just need to spread that out through my code.

from rust-native-tls.

thanks for the help in diagnosing this... I wouldn't have figured this out without this.

from rust-native-tls.

The documentation for from_der doesn't indicate that it has any side effects (and it shouldn't!), so this still needs to be fixed.

from rust-native-tls.

Yeah, though, it's in Apple's code at that point isn't it?

from rust-native-tls.

I cleaned up the test and put up a PR for it, so it can be used in the future to help validate.

from rust-native-tls.

I was certainly under the impression that a keychain was required when I originally implemented this. The requirement may have been removed in recent versions of macOS?

from rust-native-tls.

That seems to be what happened. SecPKCS12Import is implemented by calling SecKeychainItemImport, which required a keychain at some point. That function was deprecated in 10.7 in favor of SecItemImport, which is implemented by calling SecKeychainItemImport, which has apparently been modified to work even if no keychain is specified.

from rust-native-tls.

I'm going to revert the change and switch back to using a temporary keychain, but use https://developer.apple.com/reference/security/1393109-seckeychainsetsettings?language=objc to turn off the lock timeout which should prevent the issues in #17 from coming back up.

from rust-native-tls.

Okay. Reverting the change will prevent iOS targets from calling this function though.

from rust-native-tls.

As I've been investigating the ClientAuth stuff, I've been working on using SecTrust as an option for building up the verifiable chain. I was wondering if there should be two options, 1) use the default KeyChain to verify the cert, or 2) use a custom SecTrust chain?

I haven't yet had a chance to validated if SecTrust does anything funny with the default KeyChain, though.

from rust-native-tls.

Take a look at what we do in security-framework itself when adding custom CAs - I don't think things would be any different on the client auth side.

from rust-native-tls.

WRT iOS that is true - we could use the system keychain on iOS and a temp one on OSX.

I'm running into some weird issues with SecKeychainSetSettings prompting the user to unlock the keychain even though it should be unlocked, so I need to debug a bit more.

from rust-native-tls.

I'm not sure if SecPKCS12Import uses the system keychain on iOS. iOS doesn't have the concept of a keychain in the same way macOS does; almost all of the Keychain APIs are missing, including SecKeychainImportItem.

from rust-native-tls.

In that case, we may need to disable the entire server-side API on iOS.

from rust-native-tls.

Should be fixed by 304b00e.

from rust-native-tls.