Merge pull request #5248 from StackStorm/samesite_cookie_support

Kami · web-flow · commit 56005dbe04c2 · 2022-03-27T12:41:15.000+02:00
Add support for setting SameSite and Secure attribute for auth cookie we set
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
@@ -312,6 +312,21 @@ Added
 
   Contributed by @Kami.
 
+* Add new ``api.auth_cookie_secure`` and ``api.auth_cookie_same_site`` config options which
+  specify values which are set for ``secure`` and ``SameSite`` attribute for the auth cookie
+  we set when authenticating via token / api key in query parameter value (e.g. via st2web).
+
+  For security reasons, ``api.auth_cookie_secure`` defaults to ``True``. This should only be
+  changed to ``False`` if you have a valid reason to not run StackStorm behind HTTPs proxy.
+
+  Default value for ``api.auth_cookie_same_site`` is ``lax``. If you want to disable this
+  functionality so it behaves the same as in the previous releases, you can set that option
+  to ``None``.
+
+  #5248
+
+  Contributed by @Kami.
+
 * Mask secrets in output of an action execution in the API if the action has an output schema
   defined and one or more output parameters are marked as secret. #5250
 
diff --git a/conf/st2.conf.sample b/conf/st2.conf.sample
@@ -38,6 +38,11 @@ workflows_pool_size = 40
 [api]
 # List of origins allowed for api, auth and stream
 allow_origin = http://127.0.0.1:3000 # comma separated list allowed here.
+# SameSite attribute value for the auth-token cookie we set on successful authentication from st2web. If you don't have a specific reason (e.g. supporting old browsers) we recommend you set this value to strict. Setting it to "unset" will default to the behavior in previous releases and not set this SameSite header value.
+# Valid values: strict, lax, none, unset
+auth_cookie_same_site = lax
+# True if secure flag should be set for "auth-token" cookie which is set on successful authentication via st2web. You should only set this to False if you have a good reason to not run and access StackStorm behind https proxy.
+auth_cookie_secure = True
 # None
 debug = False
 # StackStorm API server host
@@ -142,6 +147,7 @@ ssl = False
 # ca_certs file contains a set of concatenated CA certificates, which are used to validate certificates passed from MongoDB.
 ssl_ca_certs = None
 # Specifies whether a certificate is required from the other side of the connection, and whether it will be validated if provided
+# Valid values: none, optional, required
 ssl_cert_reqs = None
 # Certificate file used to identify the localconnection
 ssl_certfile = None
@@ -151,7 +157,7 @@ ssl_keyfile = None
 ssl_match_hostname = True
 # username for db login
 username = None
-# Compression level when compressors is set to zlib. Valid calues are -1 to 9. Defaults to 6.
+# Compression level when compressors is set to zlib. Valid values are -1 to 9. Defaults to 6.
 zlib_compression_level =
 
 [exporter]
@@ -195,7 +201,8 @@ redirect_stderr = False
 [messaging]
 # URL of all the nodes in a messaging service cluster.
 cluster_urls =  # comma separated list allowed here.
-# Compression algorithm to use for compressing the payloads which are sent over the message bus. Valid values include: zstd, lzma, bz2, gzip. Defaults to no compression.
+# Compression algorithm to use for compressing the payloads which are sent over the message bus. Defaults to no compression.
+# Valid values: zstd, lzma, bz2, gzip, None
 compression = None
 # How many times should we retry connection before failing.
 connection_retries = 10
@@ -208,6 +215,7 @@ ssl = False
 # ca_certs file contains a set of concatenated CA certificates, which are used to validate certificates passed from RabbitMQ.
 ssl_ca_certs = None
 # Specifies whether a certificate is required from the other side of the connection, and whether it will be validated if provided.
+# Valid values: none, optional, required
 ssl_cert_reqs = None
 # Certificate file used to identify the local connection (client).
 ssl_certfile = None
diff --git a/st2api/st2api/app.py b/st2api/st2api/app.py
@@ -28,6 +28,7 @@
 from st2common.constants.system import VERSION_STRING
 from st2common.service_setup import setup as common_setup
 from st2common.util import spec_loader
+from st2api.validation import validate_auth_cookie_is_correctly_configured
 from st2api.validation import validate_rbac_is_correctly_configured
 
 LOG = logging.getLogger(__name__)
@@ -66,6 +67,7 @@ def setup_app(config=None):
         )
 
     # Additional pre-run time checks
+    validate_auth_cookie_is_correctly_configured()
     validate_rbac_is_correctly_configured()
 
     router = Router(
diff --git a/st2api/st2api/cmd/api.py b/st2api/st2api/cmd/api.py
@@ -37,6 +37,7 @@
 config.register_opts(ignore_errors=True)
 
 from st2api import app
+from st2api.validation import validate_auth_cookie_is_correctly_configured
 from st2api.validation import validate_rbac_is_correctly_configured
 
 __all__ = ["main"]
@@ -68,6 +69,7 @@ def _setup():
     )
 
     # Additional pre-run time checks
+    validate_auth_cookie_is_correctly_configured()
     validate_rbac_is_correctly_configured()
 
 
diff --git a/st2api/st2api/validation.py b/st2api/st2api/validation.py
@@ -15,10 +15,58 @@
 
 from oslo_config import cfg
 
-__all__ = ["validate_rbac_is_correctly_configured"]
+from webob import cookies
 
+__all__ = [
+    "validate_auth_cookie_is_correctly_configured",
+    "validate_rbac_is_correctly_configured",
+]
 
-def validate_rbac_is_correctly_configured():
+
+def validate_auth_cookie_is_correctly_configured() -> bool:
+    """
+    Function which verifies that SameCookie config option value is correctly configured.
+
+    This method should be called in the api init phase so we catch any misconfiguration issues
+    before startup.
+    """
+    if cfg.CONF.api.auth_cookie_same_site not in ["strict", "lax", "none", "unset"]:
+        raise ValueError(
+            'Got invalid value "%s" (type %s) for cfg.CONF.api.auth_cookie_same_site config '
+            "option. Valid values are: strict, lax, none, unset."
+            % (
+                cfg.CONF.api.auth_cookie_same_site,
+                type(cfg.CONF.api.auth_cookie_same_site),
+            )
+        )
+
+    # Now we try to make a dummy cookie to verify all the options are configured correctly. Some
+    # Options are mutually exclusive - e.g. SameSite none and Secure false.
+    try:
+        # NOTE: none and unset don't mean the same thing - unset implies not setting this attribute
+        # (backward compatibility) and none implies setting this attribute value to none
+        same_site = cfg.CONF.api.auth_cookie_same_site
+
+        kwargs = {}
+        if same_site != "unset":
+            kwargs["samesite"] = same_site
+
+        cookies.make_cookie(
+            "test_cookie",
+            "dummyvalue",
+            httponly=True,
+            secure=cfg.CONF.api.auth_cookie_secure,
+            **kwargs,
+        )
+    except Exception as e:
+        raise ValueError(
+            "Failed to validate api.auth_cookie config options: %s" % (str(e))
+        )
+
+    return True
+
+
+def validate_rbac_is_correctly_configured() -> bool:
     """
     Function which verifies that RBAC is correctly set up and configured.
     """
diff --git a/st2api/tests/unit/controllers/v1/test_auth.py b/st2api/tests/unit/controllers/v1/test_auth.py
@@ -18,6 +18,7 @@
 
 import bson
 import mock
+from oslo_config import cfg
 
 from st2tests.api import FunctionalTest
 from st2common.util import date as date_utils
@@ -69,6 +70,63 @@ def test_token_validation_token_in_query_params(self):
         self.assertIn("application/json", response.headers["content-type"])
         self.assertEqual(response.status_int, 200)
 
+    @mock.patch.object(
+        Token,
+        "get",
+        mock.Mock(
+            return_value=TokenDB(id=OBJ_ID, user=USER, token=TOKEN, expiry=FUTURE)
+        ),
+    )
+    @mock.patch.object(User, "get_by_name", mock.Mock(return_value=USER_DB))
+    def test_token_validation_token_in_query_params_auth_cookie_is_set(self):
+        response = self.app.get(
+            "/v1/actions?x-auth-token=%s" % (TOKEN), expect_errors=False
+        )
+        self.assertIn("application/json", response.headers["content-type"])
+        self.assertEqual(response.status_int, 200)
+        self.assertTrue("Set-Cookie" in response.headers)
+        self.assertTrue("HttpOnly" in response.headers["Set-Cookie"])
+
+        # Also test same cookie values + secure
+        valid_values = ["strict", "lax", "none", "unset"]
+
+        for value in valid_values:
+            cfg.CONF.set_override(
+                group="api", name="auth_cookie_same_site", override=value
+            )
+            cfg.CONF.set_override(group="api", name="auth_cookie_secure", override=True)
+
+            response = self.app.get(
+                "/v1/actions?x-auth-token=%s" % (TOKEN), expect_errors=False
+            )
+            self.assertIn("application/json", response.headers["content-type"])
+            self.assertEqual(response.status_int, 200)
+            self.assertTrue("Set-Cookie" in response.headers)
+            self.assertTrue("HttpOnly" in response.headers["Set-Cookie"])
+
+            if value == "unset":
+                self.assertFalse("SameSite" in response.headers["Set-Cookie"])
+            else:
+                self.assertTrue(
+                    "SameSite=%s" % (value) in response.headers["Set-Cookie"]
+                )
+
+            self.assertTrue("secure" in response.headers["Set-Cookie"])
+
+        # SameSite=Lax, Secure=False
+        cfg.CONF.set_override(group="api", name="auth_cookie_same_site", override="lax")
+        cfg.CONF.set_override(group="api", name="auth_cookie_secure", override=False)
+
+        response = self.app.get(
+            "/v1/actions?x-auth-token=%s" % (TOKEN), expect_errors=False
+        )
+        self.assertIn("application/json", response.headers["content-type"])
+        self.assertEqual(response.status_int, 200)
+        self.assertTrue("Set-Cookie" in response.headers)
+        self.assertTrue("HttpOnly" in response.headers["Set-Cookie"])
+        self.assertTrue("SameSite=lax" in response.headers["Set-Cookie"])
+        self.assertTrue("secure" not in response.headers["Set-Cookie"])
+
     @mock.patch.object(
         Token,
         "get",
diff --git a/st2api/tests/unit/test_validation_utils.py b/st2api/tests/unit/test_validation_utils.py
@@ -16,6 +16,7 @@
 import unittest2
 from oslo_config import cfg
 
+from st2api.validation import validate_auth_cookie_is_correctly_configured
 from st2api.validation import validate_rbac_is_correctly_configured
 from st2tests import config as tests_config
 
@@ -27,6 +28,49 @@ def setUp(self):
         super(ValidationUtilsTestCase, self).setUp()
         tests_config.parse_args()
 
+    def test_validate_auth_cookie_is_correctly_configured_success(self):
+        valid_values = [
+            "strict",
+            "lax",
+            "none",
+            "unset",
+        ]
+
+        cfg.CONF.set_override(group="api", name="auth_cookie_secure", override=True)
+
+        for value in valid_values:
+            cfg.CONF.set_override(
+                group="api", name="auth_cookie_same_site", override=value
+            )
+            self.assertTrue(validate_auth_cookie_is_correctly_configured())
+
+    def test_validate_auth_cookie_is_correctly_configured_error(self):
+        invalid_values = ["strictx", "laxx", "nonex", "invalid"]
+
+        for value in invalid_values:
+            cfg.CONF.set_override(
+                group="api", name="auth_cookie_same_site", override=value
+            )
+
+            expected_msg = "Valid values are: strict, lax, none, unset"
+            self.assertRaisesRegexp(
+                ValueError, expected_msg, validate_auth_cookie_is_correctly_configured
+            )
+
+        # SameSite=none + Secure=false is not compatible
+        cfg.CONF.set_override(
+            group="api", name="auth_cookie_same_site", override="none"
+        )
+        cfg.CONF.set_override(group="api", name="auth_cookie_secure", override=False)
+
+        expected_msg = (
+            r"Failed to validate api.auth_cookie config options: Incompatible cookie attributes: "
+            "when the samesite equals 'none', then the secure must be True"
+        )
+        self.assertRaisesRegexp(
+            ValueError, expected_msg, validate_auth_cookie_is_correctly_configured
+        )
+
     def test_validate_rbac_is_correctly_configured_succcess(self):
         result = validate_rbac_is_correctly_configured()
         self.assertTrue(result)
diff --git a/st2common/st2common/config.py b/st2common/st2common/config.py
@@ -254,7 +254,7 @@ def register_opts(ignore_errors=False):
         cfg.IntOpt(
             "zlib_compression_level",
             default="",
-            help="Compression level when compressors is set to zlib. Valid calues are -1 to 9. "
+            help="Compression level when compressors is set to zlib. Valid values are -1 to 9. "
             "Defaults to 6.",
         ),
     ]
@@ -321,9 +321,9 @@ def register_opts(ignore_errors=False):
         cfg.StrOpt(
             "compression",
             default=None,
+            choices=["zstd", "lzma", "bz2", "gzip", None],
             help="Compression algorithm to use for compressing the payloads which are sent over "
-            "the message bus. Valid values include: zstd, lzma, bz2, gzip. Defaults to no "
-            "compression.",
+            "the message bus. Defaults to no compression.",
         ),
     ]
 
@@ -373,6 +373,23 @@ def register_opts(ignore_errors=False):
             default=True,
             help="True to mask secrets in the API responses",
         ),
+        cfg.BoolOpt(
+            "auth_cookie_secure",
+            default=True,
+            help='True if secure flag should be set for "auth-token" cookie which is set on '
+            "successful authentication via st2web. You should only set this to False if you have "
+            "a good reason to not run and access StackStorm behind https proxy.",
+        ),
+        cfg.StrOpt(
+            "auth_cookie_same_site",
+            default="lax",
+            choices=["strict", "lax", "none", "unset"],
+            help="SameSite attribute value for the "
+            "auth-token cookie we set on successful authentication from st2web. If you "
+            "don't have a specific reason (e.g. supporting old browsers) we recommend you "
+            'set this value to strict. Setting it to "unset" will default to the behavior '
+            "in previous releases and not set this SameSite header value.",
+        ),
     ]
 
     do_register_opts(api_opts, "api", ignore_errors)
diff --git a/st2common/st2common/router.py b/st2common/st2common/router.py
@@ -383,11 +383,22 @@ def __call__(self, req):
                                 max_age = (
                                     auth_resp.expiry - date_utils.get_datetime_utc_now()
                                 )
+                                # NOTE: unset and none don't mean the same thing - unset implies
+                                # not setting this attribute at all (backward compatibility) and
+                                # none implies setting this attribute value to none
+                                same_site = cfg.CONF.api.auth_cookie_same_site
+
+                                kwargs = {}
+                                if same_site != "unset":
+                                    kwargs["samesite"] = same_site
+
                                 cookie_token = cookies.make_cookie(
                                     definition["x-set-cookie"],
                                     token,
                                     max_age=max_age,
                                     httponly=True,
+                                    secure=cfg.CONF.api.auth_cookie_secure,
+                                    **kwargs,
                                 )
 
                             break
diff --git a/st2tests/st2tests/config.py b/st2tests/st2tests/config.py
@@ -111,6 +111,11 @@ def _override_api_opts():
         override=["http://127.0.0.1:3000", "http://dev"],
         group="api",
     )
+    CONF.set_override(
+        name="auth_cookie_secure",
+        override=False,
+        group="api",
+    )
 
 
 def _override_keyvalue_opts():
diff --git a/tools/config_gen.py b/tools/config_gen.py
@@ -175,6 +175,14 @@ def _print_options(opt_group, options):
             value = opt.default
 
         print(("# %s" % opt.help).strip())
+
+        if isinstance(opt, cfg.StrOpt) and opt.type.choices:
+            if isinstance(opt.type.choices, list):
+                valid_values = ", ".join([str(x) for x in opt.type.choices])
+            else:
+                valid_values = opt.type.choices
+            print("# Valid values: %s" % (valid_values))
+
         print(("%s = %s" % (opt.name, value)).strip())
 
 
