jborean93 / smbprotocol

Python SMBv2 and v3 Client
MIT License
322 stars 73 forks source link

smbprotocol.connection.Connection.connect() no longer shows the low-level connection error #259

Closed adiroiban closed 10 months ago

adiroiban commented 10 months ago

Since 1.11.0 smbprotocol.connection.Connection.connect() no longer shows the low-level connection error

This is a change introduced in 1.11.0

The change is in transport.py

-raise ValueError("Failed to connect to '%s:%s': %s" % (self.server, self.port, str(err))) from err
+raise ValueError(f"Failed to connect to '{self.server}:{self.port}'") from err

https://github.com/jborean93/smbprotocol/compare/v1.10.1...v1.11.0#diff-1037611d131586bd4201108e4c80be46a17a6a1a365766e7190be44673eac0ffL70

It helps to know more about why a connection failed... it can be a DNS error for example.

To reproduce, use this code

from smbprotocol.connection import Connection
conn = Connection('some-id', 'localhost', 1234)
conn.connect()

On 1.11.0 the error is just ValueError: Failed to connect to 'localhost:1234' On 1.10.1 the error is ValueError: Failed to connect to 'localhost:1234': [Errno 111] Connection refused

Below are the stack traces


1.11.0

Python 3.11.7 (4666189, Dec 11 2023, 13:04:52) [GCC 7.3.1 20180712 (Red Hat 7.3.1-17)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from smbprotocol.connection import Connection
conn = Connection('some-id', 'localhost', 1234)
conn.connect()
>>> >>> Traceback (most recent call last):
  File "/home/adi/chevah/server/build-py3/lib/python3.11/site-packages/smbprotocol/transport.py", line 66, in connect
    self._sock = socket.create_connection((self.server, self.port), timeout=self.timeout)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/adi/chevah/server/build-py3/lib/python3.11/socket.py", line 851, in create_connection
    raise exceptions[0]
  File "/home/adi/chevah/server/build-py3/lib/python3.11/socket.py", line 836, in create_connection
    sock.connect(sa)
ConnectionRefusedError: [Errno 111] Connection refused

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/adi/chevah/server/build-py3/lib/python3.11/site-packages/smbprotocol/connection.py", line 863, in connect
    self.transport.connect()
  File "/home/adi/chevah/server/build-py3/lib/python3.11/site-packages/smbprotocol/transport.py", line 68, in connect
    raise ValueError(f"Failed to connect to '{self.server}:{self.port}'") from err
ValueError: Failed to connect to 'localhost:1234'

1.10.1

Python 3.11.7 (4666189, Dec 11 2023, 13:04:52) [GCC 7.3.1 20180712 (Red Hat 7.3.1-17)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from smbprotocol.connection import Connection
conn = Connection('some-id', 'localhost', 1234)
conn.connect()
>>> >>> Traceback (most recent call last):
  File "/home/adi/chevah/server/build-py3/lib/python3.11/site-packages/smbprotocol/transport.py", line 68, in connect
    self._sock = socket.create_connection((self.server, self.port), timeout=self.timeout)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/adi/chevah/server/build-py3/lib/python3.11/socket.py", line 851, in create_connection
    raise exceptions[0]
  File "/home/adi/chevah/server/build-py3/lib/python3.11/socket.py", line 836, in create_connection
    sock.connect(sa)
ConnectionRefusedError: [Errno 111] Connection refused

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/adi/chevah/server/build-py3/lib/python3.11/site-packages/smbprotocol/connection.py", line 861, in connect
    self.transport.connect()
  File "/home/adi/chevah/server/build-py3/lib/python3.11/site-packages/smbprotocol/transport.py", line 70, in connect
    raise ValueError("Failed to connect to '%s:%s': %s" % (self.server, self.port, str(err))) from err
ValueError: Failed to connect to 'localhost:1234': [Errno 111] Connection refused
jborean93 commented 10 months ago

Thanks for the pickup, the error should contain the original error message like before.

adiroiban commented 10 months ago

I have only now upgraded from smbprotocol==1.5.1 as I was still on Python 2.7 :(

Beside this error, there was only another minor issue with Connection.cipher_id no longer being an instance of cryptography.hazmat.primitives.ciphers.aead but we now have smbprotocol.connection.Ciphers . I have checked the Changelog file but I could not find any reference for this backward-incompatible change.

I am running end to end tests with just NTLM for Samba, Windows 2018, Windows 2019 and Azure Files. I run the tests on linux-glic, linux-musl, linux-arm64, macos-m1 and windows-x64. All good.

Many thanks for maintaining this library :)


I am still at pyspnego==0.9.2 as with latest sspilib change the tests are failing on Windows. I will still investigating to see why things are failing. I will come back with a report on pyspnego ir sspilib.

jborean93 commented 10 months ago

Beside this error, there was only another minor issue with Connection.cipher_id no longer being an instance of cryptography.hazmat.primitives.ciphers.aead but we now have smbprotocol.connection.Ciphers .

Unfortunately this is a side effect of the poor choices I made when I first created this library. I made too many things public when I probably shouldn't have. In this case SMB 3.11 introduced the ability to select a difference encryption cipher during negotiation so instead of using AES CCM it can negotiate things like AES256 GCM and so on. Granted this is technically a breaking change but I am curious what you were using the cipher_id for.

Happy to amend the changelog and while I'll try to not do these things in the future I cannot guarantee that will always be the case.

adiroiban commented 10 months ago

Granted this is technically a breaking change but I am curious what you were using the cipher_id for.

You can update the changelog, as a drive by, but I guess not many people were using it.

Don't worry too much about the API. I think it might also be safe to make the Connection.cipher_id private and instead expose something like Connection.describeEncryption() which can be used for audit and troubleshooting.

or Connection.describeProtocol() to also include information about the negociated dialect.


I am using it for audit, to log the negociated encryption algorithm. So I have a helper to generate the "human readable" version of things like smbprotocol.connection.Ciphers.AES_128_GCM or smbprotocol.connection.SigningAlgorithms.HMAC_SHA256

adiroiban commented 10 months ago

I forgot to mention. I plan to create a PR for this by the start of the next week.