From e37a98b62ea3c4d0d9576d466e8677d001ac3c9c Mon Sep 17 00:00:00 2001 From: Hugh A Miles II Date: Thu, 29 Oct 2026 20:07:47 -0400 Subject: [PATCH 1/5] fix(embedded): refuse an unreadable embedded key pair when it is saved A key-pair embedded credential with a wrong passphrase or a body that is not a PEM key saved with a 200, then broke every embedded chart (sc-130269). Completeness was the only save-time check. Add BaseEngineSpec.get_embedded_credentials_error, a hook that says why a complete credential is unusable; Snowflake decodes the key there. The database schema and the validate-parameters command call it through embedded_credentials_error, which first restores masked secrets from the stored connection so that an untouched edit still saves. The message never quotes the key or its passphrase. Co-Authored-By: Claude Opus 5.5 --- superset/commands/database/validate.py | 17 ++- superset/databases/schemas.py | 10 +- superset/databases/utils.py | 32 ++++++ superset/db_engine_specs/base.py | 17 +++ superset/db_engine_specs/snowflake.py | 23 ++++ .../commands/databases/validate_test.py | 62 ++++++++++- tests/unit_tests/databases/schema_tests.py | 100 +++++++++++++++++- .../db_engine_specs/test_snowflake.py | 86 +++++++++++++++ 8 files changed, 343 insertions(+), 4 deletions(-) diff --git a/superset/commands/database/validate.py b/superset/commands/database/validate.py index 84d54374eaae..fc8d2dd28d0a 100644 --- a/superset/commands/database/validate.py +++ b/superset/commands/database/validate.py @@ -33,7 +33,11 @@ ) from superset.constants import EMBEDDED_CREDENTIALS_KEY from superset.daos.database import DatabaseDAO -from superset.databases.utils import embedded_credentials_incomplete, make_url_safe +from superset.databases.utils import ( + embedded_credentials_error, + embedded_credentials_incomplete, + make_url_safe, +) from superset.db_engine_specs import get_engine_spec from superset.errors import ErrorLevel, SupersetError, SupersetErrorType from superset.extensions import event_logger @@ -282,6 +286,17 @@ def _get_embedded_credentials_error(self) -> Optional[SupersetError]: if not embedded_credentials_incomplete( credentials, engine_spec.validate_embedded_credentials ): + if message := embedded_credentials_error( + engine_spec, + credentials, + self._model.encrypted_extra if self._model else None, + ): + return SupersetError( + message=message, + error_type=SupersetErrorType.INVALID_PAYLOAD_SCHEMA_ERROR, + level=ErrorLevel.ERROR, + extra={"invalid": [EMBEDDED_CREDENTIALS_KEY]}, + ) return None return SupersetError( diff --git a/superset/databases/schemas.py b/superset/databases/schemas.py index 8b8303ea9feb..fc7cbd7d9caa 100644 --- a/superset/databases/schemas.py +++ b/superset/databases/schemas.py @@ -52,7 +52,11 @@ EncryptedDict, # noqa: F401 EncryptedString, # noqa: F401 ) -from superset.databases.utils import embedded_credentials_incomplete, make_url_safe +from superset.databases.utils import ( + embedded_credentials_error, + embedded_credentials_incomplete, + make_url_safe, +) from superset.db_engine_specs import get_engine_spec from superset.db_engine_specs.base import BaseEngineSpec from superset.exceptions import CertificateException, SupersetSecurityException @@ -487,6 +491,10 @@ def validate_embedded_credentials( ), field_name="masked_encrypted_extra", ) + if message := embedded_credentials_error( + engine_spec, credentials, stored.encrypted_extra if stored else None + ): + raise ValidationError(message, field_name="masked_encrypted_extra") # A credential only applies to a per-user OAuth2 connection with # impersonation on (`Database.embedded_fallback_applies`). Anywhere else diff --git a/superset/databases/utils.py b/superset/databases/utils.py index f18eccb467b0..3adba55201f4 100644 --- a/superset/databases/utils.py +++ b/superset/databases/utils.py @@ -24,6 +24,7 @@ from sqlalchemy.exc import NoSuchTableError from superset.commands.database.exceptions import DatabaseInvalidError +from superset.constants import EMBEDDED_CREDENTIALS_KEY, PASSWORD_MASK from superset.sql.parse import Table from superset.utils import json @@ -33,6 +34,7 @@ TableMetadataForeignKeysIndexesResponse, TableMetadataResponse, ) + from superset.db_engine_specs.base import BaseEngineSpec def get_foreign_keys_metadata( @@ -245,3 +247,33 @@ def embedded_credentials_incomplete( return False return validate_credentials(value) is None + + +def embedded_credentials_error( + engine_spec: type[BaseEngineSpec], + credentials: Any, + stored_encrypted_extra: str | None, +) -> str | None: + """ + Why an authored embedded credential cannot be used, or ``None``. + + Complements :func:`embedded_credentials_incomplete`, which only checks that the + fields are filled in, by asking the engine whether their values work (see + ``BaseEngineSpec.get_embedded_credentials_error``). An edit sends unchanged + secrets masked, so they are first restored from ``stored_encrypted_extra``; a + secret that is still masked has nothing to restore it from and is not checked. + """ + if not isinstance(credentials, dict): + return None + unmasked = engine_spec.unmask_encrypted_extra( + stored_encrypted_extra, + json.dumps({EMBEDDED_CREDENTIALS_KEY: credentials}), + ) + try: + credentials = json.loads(unmasked or "{}").get(EMBEDDED_CREDENTIALS_KEY) + except (AttributeError, json.JSONDecodeError): + return None + validated = engine_spec.validate_embedded_credentials(credentials) + if validated is None or PASSWORD_MASK in validated.values(): + return None + return engine_spec.get_embedded_credentials_error(validated) diff --git a/superset/db_engine_specs/base.py b/superset/db_engine_specs/base.py index f9c109e3f781..54606cc2bc5d 100644 --- a/superset/db_engine_specs/base.py +++ b/superset/db_engine_specs/base.py @@ -2584,6 +2584,23 @@ def validate_embedded_credentials(cls, credentials: Any) -> dict[str, str] | Non """ return validated_embedded_credentials(credentials) + @classmethod + def get_embedded_credentials_error( + cls, + credentials: dict[str, str], # pylint: disable=unused-argument + ) -> str | None: + """ + Explain why a complete embedded fallback credential cannot be used, or + return ``None``. + + ``validate_embedded_credentials`` only checks that the fields are filled in; + this checks that what they hold is usable, e.g. that a private key decodes + with its passphrase. Called when a credential is saved, never at query time, + and with unmasked secrets. The message is shown to the administrator saving + the connection, so it must never quote the credential's values. + """ + return None + @classmethod def apply_embedded_credentials( cls, diff --git a/superset/db_engine_specs/snowflake.py b/superset/db_engine_specs/snowflake.py index 12057db37329..472ff9a059b2 100644 --- a/superset/db_engine_specs/snowflake.py +++ b/superset/db_engine_specs/snowflake.py @@ -24,6 +24,7 @@ from urllib import parse import sqlalchemy as sa +from cryptography.exceptions import UnsupportedAlgorithm from cryptography.hazmat.backends import default_backend from cryptography.hazmat.primitives import serialization from flask import current_app as app, has_request_context @@ -470,6 +471,28 @@ def apply_embedded_credentials( return url, engine_kwargs + @classmethod + def get_embedded_credentials_error(cls, credentials: dict[str, str]) -> str | None: + """ + Refuse a key pair whose key does not decode with its passphrase. + + Decoding happens before any connection exists, so a bad key would otherwise + save cleanly and only fail once an embedded viewer opens a chart. + """ + if credentials.get("auth_method") != "keypair": + return None + try: + cls._load_private_key(credentials) + except (TypeError, ValueError, UnsupportedAlgorithm): + # The exception text is not passed on: keep key material out of the + # response however the parser chooses to describe it. + return __( + "The embedded guest private key could not be read. Provide a PEM " + "private key and its correct passphrase (leave the passphrase " + "empty for an unencrypted key)." + ) + return None + @classmethod def get_oauth2_authorization_uri( cls, diff --git a/tests/unit_tests/commands/databases/validate_test.py b/tests/unit_tests/commands/databases/validate_test.py index 1d5e4fd896d8..537757445840 100644 --- a/tests/unit_tests/commands/databases/validate_test.py +++ b/tests/unit_tests/commands/databases/validate_test.py @@ -967,6 +967,27 @@ def test_incomplete_embedded_credentials_from_the_api(mocker: MockerFixture) -> assert excinfo.value.errors[0].extra["invalid"] == ["embedded_credentials"] +def _pem(passphrase: str) -> str: + """A freshly generated RSA private key, PEM-encoded under ``passphrase``.""" + from cryptography.hazmat.primitives import serialization + from cryptography.hazmat.primitives.asymmetric import rsa + + encryption = ( + serialization.BestAvailableEncryption(passphrase.encode()) + if passphrase + else serialization.NoEncryption() + ) + return ( + rsa.generate_private_key(public_exponent=65537, key_size=2048) + .private_bytes( + serialization.Encoding.PEM, + serialization.PrivateFormat.PKCS8, + encryption, + ) + .decode() + ) + + @pytest.mark.parametrize( "credentials", [ @@ -976,7 +997,7 @@ def test_incomplete_embedded_credentials_from_the_api(mocker: MockerFixture) -> { "username": "embedded_svc", "auth_method": "keypair", - "privatekey_body": "-----BEGIN PRIVATE KEY-----", + "privatekey_body": _pem(""), }, { "username": "embedded_svc", @@ -1123,3 +1144,42 @@ def validate_embedded_credentials( else: with pytest.raises(InvalidParametersError): command.run() + + +@pytest.mark.parametrize( + "privatekey_body, privatekey_pass", + [ + (_pem("key passphrase"), "wrong passphrase"), + ("-----BEGIN PRIVATE KEY-----", ""), + ], +) +@with_feature_flags(EMBEDDED_CREDENTIAL_FALLBACK=True, EMBEDDED_SUPERSET=True) +def test_an_unreadable_embedded_key_is_reported_on_its_field( + mocker: MockerFixture, privatekey_body: str, privatekey_pass: str +) -> None: + """ + The dialog validates before saving, so an unreadable key is reported on the + embedded credential field there too, without quoting the key (sc-130269). + """ + mocker.patch("superset.commands.database.validate.DatabaseDAO") + + properties = { + "engine": "snowflake", + "parameters": { + "embedded_credentials": { + "username": "embedded_svc", + "auth_method": "keypair", + "privatekey_body": privatekey_body, + "privatekey_pass": privatekey_pass, + } + }, + } + with pytest.raises(InvalidParametersError) as excinfo: + ValidateDatabaseParametersCommand(properties).run() + + error = excinfo.value.errors[0] + assert error.extra["invalid"] == ["embedded_credentials"] + assert "private key could not be read" in error.message + for secret in (privatekey_body, privatekey_pass): + if secret: + assert secret not in error.message diff --git a/tests/unit_tests/databases/schema_tests.py b/tests/unit_tests/databases/schema_tests.py index a873e5951854..a95b4ec31b2f 100644 --- a/tests/unit_tests/databases/schema_tests.py +++ b/tests/unit_tests/databases/schema_tests.py @@ -998,7 +998,7 @@ def test_partial_update_uses_the_stored_engine_credential_validator( "masked_encrypted_extra": json.dumps({"embedded_credentials": {"token": "t"}}) } DatabasePutSchema(database_id=1).load(payload) - validator.assert_called_once_with({"token": "t"}) + validator.assert_called_with({"token": "t"}) @pytest.mark.parametrize("credentials", ["not json", "[]", "42", [], False, 0]) @@ -1099,6 +1099,104 @@ def test_embedded_credentials_requirement_skips_engines_without_the_fallback() - ) +def _pem(passphrase: str) -> str: + """A freshly generated RSA private key, PEM-encoded under ``passphrase``.""" + from cryptography.hazmat.primitives import serialization + from cryptography.hazmat.primitives.asymmetric import rsa + + return ( + rsa.generate_private_key(public_exponent=65537, key_size=2048) + .private_bytes( + serialization.Encoding.PEM, + serialization.PrivateFormat.PKCS8, + serialization.BestAvailableEncryption(passphrase.encode()), + ) + .decode() + ) + + +def _snowflake_keypair_payload(body: str, passphrase: str) -> dict[str, Any]: + return { + "database_name": "test_db", + "sqlalchemy_uri": "snowflake://base_user@xy12345/db", + "impersonate_user": True, + "masked_encrypted_extra": json.dumps( + { + "oauth2_client_info": _CLIENT, + "embedded_credentials": { + "username": "embedded_svc", + "auth_method": "keypair", + "privatekey_body": body, + "privatekey_pass": passphrase, + }, + } + ), + } + + +@pytest.mark.parametrize( + "body, passphrase, expect_error", + [ + ("right", "key passphrase", False), + ("right", "wrong passphrase", True), + ("not a PEM key", "key passphrase", True), + ], +) +@with_feature_flags(EMBEDDED_CREDENTIAL_FALLBACK=True, EMBEDDED_SUPERSET=True) +def test_an_unreadable_embedded_key_is_refused_at_save( + body: str, passphrase: str, expect_error: bool +) -> None: + """A key pair that cannot be decoded is refused when saved (sc-130269).""" + from superset.databases.schemas import DatabasePostSchema + + pem = _pem("key passphrase") if body == "right" else body + payload = _snowflake_keypair_payload(pem, passphrase) + + if not expect_error: + DatabasePostSchema().load(payload) + return + with pytest.raises(ValidationError) as excinfo: + DatabasePostSchema().load(payload) + messages = str(excinfo.value.messages) + assert "private key could not be read" in messages + assert passphrase not in messages + assert all(line not in messages for line in pem.splitlines()[1:-1] or [pem]) + + +@pytest.mark.parametrize( + "new_passphrase, expect_error", + [("XXXXXXXXXX", False), ("key passphrase", False), ("wrong passphrase", True)], +) +@with_feature_flags(EMBEDDED_CREDENTIAL_FALLBACK=True, EMBEDDED_SUPERSET=True) +def test_an_edit_checks_the_embedded_key_against_its_stored_secrets( + mocker: MockerFixture, new_passphrase: str, expect_error: bool +) -> None: + """ + An edit sends an unchanged key and passphrase masked. They are restored from + the stored connection before the key is checked, so an untouched credential + still saves and a new passphrase is checked against the stored key. + """ + from superset.databases.schemas import DatabasePutSchema + from superset.models.core import Database + + saved = _snowflake_keypair_payload(_pem("key passphrase"), "key passphrase") + stored = Database( + sqlalchemy_uri=saved["sqlalchemy_uri"], + impersonate_user=True, + encrypted_extra=saved["masked_encrypted_extra"], + ) + mocker.patch("superset.databases.schemas.db.session.get", return_value=stored) + payload = _snowflake_keypair_payload("XXXXXXXXXX", new_passphrase) + del payload["database_name"], payload["sqlalchemy_uri"] + + if expect_error: + with pytest.raises(ValidationError) as excinfo: + DatabasePutSchema(database_id=1).load(payload) + assert "private key could not be read" in str(excinfo.value.messages) + else: + DatabasePutSchema(database_id=1).load(payload) + + @pytest.mark.parametrize("stored_impersonate_user", [True, False]) @with_feature_flags(EMBEDDED_CREDENTIAL_FALLBACK=True, EMBEDDED_SUPERSET=True) def test_embedded_credentials_requirement_uses_stored_impersonation_on_update( diff --git a/tests/unit_tests/db_engine_specs/test_snowflake.py b/tests/unit_tests/db_engine_specs/test_snowflake.py index 1f01f6b01df8..04e43ea9fd40 100644 --- a/tests/unit_tests/db_engine_specs/test_snowflake.py +++ b/tests/unit_tests/db_engine_specs/test_snowflake.py @@ -1145,6 +1145,92 @@ def test_embedded_keypair_secrets_are_masked_and_restored() -> None: assert SnowflakeEngineSpec.unmask_encrypted_extra(original, masked) == original +def _pem(passphrase: str) -> str: + """A freshly generated RSA private key, PEM-encoded under ``passphrase``.""" + from cryptography.hazmat.primitives import serialization + from cryptography.hazmat.primitives.asymmetric import rsa + + encryption = ( + serialization.BestAvailableEncryption(passphrase.encode()) + if passphrase + else serialization.NoEncryption() + ) + return ( + rsa.generate_private_key(public_exponent=65537, key_size=2048) + .private_bytes( + serialization.Encoding.PEM, + serialization.PrivateFormat.PKCS8, + encryption, + ) + .decode() + ) + + +@pytest.mark.parametrize( + "key_passphrase, privatekey_pass", + [("key passphrase", "key passphrase"), ("", "")], +) +def test_a_readable_embedded_key_has_no_error( + key_passphrase: str, privatekey_pass: str +) -> None: + """An encrypted key with its passphrase, or an unencrypted one without, saves.""" + from superset.db_engine_specs.snowflake import SnowflakeEngineSpec + + credentials = { + "username": "embedded_svc", + "auth_method": "keypair", + "privatekey_body": _pem(key_passphrase), + "privatekey_pass": privatekey_pass, + } + + assert SnowflakeEngineSpec.get_embedded_credentials_error(credentials) is None + + +@pytest.mark.parametrize( + "key_passphrase, privatekey_body, privatekey_pass", + [ + ("key passphrase", None, "wrong passphrase"), + ("key passphrase", None, ""), + ("", None, "unexpected passphrase"), + ("", "not a PEM key", ""), + ("", "-----BEGIN PRIVATE KEY-----\nAAAA\n-----END PRIVATE KEY-----", ""), + ], +) +def test_an_unreadable_embedded_key_is_refused_without_echoing_it( + key_passphrase: str, privatekey_body: str | None, privatekey_pass: str +) -> None: + """ + A wrong passphrase or a body that is not a PEM key is refused when saved + (sc-130269) -- otherwise it saves and fails only once a guest opens a chart -- + and the message quotes neither the key nor its passphrase. + """ + from superset.db_engine_specs.snowflake import SnowflakeEngineSpec + + body = privatekey_body or _pem(key_passphrase) + credentials = { + "username": "embedded_svc", + "auth_method": "keypair", + "privatekey_body": body, + "privatekey_pass": privatekey_pass, + } + + message = SnowflakeEngineSpec.get_embedded_credentials_error(credentials) + + assert message is not None + assert "private key could not be read" in message + for secret in (body, privatekey_pass, *body.splitlines()): + if secret.strip("-") and "PRIVATE KEY" not in secret: + assert secret not in message + + +def test_a_password_embedded_credential_is_not_checked_for_a_key() -> None: + from superset.db_engine_specs.snowflake import SnowflakeEngineSpec + + credentials = {"username": "embedded_svc", "password": "embedded_pw"} + + assert SnowflakeEngineSpec.get_embedded_credentials_error(credentials) is None + + def test_update_params_from_encrypted_extra_prefers_the_fallback_over_keypair( mocker: MockerFixture, ) -> None: From dbdab4fe23553f7abdec37de2f24e323d508c5c2 Mon Sep 17 00:00:00 2001 From: Hugh A Miles II Date: Thu, 29 Oct 2026 20:07:47 -0400 Subject: [PATCH 2/5] fix(embedded): a key that fails at query time is a handled 400 and is logged The embedded key pair is decoded in apply_embedded_credentials, before the engine and its handle_error listener exist, so a bad key surfaced as a raw ValueError and an HTTP 500 "Issue 1011" with no admin log line (sc-130269). Catch a failure to apply the embedded credential in _get_sqla_engine, log "Embedded fallback credential rejected" with the exception class only, and raise a 400 CONNECTION_ACCESS_DENIED_ERROR, which guest redaction turns into the same generic message a rejected password gives. Co-Authored-By: Claude Opus 5.5 --- superset/models/core.py | 34 +++++++++--- tests/unit_tests/models/core_test.py | 80 ++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 7 deletions(-) diff --git a/superset/models/core.py b/superset/models/core.py index 8db9810a5c65..5e0d10345be4 100755 --- a/superset/models/core.py +++ b/superset/models/core.py @@ -829,14 +829,34 @@ def _get_sqla_engine( # pylint: disable=too-many-locals # noqa: C901 # password on the URL, a PEM key in connect_args. Both the URL and the # engine kwargs participate in the cache key below, keeping guest and # OAuth engines separate even when they use the same database username. - sqlalchemy_url, engine_kwargs = ( - self.db_engine_spec.apply_embedded_credentials( - self, - embedded_credentials, - sqlalchemy_url, - engine_kwargs, + try: + sqlalchemy_url, engine_kwargs = ( + self.db_engine_spec.apply_embedded_credentials( + self, + embedded_credentials, + sqlalchemy_url, + engine_kwargs, + ) ) - ) + except Exception as ex: + # Applying it can itself reject the credential -- a key pair is + # decoded here, before any engine (and its `handle_error` + # listener) exists. Log it and refuse it as a sign-in the database + # rejected would be, rather than letting the decoding error + # surface as a server fault (sc-130269). + _log_embedded_rejection(self.id, self.database_name, ex) + raise SupersetErrorException( + SupersetError( + message=__( + "The credential configured for embedded viewers of this " + "connection could not be used. An administrator needs " + "to update it." + ), + error_type=SupersetErrorType.CONNECTION_ACCESS_DENIED_ERROR, + level=ErrorLevel.ERROR, + ), + status=400, + ) from ex # Hooks must receive the selected connection identity, not the # username claim supplied by the guest token. effective_username = sqlalchemy_url.username diff --git a/tests/unit_tests/models/core_test.py b/tests/unit_tests/models/core_test.py index de598da79bcd..bc6493dbc038 100644 --- a/tests/unit_tests/models/core_test.py +++ b/tests/unit_tests/models/core_test.py @@ -3966,6 +3966,86 @@ def test_a_rejected_fallback_credential_is_logged_on_the_raw_connection_path( assert rejections == [] +def _encrypted_pem(passphrase: str) -> str: + """A freshly generated RSA private key, PEM-encoded under ``passphrase``.""" + from cryptography.hazmat.primitives import serialization + from cryptography.hazmat.primitives.asymmetric import rsa + + return ( + rsa.generate_private_key(public_exponent=65537, key_size=2048) + .private_bytes( + serialization.Encoding.PEM, + serialization.PrivateFormat.PKCS8, + serialization.BestAvailableEncryption(passphrase.encode()), + ) + .decode() + ) + + +@pytest.mark.parametrize( + "body, passphrase, exception", + [ + ("right", "wrong-passphrase", "ValueError"), + ("right", "", "TypeError"), + ("not a PEM key", "right-passphrase", "ValueError"), + ], +) +@with_feature_flags(EMBEDDED_CREDENTIAL_FALLBACK=True, EMBEDDED_SUPERSET=True) +def test_an_unreadable_embedded_key_is_refused_like_a_rejected_sign_in( + mocker: MockerFixture, body: str, passphrase: str, exception: str +) -> None: + """ + A key pair is decoded before any engine exists, so its failure never reaches + the engine's ``handle_error`` listener (sc-130269). It must still be logged + for administrators, and reach the viewer as a handled 400 that redaction turns + into the same generic message a rejected password gives -- not a 500. + """ + from superset.exceptions import SupersetErrorException + from superset.utils.error_sanitization import ( + GENERIC_ERROR_MESSAGE, + sanitize_superset_error, + ) + + pem = _encrypted_pem("right-passphrase") if body == "right" else body + database = _embedded_snowflake_database() + extra = database.get_encrypted_extra() + extra["embedded_credentials"] = { + "username": "embedded_svc", + "auth_method": "keypair", + "privatekey_body": pem, + "privatekey_pass": passphrase, + } + database.encrypted_extra = json.dumps(extra) + database.id = 12 + + mocker.patch("superset.models.core._ENGINE_CACHE", {}) + create_engine_mock = mocker.patch("superset.models.core.create_engine") + _become_guest(mocker) + _stub_driver_dependent_uri_validation(mocker) + warning = mocker.patch("superset.models.core.logger.warning") + + with pytest.raises(SupersetErrorException) as excinfo: + database._get_sqla_engine(nullpool=True) + + assert excinfo.value.status == 400 + assert excinfo.value.error.error_type == ( + SupersetErrorType.CONNECTION_ACCESS_DENIED_ERROR + ) + assert sanitize_superset_error(excinfo.value.error, required=True).message == ( + str(GENERIC_ERROR_MESSAGE) + ) + create_engine_mock.assert_not_called() + + rejections = [call for call in warning.call_args_list if "rejected" in call.args[0]] + assert len(rejections) == 1 + assert rejections[0].args[1:] == ("snowflake_db", 12, exception) + rendered = rejections[0].args[0] % rejections[0].args[1:] + for secret in (pem, passphrase, "embedded_svc"): + if secret: + assert secret not in rendered + assert secret not in str(excinfo.value.error.message) + + def test_a_rejection_reported_by_two_paths_is_logged_once( mocker: MockerFixture, ) -> None: From f6b3aca1f8943cc94b0e7cb4cc6ab10640545fc2 Mon Sep 17 00:00:00 2001 From: Hugh A Miles II Date: Sat, 31 Oct 2026 13:07:08 -0400 Subject: [PATCH 3/5] chore(embedded): add the new guest-credential messages to messages.pot Co-Authored-By: Claude Opus 5.5 --- superset/translations/messages.pot | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/superset/translations/messages.pot b/superset/translations/messages.pot index e25e68394249..1a16757d2afd 100644 --- a/superset/translations/messages.pot +++ b/superset/translations/messages.pot @@ -14275,6 +14275,9 @@ msgstr "" msgid "The country code standard that Superset should expect to find in the [country] column" msgstr "" +msgid "The credential configured for embedded viewers of this connection could not be used. An administrator needs to update it." +msgstr "" + msgid "The current version could not be identified. Restore is temporarily unavailable." msgstr "" @@ -14376,6 +14379,9 @@ msgstr "" msgid "The duration of time in seconds before the cache is invalidated. Set to -1 to bypass the cache." msgstr "" +msgid "The embedded guest private key could not be read. Provide a PEM private key and its correct passphrase (leave the passphrase empty for an unencrypted key)." +msgstr "" + msgid "The encoding format of the lines" msgstr "" From 5954183c5c707b1424cfc397b2f35fdcf1e6845b Mon Sep 17 00:00:00 2001 From: Hugh A Miles II Date: Sat, 31 Oct 2026 13:22:20 -0400 Subject: [PATCH 4/5] test(embedded): give the unreadable-key cases stable IDs The wrong-passphrase case is parametrized with a key encrypted at import time, and encryption is salted, so its auto-generated ID differed between pytest-xdist workers and CI aborted with "Different tests were collected". Co-Authored-By: Claude Opus 5.5 --- tests/unit_tests/commands/databases/validate_test.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit_tests/commands/databases/validate_test.py b/tests/unit_tests/commands/databases/validate_test.py index 537757445840..6750f20734cf 100644 --- a/tests/unit_tests/commands/databases/validate_test.py +++ b/tests/unit_tests/commands/databases/validate_test.py @@ -1149,8 +1149,8 @@ def validate_embedded_credentials( @pytest.mark.parametrize( "privatekey_body, privatekey_pass", [ - (_pem("key passphrase"), "wrong passphrase"), - ("-----BEGIN PRIVATE KEY-----", ""), + pytest.param(_pem("key passphrase"), "wrong passphrase", id="wrong-passphrase"), + pytest.param("-----BEGIN PRIVATE KEY-----", "", id="not-a-pem-key"), ], ) @with_feature_flags(EMBEDDED_CREDENTIAL_FALLBACK=True, EMBEDDED_SUPERSET=True) From c125752bcc62cbcaef0706d766c893d5a5207496 Mon Sep 17 00:00:00 2001 From: Hugh A Miles II Date: Sat, 31 Oct 2026 13:48:54 -0400 Subject: [PATCH 5/5] fix(embedded): refuse an unreadable imported key pair; keep its traceback quiet The API refuses a key pair whose passphrase doesn't open its key, but import still stored it, and every embedded chart then failed (sc-130269, QA on the integration branch). Import now decodes the key with the secrets it will be stored with and refuses it with ImportRefusedError, an ImportFailedError with HTTP 400. At query time the refusal was raised from the decoding error, so the generic error handler logged the decoder's traceback with it. It is now raised from None; the admin log line already names the error class. Co-Authored-By: Claude Opus 5.5 --- UPDATING.md | 3 +- .../commands/database/importers/v1/utils.py | 36 +++++- superset/commands/exceptions.py | 6 + superset/models/core.py | 6 +- .../commands/importers/v1/import_test.py | 116 ++++++++++++++++++ 5 files changed, 159 insertions(+), 8 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index 675df77b84d6..9d4d77b616c5 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -39,7 +39,8 @@ assists people when migrating to a new version. per-user result; existing cached entries for such charts are effectively invalidated for guests. A malformed or incomplete `embedded_credentials` is rejected by `POST`/`PUT /api/v1/database/` and by `validate_parameters` rather than stored and - ignored while both embedding flags are enabled; asset import is unchanged. Validation + ignored while both embedding flags are enabled. Import refuses a key pair that + doesn't decode with its passphrase, as the API does. Validation uses the engine's credential rules and edited credentials rather than stale stored values, and rejects whitespace-only usernames. Disabling either flag preserves stored credentials without blocking unrelated edits. Restored async guest queries use the same credential as synchronous queries. Connection-mutator hooks diff --git a/superset/commands/database/importers/v1/utils.py b/superset/commands/database/importers/v1/utils.py index 8fb173eb56a4..b1c70fb2a2a5 100644 --- a/superset/commands/database/importers/v1/utils.py +++ b/superset/commands/database/importers/v1/utils.py @@ -20,18 +20,18 @@ from flask import current_app as app -from superset import db, security_manager +from superset import db, is_feature_enabled, security_manager from superset.commands.database.exceptions import DatabaseInvalidError from superset.commands.database.utils import ( add_permissions, import_connection_identity_changed, ssh_tunnel_rebind_unsafe, ) -from superset.commands.exceptions import ImportFailedError -from superset.constants import PASSWORD_MASK +from superset.commands.exceptions import ImportFailedError, ImportRefusedError +from superset.constants import EMBEDDED_CREDENTIALS_KEY, PASSWORD_MASK from superset.databases.ssh_tunnel.models import SSHTunnel -from superset.databases.utils import make_url_safe -from superset.db_engine_specs import get_engine_spec +from superset.databases.utils import embedded_credentials_error, make_url_safe +from superset.db_engine_specs import BaseEngineSpec, get_engine_spec from superset.db_engine_specs.exceptions import SupersetDBAPIError from superset.exceptions import ( OAuth2RedirectError, @@ -79,6 +79,31 @@ def _refuse_stored_secret_reuse(existing: Database, config: dict[str, Any]) -> N ) +def _refuse_unreadable_embedded_key( + config: dict[str, Any], engine_spec: type[BaseEngineSpec] +) -> None: + """ + Refuse an imported embedded credential the engine can't use, as the API does. + + ``config["encrypted_extra"]`` has its masked secrets already revealed, so a + key pair is decoded with the passphrase it will actually be stored with. + """ + if not ( + is_feature_enabled("EMBEDDED_CREDENTIAL_FALLBACK") + and is_feature_enabled("EMBEDDED_SUPERSET") + ): + return + payload = json.loads(config["encrypted_extra"]) + if not isinstance(payload, dict): + return + credentials = payload.get(EMBEDDED_CREDENTIALS_KEY) + if message := embedded_credentials_error(engine_spec, credentials, None): + raise ImportRefusedError( + f"Embedded guest credentials of database '{config['database_name']}' " + f"can't be used. {message}" + ) + + def _sync_permissions_best_effort(database: Database) -> None: """ Sync catalog/schema permissions for ``database``, tolerating a transient @@ -191,6 +216,7 @@ def import_database( # noqa: C901 "Must provide value for masked_encrypted_extra field: " + ", ".join(still_masked) ) + _refuse_unreadable_embedded_key(config, engine_spec) ssh_tunnel_config = config.pop("ssh_tunnel", None) diff --git a/superset/commands/exceptions.py b/superset/commands/exceptions.py index 2e962acf635e..e14d50ddda50 100644 --- a/superset/commands/exceptions.py +++ b/superset/commands/exceptions.py @@ -105,6 +105,12 @@ class ImportFailedError(CommandException): message = "Import failed for an unknown reason" +class ImportRefusedError(ImportFailedError): + """An import refused because of what the bundle contains, not a server fault.""" + + status = 400 + + class DatasourceTypeInvalidError(ValidationError): status = 422 diff --git a/superset/models/core.py b/superset/models/core.py index 7cd8dc38a61b..7ee29b8aadd1 100755 --- a/superset/models/core.py +++ b/superset/models/core.py @@ -854,7 +854,9 @@ def _get_sqla_engine( # pylint: disable=too-many-locals # noqa: C901 # decoded here, before any engine (and its `handle_error` # listener) exists. Log it and refuse it as a sign-in the database # rejected would be, rather than letting the decoding error - # surface as a server fault (sc-130269). + # surface as a server fault (sc-130269). The decoding error is + # dropped from the chain: the line above already names its class, + # and the error handler would log its traceback with the refusal. _log_embedded_rejection(self.id, self.database_name, ex) raise SupersetErrorException( SupersetError( @@ -867,7 +869,7 @@ def _get_sqla_engine( # pylint: disable=too-many-locals # noqa: C901 level=ErrorLevel.ERROR, ), status=400, - ) from ex + ) from None # Hooks must receive the selected connection identity, not the # username claim supplied by the guest token. effective_username = sqlalchemy_url.username diff --git a/tests/unit_tests/databases/commands/importers/v1/import_test.py b/tests/unit_tests/databases/commands/importers/v1/import_test.py index d9b1ad79ab44..c403693ccf86 100644 --- a/tests/unit_tests/databases/commands/importers/v1/import_test.py +++ b/tests/unit_tests/databases/commands/importers/v1/import_test.py @@ -17,6 +17,8 @@ # pylint: disable=unused-argument, import-outside-toplevel, invalid-name import copy +from typing import TYPE_CHECKING +from uuid import UUID import pytest from flask import current_app @@ -27,6 +29,10 @@ from superset.commands.exceptions import ImportFailedError from superset.constants import PASSWORD_MASK from superset.utils import json +from tests.unit_tests.conftest import with_feature_flags + +if TYPE_CHECKING: + from superset.models.core import Database def test_import_database(mocker: MockerFixture, session: Session) -> None: @@ -1012,3 +1018,113 @@ def test_import_database_never_stores_a_mask_over_an_existing_db( import_database(masked, overwrite=True) assert existing.encrypted_extra is None + + +def _pem(passphrase: str) -> str: + """A freshly generated RSA private key, PEM-encoded under ``passphrase``.""" + from cryptography.hazmat.primitives import serialization + from cryptography.hazmat.primitives.asymmetric import rsa + + return ( + rsa.generate_private_key(public_exponent=65537, key_size=2048) + .private_bytes( + serialization.Encoding.PEM, + serialization.PrivateFormat.PKCS8, + serialization.BestAvailableEncryption(passphrase.encode()), + ) + .decode() + ) + + +def _snowflake_config(embedded_credentials: dict[str, str]) -> dict[str, object]: + """A per-user OAuth2 Snowflake connection carrying an embedded credential.""" + return { + "database_name": "imported_snowflake", + "sqlalchemy_uri": "snowflake://base_user@account/db?warehouse=WH", + "uuid": "6c1c5ee7-1d83-4f57-9a8e-1a2b3c4d5e6f", + "version": "1.0.0", + "extra": {}, + "impersonate_user": True, + "masked_encrypted_extra": json.dumps( + { + "oauth2_client_info": {"id": "client", "secret": "client-secret"}, + "embedded_credentials": embedded_credentials, + } + ), + } + + +def _store_snowflake_keypair( + mocker: MockerFixture, keypair: dict[str, str] +) -> "Database": + from superset import security_manager + from superset.models.core import Database + + mocker.patch.object(security_manager, "can_access", return_value=True) + mocker.patch("superset.commands.database.importers.v1.utils.add_permissions") + Database.metadata.create_all(db.session.get_bind()) # pylint: disable=no-member + + config = _snowflake_config(keypair) + existing = Database( + database_name=config["database_name"], + sqlalchemy_uri=config["sqlalchemy_uri"], + uuid=UUID(str(config["uuid"])), + impersonate_user=True, + encrypted_extra=config["masked_encrypted_extra"], + ) + db.session.add(existing) + db.session.flush() + return existing + + +@with_feature_flags(EMBEDDED_SUPERSET=True, EMBEDDED_CREDENTIAL_FALLBACK=True) +def test_import_database_refuses_embedded_key_with_wrong_passphrase( + mocker: MockerFixture, session: Session +) -> None: + """ + A key pair whose passphrase doesn't open its key is refused with a 400, as + the API refuses it, instead of breaking every embedded chart later; the + stored key stays (sc-130269). + """ + from superset.commands.database.importers.v1.utils import import_database + from superset.commands.exceptions import ImportRefusedError + + keypair = { + "username": "EMBED_SVC", + "auth_method": "keypair", + "privatekey_body": _pem("passphrase"), + "privatekey_pass": "passphrase", + } + existing = _store_snowflake_keypair(mocker, keypair) + + wrong = {**keypair, "privatekey_body": PASSWORD_MASK, "privatekey_pass": "wrong"} + with pytest.raises(ImportRefusedError, match="private key could not be read"): + import_database(_snowflake_config(wrong), overwrite=True) + + stored = json.loads(existing.encrypted_extra)["embedded_credentials"] + assert stored == keypair + + +@with_feature_flags(EMBEDDED_SUPERSET=True, EMBEDDED_CREDENTIAL_FALLBACK=True) +def test_import_database_keeps_readable_embedded_key( + mocker: MockerFixture, session: Session +) -> None: + """A key pair that decodes, its secrets masked, re-imports as before.""" + from superset.commands.database.importers.v1.utils import import_database + + keypair = { + "username": "EMBED_SVC", + "auth_method": "keypair", + "privatekey_body": _pem("passphrase"), + "privatekey_pass": "passphrase", + } + _store_snowflake_keypair(mocker, keypair) + + masked = { + **keypair, + "privatekey_body": PASSWORD_MASK, + "privatekey_pass": PASSWORD_MASK, + } + database = import_database(_snowflake_config(masked), overwrite=True) + + assert json.loads(database.encrypted_extra)["embedded_credentials"] == keypair