[BUG] TCP transport broken on 3004.1 and relevant bugfixes

[lukasraska]

Description
Salt Master on TCP transport is broken on newly released version (https://saltproject.io/security_announcements/salt-security-advisory-release/) that mitigates several CVEs, resulting in no jobs being published.

In master logs following stacktrace can be seen:

2022-03-28 20:15:02,912 [tornado.application:640 ][ERROR   ][25004] Exception in callback functools.partial(<function wrap.<locals>.null_wrapper at 0x7f354fda3ea0>, <sal
t.ext.tornado.concurrent.Future object at 0x7f35462c6940>)
Traceback (most recent call last):
  File "/usr/lib/python3.6/site-packages/salt/ext/tornado/ioloop.py", line 606, in _run_callback
    ret = callback()
  File "/usr/lib/python3.6/site-packages/salt/ext/tornado/stack_context.py", line 278, in null_wrapper
    return fn(*args, **kwargs)
  File "/usr/lib/python3.6/site-packages/salt/ext/tornado/ioloop.py", line 628, in _discard_future_result
    future.result()
  File "/usr/lib/python3.6/site-packages/salt/ext/tornado/concurrent.py", line 249, in result
    raise_exc_info(self._exc_info)
  File "<string>", line 4, in raise_exc_info
  File "/usr/lib/python3.6/site-packages/salt/ext/tornado/gen.py", line 294, in wrapper
    result = func(*args, **kwargs)
  File "/usr/lib64/python3.6/types.py", line 248, in wrapped
    coro = func(*args, **kwargs)
  File "/usr/lib/python3.6/site-packages/salt/transport/tcp.py", line 1565, in publish_payload
    payload = salt.transport.frame.frame_msg(package["payload"])
KeyError: 'payload'

Setup
TCP transport on master & minion, 3004.1 master.

Please be as specific as possible and give set-up details.

• on-prem machine
• VM (Virtualbox, KVM, etc. please specify)
• VM running on a cloud service, please be explicit and add details
• container (Kubernetes, Docker, containerd, etc. please specify)
• or a combination, please be explicit
• jails if it is FreeBSD

Steps to Reproduce the behavior
Run any job, doesn't get published and no results can be obtained.

Expected behavior
Jobs can actually be published.

Screenshots
If applicable, add screenshots to help explain your problem.

Versions Report

salt --versions-report

    Salt Version:
              Salt: 3004.1
     
    Dependency Versions:
              cffi: 1.9.1
          cherrypy: Not Installed
          dateutil: 2.4.2
         docker-py: Not Installed
             gitdb: 0.6.4
         gitpython: 1.0.1
            Jinja2: 2.11.1
           libgit2: Not Installed
          M2Crypto: 0.35.2
              Mako: Not Installed
           msgpack: 0.6.2
      msgpack-pure: Not Installed
      mysql-python: Not Installed
         pycparser: 2.14
          pycrypto: Not Installed
      pycryptodome: Not Installed
            pygit2: Not Installed
            Python: 3.6.8 (default, Aug 13 2020, 07:46:32)
      python-gnupg: Not Installed
            PyYAML: 3.13
             PyZMQ: 17.0.0
             smmap: 0.9.0
           timelib: Not Installed
           Tornado: 4.5.3
               ZMQ: 4.1.4
     
    System Versions:
              dist: rhel 7.9 Maipo
            locale: UTF-8
           machine: x86_64
           release: 3.10.0-1160.53.1.el7.x86_64
            system: Linux
           version: Red Hat Enterprise Linux Server 7.9 Maipo

Additional context
https://github.com/saltstack/salt/blob/v3004.1/salt/transport/tcp.py#L1564 should be package = self.pack_publish(package) (package instead of payload) - can be applied as a workaround

[dwoz]

@lukasraska Can you provide us a version report from both master and minion?

[lukasraska]

@dwoz master version report is under the Versions Report section, minion version isn't relevant (the payload is never sent)... so even simple salt -L master.tld test.ping is affected (so same version report as from master)

[dwoz]

@lukasraska I've confirmed this is an issue and your suggestion seems to resolve it.

From 21166b20f01ab9b49d4c43c6a19aa21ecba1d72a Mon Sep 17 00:00:00 2001
From: "Daniel A. Wozniak" <dwozniak@saltstack.com>
Date: Mon, 28 Mar 2022 14:11:50 -0700
Subject: [PATCH] Tcp transport bugfix

---
 salt/transport/tcp.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/salt/transport/tcp.py b/salt/transport/tcp.py
index f00b3c40eb..2c4844a2a6 100644
--- a/salt/transport/tcp.py
+++ b/salt/transport/tcp.py
@@ -1562,7 +1562,7 @@ class PubServer(salt.ext.tornado.tcpserver.TCPServer):
     def publish_payload(self, package, _):
         log.debug("TCP PubServer sending payload: %s", package)
         payload = self.pack_publish(package)
-        payload = salt.transport.frame.frame_msg(package["payload"])
+        payload = salt.transport.frame.frame_msg(payload["payload"])
 
         to_remove = []
         if "topic_lst" in package:
-- 
2.30.2

[lukasraska]

@dwoz great, I initially thought the package -> payload would be a way t go, but further down the topic_lst works around package, rather than the payload variable... so when the topic_lst content is list, it's working as intended, but when it's string, it will fail (because that's processed in the pack_publish), so following patch is probably for the best

From 1399acfb8cb466f13fbd8f7177653f27746d6ed7 Mon Sep 17 00:00:00 2001
From: Lukas Raska <lukas@raska.me>
Date: Mon, 28 Mar 2022 21:38:44 +0200
Subject: [PATCH] Assign packaged payload to proper variable in TCP transport
 layer

---
 salt/transport/tcp.py | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/salt/transport/tcp.py b/salt/transport/tcp.py
index f00b3c40eb6af..48ece5f83d350 100644
--- a/salt/transport/tcp.py
+++ b/salt/transport/tcp.py
@@ -1561,7 +1561,7 @@ def handle_stream(self, stream, address):
     @salt.ext.tornado.gen.coroutine
     def publish_payload(self, package, _):
         log.debug("TCP PubServer sending payload: %s", package)
-        payload = self.pack_publish(package)
+        package = self.pack_publish(package)
         payload = salt.transport.frame.frame_msg(package["payload"])
 
         to_remove = []

[dwoz]

@dwoz great, I initially thought the package -> payload would be a way t go, but further down the topic_lst works around package, rather than the payload variable... so when the topic_lst content is list, it's working as intended, but when it's string, it will fail (because that's processed in the pack_publish), so following patch is probably for the best

From 1399acfb8cb466f13fbd8f7177653f27746d6ed7 Mon Sep 17 00:00:00 2001
From: Lukas Raska <lukas@raska.me>
Date: Mon, 28 Mar 2022 21:38:44 +0200
Subject: [PATCH] Assign packaged payload to proper variable in TCP transport
 layer

---
 salt/transport/tcp.py | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/salt/transport/tcp.py b/salt/transport/tcp.py
index f00b3c40eb6af..48ece5f83d350 100644
--- a/salt/transport/tcp.py
+++ b/salt/transport/tcp.py
@@ -1561,7 +1561,7 @@ def handle_stream(self, stream, address):
     @salt.ext.tornado.gen.coroutine
     def publish_payload(self, package, _):
         log.debug("TCP PubServer sending payload: %s", package)
-        payload = self.pack_publish(package)
+        package = self.pack_publish(package)
         payload = salt.transport.frame.frame_msg(package["payload"])
 
         to_remove = []

Yes, you are correct. This is how it is on the 3003.4 branch which does not have the bug. Oddly enough 3002.8 does have the bug.