Avoid caching PEMs on disk #213
Conversation
| if (error) { | ||
| *error = TunnelKitErrorWithCode(TunnelKitErrorCodeTLSCertificateAuthority); | ||
|
|
||
| if (self.caPEM) { |
There was a problem hiding this comment.
@roop I trusted you on this bit (L249-261), but the replaced logic doesn't seem equivalent. The change broke TLS handshake on certain devices (Synology, pfSense etc.):
https://www.reddit.com/r/passepartout/comments/qvegv3/unable_to_connect_after_update_to_1170/
There was a problem hiding this comment.
@keeshux Sorry about the regression.
This is my understanding:
- Old:
SSL_CTX_load_verify_locations(ctx, caPath, NULL)says that the file at caPath should be trusted when performing verification - New: Use
SSL_CTX_get_cert_storeto get the trusted store and add the ca to it withX509_STORE_add_cert
The man page for SSL_CTX_get_cert_store says:
Typically the trusted certificate store is handled indirectly via using SSL_CTX_load_verify_locations(3). Using the SSL_CTX_set_cert_store() and SSL_CTX_get_cert_store() functions it is possible to manipulate the X509_STORE object beyond the SSL_CTX_load_verify_locations(3) call.
That's why I thought this was the appropriate replacement.
| if (error) { | ||
| *error = TunnelKitErrorWithCode(TunnelKitErrorCodeTLSCertificateAuthority); | ||
| } | ||
| return NO; |
There was a problem hiding this comment.
According to L274, ca should also be deallocated.
Revert part of #213 again, because `SSL_CTX_load_verify_locations` is just more reliable at setting up the trust store. It looks like it's able to reference the .pem multiple times in those cases where the root issuer of the CA is also embedded in the file (which is the case with e.g. Let's Encrypt). This is better than the current implementation, and I couldn't easily find a way to do the same in-memory. I'd rather use the standard API here. See 7a85d3c
* Verify CA from on-disk file Revert part of #213 again, because `SSL_CTX_load_verify_locations` is just more reliable at setting up the trust store. It looks like it's able to reference the .pem multiple times in those cases where the root issuer of the CA is also embedded in the file (which is the case with e.g. Let's Encrypt). This is better than the current implementation, and I couldn't easily find a way to do the same in-memory. I'd rather use the standard API here. See 7a85d3c
Previously, the CA key, the client certificate and client key were stored as PEMs on disk for invoking OpenSSL calls.
This PR uses the equivalent OpenSSL calls that operate on in-memory data, thereby avoiding writing these sensitive data to disk at all. (Previously discussed in PR #203.)