-
Notifications
You must be signed in to change notification settings - Fork 17
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
upd: readme to fix OpenSSL v1.0.2 errors #103
Conversation
Pull Request Test Coverage Report for Build 6273550137
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iurev I did some additional research on the topic and it appears that we don't need to introduce extra complexity in the code, the issue can be solved easily by configuring certificate store on the system.
For CentOS 7 specifically the most simple and safe approach that I came up with is:
ln -s /etc/pki/tls/cert.pem /etc/ssl/cert.pem
Which is compatible with subsequent update-ca-trust
runs and will benefit from CA bundle updates delivered via ca-certificates
.
I believe we should revert all the code changes and instead add a note into README on troubleshooting this SSL error.
# examples: | ||
# OpenSSL 1.0.2k-fips 26 Jan 2017 | ||
# OpenSSL 3.0.8 7 Feb 2023 (Library: OpenSSL 3.0.8 7 Feb 2023) | ||
`openssl version -v` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wouldn't work on default CentOS 7 system, because it only has openssl-libs
package installed, but the CLI util comes in a separate openssl
package.
$ docker run -t --rm centos:7 openssl version -v
docker: Error response from daemon: failed to create task for container: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: exec: "openssl": executable file not found in $PATH: unknown.
$ docker run -t --rm centos:7 rpm -q 'openssl' 'openssl-libs'
package openssl is not installed
openssl-libs-1.0.2k-19.el7.x86_64
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Yes! That's much safer and easier 🔥
src/coverage_reporter/api.cr
Outdated
rescue ex : OpenSSL::SSL::Error | ||
if OpenSSLVersion.new.can_fail? | ||
Log.error <<-ERROR | ||
Consider upgrading `openssl` library to version >= #{OpenSSLVersion::WORKS} or using --force-insecure-requests flag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're distributing statically compiled version of the reporter for all Linux systems, upgrading openssl
on the target machine wouldn't actually do anything. The issue here is incompatibility between the OpenSSL 3.0.9 included with the reporter, and the PKI infrastructure on the server (shared trusted root CA certs etc)
@iurev I found even better option to fix this on the client system — provide correct path to cert bundle file with the env var: $ coveralls done -r123 2>&1 | tail -2
⭐️ Calling parallel done webhook: https://coveralls.io/webhook
#<OpenSSL::SSL::Error:SSL_connect: error:0A000086:SSL routines::certificate verify failed>
$ env SSL_CERT_FILE=/etc/pki/tls/cert.pem coveralls done -r123 2>&1 | tail -2
✅ API Response: {"error":"Invalid repo token"}
- 💛, Coveralls This doesn't require any changes to system-wide configuration 🚀 |
@iurev @nebolsin thanks! I have shared the solution with the customer. I Cc'd Vitalii on the email thread so he can confirm I gave the right info, and be included if the customer replied with a failure, etc. MOVING THIS and the ORIGINAL ISSUE to IN REVIEW. |
Closing this PR. Added the changes directly to the latest README, here: 🙏 |
Closes https://github.com/coverallsapp/coveralls/issues/1829
⚡ Summary
Added section to the README which describes solution
☑️ Checklist