From 3e1dbe14773551c227ef44049fe53adab430ec8a Mon Sep 17 00:00:00 2001 From: W Chan Date: Sat, 22 Oct 2022 18:03:02 -0700 Subject: [PATCH 1/5] Add backward compatibility to secret masking for legacy output schema A new output schema using full JSON schema was introduced and secrets previously masked using the legacy output schema now being displayed as plain text. To prevent security relative issues, add backward compatibility to secret masking. Full output schema validattion will need to be migrated to the new schema. --- st2common/st2common/util/output_schema.py | 42 +++++++++++++++++-- .../tests/unit/test_util_output_schema.py | 11 ++++- 2 files changed, 48 insertions(+), 5 deletions(-) diff --git a/st2common/st2common/util/output_schema.py b/st2common/st2common/util/output_schema.py index 902908b653..a3a341c62e 100644 --- a/st2common/st2common/util/output_schema.py +++ b/st2common/st2common/util/output_schema.py @@ -36,7 +36,14 @@ def _output_schema_is_valid(_schema): if not isinstance(_schema, Mapping): # malformed schema return False + + if "type" not in _schema: + # legacy schema format + return False + try: + # the validator is smart enough to handle + # schema that is similar to the input schema schema.validate( _schema, schema.get_action_output_schema(), @@ -44,11 +51,24 @@ def _output_schema_is_valid(_schema): ) except jsonschema.ValidationError as e: LOG.debug("output_schema not valid: %s", e) - # likely a legacy partial object schema (only defines properties) return False + return True +def _normalize_legacy_output_schema(_schema): + if not isinstance(_schema, Mapping): + return _schema + + _normalized_schema = { + "type": "object", + "properties": _schema, + "additionalProperties": True, + } + + return _normalized_schema + + def _validate_runner(runner_schema, result): LOG.debug("Validating runner output: %s", runner_schema) @@ -183,15 +203,31 @@ def mask_secret_output(ac_ex, output_value): or output_key not in output_value # no action output_schema defined or not output_schema - # malformed action output_schema - or not _output_schema_is_valid(output_schema) ): # nothing to mask return output_value + # backward compatibility for legacy output_schema so secrets stay masked + if not _output_schema_is_valid(output_schema): + # normalized the legacy schema to a full JSON schema and check if it is valid + normalized_output_schema = _normalize_legacy_output_schema(output_schema) + + if not _output_schema_is_valid(normalized_output_schema): + # nothing to mask + return output_value + + # mask secret for the legacy output schema + output_value[output_key] = _get_masked_value( + normalized_output_schema, output_value[output_key] + ) + + return output_value + + # mask secret for the output schema output_value[output_key] = _get_masked_value( output_schema, output_value[output_key] ) + return output_value diff --git a/st2common/tests/unit/test_util_output_schema.py b/st2common/tests/unit/test_util_output_schema.py index c4db4921b7..14653f288f 100644 --- a/st2common/tests/unit/test_util_output_schema.py +++ b/st2common/tests/unit/test_util_output_schema.py @@ -399,8 +399,15 @@ def test_mask_secret_output_noop_legacy_schema(self): "output_schema": RUNNER_OUTPUT_SCHEMA, }, } - ac_ex_result = {"output_1": "foobar"} - expected_masked_output = {"output_1": "foobar"} + + ac_ex_result = {OUTPUT_KEY: {"output_1": "foobar", "output_3": "fubar"}} + + expected_masked_output = { + OUTPUT_KEY: { + "output_1": "foobar", + "output_3": MASKED_ATTRIBUTE_VALUE, + } + } # Legacy schemas should be ignored since they aren't full json schemas. masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) From 316a036bcde7ebaef4d4dfb9c13b789bd448d43e Mon Sep 17 00:00:00 2001 From: W Chan Date: Sat, 22 Oct 2022 18:06:40 -0700 Subject: [PATCH 2/5] Add changelog entry --- CHANGELOG.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 6a792ea942..b55ae0818e 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -39,6 +39,13 @@ Fixed * Fixed generation of `st2.conf.sample` to show correct syntax for `[sensorcontainer].partition_provider` (space separated `key:value` pairs). #5710 Contributed by @cognifloyd +* A new output schema using full JSON schema was introduced and secrets previously masked using + the legacy output schema are now being displayed as plain text. To prevent security relative + issues, add backward compatibility to secret masking. Full output schema validattion will need + to be migrated to the new schema. + + Contributed by @m4dcoder + Added ~~~~~ From 856ad86ae75c8deb5f5045dfe9d693fe44652147 Mon Sep 17 00:00:00 2001 From: W Chan Date: Sat, 22 Oct 2022 18:08:23 -0700 Subject: [PATCH 3/5] Fix minor typo in changelog entry --- CHANGELOG.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index b55ae0818e..5e9aa22fe3 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -41,7 +41,7 @@ Fixed * A new output schema using full JSON schema was introduced and secrets previously masked using the legacy output schema are now being displayed as plain text. To prevent security relative - issues, add backward compatibility to secret masking. Full output schema validattion will need + issues, add backward compatibility to secret masking. Full output schema validation will need to be migrated to the new schema. Contributed by @m4dcoder From 369e3965975f9d65cbf9a4ace654e45b54ba2de7 Mon Sep 17 00:00:00 2001 From: W Chan Date: Tue, 25 Oct 2022 21:53:03 -0700 Subject: [PATCH 4/5] Reword the change log entry for clarification --- CHANGELOG.rst | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 0720b9cdc5..03732d62c6 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -23,7 +23,6 @@ Fixed Contributed by @bharath-orchestral - * Fixed ``st2client/st2client/base.py`` file to use ``https_proxy``(not ``http_proxy``) to check HTTPS_PROXY environment variables. Contributed by @wfgydbu @@ -39,17 +38,16 @@ Fixed * Fixed generation of `st2.conf.sample` to show correct syntax for `[sensorcontainer].partition_provider` (space separated `key:value` pairs). #5710 Contributed by @cognifloyd -* A new output schema using full JSON schema was introduced and secrets previously masked using - the legacy output schema are now being displayed as plain text. To prevent security relative - issues, add backward compatibility to secret masking. Full output schema validation will need - to be migrated to the new schema. +* Fix access to key-value pairs in workflow and action execution where RBAC rules did not get applied #5764 Contributed by @m4dcoder -* Fix access to key-value pairs in workflow and action execution where RBAC rules did not get applied +* Add backward compatibility to secret masking introduced in #5319 to prevent security-relative issues. + Migration to the new schema is required to take advantage of the full output schema validation. #5783 Contributed by @m4dcoder + Added ~~~~~ From b54de929e6d08ca573a789174b3a54989183ff84 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Wed, 26 Oct 2022 00:04:49 -0500 Subject: [PATCH 5/5] Rename modified test --- st2common/tests/unit/test_util_output_schema.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/st2common/tests/unit/test_util_output_schema.py b/st2common/tests/unit/test_util_output_schema.py index 14653f288f..9e27bff61e 100644 --- a/st2common/tests/unit/test_util_output_schema.py +++ b/st2common/tests/unit/test_util_output_schema.py @@ -389,7 +389,7 @@ def test_mask_secret_output_noop(self): masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) self.assertDictEqual(masked_output, expected_masked_output) - def test_mask_secret_output_noop_legacy_schema(self): + def test_mask_secret_output_with_legacy_schema(self): ac_ex = { "action": { "output_schema": LEGACY_ACTION_OUTPUT_SCHEMA, @@ -409,7 +409,7 @@ def test_mask_secret_output_noop_legacy_schema(self): } } - # Legacy schemas should be ignored since they aren't full json schemas. + # Legacy schemas are not full json schemas, but they should still mask secrets. masked_output = output_schema.mask_secret_output(ac_ex, ac_ex_result) self.assertDictEqual(masked_output, expected_masked_output)