Reject eauth tokens supplied via query string in salt-api Events

changelog/69071.fixed.md

@@ -0,0 +1,11 @@
+Fixed `salt-api`'s `/events` endpoint accepting eauth tokens via query
+string (``?token=...`` or ``?salt_token=...``). Tokens supplied that
+way end up in HTTP access logs, the browser ``Referer`` header, log-
+aggregation systems and shell history; the token retains validity for
+``token_expire`` (12h by default), so any party reading those logs can
+replay the token. The endpoint now rejects query-string tokens with a
+400 error pointing at the ``X-Auth-Token`` header (for non-browser
+clients) or the session cookie established by ``/login`` (for browser
+``EventSource`` clients) as the supported channels. ``X-Auth-Token``
+header support is added; cookie-based auth continues to work
+unchanged.

salt/netapi/rest_cherrypy/app.py

@@ -2229,7 +2229,7 @@ def _is_valid_token(self, auth_token):
 
         return False
 
-    def GET(self, token=None, salt_token=None):
+    def GET(self, **kwargs):
         r"""
         An HTTP stream of the Salt master event bus
 
@@ -2239,28 +2239,39 @@ def GET(self, token=None, salt_token=None):
         .. http:get:: /events
 
             :status 200: |200|
+            :status 400: |400| -- the endpoint takes no query parameters; in
+                particular tokens must not be passed in the URL.
             :status 401: |401|
             :status 406: |406|
-            :query token: **optional** parameter containing the token
-                ordinarily supplied via the X-Auth-Token header in order to
-                allow cross-domain requests in browsers that do not include
-                CORS support in the EventSource API. E.g.,
-                ``curl -NsS localhost:8000/events?token=308650d``
-            :query salt_token: **optional** parameter containing a raw Salt
-                *eauth token* (not to be confused with the token returned from
-                the /login URL). E.g.,
-                ``curl -NsS localhost:8000/events?salt_token=30742765``
 
-        **Example request:**
+        Authentication channels:
+
+        - ``X-Auth-Token`` header (the recommended path for non-browser
+          clients -- ``curl``, scripts, server-side integrations).
+        - Session cookie set by ``/login`` (the recommended path for
+          browser ``EventSource`` clients, which the EventSource API
+          does not let you set custom headers on).
+
+        Tokens passed via the query string (``?token=...`` or
+        ``?salt_token=...``) used to be accepted as a workaround for
+        the browser EventSource API. They are no longer accepted: the
+        URL ends up in HTTP access logs, the browser ``Referer``
+        header, log-aggregation pipelines, and error reports, none of
+        which are appropriate channels for a bearer credential. Use
+        the cookie path instead -- log in via ``/login`` first, the
+        cookie is sent automatically when the EventSource opens.
+
+        **Example request (non-browser client):**
 
         .. code-block:: bash
 
-            curl -NsS localhost:8000/events
+            curl -NsS -H "X-Auth-Token: <token>" localhost:8000/events
 
         .. code-block:: text
 
             GET /events HTTP/1.1
             Host: localhost:8000
+            X-Auth-Token: <token>
 
         **Example response:**
 
@@ -2317,11 +2328,13 @@ def GET(self, token=None, salt_token=None):
         It can be viewed by pointing a browser at the ``/app`` endpoint in a
         running ``rest_cherrypy`` instance.
 
-        Or using CORS:
+        For cross-origin EventSource, set ``withCredentials`` and rely
+        on the session cookie established by ``/login`` rather than a
+        query-string token:
 
         .. code-block:: javascript
 
-            var source = new EventSource('/events?token=ecd589e4e01912cf3c4035afad73426dbb8dba75', {withCredentials: true});
+            var source = new EventSource('/events', {withCredentials: true});
 
         It is also possible to consume the stream via the shell.
 
@@ -2336,7 +2349,7 @@ def GET(self, token=None, salt_token=None):
 
         .. code-block:: bash
 
-            curl -NsS localhost:8000/events |\
+            curl -NsS -H "X-Auth-Token: <token>" localhost:8000/events |\
                     while IFS= read -r line ; do
                         echo $line
                     done
@@ -2345,7 +2358,7 @@ def GET(self, token=None, salt_token=None):
 
         .. code-block:: bash
 
-            curl -NsS localhost:8000/events |\
+            curl -NsS -H "X-Auth-Token: <token>" localhost:8000/events |\
                     awk '
                         BEGIN { RS=""; FS="\\n" }
                         $1 ~ /^tag: salt\/job\/[0-9]+\/new$/ { print $0 }
@@ -2355,11 +2368,30 @@ def GET(self, token=None, salt_token=None):
             tag: 20140112010149808995
             data: {"tag": "20140112010149808995", "data": {"fun_args": [], "jid": "20140112010149808995", "return": true, "retcode": 0, "success": true, "cmd": "_return", "_stamp": "2014-01-12_01:01:49.819316", "fun": "test.ping", "id": "jerry"}}
         """
+        # The Events endpoint takes no query parameters. Reject any --
+        # in particular tokens. Bearer tokens supplied via the URL end
+        # up in HTTP access logs, the browser ``Referer`` header, log-
+        # aggregation systems, error reports, and shell history -- none
+        # of which are appropriate channels for a bearer credential.
+        # Tokens must come through the ``X-Auth-Token`` header or the
+        # CherryPy session cookie instead. Rejecting *all* query
+        # parameters (not just ``token`` / ``salt_token``) keeps a
+        # future contributor from silently re-introducing a similar
+        # leak via a differently-named parameter.
+        if kwargs:
+            raise cherrypy.HTTPError(
+                400,
+                "The /events endpoint takes no query parameters; in "
+                "particular, tokens must not be passed via the query "
+                "string -- they end up in access logs and the Referer "
+                "header. Use the 'X-Auth-Token' header (for non-browser "
+                "clients) or the session cookie set by /login (for "
+                "browser EventSource clients) instead.",
+            )
+
         cookies = cherrypy.request.cookie
-        auth_token = (
-            token
-            or salt_token
-            or (cookies["session_id"].value if "session_id" in cookies else None)
+        auth_token = cherrypy.request.headers.get("X-Auth-Token") or (
+            cookies["session_id"].value if "session_id" in cookies else None
         )
 
         if not self._is_valid_token(auth_token):

tests/pytests/unit/netapi/cherrypy/test_events.py

@@ -1,14 +1,26 @@
 import time
+from types import SimpleNamespace
 
 import pytest
 
 import salt.netapi.rest_cherrypy.app as cherrypy_app
 from tests.support.mock import MagicMock, patch
 
 
+class _MockHTTPError(Exception):
+    """Stand-in for ``cherrypy.HTTPError`` so tests can assert on status
+    codes raised by handlers under test."""
+
+    def __init__(self, status=None, message=None):
+        self.status = status
+        self.message = message
+        super().__init__(f"{status}: {message}")
+
+
 class MockCherryPy:
     session = MagicMock(cache={})
     config = {"saltopts": {}}
+    HTTPError = _MockHTTPError
 
 
 class MockResolver:
@@ -42,3 +54,98 @@ def test__is_valid_token_expired():
                 events.resolver, "get_token", return_value={"expire": time.time() - 60}
             ):
                 assert not events._is_valid_token("ABCDEF")
+
+
+# ---------------------------------------------------------------------------
+# Token-channel restrictions: ``Events.GET`` must reject query-string
+# tokens. URLs end up in HTTP access logs, browser ``Referer`` headers and
+# log-aggregation pipelines, so they are not a safe channel for a bearer
+# credential. ``X-Auth-Token`` and the CherryPy session cookie are.
+# ---------------------------------------------------------------------------
+
+
+def _cherrypy_for_events(headers=None, cookie_session_id=None):
+    """Build a ``cherrypy``-shaped namespace just rich enough that
+    ``Events.GET`` can reach its auth checks. Caller controls what
+    arrives via headers / cookies; the response namespace is provided
+    so the handler can populate SSE response headers without raising
+    AttributeError."""
+    headers = headers or {}
+    cookie_dict = {}
+    if cookie_session_id is not None:
+        cookie_dict["session_id"] = SimpleNamespace(value=cookie_session_id)
+
+    session = MagicMock(cache={})
+    session.release_lock = MagicMock()
+
+    return SimpleNamespace(
+        config={"saltopts": {}},
+        request=SimpleNamespace(headers=headers, cookie=cookie_dict),
+        response=SimpleNamespace(headers={}),
+        session=session,
+        HTTPError=_MockHTTPError,
+    )
+
+
+def test_events_get_rejects_token_in_query_string():
+    """Headline regression: ``?token=...`` must be rejected with 400
+    rather than authenticated. The token-in-URL anti-pattern leaks the
+    bearer credential through HTTP access logs and the browser
+    ``Referer`` header."""
+    with patch("salt.netapi.rest_cherrypy.app.cherrypy", _cherrypy_for_events()):
+        with patch("salt.auth.Resolver", MockResolver):
+            events = cherrypy_app.Events()
+            with pytest.raises(_MockHTTPError) as excinfo:
+                events.GET(token="cafebabe")
+
+    assert excinfo.value.status == 400
+
+
+def test_events_get_rejects_salt_token_in_query_string():
+    """The ``?salt_token=...`` alias is the same anti-pattern and must
+    also be rejected."""
+    with patch("salt.netapi.rest_cherrypy.app.cherrypy", _cherrypy_for_events()):
+        with patch("salt.auth.Resolver", MockResolver):
+            events = cherrypy_app.Events()
+            with pytest.raises(_MockHTTPError) as excinfo:
+                events.GET(salt_token="cafebabe")
+
+    assert excinfo.value.status == 400
+
+
+def test_events_get_rejects_any_unexpected_query_parameter():
+    """The endpoint takes no query parameters at all. Rejecting unknown
+    parameters keeps a future contributor from silently re-introducing
+    a token-leak via a differently-named query parameter."""
+    with patch("salt.netapi.rest_cherrypy.app.cherrypy", _cherrypy_for_events()):
+        with patch("salt.auth.Resolver", MockResolver):
+            events = cherrypy_app.Events()
+            with pytest.raises(_MockHTTPError) as excinfo:
+                events.GET(some_future_param="anything")
+
+    assert excinfo.value.status == 400
+
+
+def test_events_get_accepts_token_in_x_auth_token_header():
+    """``X-Auth-Token`` header is the supported channel for non-browser
+    clients (curl, scripts, server-side integrations). A valid token
+    there must pass auth and reach the SSE-headers stage."""
+    cherrypy_mock = _cherrypy_for_events(headers={"X-Auth-Token": "ABCDEF"})
+
+    with patch("salt.netapi.rest_cherrypy.app.cherrypy", cherrypy_mock):
+        with patch("salt.auth.Resolver", MockResolver):
+            events = cherrypy_app.Events()
+            with patch.object(
+                events.resolver,
+                "get_token",
+                return_value={"expire": time.time() + 60},
+            ):
+                # GET returns a generator; the auth check fires before
+                # the generator is built. If we get one back, auth
+                # passed and the SSE response headers were set.
+                gen = events.GET()
+                assert gen is not None
+                assert (
+                    cherrypy_mock.response.headers["Content-Type"]
+                    == "text/event-stream"
+                )