saltstack / salt

Software to automate the management and configuration of any infrastructure or application at scale. Get access to the Salt software package repository here:
https://repo.saltproject.io/
Apache License 2.0
14.1k stars 5.47k forks source link

[BUG] `utils.http` - usage of `http.cookiejar.Cookie()` breaks `http.query` when a cookie contains `max-age` #63943

Open eliasp opened 1 year ago

eliasp commented 1 year ago

Description When a query done by salt.utils.query receives a cookie, which contains Max-Age=...., it breaks: TypeError: Cookie.__init__() got an unexpected keyword argument 'max-age'

Setup We upgraded a very ancient Salt environment from 3002.x to 3005.1 when we started seeing this breakage. I could also reproduce it successfully using 3006-rc2 and current master. Salt itself doesn't need any specific setup/configuration, a masterless salt-call http.query is all that's needed, but since emulating an HTTP endpoint which provides the required response is a little bit tricky, using Salt's test suite instead with the following change reproduces this issue as well:

diff --git a/tests/unit/utils/test_http.py b/tests/unit/utils/test_http.py
index d9a84f9582..8236765b33 100644
--- a/tests/unit/utils/test_http.py
+++ b/tests/unit/utils/test_http.py
@@ -152,6 +152,7 @@ class HTTPTestCase(TestCase):
                 "HttpOnly",
                 "SameSite=Lax",
                 "Secure",
+                "Max-Age=3600",
             ]
         )
         ret = http.parse_cookie_header(header)

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

Steps to Reproduce the behavior

alternatively:

Expected behavior The response to a http.query request is handled without a Traceback.

Versions Report

salt --versions-report ```yaml Salt Version: Salt: 3006.0rc2+101.g85a8358975 Python Version: Python: 3.10.6 (main, Mar 10 2023, 10:55:28) [GCC 11.3.0] Dependency Versions: cffi: Not Installed cherrypy: Not Installed dateutil: Not Installed docker-py: Not Installed gitdb: Not Installed gitpython: Not Installed Jinja2: 3.1.2 libgit2: Not Installed looseversion: 1.1.2 M2Crypto: Not Installed Mako: Not Installed msgpack: 1.0.4 msgpack-pure: Not Installed mysql-python: Not Installed packaging: 21.3 pycparser: Not Installed pycrypto: Not Installed pycryptodome: 3.17 pygit2: Not Installed python-gnupg: Not Installed PyYAML: 6.0 PyZMQ: 25.0.0 relenv: Not Installed smmap: Not Installed timelib: Not Installed Tornado: 4.5.3 ZMQ: 4.3.4 System Versions: dist: ubuntu 22.04.2 jammy locale: utf-8 machine: x86_64 release: 5.15.0-58-generic system: Linux version: Ubuntu 22.04.2 jammy ```

Additional context According to @OrangeDog on Slack, this is a result of Salt using http.cookiejar.Cookie() even though the docs explicitly say not to do that and to use CookieJar.make_cookies() instead.

eliasp commented 1 year ago

For anyone affected by this: as a workaround, try using the backend= parameter for http.query with either backend=requests or backend=urllib2.

eth7 commented 1 year ago

I believe I've identified the issue with the Max-Age cookie. When utilizing the cookie variable to generate the cookiejar object, several attributes such as httponly and samesite were already removed, but max-age was not.

I'm new to this project and would like to contribute, but first I need to read the contributing criteria and add a proper test and changelog, among other things.

OrangeDog commented 1 year ago

@eth7 your fix is incorrect. The Cookie object is supposed to have an expiry. The problem, as previously mentioned, is that the code is a re-implementation of what the standard library already provides, using private APIs that can (and have) changed without notice.

eth7 commented 1 year ago

@OrangeDog Unfortunately the implementation from CookieJar.make_cookies (Cookiejar docs) is used to extract cookies from an urllib2 response/request, which we cannot use in that case, since the tornado HTTP client is used. Therefore, the function parse_cookie_header was written to construct the object from a given header.

You are right though about the expiry date, I added functionality to calculate the expiry based on the max-age. See the following branch.

OrangeDog commented 1 year ago

That's not going to work on different Python versions where the implementation of Cookie is different. It needs to adapt the tornado request/response to the urlib api, or otherwise use a public method for constructing the right cookie objects.