Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(keys-rotation): manage cookie secret key #15296

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ combine_as_imports = true
include_trailing_comma = true
line_length = 88
known_first_party = superset
known_third_party =alembic,apispec,backoff,bleach,cachelib,celery,click,colorama,contextlib2,cron_descriptor,croniter,cryptography,dateutil,deprecation,flask,flask_appbuilder,flask_babel,flask_caching,flask_compress,flask_jwt_extended,flask_login,flask_migrate,flask_sqlalchemy,flask_talisman,flask_testing,flask_wtf,freezegun,geohash,geopy,graphlib,holidays,humanize,isodate,jinja2,jwt,markdown,markupsafe,marshmallow,marshmallow_enum,msgpack,numpy,pandas,parameterized,parsedatetime,pathlib2,pgsanity,pkg_resources,polyline,prison,pyarrow,pyhive,pyparsing,pytest,pytz,redis,requests,retry,selenium,setuptools,simplejson,slack,sqlalchemy,sqlalchemy_utils,sqlparse,typing_extensions,werkzeug,wtforms,wtforms_json,yaml
known_third_party =alembic,apispec,backoff,bleach,cachelib,celery,click,colorama,contextlib2,cron_descriptor,croniter,cryptography,dateutil,deprecation,flask,flask_appbuilder,flask_babel,flask_caching,flask_compress,flask_jwt_extended,flask_login,flask_migrate,flask_sqlalchemy,flask_talisman,flask_testing,flask_wtf,freezegun,geohash,geopy,graphlib,holidays,humanize,isodate,itsdangerous,jinja2,jwt,keys_management,markdown,markupsafe,marshmallow,marshmallow_enum,msgpack,numpy,pandas,parameterized,parsedatetime,pathlib2,pgsanity,pkg_resources,polyline,prison,pyarrow,pyhive,pyparsing,pytest,pytz,redis,requests,retry,selenium,setuptools,simplejson,slack,sqlalchemy,sqlalchemy_utils,sqlparse,typing_extensions,werkzeug,wtforms,wtforms_json,yaml
multi_line_output = 3
order_by_type = false

Expand Down
1 change: 1 addition & 0 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ def get_git_sha():
"humanize",
"itsdangerous>=1.0.0, <2.0.0",
"isodate",
"keys-management>=0.1.4, <0.2.0",
"markdown>=3.0",
"msgpack>=1.0.0, <1.1",
"pandas>=1.2.2, <1.3",
Expand Down
15 changes: 13 additions & 2 deletions superset/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,17 @@
import os
import sys
from types import ModuleType
from typing import Any, Dict, Union
from typing import Any, Dict, Optional, TYPE_CHECKING, Union

from flask import Flask
from werkzeug.utils import import_string

from superset.initialization import SupersetAppInitializer
from superset.utils.core import is_test

if TYPE_CHECKING:
from keys_management import KeysManagement

logger = logging.getLogger(__name__)


Expand Down Expand Up @@ -104,4 +107,12 @@ def load_override_config() -> Union[Dict[Any, Any], ModuleType]:


class SupersetApp(Flask):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I agree with this pattern of hanging fields off of the App. A more widely accepted/established pattern is to use a custom extension that can be added to the Flask app via the extensions. We already have a mechanism for this (extensions.py)

pass
_keys_management: Optional[KeysManagement]

@property
def keys_management(self) -> Optional[KeysManagement]:
return self._keys_management

@keys_management.setter
def keys_management(self, keys_management: KeysManagement) -> None:
self._keys_management = keys_management
9 changes: 9 additions & 0 deletions superset/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,7 @@ def _try_json_readsha( # pylint: disable=unused-argument
# Allow users to export full CSV of table viz type.
# This could cause the server to run out of memory or compute.
"ALLOW_FULL_CSV_EXPORT": False,
"SECRET_KEYS_ROTATIONS": False,
}

# Feature flags may also be set via 'SUPERSET_FEATURE_' prefixed environment vars.
Expand Down Expand Up @@ -1222,6 +1223,14 @@ def CSV_TO_HIVE_UPLOAD_DIRECTORY_FUNC(
SQLALCHEMY_DOCS_URL = "https://docs.sqlalchemy.org/en/13/core/engines.html"
SQLALCHEMY_DISPLAY_TEXT = "SQLAlchemy docs"

# The environment type the application run on: "multithreaded", "multiprocess" and
# "single"
SECRET_KEYS_MANAGEMENT = {
"ENVIRONMENT_TYPE": os.environ.get("ENVIRONMENT_TYPE", "multithreaded"),
"COOKIE_SIGNING_KEY": os.environ.get("COOKIE_SIGNING_KEY", SECRET_KEY),
}


# -------------------------------------------------------------------
# * WARNING: STOP EDITING HERE *
# -------------------------------------------------------------------
Expand Down
7 changes: 7 additions & 0 deletions superset/initialization/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -563,6 +563,8 @@ def init_app(self) -> None:
# Configuration of feature_flags must be done first to allow init features
# conditionally
self.configure_feature_flags()
if feature_flag_manager.is_feature_enabled("SECRET_KEYS_ROTATIONS"):
self.configure_secret_keys()
self.configure_db_encrypt()
self.setup_db()
self.configure_celery()
Expand Down Expand Up @@ -716,6 +718,11 @@ def register_blueprints(self) -> None:
def setup_bundle_manifest(self) -> None:
manifest_processor.init_app(self.superset_app)

def configure_secret_keys(self) -> None:
from superset.initialization.secret_keys import configure

configure(self.superset_app)


class SupersetIndexView(IndexView):
@expose("/")
Expand Down
47 changes: 47 additions & 0 deletions superset/initialization/secret_keys/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
# Licensed to the Apache Software Foundation (ASF) under one
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep init empty.

# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
from __future__ import annotations

from typing import Dict, TYPE_CHECKING

from keys_management import EnvironemtType, KeysManagementImpl

from superset.exceptions import SupersetException

from .cookie_signing_key import define_cookie_signing_key

if TYPE_CHECKING:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this?

from superset.app import SupersetApp


def configure(app: SupersetApp) -> None:
keys_management_configurations = app.config.get("SECRET_KEYS_MANAGEMENT", {})
keys_management = KeysManagementImpl(
environemt_type=determine_environment_type(keys_management_configurations)
)
app.keys_management = keys_management
define_cookie_signing_key(app)


def determine_environment_type(
keys_management_configurations: Dict[str, str]
) -> EnvironemtType:
environment_type = keys_management_configurations.get("ENVIRONMENT_TYPE", "single")
try:
return EnvironemtType[environment_type.upper()]
except KeyError as ex:
raise SupersetException("environment_type config value is invalid", ex)
18 changes: 18 additions & 0 deletions superset/initialization/secret_keys/consts.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.

COOKIE_SIGNING_KEY = "COOKIE_SIGNING_KEY"
69 changes: 69 additions & 0 deletions superset/initialization/secret_keys/cookie_signing_key.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
from __future__ import annotations

from typing import Optional, TYPE_CHECKING

from flask.sessions import SecureCookieSessionInterface
from itsdangerous import URLSafeTimedSerializer
from keys_management import OnChangeKeyDefinition, SecretKeyFlow, SecretKeyUseCase

from .consts import COOKIE_SIGNING_KEY

if TYPE_CHECKING:
from superset.app import SupersetApp


def define_cookie_signing_key(app: SupersetApp) -> None:
# pylint: disable=W0613
app.keys_management.define_key( # type: ignore
name=COOKIE_SIGNING_KEY,
keys_store=lambda: app.config.get(COOKIE_SIGNING_KEY),
use_case=SecretKeyUseCase.ONE_WAY_TRIP,
stateless=True,
keep_in_cache=False,
)

def on_key_change(
old_key: str, new_key: str, on_change_key_definition: OnChangeKeyDefinition
) -> None:
app.config[COOKIE_SIGNING_KEY] = new_key

app.keys_management.register_on_change( # type: ignore
COOKIE_SIGNING_KEY, on_key_change
)
app.session_interface = SupersetSecureCookieSession()


class SupersetSecureCookieSession(SecureCookieSessionInterface):
def get_signing_serializer(
self, app: SupersetApp
) -> Optional[URLSafeTimedSerializer]:
current_signing_key = app.keys_management.get_key( # type: ignore
COOKIE_SIGNING_KEY, SecretKeyFlow.DEFAULT
)
if not current_signing_key:
return None
signer_kwargs = dict(
key_derivation=self.key_derivation, digest_method=self.digest_method
)
return URLSafeTimedSerializer(
current_signing_key,
salt=self.salt,
serializer=self.serializer,
signer_kwargs=signer_kwargs,
)