Salt: Salt TCP Transport with TLS enabled does not properly set the SNI value

[dcovello]

Description of #Issue/Question

With the recent implementation of TLS support for the Tornado TCP communication transport covered in #37449 I was hoping to be able to utilize the SNI attribute of TLS connections in a proxy to forward salt-minion connections to different salt-masters based on the hostname a salt-minion attempts to connect with. However upon setting up a salt-minion configured for TCP transport with ssl settings for TLS to connect to a salt-master through an SNI Proxy I observed that the value of the SNI set in the TLS handshake was the resolved IP-Address of the salt-master and not the hostname.

As a result of this observation I began to look through the minion code as to why this is happening and I found that the minion uses the resolve_dns function to define the 'master_uri' value within the opts dict in the following format

_/salt/minion.py_

def resolve_dns(opts, fallback=True, connect=True):

...

    ret['master_uri'] = 'tcp://{ip}:{port}'.format(ip=ret['master_ip'],
                                                   port=opts['master_port'])
return ret

If we then look into the tcp transport implementation we can observe that the opt['master_uri'] is passed onto the underlying Tornado TCP client implementation within the AsyncTCPReqChannel class

_/salt/transport/tcp.py_

class AsyncTCPReqChannel(salt.transport.client.ReqChannel):

...

def __singleton_init__(self, opts, **kwargs):
        self.opts = dict(opts)

        self.serial = salt.payload.Serial(self.opts)

        # crypt defaults to 'aes'
        self.crypt = kwargs.get('crypt', 'aes')

        self.io_loop = kwargs.get('io_loop') or tornado.ioloop.IOLoop.current()

        if self.crypt != 'clear':
            self.auth = salt.crypt.AsyncAuth(self.opts, io_loop=self.io_loop)

        resolver = kwargs.get('resolver')

        parse = urlparse.urlparse(self.opts['master_uri'])
        host, port = parse.netloc.rsplit(':', 1)
        self.master_addr = (host, int(port))
        self._closing = False
        self.message_client = SaltMessageClient(
            self.opts, host, int(port), io_loop=self.io_loop,
resolver=resolver)

From this I was able to determine that the salt minion never actually passes the hostname to the TCP transport implementation and instead passes the IP address which is then set as the SNI value by the tornado client implementation. The SNI is meant to allow multiple TLS services to be served by the same IP address but this cannot be achieved if the SNI is set to that IP address.

Setup

1. Salt-minion configured for TCP transport with TLS as described here (https://docs.saltstack.com/en/latest/topics/transports/tcp.html) with its salt-master endpoint set to a SNI Proxy
2. an SNI Proxy configured to proxy connections to the salt-master based on the SNI value in the TLS request .(you could probably also just inspect the TLS traffic between salt minion and master)
3. Salt-master configured for TCP transport with TLS

Attempt to connect the salt-minion to the salt-master through the SNI Proxy and observe the SNI value within the TLS handshake at the SNIProxy or using some sort of packet viewer.

Steps to Reproduce Issue

Attempt to connect the salt-minion to the salt-master through the SNI Proxy and observe the SNI value within the TLS handshake at the SNIProxy or using some sort of packet viewer.

Versions Report

Salt-Minion

Salt Version:
           Salt: 2016.11.1

Dependency Versions:
           cffi: 1.9.1
       cherrypy: Not Installed
       dateutil: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.8
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: 0.21.1
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.6
   mysql-python: Not Installed
      pycparser: 2.17
       pycrypto: 2.6.1
         pygit2: Not Installed
         Python: 2.7.12 (default, Nov 19 2016, 06:48:10)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 15.2.0
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.1.4

System Versions:
           dist: Ubuntu 16.04 xenial
        machine: x86_64
        release: 4.2.0-42-generic
         system: Linux
        version: Ubuntu 16.04 xenial

Salt-Master

Salt Version:
           Salt: 2016.11.0-n/a-9f370ed

Dependency Versions:
           cffi: 1.9.1
       cherrypy: unknown
       dateutil: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.8
        libgit2: 0.25.1
        libnacl: Not Installed
       M2Crypto: 0.21.1
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.6
   mysql-python: Not Installed
      pycparser: 2.17
       pycrypto: 2.6.1
         pygit2: 0.25.0
         Python: 2.7.12 (default, Nov 19 2016, 06:48:10)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 15.2.0
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.1.4

System Versions:
           dist: Ubuntu 16.04 xenial
        machine: x86_64
        release: 4.2.0-42-generic
         system: Linux
        version: Ubuntu 16.04 xenial

[Ch3LL]

Seems we need to add an option to not resolve the dns. I'll approve as a feature request. Thanks

[dhananjaysathe]

Is there a PR about this already? I need this functionality and could contribute a patch

[Ch3LL]

There is not and it has not been assigned out to any engineers yet so please feel free to contribute. It would be greatly appreciated.

[bbinet]

This feature would actually be a great enhancement to salt tcp transport!
@dhananjaysathe Have you been able to contribute a patch?

[taemon1337]

Any movement on this? I have the same issue.

[Ch3LL]

there is not any movement yet, please feel free to submit a PR

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[bbinet]

please do not close

[stale[bot]]

Thank you for updating this issue. It is no longer marked as stale.