When we dynamically link against OpenSSL, the bundles are not portable, as CentOS and friends use other SO_NAMEs than Debian and friends. Also, soft-fallback for later features such as host name verification is hard.
When we statically link, we might lack behind the OS, the binaries are bigger, and the legal situation is less clear.
So we now support not linking at all but rather lookup all necessary functions/symbols at runtime.
This can be disabled with SQSSL_OPENSSL_LINKED which effectively results in the dynamically-linked behavior. (This is preferable for platform builds, eg, debs and rpms) You can view, comment on, or merge this pull request online at:
https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/205
-- Commit Summary --
* [SqueakSSL] Overlay OpenSSL for linux/unix
-- File Changes --
M platforms/unix/plugins/SqueakSSL/Makefile.inc (9) M platforms/unix/plugins/SqueakSSL/config.cmake (10) A platforms/unix/plugins/SqueakSSL/openssl_overlay.h (299) M platforms/unix/plugins/SqueakSSL/sqUnixOpenSSL.c (173)
-- Patch Links --
https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/205.patch https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/205.diff
@krono pushed 1 commit.
8e48e6f Allow for possibly undefined X509 constant
Thanks, @krono! These are important SqueakSSL plugin updates and we might want them in the release which is due in two days. I'll give everyone 12 hours from now to review the code changes. If no one objects, I will merge this in once CI has passed.
@krono Could you please provide some concrete examples that can be used to test these changes from inside an image, so it's easier to verify these changes on all platforms?
/cc @eliotmiranda @estebanlm
fniephaus commented on this pull request.
@@ -432,19 +438,20 @@ sqInt sqConnectSSL(sqInt handle, char* srcBuf, sqInt srcLen, char *dstBuf, sqInt
if (ssl->serverName) { const size_t serverNameLength = strnlen(ssl->serverName, MAX_HOSTNAME_LENGTH); -#ifdef X509_CHECK_FLAG_SINGLE_LABEL_SUBDOMAINS - if(ssl->loglevel) printf("sqConnectSSL: X509_check_host."); - /* Try IP first, expect INVALID_IP_STRING to continue with hostname */ - matched = (enum sqMatchResult) X509_check_ip_asc(cert, ssl->serverName, 0); - if (matched == INVALID_IP_STRING) { - matched = (enum sqMatchResult) X509_check_host(cert, ssl->serverName, serverNameLength, X509_CHECK_FLAG_SINGLE_LABEL_SUBDOMAINS, NULL); - } -#else - matched = sqVerifyIP(ssl, cert, ssl->serverName, serverNameLength); - if (matched == INVALID_IP_STRING) { - matched = sqVerifyDNS(ssl, cert, ssl->serverName, serverNameLength); + //#ifdef X509_CHECK_FLAG_SINGLE_LABEL_SUBDOMAINS
Is this comment/ifdef still needed?
+#define sqo_SKM_sk_value(type, st,i) \
+ ((type *)sqo_sk_value(CHECKED_STACK_OF(type, st), i)) +#define sqo_SKM_sk_free(type, st) \ + sqo_sk_free(CHECKED_STACK_OF(type, st)) +#define sqo_SKM_sk_pop_free(type, st, free_func) \ + sqo_sk_pop_free(CHECKED_STACK_OF(type, st), CHECKED_SK_FREE_FUNC(type, free_func)) +#define sqo_sk_GENERAL_NAME_num(st) \ + sqo_SKM_sk_num(GENERAL_NAME, (st)) +#define sqo_sk_GENERAL_NAME_value(st, i) \ + sqo_SKM_sk_value(GENERAL_NAME, (st), (i)) +#define sqo_sk_GENERAL_NAME_free(st) \ + sqo_SKM_sk_free(GENERAL_NAME, (st)) +#define sqo_sk_GENERAL_NAME_pop_free(st, free_func) \ + sqo_SKM_sk_pop_free(GENERAL_NAME, (st), (free_func)) + +#if !defined(X509_CHECK_FLAG_SINGLE_LABEL_SUBDOMAINS)
Could you add a comment when this is not defined? Otherwise, prefix it as well? -> `sqo_X509_CHECK_FLAG_SINGLE_LABEL_SUBDOMAINS`
@@ -275,14 +275,20 @@ sqInt sqCreateSSL(void) {
sqInt handle = 0; sqSSL *ssl = NULL;
- SSL_library_init(); - SSL_load_error_strings(); + if (!wasInitialized) { + if (!loadLibrary()) { + return 0; + } + sqo_SSL_library_init();
Fix indentation
@@ -275,14 +275,20 @@ sqInt sqCreateSSL(void) {
sqInt handle = 0; sqSSL *ssl = NULL;
- SSL_library_init(); - SSL_load_error_strings(); + if (!wasInitialized) { + if (!loadLibrary()) { + return 0; + } + sqo_SSL_library_init(); + sqo_SSL_load_error_strings();
Same
if ((sAN->type == matchType) &&
sqVerifySAN(ssl, sAN, serverName, serverNameLength, matchType)) { matchFound = MATCH_FOUND; break; } } - sk_GENERAL_NAME_pop_free(sANs, GENERAL_NAME_free); + sqo_sk_GENERAL_NAME_pop_free(sANs, (void(*)(void*))sqo_sk_free);
I don't understand what's going on here, but please double check if `(void(*)(void*))sqo_sk_free` is the correct substitution for `GENERAL_NAME_free` here.
+#if OPENSSL_VERSION_NUMBER >= 0x10000000L
+ _C(sqo_sk_new_null = (_STACK *(*)(void)) _sqo_find("sk_new_null")); + _C(sqo_sk_push = (int (*)(_STACK *st, void *data)) _sqo_find("sk_push")); + _C(sqo_sk_free = (void (*)(_STACK *st)) _sqo_find("sk_free")); + _C(sqo_sk_value = (void *(*)(const _STACK *st, int i)) _sqo_find("sk_value")); + _C(sqo_sk_num = (int (*)(const _STACK *st)) _sqo_find("sk_num")); + _C(sqo_sk_pop_free = (void (*)(_STACK *st, void (*func) (void *))) _sqo_find("sk_pop_free")); +#else + _C(sqo_sk_new_null = (STACK *(*)(void)) _sqo_find("sk_new_null")); + _C(sqo_sk_push = (int (*)(STACK *st, char *data)) _sqo_find("sk_push")); + _C(sqo_sk_free = (void (*)(STACK *st)) _sqo_find("sk_free")); + _C(sqo_sk_value = (char *(*)(STACK *st, int i)) _sqo_find("sk_value")); + _C(sqo_sk_num = (int (*)(STACK *st)) _sqo_find("sk_num")); + _C(sqo_sk_pop_free = (void (*)(STACK *st, void (*func) (void *))) _sqo_find("sk_pop_free")); +#endif // OPENSSL_VERSION_NUMBER >= 0x10000000L + return true;
Can't this be in the previous `#if OPENSSL_VERSION_NUMBER >= 0x10000000L` in line 282?
krono commented on this pull request.
if ((sAN->type == matchType) &&
sqVerifySAN(ssl, sAN, serverName, serverNameLength, matchType)) { matchFound = MATCH_FOUND; break; } } - sk_GENERAL_NAME_pop_free(sANs, GENERAL_NAME_free); + sqo_sk_GENERAL_NAME_pop_free(sANs, (void(*)(void*))sqo_sk_free);
Because some expansion is taking place here that cannot be easily emulate. However, all `..._free` in the stack sense ultimately end up _being_ `sqo_sk_free`. Qt does it similarly.
krono commented on this pull request.
@@ -432,19 +438,20 @@ sqInt sqConnectSSL(sqInt handle, char* srcBuf, sqInt srcLen, char *dstBuf, sqInt
if (ssl->serverName) { const size_t serverNameLength = strnlen(ssl->serverName, MAX_HOSTNAME_LENGTH); -#ifdef X509_CHECK_FLAG_SINGLE_LABEL_SUBDOMAINS - if(ssl->loglevel) printf("sqConnectSSL: X509_check_host."); - /* Try IP first, expect INVALID_IP_STRING to continue with hostname */ - matched = (enum sqMatchResult) X509_check_ip_asc(cert, ssl->serverName, 0); - if (matched == INVALID_IP_STRING) { - matched = (enum sqMatchResult) X509_check_host(cert, ssl->serverName, serverNameLength, X509_CHECK_FLAG_SINGLE_LABEL_SUBDOMAINS, NULL); - } -#else - matched = sqVerifyIP(ssl, cert, ssl->serverName, serverNameLength); - if (matched == INVALID_IP_STRING) { - matched = sqVerifyDNS(ssl, cert, ssl->serverName, serverNameLength); + //#ifdef X509_CHECK_FLAG_SINGLE_LABEL_SUBDOMAINS
No, its just a reminder.
fniephaus commented on this pull request.
@@ -432,19 +438,20 @@ sqInt sqConnectSSL(sqInt handle, char* srcBuf, sqInt srcLen, char *dstBuf, sqInt
if (ssl->serverName) { const size_t serverNameLength = strnlen(ssl->serverName, MAX_HOSTNAME_LENGTH); -#ifdef X509_CHECK_FLAG_SINGLE_LABEL_SUBDOMAINS - if(ssl->loglevel) printf("sqConnectSSL: X509_check_host."); - /* Try IP first, expect INVALID_IP_STRING to continue with hostname */ - matched = (enum sqMatchResult) X509_check_ip_asc(cert, ssl->serverName, 0); - if (matched == INVALID_IP_STRING) { - matched = (enum sqMatchResult) X509_check_host(cert, ssl->serverName, serverNameLength, X509_CHECK_FLAG_SINGLE_LABEL_SUBDOMAINS, NULL); - } -#else - matched = sqVerifyIP(ssl, cert, ssl->serverName, serverNameLength); - if (matched == INVALID_IP_STRING) { - matched = sqVerifyDNS(ssl, cert, ssl->serverName, serverNameLength); + //#ifdef X509_CHECK_FLAG_SINGLE_LABEL_SUBDOMAINS
It doesn't remind me of anything, maybe add a comment? ;)
Here's the most simple use case:
```Smalltalk (WebClient httpGet: 'https://danluu.com') content ```
@krono pushed 1 commit.
438311c Refactor the overlay, comments where due
@fniephaus pushed 1 commit.
74e443c Nit picks [ci skip]
With the help of @krono, I was able to verify that this is working fine for squeak.cog.spur on CentOS (x64 and i386).
Merged #205.
vm-dev@lists.squeakfoundation.org
-
Fabio Niephaus -
Tobias Pape