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

Extend macOS implementation of SqueakSSL plugin to support setting a certificate on the SSL session context #816

Merged
merged 2 commits into from
Jun 28, 2024

Conversation

Rinzwind
Copy link
Contributor

This pull request extends the macOS implementation of the SqueakSSL plugin to support setting a certificate on the SSL session context. It differs from pull request #812 in that the ‘CERTNAME’ property is used to identify a certificate and private key in a keychain rather than as the path to a file containing the certificate and private key. Commit 539aedd in this pull request corresponds to commit 3d9d900 in the earlier pull request and fixes a bug in ‘sqAcceptSSL’.

This can be used to set up a ZnSecureServer on macOS as follows:

"Generate a private key and certificate for a certificate authority:"
result1 := LibC resultOfCommand: 'cd /tmp && /usr/bin/openssl ' ,
	'req -x509 -newkey rsa -nodes -keyout ca-key -out ca-cert ' ,
	'-subj "/CN=ZnSecureServer Test CA Certificate" 2>&1'.

"Generate a private key and certificate for the server:"
result2 := LibC resultOfCommand: 'cd /tmp && /usr/bin/openssl ' ,
	'req -x509 -newkey rsa -nodes -keyout s-key -out s-cert1 ' ,
	'-subj "/CN=ZnSecureServer Test Server Certificate" ' ,
	'-addext subjectAltName=DNS:localhost 2>&1'.

"Generate a certificate for the server signed by the certificate authority:"
result3 := LibC resultOfCommand: 'cd /tmp && /usr/bin/openssl ' ,
	'x509 -in s-cert1 -out s-cert2 ' ,
	'-CAkey ca-key -CA ca-cert -CAserial ca-srl -CAcreateserial 2>&1'.

"Add the server certificate signed by the certificate authority to the ‘login’ keychain:"
result4 := LibC resultOfCommand: 'cd /tmp && /usr/bin/security ' ,
	'add-certificates -k ~/Library/Keychains/login.keychain-db s-cert2 2>&1'.

"Add the server private key to the ‘login’ keychain:"
result5 := LibC resultOfCommand: 'cd /tmp && /usr/bin/security ' ,
	'import s-key -k ~/Library/Keychains/login.keychain-db 2>&1'.

"Add the certificate authority’s certificate as trusted to the ‘login’ keychain (needs confirmation):"
result6 := LibC resultOfCommand: 'cd /tmp && /usr/bin/security ' ,
	'add-trusted-cert -k ~/Library/Keychains/login.keychain-db ca-cert 2>&1'.

"Start a ZnSecureServer with ‘certificate’ set to the common name of the server certificate’s subject:"
(server := ZnSecureServer on: 1443)
	certificate: 'ZnSecureServer Test Server Certificate';
	logToTranscript;
	start.

"Get ‘/’ from the server (needs permission to use the private key, use the button to always allow):"
response := ZnEasy get: 'https://localhost:1443'.

Browsing ‘https://localhost:1443’ works with Chrome and Firefox. As noted in the earlier pull request, with Safari, there’s a problem which seems to be related to the use of IPv4 rather than IPv6.

The earlier pull request was closed, and this new one opened, following comments given in OpenSmalltalk VM issue #680. As mentioned there, a concern now could be that there can be multiple certificates for the same subject, which one is then used is not really specified, except that expired certificates are excluded from the search performed through ‘SecItemCopyMatching’ by using ‘kSecMatchValidOnDate’ in the query.

…tate to ‘SQSSL_CONNECTED’, returned the value of ‘SQSSL_OK’ (zero) rather than the number of bytes written to the output buffer.
…ng been set (by searching for a valid identity with the property value as its subject through ‘SecItemCopyMatching’ and setting it as the SSL session context’s certificate through ‘SSLSetCertificate’).
@Rinzwind
Copy link
Contributor Author

I replaced commit a675a2b by commit 28670bc to fix a mistake: the return value of ‘SSLSetCertificate’ didn’t get assigned to ‘status’.

Copy link
Member

@guillep guillep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok to me.

Would you mind checking my comment?

Also, tell me if you feel this is ready for integration.

@@ -626,7 +660,7 @@ sqInt sqAcceptSSL(sqInt handle, char* srcBuf, sqInt srcLen, char* dstBuf,
}
/* We are connected. Verify the cert. */
ssl->state = SQSSL_CONNECTED;
return SQSSL_OK;
return ssl->outLen;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naive question: does this not break anything ? ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that I know of.

Doing the following may help clarify this change: using the new version of the SqueakSSL plugin, put (#result -> result) inspect before the assignment of false to connecting in #accept on ZdcSecureSocketStream, then try my snippet for setting up a ZnSecureServer.

Assuming it works and you get a ZnResponse with status 200, you should also have gotten a value for result greater than 0, so there was a send of #flushEncryptedBytes:startingAt:count: in the last iteration of the #whileFalse: loop. Without the change, result would have been 0, meaning those bytes got lost.

@Rinzwind
Copy link
Contributor Author

It’s ready for integration yes.

@guillep
Copy link
Member

guillep commented Jun 28, 2024

Thanks!

@guillep guillep merged commit 273ac6a into pharo-project:pharo-10 Jun 28, 2024
1 of 2 checks passed
@guillep guillep mentioned this pull request Jun 28, 2024
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

Successfully merging this pull request may close these issues.

2 participants