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

ofekisr · 2021-06-22T14:16:53Z

SUMMARY

#15362 P2 implementaion, based on #15423

codecov · 2021-06-24T18:29:44Z

Codecov Report

Merging #15296 (4dc87f0) into master (2cb13e6) will decrease coverage by 0.29%.
The diff coverage is 17.85%.

@@            Coverage Diff             @@
##           master   #15296      +/-   ##
==========================================
- Coverage   76.96%   76.66%   -0.30%     
==========================================
  Files         976      979       +3     
  Lines       51296    51350      +54     
  Branches     6911     6911              
==========================================
- Hits        39478    39370     -108     
- Misses      11599    11761     +162     
  Partials      219      219

Flag	Coverage Δ
hive	`?`
mysql	`81.47% <14.81%> (-0.14%)`	⬇️
postgres	`81.48% <17.85%> (-0.14%)`	⬇️
presto	`?`
python	`81.57% <17.85%> (-0.58%)`	⬇️
sqlite	`81.10% <14.81%> (-0.14%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/initialization/secret_keys/__init__.py	`0.00% <0.00%> (ø)`
superset/initialization/secret_keys/consts.py	`0.00% <0.00%> (ø)`
...t/initialization/secret_keys/cookie_signing_key.py	`0.00% <0.00%> (ø)`
superset/initialization/__init__.py	`87.05% <40.00%> (-0.87%)`	⬇️
superset/app.py	`63.38% <70.00%> (-0.12%)`	⬇️
superset/config.py	`97.04% <100.00%> (+0.01%)`	⬆️
superset/db_engines/hive.py	`0.00% <0.00%> (-82.15%)`	⬇️
superset/db_engine_specs/hive.py	`69.44% <0.00%> (-17.07%)`	⬇️
superset/db_engine_specs/presto.py	`83.36% <0.00%> (-6.53%)`	⬇️
superset/views/database/mixins.py	`81.03% <0.00%> (-1.73%)`	⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cb13e6...4dc87f0. Read the comment docs.

amitmiran137 · 2021-06-24T18:55:51Z

superset/config.py

+# "single"
+ENVIRONMENT_TYPE = os.environ.get("ENVIRONMENT_TYPE", "multithreaded")
+
+COOKIE_SIGNING_KEY = os.environ.get("COOKIE_SIGNING_KEY", SECRET_KEY)


Suggested change

COOKIE_SIGNING_KEY = os.environ.get("COOKIE_SIGNING_KEY", SECRET_KEY)

# it is also possible to use a lambda to load on every usage and change on the fly upon env var update like the following

# COOKIE_SIGNING_KEY = lambda: os.environ.get("COOKIE_SIGNING_KEY", SECRET_KEY)

COOKIE_SIGNING_KEY = os.environ.get("COOKIE_SIGNING_KEY", SECRET_KEY)

ktmud · 2021-07-06T21:59:28Z

Please add PR description or convert this to a draft if not ready for review yet

craig-rueda

Is this part of SIP 70? Should we wait for approval before this is merged?

craig-rueda · 2021-07-06T22:43:41Z

superset/initialization/secret_keys/__init__.py

@@ -0,0 +1,47 @@
+# Licensed to the Apache Software Foundation (ASF) under one


Let's keep init empty.

craig-rueda · 2021-07-06T22:53:02Z

superset/app.py

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


 class SupersetApp(Flask):


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)

craig-rueda · 2021-07-06T22:55:27Z

superset/initialization/secret_keys/__init__.py

+
+from .cookie_signing_key import define_cookie_signing_key
+
+if TYPE_CHECKING:


Do we need this?

craig-rueda

Also wondering if there's some overlap with the encrypted field manager I added a while back. I'd prefer if we merged the functionality in order to prevent duplication.

Lastly, I don't see any tests.

ofekisr · 2021-07-07T10:43:09Z

Also wondering if there's some overlap with the encrypted field manager I added a while back. I'd prefer if we merged the functionality in order to prevent duplication.

Lastly, I don't see any tests.

what you added is a specific concern - encryptedColumnFactory
here is totally different concern

craig-rueda · 2021-07-07T20:41:55Z

Also wondering if there's some overlap with the encrypted field manager I added a while back. I'd prefer if we merged the functionality in order to prevent duplication.
Lastly, I don't see any tests.

what you added is a specific concern - encryptedColumnFactory
here is totally different concern

Sorta - They both deal with keys and the source of key material. I think it would be great to have a central secrets manager that can be easily overridden by orgs that want to extend functionality. Also, we have established patterns for this kinda stuff (app factory pattern) that should be followed here.

pull-request-size bot added the size/L label Jun 22, 2021

ofekisr changed the title ~~Feat/keys management cookie~~ [WIP] Feat/keys management cookie Jun 22, 2021

amitmiran137 changed the title ~~[WIP] Feat/keys management cookie~~ feat(keys-management) : manage cookie secret key Jun 22, 2021

amitmiran137 changed the title ~~feat(keys-management) : manage cookie secret key~~ feat(keys-mgmt) : manage cookie secret key Jun 22, 2021

amitmiran137 changed the title ~~feat(keys-mgmt) : manage cookie secret key~~ feat(keys-rotation) : manage cookie secret key Jun 24, 2021

amitmiran137 mentioned this pull request Jun 24, 2021

[SIP-70] Secret keys rotations and stop using single SECRET_KEY #15362

Closed

ofekisr marked this pull request as ready for review June 24, 2021 18:13

amitmiran137 changed the title ~~feat(keys-rotation) : manage cookie secret key~~ feat(keys-rotation): manage cookie secret key Jun 24, 2021

amitmiran137 linked an issue Jun 24, 2021 that may be closed by this pull request

[SIP-70] Secret keys rotations and stop using single SECRET_KEY #15362

Closed

apache deleted a comment from ofekisr Jun 24, 2021

amitmiran137 reviewed Jun 24, 2021

View reviewed changes

dpgaspar requested a review from a team June 28, 2021 11:10

ofekisr added 4 commits July 4, 2021 13:41

feat: add keys-management as new dependency

f62ae09

feat: defining new optional dependency

bceaf24

feat: init keys_management

7390499

feat: add signing cookie key definition

b500f19

ofekisr force-pushed the feat/keys_management_cookie branch from a1015a6 to b500f19 Compare July 4, 2021 11:26

fix lints

4dc87f0

craig-rueda reviewed Jul 6, 2021

View reviewed changes

craig-rueda requested changes Jul 6, 2021

View reviewed changes

amitmiran137 closed this Dec 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ofekisr commented Jun 22, 2021 •

edited

codecov bot commented Jun 24, 2021 •

edited

amitmiran137 Jun 24, 2021 •

edited

ktmud commented Jul 6, 2021 •

edited

craig-rueda left a comment

craig-rueda Jul 6, 2021

craig-rueda Jul 6, 2021

craig-rueda Jul 6, 2021

craig-rueda left a comment

ofekisr commented Jul 7, 2021

craig-rueda commented Jul 7, 2021

		@@ -0,0 +1,47 @@
		# Licensed to the Apache Software Foundation (ASF) under one

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


		class SupersetApp(Flask):


		from .cookie_signing_key import define_cookie_signing_key

		if TYPE_CHECKING:

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

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

Conversation

ofekisr commented Jun 22, 2021 • edited

SUMMARY

codecov bot commented Jun 24, 2021 • edited

Codecov Report

amitmiran137 Jun 24, 2021 • edited

Choose a reason for hiding this comment

ktmud commented Jul 6, 2021 • edited

craig-rueda left a comment

Choose a reason for hiding this comment

craig-rueda Jul 6, 2021

Choose a reason for hiding this comment

craig-rueda Jul 6, 2021

Choose a reason for hiding this comment

craig-rueda Jul 6, 2021

Choose a reason for hiding this comment

craig-rueda left a comment

Choose a reason for hiding this comment

ofekisr commented Jul 7, 2021

craig-rueda commented Jul 7, 2021

ofekisr commented Jun 22, 2021 •

edited

codecov bot commented Jun 24, 2021 •

edited

amitmiran137 Jun 24, 2021 •

edited

ktmud commented Jul 6, 2021 •

edited