Repository: twisted/twisted

Issue:
TLS client endpoint parser mishandles bindAddress 
**Describe the incorrect behavior you saw**

Creating a TLS client endpoint from a string, and including a `bindAddress=ADDR` value (e.g. `tls:github.com:443:bindAddress=1.2.3.4` will cause a crash during connection.

The `twisted.internet.endpoints._parseClientTLS` function passes the `ADDR` into the `HostnameEndpoint()` constructor as a *string*, whereas the function is expecting a tuple of `(ADDR, PORT)`. This doesn't trigger an error during Endpoint construction, however actually trying to connect with the endpoint (`ep.connect(factory)`) gets an error as it attempts to dereference the first element of the tuple and examine it as a hostname.

The following test program ought to connect to Github's TLS endpoint and say hello (note that `bindAddress=`, without a hostname, is a legitimate way to bind to the "ANY" interface, and avoids the need to e.g. call `ifconfig` to figure out a suitable host for binding first.. `bindAddress=0.0.0.0` should work the same way).

```python
from twisted.internet import endpoints, task, protocol, defer

class P(protocol.Protocol):
    def connectionMade(self):
        print("connectionMade")
        self.transport.write(b"GET / HTTP/1.0\n\n")
        self.transport.loseConnection()
    def connectionLost(self, reason):
        self.factory.d.callback("done")
    def dataReceived(self, data):
        print("dataReceived", data)

@defer.inlineCallbacks
def go(reactor):
    #ep = endpoints.clientFromString(reactor, 'tcp:github.com:443:bindAddress=')
    #print(ep._bindAddress)
    ep = endpoints.clientFromString(reactor, 'tls:github.com:443:bindAddress=')
    #print(ep._wrappedEndpoint._bindAddress)
    client_f = protocol.Factory.forProtocol(P)
    client_f.d = defer.Deferred()
    c = yield ep.connect(client_f)
    yield client_f.d
task.react(go)
```

yields the following error:

```
% python3 t.py
main function encountered error
Traceback (most recent call last):
  File "/Users/warner/stuff/python/twisted/src/twisted/internet/defer.py", line 2013, in _inlineCallbacks
    result = context.run(
  File "/Users/warner/stuff/python/twisted/src/twisted/python/failure.py", line 467, in throwExceptionIntoGenerator
    return g.throw(self.value.with_traceback(self.tb))
  File "/Users/warner/stuff/python/twisted/t.py", line 21, in go
    c = yield ep.connect(client_f)
  File "/Users/warner/stuff/python/twisted/src/twisted/internet/endpoints.py", line 600, in connect
    self._reactor.connectTCP(
  File "/Users/warner/stuff/python/twisted/src/twisted/internet/posixbase.py", line 368, in connectTCP
    c.connect()
  File "/Users/warner/stuff/python/twisted/src/twisted/internet/base.py", line 1266, in connect
    self.transport: Optional[Client] = self._makeTransport()
  File "/Users/warner/stuff/python/twisted/src/twisted/internet/tcp.py", line 1529, in _makeTransport
    return Client(self.host, self.port, self.bindAddress, self, self.reactor)
  File "/Users/warner/stuff/python/twisted/src/twisted/internet/tcp.py", line 730, in __init__
    if abstract.isIPv6Address(bindAddress[0]):
builtins.IndexError: string index out of range
```

The `bindAddress[0]` is attempting to extract the host portion of the `(host, port)` tuple, but in this case `bindAddress` is an empty string, so `bindAddress[0]` is an attempt to extract its zeroth character, which fails with IndexError.

If we change the program to use `bindAddress=0.0.0.0`, we get:

```
  File "/Users/warner/stuff/python/twisted/src/twisted/internet/tcp.py", line 1529, in _makeTransport
    return Client(self.host, self.port, self.bindAddress, self, self.reactor)
  File "/Users/warner/stuff/python/twisted/src/twisted/internet/tcp.py", line 734, in __init__
    skt.bind(bindinfo)
builtins.TypeError: bind(): AF_INET address must be tuple, not str
```

because `bindinfo` is a copy of `bindAddress`, and `socket.bind()` expects a tuple.

Changing the program to use the `tcp:` endpoint instead of the `tls:` one yields success:

```
% python3 t.py
connectionMade
```

The problem lies in `_parseClientTLS`. The `bindAddress` is extracted as a string:

https://github.com/twisted/twisted/blob/05cc97afa3c9c00386e5cd0c50e0807e9ef963d4/src/twisted/internet/endpoints.py#L2287-L2291

but then passed into `HostnameEndpoint()` as is:

https://github.com/twisted/twisted/blob/05cc97afa3c9c00386e5cd0c50e0807e9ef963d4/src/twisted/internet/endpoints.py#L2300-L2302

whereas for e.g. `_parseClientTCP`, it converts the string into a `(string, 0)` tuple:

https://github.com/twisted/twisted/blob/05cc97afa3c9c00386e5cd0c50e0807e9ef963d4/src/twisted/internet/endpoints.py#L1880

It looks like the types are not consistent:
* `_parseClientTLS` passes a string `bindAddress` into `HostnameEndpoint`
* [`HostnameEndpoint` is documented](https://github.com/twisted/twisted/blob/05cc97afa3c9c00386e5cd0c50e0807e9ef963d4/src/twisted/internet/endpoints.py#L811-L813) to take a bytestring as `bindAddress`, and after DNS resolution, passes it unmodified into [`TCP6ClientEndpoint`](https://github.com/twisted/twisted/blob/05cc97afa3c9c00386e5cd0c50e0807e9ef963d4/src/twisted/internet/endpoints.py#L975-L980) or [`TCP4ClientEndpoint`](https://github.com/twisted/twisted/blob/05cc97afa3c9c00386e5cd0c50e0807e9ef963d4/src/twisted/internet/endpoints.py#L982-L988)
* [`TCP4ClientEndpoint` is documented](https://github.com/twisted/twisted/blob/05cc97afa3c9c00386e5cd0c50e0807e9ef963d4/src/twisted/internet/endpoints.py#L584-L586) to take `bindAddress` as a *tuple*, as is TCP6ClientEndpoint

I think stronger typechecking might have caught the inconsistency.

`test_endpoints.py` is meant to test this code. However it asserts that the parsed `hostnameEndpoint._bindAddress` is a string, when it should be asserting a tuple.

https://github.com/twisted/twisted/blob/05cc97afa3c9c00386e5cd0c50e0807e9ef963d4/src/twisted/internet/test/test_endpoints.py#L4199

I couldn't find a test that uses `endpointFromtString("tls:...:bindAddress=ADDR")` and then actually tries an `ep.connect()` on it, possibly because generating a suitable TLS server cert is too much of a hassle, and also because it's not obvious that you can use `bindAddress=` or `bindAddress=0.0.0.0` to avoid also needing to probe for a safe address to bind to. The `test_tls` case uses `endpoint.connect()`, but I think with mocked out values which means it doesn't make it all the way to `_BaseTCPClient` and the call to `skt.bind()`, where the error would arise.

My recommended fix is:
* add an `assert(isinstance(bindaddress, (tuple, type(None))))` to _BaseTCPClient.__init__`, `TCP4ClientEndpoint`, and `TCP6ClientEndpoint`
* change the specification of `HostnameEndpoint` to expect a `tuple` for the `bindAddress`
* change `_parseClientTLS()` to call `HostnameEndpoint()` with `(bindAddress, 0)` instead of `bindAddress`
* update test_endpoints.py to check tuple-ness of parsed bindAddress

I'd really like to incorporate a proper `ep.connect()` test, but I didn't see an obvious way to do so.. maybe someone more familiar with the code can come up with a good approach.

I'll attach a PR with the fix and an update to the unit tests.

**Testing environment**

MacOS and Linux. Twisted 24.7.0 and current trunk. Default reactor.


Discussion:
codspeed-hq[bot]: ## [CodSpeed Performance Report](https://codspeed.io/twisted/twisted/branches/warner:12325-fix-tls-bindaddress)

### Merging #12326 will **not alter performance**

<sub>Comparing <code>warner:12325-fix-tls-bindaddress</code> (beb7b23) with <code>trunk</code> (05cc97a)</sub>



### Summary

`✅ 23` untouched benchmarks





warner: Huh, so clearly those asserts are too aggressive, since lots of other tests started failing. Or, those other tests are similarly confused about what `bindAddress` ought to be generated. Or *I'm* confused about the same.

I'll dig deeper.

warner: Results of digging deeper are in the issue: https://github.com/twisted/twisted/issues/12325#issuecomment-2381471729

adiroiban: Hi Brian. Thanks for this PR that is nicely put together.

Is this ready for review ?


warner: yes please

Things I'm not sure of:
* str vs bytes? should `ep._bindAddress` always hold one or the other? I think the status quo is to hold whatever the caller provided, which seems non-ideal
* am I testing enough? I'm not satisfied that my tests don't capture the original crash (`ep.connect()`), do you know of a test I could copy to build something more end-to-end?
* are the assertions I added too much? they might not be sufficiently idiomatic. I've been working in JavaScript and Rust for a while and I've gotten paranoid about making type mistakes in untyped languages, so I might be overrotating on the asserts.
warner: how do I request a review from @glyph ? lemme see what this does:

please review

adiroiban: To request a review you can use the GitHub UI 

![image](https://github.com/user-attachments/assets/db204ed4-bc8a-4325-a56b-47a02766cbb7)


We usually request a review from the team... so any team member can review and approve the change

-----

Mentioning another team member can also do the job.

But I don't know if Glyph will have time for the review.

So, as long as the changes from the PR looks good, we can have them merge

We can follow with more changes at any time
glyph: Feel free to @-mention me whenever you want me to look at something, but let's not request reviews from individual people; the robot requests from twisted-contributors on purpose, we already have enough process bottlenecks :)
warner: > Feel free to @-mention me whenever you want me to look at something, but let's not request reviews from individual people; the robot requests from twisted-contributors on purpose, we already have enough process bottlenecks :)

Aye, makes sense. I tagged you just because in https://github.com/twisted/twisted/pull/12326#pullrequestreview-2341602050 @adiroiban said:

> I don't really understand how clientFromString should be used . I had a chat with Glyph and he told me that I am not using the API the was it was designed

so I figured we were both missing something, and should follow your lead.

Patch:
diff --git a/src/twisted/internet/endpoints.py b/src/twisted/internet/endpoints.py
index 7ab1d81731..a98fd2ba43 100644
--- a/src/twisted/internet/endpoints.py
+++ b/src/twisted/internet/endpoints.py
@@ -808,9 +808,18 @@ class HostnameEndpoint:
             seconds to wait before assuming the connection has failed.
         @type timeout: L{float} or L{int}
 
-        @param bindAddress: the local address of the network interface to make
-            the connections from.
-        @type bindAddress: L{bytes}
+        @param bindAddress: The client socket normally uses whatever
+            local interface (eth0, en0, lo, etc) is best suited for the
+            target address, and a randomly-assigned port. This argument
+            allows that local address/port to be overridden. Providing
+            just an address (as a str) will bind the client socket to
+            whichever interface is assigned that address. Providing a
+            tuple of (str, int) will bind it to both an interface and a
+            specific local port. To bind the port, but leave the
+            interface unbound, use a tuple of ("", port), or ("0.0.0.0",
+            port) for IPv4, or ("::0", port) for IPv6. To leave both
+            interface and port unbound, just use None.
+        @type bindAddress: L{str}, L{tuple}, or None
 
         @param attemptDelay: The number of seconds to delay between connection
             attempts.
@@ -827,6 +836,11 @@ class HostnameEndpoint:
         self._hostStr = self._hostBytes if bytes is str else self._hostText
         self._port = port
         self._timeout = timeout
+        if bindAddress is not None:
+            if isinstance(bindAddress, (bytes, str)):
+                bindAddress = (bindAddress, 0)
+            if isinstance(bindAddress[0], bytes):
+                bindAddress = (bindAddress[0].decode(), bindAddress[1])
         self._bindAddress = bindAddress
         if attemptDelay is None:
             attemptDelay = self._DEFAULT_ATTEMPT_DELAY
@@ -2299,7 +2313,9 @@ def _parseClientTLS(
         ),
         clientFromString(reactor, endpoint)
         if endpoint is not None
-        else HostnameEndpoint(reactor, _idnaBytes(host), port, timeout, bindAddress),
+        else HostnameEndpoint(
+            reactor, _idnaBytes(host), port, timeout, (bindAddress, 0)
+        ),
     )
 
 
diff --git a/src/twisted/internet/tcp.py b/src/twisted/internet/tcp.py
index 8f85025556..018d1912d2 100644
--- a/src/twisted/internet/tcp.py
+++ b/src/twisted/internet/tcp.py
@@ -727,6 +727,7 @@ class _BaseTCPClient:
             whenDone = None
         if whenDone and bindAddress is not None:
             try:
+                assert type(bindAddress) == tuple
                 if abstract.isIPv6Address(bindAddress[0]):
                     bindinfo = _resolveIPv6(*bindAddress)
                 else:
diff --git a/src/twisted/newsfragments/12325.bugfix b/src/twisted/newsfragments/12325.bugfix
new file mode 100644
index 0000000000..ed48539244
--- /dev/null
+++ b/src/twisted/newsfragments/12325.bugfix
@@ -0,0 +1,1 @@
+twisted.internet.endpoints.clientFromString for TLS endpoints with "bindAddress="  no longer crashes during connect. twisted.internet.endpoints.HostnameEndpoint() no longer crashes when given a bindAddress= argument that is just a string, and that argument now accepts either address strings or (address, port) tuples.
\ No newline at end of file


Test patch:
diff --git a/src/twisted/internet/test/test_endpoints.py b/src/twisted/internet/test/test_endpoints.py
index 7b755e61a3..b47b8181bc 100644
--- a/src/twisted/internet/test/test_endpoints.py
+++ b/src/twisted/internet/test/test_endpoints.py
@@ -58,7 +58,6 @@ from twisted.logger import ILogObserver, globalLogPublisher
 from twisted.plugin import getPlugins
 from twisted.protocols import basic, policies
 from twisted.python import log
-from twisted.python.compat import nativeString
 from twisted.python.components import proxyForInterface
 from twisted.python.failure import Failure
 from twisted.python.filepath import FilePath
@@ -2699,6 +2698,41 @@ class HostnameEndpointsFasterConnectionTests(unittest.TestCase):
         self.assertEqual([], self.mreactor.getDelayedCalls())
 
 
+class HostnameEndpointBindAddressTypes(unittest.TestCase):
+    """
+    Tests that HostnameEndpoint accepts all specified types for the
+    'bindAddress=' argument.
+    """
+
+    def setUp(self):
+        self.drr = deterministicResolvingReactor(MemoryReactor(), ["127.0.0.1"])
+
+    def test_bytes(self):
+        ba = b"1.2.3.4"
+        ep = endpoints.HostnameEndpoint(self.drr, b"example.com", 80, bindAddress=ba)
+        self.assertEqual(ep._bindAddress, ("1.2.3.4", 0))
+
+    def test_str(self):
+        ba = "1.2.3.4"
+        ep = endpoints.HostnameEndpoint(self.drr, b"example.com", 80, bindAddress=ba)
+        self.assertEqual(ep._bindAddress, ("1.2.3.4", 0))
+
+    def test_tuple_bytes(self):
+        ba = (b"1.2.3.4", 1234)
+        ep = endpoints.HostnameEndpoint(self.drr, b"example.com", 80, bindAddress=ba)
+        self.assertEqual(ep._bindAddress, ("1.2.3.4", 1234))
+
+    def test_tuple_str(self):
+        ba = ("1.2.3.4", 1234)
+        ep = endpoints.HostnameEndpoint(self.drr, b"example.com", 80, bindAddress=ba)
+        self.assertEqual(ep._bindAddress, ("1.2.3.4", 1234))
+
+    def test_none(self):
+        ba = None
+        ep = endpoints.HostnameEndpoint(self.drr, b"example.com", 80, bindAddress=ba)
+        self.assertEqual(ep._bindAddress, None)
+
+
 @skipIf(skipSSL, skipSSLReason)
 class SSL4EndpointsTests(EndpointTestCaseMixin, unittest.TestCase):
     """
@@ -4189,14 +4223,14 @@ class WrapClientTLSParserTests(unittest.TestCase):
         reactor = object()
         endpoint = endpoints.clientFromString(
             reactor,
-            nativeString("tls:example.com:443:timeout=10:bindAddress=127.0.0.1"),
+            "tls:example.com:443:timeout=10:bindAddress=127.0.0.1",
         )
         hostnameEndpoint = endpoint._wrappedEndpoint
         self.assertIs(hostnameEndpoint._reactor, reactor)
         self.assertEqual(hostnameEndpoint._hostBytes, b"example.com")
         self.assertEqual(hostnameEndpoint._port, 443)
         self.assertEqual(hostnameEndpoint._timeout, 10)
-        self.assertEqual(hostnameEndpoint._bindAddress, nativeString("127.0.0.1"))
+        self.assertEqual(hostnameEndpoint._bindAddress, ("127.0.0.1", 0))
 
     def test_utf8Encoding(self):
         """
@@ -4239,14 +4273,18 @@ class WrapClientTLSParserTests(unittest.TestCase):
         # containing the cert itself for the CAs list.
         endpoint = endpoints.clientFromString(
             deterministicResolvingReactor(reactor, ["127.0.0.1"]),
-            "tls:localhost:4321:privateKey={}:certificate={}:trustRoots={}".format(
+            "tls:localhost:4321:privateKey={}:certificate={}:trustRoots={}:bindAddress=127.0.0.1".format(
                 escapedPEMPathName,
                 escapedPEMPathName,
                 endpoints.quoteStringArgument(pemPath.parent().path),
-            ).encode("ascii"),
+            ).encode(
+                "ascii"
+            ),
         )
         d = endpoint.connect(Factory.forProtocol(Protocol))
         host, port, factory, timeout, bindAddress = reactor.tcpClients.pop()
+        self.assertIs(type(bindAddress), tuple)
+        self.assertEqual(bindAddress, ("127.0.0.1", 0))
         clientProtocol = factory.buildProtocol(None)
         self.assertNoResult(d)
         assert clientProtocol is not None
diff --git a/src/twisted/web/test/test_agent.py b/src/twisted/web/test/test_agent.py
index d042f9caf7..fc193fbb1f 100644
--- a/src/twisted/web/test/test_agent.py
+++ b/src/twisted/web/test/test_agent.py
@@ -1256,7 +1256,7 @@ class AgentTests(
         agent = client.Agent(self.reactor, bindAddress="192.168.0.1")
         agent.request(b"GET", b"http://foo/")
         address = self.reactor.tcpClients.pop()[4]
-        self.assertEqual("192.168.0.1", address)
+        self.assertEqual(("192.168.0.1", 0), address)
 
     @skipIf(not sslPresent, "SSL not present, cannot run SSL tests.")
     def test_bindAddressSSL(self):
@@ -1267,7 +1267,7 @@ class AgentTests(
         agent = client.Agent(self.reactor, bindAddress="192.168.0.1")
         agent.request(b"GET", b"https://foo/")
         address = self.reactor.tcpClients.pop()[4]
-        self.assertEqual("192.168.0.1", address)
+        self.assertEqual(("192.168.0.1", 0), address)
 
     def test_responseIncludesRequest(self):
         """