Salt TCP Transport with TLS enabled does not properly set the SNI value
Opened this issue · 11 comments
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
- 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
- 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)
- 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
Seems we need to add an option to not resolve the dns. I'll approve as a feature request. Thanks
Is there a PR about this already? I need this functionality and could contribute a patch
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.
This feature would actually be a great enhancement to salt tcp transport!
@dhananjaysathe Have you been able to contribute a patch?
Any movement on this? I have the same issue.
there is not any movement yet, please feel free to submit a PR
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.
please do not close
Thank you for updating this issue. It is no longer marked as stale.
For me the request is that TCP Transport be augmented to allow SNI and also a single port. Long-term a transport compatible with HTTP proxies would be ideal for enterprise deployments.
Quick fix in minion.py's resolve_dns:
if opts.get("defer_dns", ""):
if opts["master"] == "":
raise SaltSystemExit
ret["master_ip"] = opts["master"]
elif check_dns is True:
try:
This skips the call to salt.utils.network.dns_check which does a connection test so that doesn't happen anymore. I think that's OK but perhaps isn't in all cases. I may submit PR once I get my next part done to pass hostnames through to salt.ext.tornado.tcpclient. Thinking the configuration could look something like:
master:
- salt-master1.example.com
- 192.0.2.0:8443
master_type: failover
retry_dns: 0
transport: tcp
defer_dns: True
master_sni: salt-ret.example.com
master_port: 443
publish_sni: salt-pub.example.com
publish_port_shared: True
This last option will make sure port 8443 from 2nd master is copied to publish_port before use. SNI is static between the two masters. If each master needs separate SNI for publish then we could extend the master list to use more of the url parser. Perhaps:
master:
- tcp://salt-master1.example.com:443/?publish_sni=salt-pub.example.com
- tcp://192.0.2.0:4506/?publish_port=4505