Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Rinzwind opened this issue Apr 9, 2024 · 4 comments
Open

Comments

@Rinzwind
Copy link

Rinzwind commented Apr 9, 2024

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:

case SQSSL_PROP_CERTNAME:
if (ssl->certName) free(ssl->certName);
ssl->certName = property;

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

/* if a cert is provided, use it */
if(ssl->certName) {
if(ssl->loglevel) {
printf("sqSetupSSL: Using cert file %s\n", ssl->certName);
}
if(sqo_SSL_CTX_use_certificate_file(ssl->ctx, ssl->certName, SSL_FILETYPE_PEM)<=0) {
sqo_ERR_print_errors_fp(stderr);
}
if(sqo_SSL_CTX_use_PrivateKey_file(ssl->ctx, ssl->certName, SSL_FILETYPE_PEM)<=0) {
sqo_ERR_print_errors_fp(stderr);
}
}

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

case SQSSL_PROP_CERTNAME:
if (ssl->certName) {
free(ssl->certName);
}
ssl->certName = property;

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

OSStatus sqSetupSSL(sqSSL* ssl, int isServer)
{
OSStatus status = noErr;
logprintf("sqSetupSSL: Setting up new context\n");
/* Create the new context */
#if MAC_OS_X_VERSION_MAX_ALLOWED >= 1080
ssl->ctx = SSLCreateContext(NULL,
isServer ? kSSLServerSide : kSSLClientSide,
kSSLStreamType);
#else
status = SSLNewContext(isServer, &ssl->ctx);
if (status != noErr) {
logprintf_status(status, "SSLNewContext failed");
return status;
}
#endif // MAC_OS_X_VERSION_MAX_ALLOWED >= 1080
/* Set the connection ref */
status = SSLSetConnection(ssl->ctx, ssl);
if (status != noErr) {
logprintf_status(status, "SSLSetConnection failed");
return status;
}
/* Set up the read/write functions */
status = SSLSetIOFuncs(ssl->ctx, SqueakSSLRead, SqueakSSLWrite);
if (status != noErr) {
logprintf_status(status, "SSLSetIOFuncs failed");
return status;
}
#if MAC_OS_X_VERSION_MAX_ALLOWED >= 1080
/* At least TLS 1 */
status = SSLSetProtocolVersionMin(ssl->ctx, kTLSProtocol1);
#else
/* Prefer TLS 1 */
status = SSLSetProtocolVersionEnabled(ssl->ctx, kTLSProtocol1, true);
#endif // MAC_OS_X_VERSION_MAX_ALLOWED >= 1080
if (status != noErr) {
logprintf_status(status, "SSLSetProtocolVersion{Min,Enabled} failed");
return status;
}
/* Disable cert verification since we do that ourselves */
status = SSLSetEnableCertVerify(ssl->ctx, false);
if (status != noErr) {
logprintf_status(status, "SSLSetEnableCertVerify failed");
return status;
}
if (ssl->serverName) {
/* Try for SNI */
#if !(MAC_OS_X_VERSION_MAX_ALLOWED >= 1070)
status = SSLSetPeerDomainName(ssl->ctx, ssl->serverName,
strlen(ssl->serverName));
#else
status = SSLSetPeerDomainName(ssl->ctx, ssl->serverName,
strnlen(ssl->serverName, CN_MAX - 1));
#endif // MAC_OS_X_VERSION_MAX_ALLOWED >= 1070
if (status != noErr) {
logprintf_status(status, "SSLSetPeerDomainName failed");
return status;
}
}
return status;
}

@Rinzwind
Copy link
Author

Rinzwind commented Jun 1, 2024

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
Copy link
Member

krono commented Jun 1, 2024

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
Copy link
Author

Rinzwind commented Jun 2, 2024

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
Copy link
Member

krono commented Jun 6, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants