Skip to content
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

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

Closed
adiroiban opened this issue Jan 22, 2024 · 5 comments · Fixed by #264
Closed

Comments

@adiroiban
Copy link
Contributor

adiroiban commented Jan 22, 2024

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

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
Copy link
Owner

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

@adiroiban
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

adiroiban commented Jan 23, 2024

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants