search

OpenSmalltalk / opensmalltalk-vm

Cross-platform virtual machine for Squeak, Pharo, Cuis, and Newspeak.
http://opensmalltalk.org/
Other
564 stars 111 forks source link

Setting SqueakSSL string property ‘CERTNAME’ has no effect on macOS #680

Open Rinzwind opened 7 months ago

Rinzwind commented 7 months ago

As far as I understand, in the SqueakSSL plugin, setting the string property identified by SQSSL_PROP_CERTNAME has no effect on macOS.

In sqUnixOpenSSL.inc, setting the property sets the ‘certName’ field of the ‘ssl’ structure:

https://github.com/OpenSmalltalk/opensmalltalk-vm/blob/8e9abfbe6aa6ca35dfb7c5a1238147fddb3a82aa/platforms/unix/plugins/SqueakSSL/sqUnixOpenSSL.inc#L708-L710

The field is used in sqSetupSSL to load the certificate and private key:

https://github.com/OpenSmalltalk/opensmalltalk-vm/blob/8e9abfbe6aa6ca35dfb7c5a1238147fddb3a82aa/platforms/unix/plugins/SqueakSSL/sqUnixOpenSSL.inc#L280-L291

In sqMacSSL.c, setting the property similarly sets the ‘certName’ field of the ‘ssl’ structure:

https://github.com/OpenSmalltalk/opensmalltalk-vm/blob/8e9abfbe6aa6ca35dfb7c5a1238147fddb3a82aa/platforms/Mac%20OS/plugins/SqueakSSL/sqMacSSL.c#L607-L611

But in sqSetupSSL, the field does not seem to be used:

https://github.com/OpenSmalltalk/opensmalltalk-vm/blob/8e9abfbe6aa6ca35dfb7c5a1238147fddb3a82aa/platforms/Mac%20OS/plugins/SqueakSSL/sqMacSSL.c#L108-L176

Rinzwind commented 6 months ago

I opened a pull request for this in the Pharo VM repository. Porting the changes to the OpenSmalltalk VM would largely just seem to need to take the difference between the use of ‘logStatus’ and ‘logprintf_status’ in ‘sqSetupSSL’ into account.

krono commented 6 months ago

HI @Rinzwind, These are interesting changes. Given that SqueakSSL always tried to be as platfrom-close as possible I would expect a certName on macOS not name a file but rather a certificate in Keychain. This would be in line with the Windows implementation which uses the cert store.

Other than that, the changes over ther introducing the certpass would at least require updating the SQSSL_VERSION.

Rinzwind commented 5 months ago

I gave that a try: commit bf1931e7083c5e8d. It might make more sense given that ‘SecPKCS12Import’ also adds the items to the default keychain anyway (contrary to what its documentation implies in saying “you can use the SecPKCS12Import function to obtain SecIdentityRef objects […] you can then use the Keychain Services API […] to put the identities and associated certificates in the keychain”, see the thread regarding “SecIdentityRef without importing (SecPKCS12Import) into the keychain” on the Apple Developer Forums). A concern could however be that there can be multiple certificates for the same subject. The ‘friendly name’ used in the Windows implementation is, as far as I understand from some of Microsoft’s documentation on ‘Certificates’, a name that can be freely associated with a certificate. I don’t think there’s an equivalent in Keychain Access, it allows entering a name for a private key but not for a certificate as far as I can tell. The use of ‘kSecMatchValidOnDate’ in the query passed to ‘SecItemCopyMatching’ does at least avoid using a certificate that’s expired.

krono commented 5 months ago

Look ok for now. sorry, I dont have too much time atm