Open fazledyn-or opened 1 year ago
Thanks for reporting this.
That code is pretty old and I'm wondering how much of it still relevant. It appears a lot of it was needed in the past due to compatibility with old Python version.
I would personally feel much more comfortable if that code could be simplified to reduce all the possible permutations and edge cases. This should reduce the surface area and make security and other issues less likely.
And in general I think that code is not following best practices. Doing something like this here (https://github.com/apache/libcloud/blob/abba8c1719a8bda6db8efde2d46fd1b423ae4304/libcloud/container/types.py#L19) and simply accepting any server certificate when that error is received seems like a bad and insecure practice to me. So if anything, that could should be removed.
If we want to leave such option in the code, it should be an explicit opt-in by the end user - they need to understand and accept the consequences if they chose to skip ca cert validation.
Hi @Kami, thanks for replying. I think we can apply the following fixes-
We can rewrite the vsphere.py
file into something like this- we put option for keyfile
and certfile
. Then it's completely depends on the user. They may not use the certificate chain but the support is there in the library.
...
context = ssl.create_default_context(cafile=ca_cert)
if certfile and keyfile:
context.load_cert_chain(certfile=certfile, keyfile=keyfile)
self.connection = connect.SmartConnect(
host=host,
port=port,
user=username,
pwd=password,
sslContext=context,
)
Since VSphereNodeDriver
is used by this class too, we add support for certificate chain here too. The updated code would look something like this-
def __init__(self, key, secret=None, secure=True, host=None, port=443, ca_cert=None, certfile=None, keyfile=None):
...
if ca_cert:
self.connection.connection.ca_cert = ca_cert
else:
self.connection.connection.ca_cert = False
if certfile and keyfile:
self.connection.connection.certfile = certfile
self.connection.connection.keyfile = keyfile
else:
self.connection.connection.certfile = False
self.connection.connection.keyfile = False
And then use it as below-
self.driver_soap = VSphereNodeDriver(
...
ca_cert=self.connection.connection.ca_cert,
certfile=self.connection.connection.certfile,
keyfile=self.connection.connection.keyfile,
)
Since I don't have the entire context about the whole project, I think simply replacing PROTOCOL_SSLv23
with PROTOCOL_TLS
will suffice. The fixed code would look something like this-
if "certificate verify failed" in error_message:
# bypass self signed certificates
try:
context = ssl.SSLContext(ssl.PROTOCOL_TLS)
context.verify_mode = ssl.CERT_NONE
Please let me know what you think.
Summary
This bug report is created by manually analyzing the source codes based on two fixes generated by Intelligent Code Repair tool (iCR).
Detailed Information
Suggested Fix 1
In your project file libcloud/compute/drivers/vsphere.py on Line 111, there’s a code segment that goes-
While triaging your repository, we noticed that the
connect.SmartConnect
method frompyVim
library uses a method calledConnect
that calls a method called__Login
which creates aSoapStubAdapter
class object. A comment on that class on Line 1380 - 1384 goes-However, in your source file the Certificate Chain isn’t loaded into
sslContext
object. We suggest that you load the certificate chain into thesslContext
object as mentioned in the comments.Suggested Fix 2
In the same file on Line 131 - 135, it goes-
Now, it says here that the following code is to bypass the self-signed certificates. In this case, the official documentation for ssl says-
To clear the confusion, it’s suggested that you use
PROTOCOL_TLS
while instantiating thecontext
object. However, if the code is used for some other reason that bypassing self-signed certificates, please let us have a discussion.CLA Requirements:
This section is only relevant if your project requires contributors to sign a Contributor License Agreement (CLA) for external contributions.
All contributed commits are already automatically signed off.
The meaning of a signoff depends on the project, but it typically certifies that committer has the rights to submit this work under the same license and agrees to a Developer Certificate of Origin (see https://developercertificate.org/ for more information).
Sponsorship and Support
This work is done by the security researchers from OpenRefactory and is supported by the Open Source Security Foundation (OpenSSF): Project Alpha-Omega. Alpha-Omega is a project partnering with open source software project maintainers to systematically find new, as-yet-undiscovered vulnerabilities in open source code - and get them fixed - to improve global software supply chain security.
The bug is found by running the iCR tool by OpenRefactory, Inc. and then manually triaging the results.