aminghadersohi commented on code in PR #43827: URL: https://github.com/apache/superset/pull/43827#discussion_r3931428632
########## superset/semantic_layers/masking.py: ########## @@ -0,0 +1,334 @@ +# 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. +"""Masking of secret material in semantic layer configurations. + +A semantic layer's ``configuration`` is a credentialed connection payload +(the analogue of a ``Database`` row's ``encrypted_extra``). Provider +configuration schemas mark secret fields with pydantic ``SecretStr``, which +renders in JSON schema as ``{"type": "string", "format": "password", +"writeOnly": true}``. :func:`mask_configuration` walks the registered +provider's schema and replaces the values of those fields with +``PASSWORD_MASK`` before a configuration leaves the server; every other +field passes through untouched so clients can still display and edit the +non-secret parts. + +Fail-closed posture: when the schema cannot say which fields are secret — +the layer's type has no registered provider (extension not loaded), schema +generation fails, or a key is not described by the schema — every scalar Review Comment: **The module docstring contradicts the implemented posture, in the one clause a reader of a redaction control will trust most.** > "when the schema cannot say which fields are secret — the layer's type has no registered provider, schema generation fails, **or a key is not described by the schema** — every scalar value in the affected subtree is masked rather than exposed." The first two clauses are true. The third is not: `_mask_object` reveals undescribed keys (L219-224, `else item`), and `test_reveals_keys_the_schema_does_not_describe_but_masks_marked_secrets` asserts exactly that. Measured against a provider whose schema declares `account`/`token`/`auth` but not `legacy_password`: ``` in : {"account":"acme","token":"TOK","auth":{...},"legacy_password":"OLD-SECRET"} out: {"account":"acme","token":"XXXXXXXXXX","auth":{...masked...},"legacy_password":"OLD-SECRET"} ``` Same at nested depth: an undeclared `auth.legacy_token` comes back in the clear. I think **the code is right and the docstring is wrong**. The PR description states the intent plainly — "keeping #43474's established behavior: mask only fields the schema marks secret, reveal everything else" — and strict fail-closed here would mask legitimate non-secret fields every time the stored payload outlives a schema revision, which is a real UX regression for a routine schema evolution. So this is a docs fix, not a behavior change. But please make it explicit, because as written the docstring promises a security property the module does not provide, and the next person to touch this will either weaken the code to match the docs or trust a guarantee that isn't there. Something like: *fail closed when the schema as a whole is unusable (no provider, generation raises, non-dict); within a usable schema, reveal anything it does not mark secret.* ########## superset/semantic_layers/masking.py: ########## @@ -0,0 +1,334 @@ +# 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. +"""Masking of secret material in semantic layer configurations. + +A semantic layer's ``configuration`` is a credentialed connection payload +(the analogue of a ``Database`` row's ``encrypted_extra``). Provider +configuration schemas mark secret fields with pydantic ``SecretStr``, which +renders in JSON schema as ``{"type": "string", "format": "password", +"writeOnly": true}``. :func:`mask_configuration` walks the registered +provider's schema and replaces the values of those fields with +``PASSWORD_MASK`` before a configuration leaves the server; every other +field passes through untouched so clients can still display and edit the +non-secret parts. + +Fail-closed posture: when the schema cannot say which fields are secret — +the layer's type has no registered provider (extension not loaded), schema +generation fails, or a key is not described by the schema — every scalar +value in the affected subtree is masked rather than exposed. + +Every client-facing path that emits a stored configuration must route through +:func:`mask_configuration` — today that is only ``_serialize_layer`` on the two +GET endpoints. Any future export/import of a semantic layer (there is none yet) +must mask through this same function rather than emitting the raw column. + +Masking covers the stored *configuration payload* only. It cannot reach a +schema a provider builds from that payload: ``get_configuration_schema`` and +``get_runtime_schema`` responses are returned to clients verbatim (e.g. the +``runtime_schema`` endpoint), so a provider MUST NOT echo configuration values +— least of all secret ones — back into the schema it returns. Enrichment must +carry only field *shapes* (option lists, defaults for non-secret fields), never +the submitted credential material. + +:func:`unmask_configuration` is the write-side counterpart, mirroring the +``Database`` API's ``masked_encrypted_extra`` round-trip: a client may echo +a read payload back on update, so any submitted value equal to +``PASSWORD_MASK`` is replaced with the currently stored value at the same +path. The sentinel swap is schema-independent, which keeps edits safe even +when the provider schema evolved after the row was stored; a mask with no +stored counterpart passes through unchanged (matching +``BaseEngineSpec.unmask_encrypted_extra``), where provider validation +rejects it. +""" + +from __future__ import annotations + +from typing import Any + +from superset.constants import PASSWORD_MASK +from superset.semantic_layers.registry import registry + +_UNION_KEYS = ("anyOf", "oneOf", "allOf") + +JsonSchema = dict[str, Any] + + +def _resolve_ref(schema: JsonSchema, defs: dict[str, JsonSchema]) -> JsonSchema: + """Follow a ``$ref`` into ``$defs`` (one level; refs to refs iterate).""" + seen: set[str] = set() + while "$ref" in schema: + ref_name = schema["$ref"].rsplit("/", 1)[-1] + if ref_name in seen or ref_name not in defs: + return {} + seen.add(ref_name) + schema = defs[ref_name] + return schema + + +def _is_secret_schema( + schema: JsonSchema, + defs: dict[str, JsonSchema], + _depth: int = 0, +) -> bool: + """Whether a field schema denotes secret material (``SecretStr``). + + Union branches (``SecretStr | None``, discriminated unions of scalar + credential kinds) count as secret when any branch does: a field that + may hold a secret must always be masked. + """ + if _depth > 16: + return True # pathological schema: fail closed + schema = _resolve_ref(schema, defs) + if schema.get("format") == "password" or schema.get("writeOnly") is True: + return True + return any( + _is_secret_schema(branch, defs, _depth + 1) + for key in _UNION_KEYS + for branch in schema.get(key, []) + # An object variant is not itself a secret; its own properties are + # classified field by field when the value is walked. + if _resolve_ref(branch, defs).get("type") != "object" + ) + + +def _mask_all(value: Any) -> Any: + """Mask every scalar in a subtree the schema cannot vouch for.""" + if isinstance(value, dict): + return {key: _mask_all(item) for key, item in value.items()} + if isinstance(value, list): + return [_mask_all(item) for item in value] + if value is None: + return None + return PASSWORD_MASK if value else value + + +def _combine_masked(item: Any, candidates: list[Any]) -> Any: + """Merge the masking results of one value under several candidate schemas. + + A position is revealed only when *every* candidate reveals it identically; + any divergence keeps it masked. This is what makes union handling + fail-closed: a secret nested inside a single union branch is masked even + when a sibling branch would have revealed the same key, so trusting one + branch can never expose the other's secret. + """ + first = candidates[0] + if all(candidate == first for candidate in candidates): + return first + if isinstance(item, dict) and all(isinstance(c, dict) for c in candidates): + keys = set().union(*(c.keys() for c in candidates)) + return { + key: _combine_masked( + item.get(key), [c[key] for c in candidates if key in c] + ) + for key in keys + } + if isinstance(item, list) and all(isinstance(c, list) for c in candidates): + return [ + _combine_masked( + item[index] if index < len(item) else None, + [c[index] for c in candidates if index < len(c)], + ) + for index in range(max(len(c) for c in candidates)) + ] + # Irreconcilable classifications for the same value: fail closed. + return _mask_all(item) + + +def _mask_against( + item: Any, schemas: list[JsonSchema], defs: dict[str, JsonSchema] +) -> Any: + """Mask ``item`` conservatively against every schema it might match.""" + return _combine_masked( + item, [_mask_value(item, schema, defs) for schema in schemas] + ) + + +def _object_variants( + schema: JsonSchema, + defs: dict[str, JsonSchema], +) -> list[JsonSchema]: + """The object schemas a value may conform to: itself plus union branches.""" + schema = _resolve_ref(schema, defs) + variants = [schema] + for key in _UNION_KEYS: + variants.extend(_resolve_ref(branch, defs) for branch in schema.get(key, [])) + return [ + variant + for variant in variants + if "properties" in variant + or "additionalProperties" in variant + or variant.get("type") == "object" + ] + + +def _mask_object( + value: dict[str, Any], + schema: JsonSchema, + defs: dict[str, JsonSchema], +) -> dict[str, Any]: + """Mask a dict value against the object schemas it may conform to.""" + variants = _object_variants(schema, defs) + if not variants: + # The schema does not describe this position as an object (e.g. an + # unresolvable $ref). Reveal it, matching #43474's top-level behavior + # of masking only fields the schema marks secret; the top-level + # fail-closed (whole schema unavailable) is handled in + # ``mask_configuration``. + return value + properties: dict[str, list[JsonSchema]] = {} + for variant in variants: + for key, sub in variant.get("properties", {}).items(): + properties.setdefault(key, []).append(sub) + # Free-form keys (not in any variant's ``properties``) are classified by + # ``additionalProperties``. Mask against EVERY variant's + # additionalProperties schema, not just the first: when variants declare + # differing additionalProperties, trusting one branch could reveal a + # value another branch marks secret. If any variant does not describe + # such keys with a schema (no dict ``additionalProperties``), the key is + # unclassifiable there, so fail closed and mask it. + additional_schemas = [ + variant["additionalProperties"] + for variant in variants + if isinstance(variant.get("additionalProperties"), dict) + ] + all_variants_classify_extra = all( + isinstance(variant.get("additionalProperties"), dict) for variant in variants + ) + masked: dict[str, Any] = {} + for key, item in value.items(): + subs = properties.get(key) + if subs is None: + # A key no variant declares. If every variant constrains extra + # keys with an ``additionalProperties`` schema, classify against + # all of them (differing variants must agree to reveal); otherwise + # the key is schema-undescribed, so reveal it (a nested secret is + # only masked where the schema marks it, matching #43474). + masked[key] = ( + _mask_against(item, additional_schemas, defs) + if additional_schemas and all_variants_classify_extra Review Comment: **`all_variants_classify_extra` inverts this block's own stated rule, and leaves "Review Finding 1" only half-fixed.** The comment two lines up says: *"If any variant does not describe such keys with a schema (no dict `additionalProperties`), the key is unclassifiable there, so fail closed and mask it."* The code does the opposite — when `all_variants_classify_extra` is `False` it takes the `else item` branch and reveals. Measured, on a union where one variant marks free-form keys secret and the sibling is simply silent about them: ``` anyOf: [ {properties:{host}, additionalProperties:{format:password, writeOnly:true}}, {properties:{host}} ] value: {"host":"h","extra_cred":"LEAKY"} -> {"host":"h","extra_cred":"LEAKY"} # revealed ``` Add `additionalProperties: {"type":"string"}` to the second variant and the same value masks correctly. So the leak isn't triggered by two variants *disagreeing* — it's triggered by one variant *not saying anything*, which is the more common shape. This is the same principle `_combine_masked`'s docstring states ("a secret nested inside a single union branch is masked even when a sibling branch would have revealed the same key") and the same one the PR description claims for Finding 1 ("a variant marking such a key secret can't be overridden by an earlier variant that would reveal it"). Here a variant marking the key secret *is* overridden — by a variant that says nothing at all. Suggested fix is to drop the extra gate: ```python masked[key] = _mask_against(item, additional_schemas, defs) if additional_schemas else item ``` A silent variant means "unconstrained", not "not secret", so treating it as unclassifiable and masking is the safe direction. `_combine_masked` already handles the divergence. Worth a test alongside `test_additionalproperties_divergent_union_variants_mask_conservatively`, which only covers the both-variants-classify case. (Independently flagged by the bito bot as CWE-200 on this hunk; I confirmed it by execution.) ########## superset/semantic_layers/masking.py: ########## @@ -0,0 +1,334 @@ +# 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. +"""Masking of secret material in semantic layer configurations. + +A semantic layer's ``configuration`` is a credentialed connection payload +(the analogue of a ``Database`` row's ``encrypted_extra``). Provider +configuration schemas mark secret fields with pydantic ``SecretStr``, which +renders in JSON schema as ``{"type": "string", "format": "password", +"writeOnly": true}``. :func:`mask_configuration` walks the registered +provider's schema and replaces the values of those fields with +``PASSWORD_MASK`` before a configuration leaves the server; every other +field passes through untouched so clients can still display and edit the +non-secret parts. + +Fail-closed posture: when the schema cannot say which fields are secret — +the layer's type has no registered provider (extension not loaded), schema +generation fails, or a key is not described by the schema — every scalar +value in the affected subtree is masked rather than exposed. + +Every client-facing path that emits a stored configuration must route through +:func:`mask_configuration` — today that is only ``_serialize_layer`` on the two +GET endpoints. Any future export/import of a semantic layer (there is none yet) +must mask through this same function rather than emitting the raw column. + +Masking covers the stored *configuration payload* only. It cannot reach a +schema a provider builds from that payload: ``get_configuration_schema`` and +``get_runtime_schema`` responses are returned to clients verbatim (e.g. the +``runtime_schema`` endpoint), so a provider MUST NOT echo configuration values +— least of all secret ones — back into the schema it returns. Enrichment must +carry only field *shapes* (option lists, defaults for non-secret fields), never +the submitted credential material. + +:func:`unmask_configuration` is the write-side counterpart, mirroring the +``Database`` API's ``masked_encrypted_extra`` round-trip: a client may echo +a read payload back on update, so any submitted value equal to +``PASSWORD_MASK`` is replaced with the currently stored value at the same +path. The sentinel swap is schema-independent, which keeps edits safe even +when the provider schema evolved after the row was stored; a mask with no +stored counterpart passes through unchanged (matching +``BaseEngineSpec.unmask_encrypted_extra``), where provider validation +rejects it. +""" + +from __future__ import annotations + +from typing import Any + +from superset.constants import PASSWORD_MASK +from superset.semantic_layers.registry import registry + +_UNION_KEYS = ("anyOf", "oneOf", "allOf") + +JsonSchema = dict[str, Any] + + +def _resolve_ref(schema: JsonSchema, defs: dict[str, JsonSchema]) -> JsonSchema: + """Follow a ``$ref`` into ``$defs`` (one level; refs to refs iterate).""" + seen: set[str] = set() + while "$ref" in schema: + ref_name = schema["$ref"].rsplit("/", 1)[-1] + if ref_name in seen or ref_name not in defs: + return {} + seen.add(ref_name) + schema = defs[ref_name] + return schema + + +def _is_secret_schema( + schema: JsonSchema, + defs: dict[str, JsonSchema], + _depth: int = 0, +) -> bool: + """Whether a field schema denotes secret material (``SecretStr``). + + Union branches (``SecretStr | None``, discriminated unions of scalar + credential kinds) count as secret when any branch does: a field that + may hold a secret must always be masked. + """ + if _depth > 16: + return True # pathological schema: fail closed + schema = _resolve_ref(schema, defs) + if schema.get("format") == "password" or schema.get("writeOnly") is True: + return True + return any( + _is_secret_schema(branch, defs, _depth + 1) + for key in _UNION_KEYS + for branch in schema.get(key, []) + # An object variant is not itself a secret; its own properties are + # classified field by field when the value is walked. + if _resolve_ref(branch, defs).get("type") != "object" + ) + + +def _mask_all(value: Any) -> Any: + """Mask every scalar in a subtree the schema cannot vouch for.""" + if isinstance(value, dict): + return {key: _mask_all(item) for key, item in value.items()} + if isinstance(value, list): + return [_mask_all(item) for item in value] + if value is None: + return None + return PASSWORD_MASK if value else value + + +def _combine_masked(item: Any, candidates: list[Any]) -> Any: + """Merge the masking results of one value under several candidate schemas. + + A position is revealed only when *every* candidate reveals it identically; + any divergence keeps it masked. This is what makes union handling + fail-closed: a secret nested inside a single union branch is masked even + when a sibling branch would have revealed the same key, so trusting one + branch can never expose the other's secret. + """ + first = candidates[0] + if all(candidate == first for candidate in candidates): + return first + if isinstance(item, dict) and all(isinstance(c, dict) for c in candidates): + keys = set().union(*(c.keys() for c in candidates)) + return { + key: _combine_masked( + item.get(key), [c[key] for c in candidates if key in c] + ) + for key in keys + } + if isinstance(item, list) and all(isinstance(c, list) for c in candidates): + return [ + _combine_masked( + item[index] if index < len(item) else None, + [c[index] for c in candidates if index < len(c)], + ) + for index in range(max(len(c) for c in candidates)) + ] + # Irreconcilable classifications for the same value: fail closed. + return _mask_all(item) + + +def _mask_against( + item: Any, schemas: list[JsonSchema], defs: dict[str, JsonSchema] +) -> Any: + """Mask ``item`` conservatively against every schema it might match.""" + return _combine_masked( + item, [_mask_value(item, schema, defs) for schema in schemas] + ) + + +def _object_variants( + schema: JsonSchema, + defs: dict[str, JsonSchema], +) -> list[JsonSchema]: + """The object schemas a value may conform to: itself plus union branches.""" + schema = _resolve_ref(schema, defs) + variants = [schema] + for key in _UNION_KEYS: + variants.extend(_resolve_ref(branch, defs) for branch in schema.get(key, [])) + return [ + variant + for variant in variants + if "properties" in variant + or "additionalProperties" in variant + or variant.get("type") == "object" + ] + + +def _mask_object( + value: dict[str, Any], + schema: JsonSchema, + defs: dict[str, JsonSchema], +) -> dict[str, Any]: + """Mask a dict value against the object schemas it may conform to.""" + variants = _object_variants(schema, defs) + if not variants: + # The schema does not describe this position as an object (e.g. an + # unresolvable $ref). Reveal it, matching #43474's top-level behavior + # of masking only fields the schema marks secret; the top-level + # fail-closed (whole schema unavailable) is handled in + # ``mask_configuration``. + return value + properties: dict[str, list[JsonSchema]] = {} + for variant in variants: + for key, sub in variant.get("properties", {}).items(): + properties.setdefault(key, []).append(sub) + # Free-form keys (not in any variant's ``properties``) are classified by + # ``additionalProperties``. Mask against EVERY variant's + # additionalProperties schema, not just the first: when variants declare + # differing additionalProperties, trusting one branch could reveal a + # value another branch marks secret. If any variant does not describe + # such keys with a schema (no dict ``additionalProperties``), the key is + # unclassifiable there, so fail closed and mask it. + additional_schemas = [ + variant["additionalProperties"] + for variant in variants + if isinstance(variant.get("additionalProperties"), dict) + ] + all_variants_classify_extra = all( + isinstance(variant.get("additionalProperties"), dict) for variant in variants + ) + masked: dict[str, Any] = {} + for key, item in value.items(): + subs = properties.get(key) + if subs is None: + # A key no variant declares. If every variant constrains extra + # keys with an ``additionalProperties`` schema, classify against + # all of them (differing variants must agree to reveal); otherwise + # the key is schema-undescribed, so reveal it (a nested secret is + # only masked where the schema marks it, matching #43474). + masked[key] = ( + _mask_against(item, additional_schemas, defs) + if additional_schemas and all_variants_classify_extra + else item + ) + else: + # A key may be described by several union variants. Mask against + # all of them so a secret nested in one variant is never revealed + # by trusting another (``_is_secret_schema`` does not descend into + # object ``properties``, so a single-branch check would miss it). + masked[key] = _mask_against(item, subs, defs) + return masked + + +def _mask_list( + value: list[Any], + schema: JsonSchema, + defs: dict[str, JsonSchema], +) -> list[Any]: + """Mask a list value against its item schema (may sit in a union branch).""" + resolved = _resolve_ref(schema, defs) + # ``list[str] | None`` puts the array schema in an anyOf branch; + # check the schema itself first, then its union branches. + candidates = [resolved] + [ + _resolve_ref(branch, defs) + for key in _UNION_KEYS + for branch in resolved.get(key, []) + ] + item_schemas = [ + candidate["items"] + for candidate in candidates + if isinstance(candidate.get("items"), dict) + ] + if not item_schemas: + return value + # Mask each element against every candidate item schema, so an element + # matching a secret-bearing union branch is masked even when another + # branch would reveal it. + return [_mask_against(item, item_schemas, defs) for item in value] + + +def _mask_value(value: Any, schema: JsonSchema, defs: dict[str, JsonSchema]) -> Any: + """Mask secrets in ``value`` as classified by ``schema``.""" + if _is_secret_schema(schema, defs): + # Mask only a truthy secret; an empty/None/0/False value hides + # nothing and is left as-is (matching the top-level masker in #43474). + return PASSWORD_MASK if value else value + if isinstance(value, dict): + return _mask_object(value, schema, defs) + if isinstance(value, list): + return _mask_list(value, schema, defs) + return value + + +def mask_configuration(layer_type: str, configuration: Any) -> dict[str, Any]: + """Return ``configuration`` with all secret material replaced. + + ``layer_type`` selects the registered provider whose published + configuration schema (``get_configuration_schema``) classifies the + fields. With no registered provider (or an unusable schema) every + scalar is masked — a configuration whose secrecy cannot be established + is never exposed. + """ + if not configuration or not isinstance(configuration, dict): + return {} + cls = registry.get(layer_type) + if cls is None: + return _mask_all(configuration) + try: + # The connector's own published shape (the same source #43474's + # top-level masker used); this walker extends it to nested/union + # secrets rather than only top-level ``writeOnly`` properties. + schema: JsonSchema = cls.get_configuration_schema() + except Exception: # pylint: disable=broad-except + return _mask_all(configuration) + if not isinstance(schema, dict): + return _mask_all(configuration) + return _mask_value(configuration, schema, schema.get("$defs", {})) Review Comment: **Three fail-closed guards, but a fourth degenerate schema falls through to a full reveal.** `mask_configuration` fails closed on no provider, on a raising `get_configuration_schema`, and on a non-dict schema. It does not fail closed on a *dict* schema that describes nothing. Measured against `{"account":"acme","password":"S3CRET","auth":{"user":"bob","password":"NESTED"}}`: | schema returned | result | |---|---| | raises | all masked ✅ | | `None` | all masked ✅ | | `{}` | **whole config in the clear** | | `{"type": "object"}` | **whole config in the clear** | | `{"title": "C"}` | **whole config in the clear** | `{}` and `None` are two spellings of the same failure and they land on opposite sides of the fence. This is structurally the same shape as #43474's `if not secret_keys: return config` — the branch this PR is here to remove — just relocated from "no secret keys found" to "no keys found at all". Cheap to close: ```python if not isinstance(schema, dict) or not schema: return _mask_all(configuration) ``` and, if you want the stronger version, treat a top-level schema that yields no `_object_variants` the same way — a provider that can't describe its own configuration as an object hasn't told you anything about it. ########## superset/semantic_layers/masking.py: ########## @@ -0,0 +1,334 @@ +# 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. +"""Masking of secret material in semantic layer configurations. + +A semantic layer's ``configuration`` is a credentialed connection payload +(the analogue of a ``Database`` row's ``encrypted_extra``). Provider +configuration schemas mark secret fields with pydantic ``SecretStr``, which +renders in JSON schema as ``{"type": "string", "format": "password", +"writeOnly": true}``. :func:`mask_configuration` walks the registered +provider's schema and replaces the values of those fields with +``PASSWORD_MASK`` before a configuration leaves the server; every other +field passes through untouched so clients can still display and edit the +non-secret parts. + +Fail-closed posture: when the schema cannot say which fields are secret — +the layer's type has no registered provider (extension not loaded), schema +generation fails, or a key is not described by the schema — every scalar +value in the affected subtree is masked rather than exposed. + +Every client-facing path that emits a stored configuration must route through +:func:`mask_configuration` — today that is only ``_serialize_layer`` on the two +GET endpoints. Any future export/import of a semantic layer (there is none yet) +must mask through this same function rather than emitting the raw column. + +Masking covers the stored *configuration payload* only. It cannot reach a +schema a provider builds from that payload: ``get_configuration_schema`` and +``get_runtime_schema`` responses are returned to clients verbatim (e.g. the +``runtime_schema`` endpoint), so a provider MUST NOT echo configuration values +— least of all secret ones — back into the schema it returns. Enrichment must +carry only field *shapes* (option lists, defaults for non-secret fields), never +the submitted credential material. + +:func:`unmask_configuration` is the write-side counterpart, mirroring the +``Database`` API's ``masked_encrypted_extra`` round-trip: a client may echo +a read payload back on update, so any submitted value equal to +``PASSWORD_MASK`` is replaced with the currently stored value at the same +path. The sentinel swap is schema-independent, which keeps edits safe even +when the provider schema evolved after the row was stored; a mask with no +stored counterpart passes through unchanged (matching +``BaseEngineSpec.unmask_encrypted_extra``), where provider validation +rejects it. +""" + +from __future__ import annotations + +from typing import Any + +from superset.constants import PASSWORD_MASK +from superset.semantic_layers.registry import registry + +_UNION_KEYS = ("anyOf", "oneOf", "allOf") + +JsonSchema = dict[str, Any] + + +def _resolve_ref(schema: JsonSchema, defs: dict[str, JsonSchema]) -> JsonSchema: + """Follow a ``$ref`` into ``$defs`` (one level; refs to refs iterate).""" + seen: set[str] = set() + while "$ref" in schema: + ref_name = schema["$ref"].rsplit("/", 1)[-1] + if ref_name in seen or ref_name not in defs: + return {} + seen.add(ref_name) + schema = defs[ref_name] + return schema + + +def _is_secret_schema( + schema: JsonSchema, + defs: dict[str, JsonSchema], + _depth: int = 0, +) -> bool: + """Whether a field schema denotes secret material (``SecretStr``). + + Union branches (``SecretStr | None``, discriminated unions of scalar + credential kinds) count as secret when any branch does: a field that + may hold a secret must always be masked. + """ + if _depth > 16: + return True # pathological schema: fail closed + schema = _resolve_ref(schema, defs) + if schema.get("format") == "password" or schema.get("writeOnly") is True: + return True + return any( + _is_secret_schema(branch, defs, _depth + 1) + for key in _UNION_KEYS + for branch in schema.get(key, []) + # An object variant is not itself a secret; its own properties are + # classified field by field when the value is walked. + if _resolve_ref(branch, defs).get("type") != "object" + ) + + +def _mask_all(value: Any) -> Any: + """Mask every scalar in a subtree the schema cannot vouch for.""" + if isinstance(value, dict): + return {key: _mask_all(item) for key, item in value.items()} + if isinstance(value, list): + return [_mask_all(item) for item in value] + if value is None: + return None + return PASSWORD_MASK if value else value + + +def _combine_masked(item: Any, candidates: list[Any]) -> Any: + """Merge the masking results of one value under several candidate schemas. + + A position is revealed only when *every* candidate reveals it identically; + any divergence keeps it masked. This is what makes union handling + fail-closed: a secret nested inside a single union branch is masked even + when a sibling branch would have revealed the same key, so trusting one + branch can never expose the other's secret. + """ + first = candidates[0] + if all(candidate == first for candidate in candidates): + return first + if isinstance(item, dict) and all(isinstance(c, dict) for c in candidates): + keys = set().union(*(c.keys() for c in candidates)) + return { + key: _combine_masked( + item.get(key), [c[key] for c in candidates if key in c] + ) + for key in keys + } + if isinstance(item, list) and all(isinstance(c, list) for c in candidates): + return [ + _combine_masked( + item[index] if index < len(item) else None, + [c[index] for c in candidates if index < len(c)], + ) + for index in range(max(len(c) for c in candidates)) + ] + # Irreconcilable classifications for the same value: fail closed. + return _mask_all(item) + + +def _mask_against( + item: Any, schemas: list[JsonSchema], defs: dict[str, JsonSchema] +) -> Any: + """Mask ``item`` conservatively against every schema it might match.""" + return _combine_masked( + item, [_mask_value(item, schema, defs) for schema in schemas] + ) + + +def _object_variants( + schema: JsonSchema, + defs: dict[str, JsonSchema], +) -> list[JsonSchema]: + """The object schemas a value may conform to: itself plus union branches.""" + schema = _resolve_ref(schema, defs) + variants = [schema] + for key in _UNION_KEYS: + variants.extend(_resolve_ref(branch, defs) for branch in schema.get(key, [])) + return [ + variant + for variant in variants + if "properties" in variant + or "additionalProperties" in variant + or variant.get("type") == "object" + ] + + +def _mask_object( + value: dict[str, Any], + schema: JsonSchema, + defs: dict[str, JsonSchema], +) -> dict[str, Any]: + """Mask a dict value against the object schemas it may conform to.""" + variants = _object_variants(schema, defs) + if not variants: Review Comment: **An unresolvable `$ref` reveals a subtree the schema *does* mark secret.** `_resolve_ref` returns `{}` for a missing or cyclic `$defs` target, `_object_variants` then returns `[]`, and this branch reveals the whole subtree. Measured — same `Config` as above, with `$defs` emptied but `properties.auth` still `{"$ref": "#/$defs/Auth"}`: ``` {"account":"acme","token":"XXXXXXXXXX","api_key":"XXXXXXXXXX", "auth":{"user":"bob","password":"NESTED-SECRET"}} ``` `Auth.password` is `writeOnly: true` in the schema the provider intended to publish. It comes back in the clear only because the reference couldn't be followed. I recognise this is deliberate — `test_unresolvable_ref_reveals_its_subtree` codifies it, and the reasoning ("a real provider schema resolves its refs") is fair. But it isn't the same case as "the schema doesn't describe this key": here the schema *did* try to describe it and the walker couldn't follow. That's a schema-resolution failure, and every other schema-resolution failure in this module fails closed. Worth distinguishing "`$ref` present but unresolvable" (→ `_mask_all`) from "no object schema at this position" (→ reveal). `_resolve_ref` already knows the difference; it just flattens both into `{}`. Low likelihood, but the cost of being wrong is a nested credential in a GET response, and the fix is local. Same argument applies to `_mask_list`'s untyped-array reveal (L254) if the array schema came from an unresolvable `$ref`. ########## superset/semantic_layers/api.py: ########## @@ -82,45 +83,17 @@ def _mask_configuration(layer: SemanticLayer, config: dict[str, Any]) -> dict[str, Any]: + """Redact configuration values the connector marks secret, at any depth. + + Delegates to :func:`superset.semantic_layers.masking.mask_configuration`, + which walks the connector's published ``get_configuration_schema`` and + masks every ``writeOnly`` / ``SecretStr`` field it finds --- including + ones nested inside objects, discriminated unions, and lists. This extends + the original top-level-only masking (#43474) to close the nested/union + secret leak its flat scan missed, and fails closed (masks everything) when + the schema is unavailable. """ - Redact configuration values the connector's schema marks as write-only. - - A connector publishes its configuration shape via ``get_configuration_schema``; - a property with ``"writeOnly": true`` (the standard JSON Schema way of - marking a field that's set but never echoed back, e.g. a password or API - key) is replaced with ``PASSWORD_MASK`` here rather than returned in the - clear. - """ - schema: dict[str, Any] | None = None - if cls := registry.get(layer.type): - try: - schema = cls.get_configuration_schema() - except Exception: # pylint: disable=broad-except - schema = None - - if schema is None: - # Either the type isn't registered or its schema couldn't load, so we - # can't tell which fields are secret. Fail closed: mask every truthy - # value rather than risk echoing a credential back in the clear. - logger.warning( - "Could not determine the configuration schema for semantic layer " - "type %s; masking all configuration values.", - layer.type, - ) - return {key: PASSWORD_MASK if value else value for key, value in config.items()} - - secret_keys = { - key - for key, prop in schema.get("properties", {}).items() - if isinstance(prop, dict) and prop.get("writeOnly") - } - if not secret_keys: - return config - - return { - key: PASSWORD_MASK if key in secret_keys and value else value - for key, value in config.items() - } + return mask_configuration(layer.type, config) Review Comment: The old `_mask_configuration` logged a `logger.warning` naming the layer type whenever it hit the fail-closed path. That's gone, and `mask_configuration` fails closed silently in all three of its branches (`cls is None`, `get_configuration_schema` raising, non-dict schema). The behavior is right, but the observability regressed: an operator looking at a fully-`XXXXXXXXXX` configuration can no longer distinguish "the extension isn't loaded" from "the provider's schema call is throwing" from "this payload really is all secrets" — and the first two are outages that will otherwise present as a confusing UI. A `logger.warning` in each fail-closed branch of `masking.mask_configuration`, carrying `layer_type` and the reason, would restore it. (Also raised by the bito bot.) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
