Implement certificate lookup, closes #24 #29
Conversation
If set, CA certificates will not be used when downloading the runtime
|
Tested successfully on openSUSE Leap 15.5 |
TheAssassin
left a comment
There was a problem hiding this comment.
This is not an in-depth code review. Let's call it a workflow review. When all of the points are discussed, I'll have another look.
| - `APPIMAGETOOL_SIGN_PASSPHRASE`: If the `--sign-key` is encrypted and requires a passphrase to be used for signing (and, for some reason, GnuPG cannot be used interactively, e.g., in a CI environment), this environment variable can be used to safely pass the key. | ||
| - `VERSION`: This value will be inserted by appimagetool into the root desktop file and (if the destination parameter is not provided by the user) in the output filename. | ||
| - `APPIMAGETOOL_FETCH_RUNTIME_INSECURE`: If set, CA certificates will not be used when downloading the runtime. | ||
| - `SSL_CERT_FILE`: If set, CA certificates will be looked up in this file when downloading the runtime. |
There was a problem hiding this comment.
Should be CA_* (or maybe even APPIMAGETOOL_CA_*).
| - `APPIMAGETOOL_FORCE_SIGN`: By default, if signing fails, appimagetool just logs a warning but will not abort immediately. If this environment variable is set, appimagetool exits with a non-zero return code. | ||
| - `APPIMAGETOOL_SIGN_PASSPHRASE`: If the `--sign-key` is encrypted and requires a passphrase to be used for signing (and, for some reason, GnuPG cannot be used interactively, e.g., in a CI environment), this environment variable can be used to safely pass the key. | ||
| - `VERSION`: This value will be inserted by appimagetool into the root desktop file and (if the destination parameter is not provided by the user) in the output filename. | ||
| - `APPIMAGETOOL_FETCH_RUNTIME_INSECURE`: If set, CA certificates will not be used when downloading the runtime. |
There was a problem hiding this comment.
We should really emphasize the security issues.
... maybe we should just not mention this option at all? Or even remove it completely?
| if (verbose) | ||
| printf("Size of the embedded runtime: %d bytes\n", size); | ||
|
|
||
| if (verbose) |
There was a problem hiding this comment.
This change is completely unrelated. You should either move your commit to a new branch and send a separate PR or clean up your Git history so we can merge it as a part of this PR.
... also, I'm not sure whether it's redundant to the other messages.
| *.*swp* | ||
| *.bak | ||
| *.AppImage* | ||
| squashfs-root/ |
There was a problem hiding this comment.
Trailing line break missing.
| // use SSL_CERT_FILE and/or SSL_CERT_DIR if set, otherwise check for common locations | ||
| // https://curl.haxx.se/docs/sslcerts.html | ||
| // https://gitlab.com/probono/platformissues#certificates | ||
| // https://go.dev/src/crypto/x509/root_linux.go | ||
| if (getenv("SSL_CERT_FILE") != NULL) { | ||
| if (curl_easy_setopt(handle, CURLOPT_CAINFO, getenv("SSL_CERT_FILE")) == CURLE_OK) { | ||
| fprintf(stderr, "Using certificate file %s\n", getenv("SSL_CERT_FILE")); | ||
| } | ||
| } else { | ||
| curl_easy_setopt(handle, CURLOPT_CAINFO, NULL); | ||
| const char* certFiles[] = { | ||
| "/etc/ssl/certs/ca-certificates.crt", // Debian/Ubuntu/Gentoo etc. | ||
| "/etc/pki/tls/certs/ca-bundle.crt", // Fedora/RHEL 6 | ||
| "/etc/ssl/ca-bundle.pem", // OpenSUSE | ||
| "/etc/pki/tls/cacert.pem", // OpenELEC | ||
| "/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem", // CentOS/RHEL 7 | ||
| "/etc/ssl/cert.pem", // Alpine Linux | ||
| }; | ||
| for (size_t i = 0; i < sizeof(certFiles) / sizeof(certFiles[0]); ++i) { | ||
| if (access(certFiles[i], F_OK) != -1) { | ||
| if (curl_easy_setopt(handle, CURLOPT_CAINFO, certFiles[i]) == CURLE_OK) { | ||
| fprintf(stderr, "Using certificate file %s\n", certFiles[i]); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (getenv("SSL_CERT_DIR") != NULL) { | ||
| if (curl_easy_setopt(handle, CURLOPT_CAPATH, getenv("SSL_CERT_DIR")) == CURLE_OK) { | ||
| fprintf(stderr, "Using certificate directory %s\n", getenv("SSL_CERT_DIR")); | ||
| } | ||
| } else { | ||
| curl_easy_setopt(handle, CURLOPT_CAPATH, NULL); | ||
| const char* certDirectories[] = { | ||
| "/etc/ssl/certs", // SLES10/SLES11, https://golang.org/issue/12139 | ||
| "/etc/pki/tls/certs", // Fedora/RHEL | ||
| "/system/etc/security/cacerts", // Android | ||
| }; | ||
|
|
||
| for (size_t i = 0; i < sizeof(certDirectories) / sizeof(certDirectories[0]); ++i) { | ||
| if (access(certDirectories[i], F_OK) != -1) { | ||
| if (curl_easy_setopt(handle, CURLOPT_CAPATH, certDirectories[i]) == CURLE_OK) { | ||
| fprintf(stderr, "Using certificate directory %s\n", certDirectories[i]); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
All of this code should be in a separate function. This is pure bloat to this one.
| "/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem", // CentOS/RHEL 7 | ||
| "/etc/ssl/cert.pem", // Alpine Linux | ||
| }; | ||
| for (size_t i = 0; i < sizeof(certFiles) / sizeof(certFiles[0]); ++i) { |
There was a problem hiding this comment.
Might be easier to just use a NULL-terminated list above. This works of course, but it might be more readable. You don't need any numeric index then, but can just ++ your pointer until you encounter a NULL value. We do this a lot in this code.
| fprintf(stderr, "Using certificate directory %s\n", getenv("SSL_CERT_DIR")); | ||
| } | ||
| } else { | ||
| curl_easy_setopt(handle, CURLOPT_CAPATH, NULL); |
There was a problem hiding this comment.
Why? You do not need to NULL it explicitly. Code like this needs comments that explain the rationale.
| const char* certDirectories[] = { | ||
| "/etc/ssl/certs", // SLES10/SLES11, https://golang.org/issue/12139 | ||
| "/etc/pki/tls/certs", // Fedora/RHEL | ||
| "/system/etc/security/cacerts", // Android |
There was a problem hiding this comment.
I'm not saying we absolutely need to remove it, but do we really need Android support for this tool?
| const char* certDirectories[] = { | ||
| "/etc/ssl/certs", // SLES10/SLES11, https://golang.org/issue/12139 | ||
| "/etc/pki/tls/certs", // Fedora/RHEL | ||
| "/system/etc/security/cacerts", // Android |
There was a problem hiding this comment.
Please avoid the use of in-line comments. Especially ones aligned like these are just annoying to work with.
| } | ||
|
|
||
| curl_easy_cleanup(handle); | ||
| handle = NULL; |
There was a problem hiding this comment.
The code below assumes in its error handling that the handle has been cleaned up. If you really want to reuse it, you need to continuously call curl_easy_cleanup wherever the function returns.
... this is why we need C++ sooner than later. I consider adding some before the 1.0.0 release. Handling object lifetime correctly is much easier there due to RAII.
|
See #32 for an alternative implementation. This alternative version does not support the environment variables proposed in this PR. In my opinion, it is a viable workaround to require from users to manually download the runtime if the TLS validation fails. And if it's a bug, we want to motivate them to create an issue this way. |
|
Closing in favor of #32 |
Tested on openSUSE.
SSL_CERT_FILEand/orSSL_CERT_DIRenvironment variables if set, otherwise check for common locationsAPPIMAGETOOL_FETCH_RUNTIME_INSECUREis setReferences: