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/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/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/databases/schemas.py b/superset/databases/schemas.py index 6b2361afc272..1dfe81a1d6b6 100644 --- a/superset/databases/schemas.py +++ b/superset/databases/schemas.py @@ -53,7 +53,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 @@ -488,6 +492,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 f237711c6d7d..05c556fbff8a 100644 --- a/superset/db_engine_specs/base.py +++ b/superset/db_engine_specs/base.py @@ -2651,6 +2651,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 9a4151a690a0..42580cc36ee4 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/superset/models/core.py b/superset/models/core.py index 072cdc751eb0..7ee29b8aadd1 100755 --- a/superset/models/core.py +++ b/superset/models/core.py @@ -840,14 +840,36 @@ 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). 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( + 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 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/superset/translations/messages.pot b/superset/translations/messages.pot index e2ab00667f45..bbcc50df3979 100644 --- a/superset/translations/messages.pot +++ b/superset/translations/messages.pot @@ -14305,6 +14305,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 "" @@ -14406,6 +14409,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 "" diff --git a/tests/unit_tests/commands/databases/validate_test.py b/tests/unit_tests/commands/databases/validate_test.py index 1d5e4fd896d8..6750f20734cf 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", + [ + 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) +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/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 diff --git a/tests/unit_tests/databases/schema_tests.py b/tests/unit_tests/databases/schema_tests.py index 99e0aa169c45..941c8461b01b 100644 --- a/tests/unit_tests/databases/schema_tests.py +++ b/tests/unit_tests/databases/schema_tests.py @@ -1069,7 +1069,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]) @@ -1170,6 +1170,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: diff --git a/tests/unit_tests/models/core_test.py b/tests/unit_tests/models/core_test.py index 69d00c468dc3..c14726f536cb 100644 --- a/tests/unit_tests/models/core_test.py +++ b/tests/unit_tests/models/core_test.py @@ -4017,6 +4017,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: