Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add snowflake keypair authentication #21322

Merged
merged 12 commits into from
Sep 9, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion requirements/testing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,10 @@ websocket-client==1.2.0
# via docker
wrapt==1.12.1
# via astroid

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add the removed newline to avoid introducing unnecessary changes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in 78f625a

snowflake-connector-python==2.7.9
# via snowflake
snowflake-sqlalchemy==1.2.4
# via snowflake
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we need these, as there are no integration tests added here that require having the snowflake drivers present.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the previous pr if snowflake drivers are not included, the GitHub actions will raise snowflake not found error even no integration tests added.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because the previous PR did imports from the snowflake-sqlalchemy package (this one no longer does)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I forgot that, thanks for reminding me. In this case, snowflake drivers should be removed from testing requirements.

# The following packages are considered to be unsafe in a requirements file:
# pip
# setuptools
35 changes: 35 additions & 0 deletions superset/db_engine_specs/snowflake.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
# under the License.
import json
import re
import logging
from datetime import datetime
from typing import Any, Dict, List, Optional, Pattern, Tuple, TYPE_CHECKING
from urllib import parse
Expand All @@ -26,6 +27,8 @@
from marshmallow import fields, Schema
from sqlalchemy.engine.url import URL
from typing_extensions import TypedDict
from cryptography.hazmat.backends import default_backend
from cryptography.hazmat.primitives import serialization

from superset.databases.utils import make_url_safe
from superset.db_engine_specs.postgres import PostgresBaseEngineSpec
Expand All @@ -46,6 +49,8 @@
"unexpected '(?P<syntax_error>.+?)'."
)

logger = logging.getLogger(__name__)


class SnowflakeParametersSchema(Schema):
username = fields.Str(required=True)
Expand Down Expand Up @@ -279,3 +284,33 @@ def parameters_json_schema(cls) -> Any:

spec.components.schema(cls.__name__, schema=cls.parameters_schema)
return spec.to_dict()["components"]["schemas"][cls.__name__]

@staticmethod
def update_params_from_encrypted_extra(
database: "Database",
params: Dict[str, Any],
) -> None:
if not database.encrypted_extra:
return
try:
encrypted_extra = json.loads(database.encrypted_extra)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we can move the except block to here, since only json.loads will raise JSONDecodeError. More readable and less nesting

auth_method = encrypted_extra.pop("auth_method", None)
auth_params = encrypted_extra.pop("auth_params", {})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any reason to .pop() here, let's just .get()

if not auth_method:
return
connect_args = params.setdefault("connect_args", {})
if auth_method == "keypair":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should add an else and support ALLOWED_EXTRA_AUTHENTICATIONS

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added in 89cf4d3

with open(auth_params['privatekey_path'], "rb") as key:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There appears to be some linting issues here (we should always use double quotes)

p_key = serialization.load_pem_private_key(
key.read(),
password=auth_params['privatekey_pass'].encode(),
backend=default_backend()
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also support passing the privatekey directly to avoid having to place the key inside the Superset instance/pod. This way it would be possible to either pass privatekey_path or privatekey.

pkb = p_key.private_bytes(
encoding=serialization.Encoding.DER,
format=serialization.PrivateFormat.PKCS8,
encryption_algorithm=serialization.NoEncryption())
connect_args["private_key"] = pkb
except json.JSONDecodeError as ex:
logger.error(ex, exc_info=True)
Copy link
Member

@dpgaspar dpgaspar Sep 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: use logger.exception(ex). Also wonder if there is value on this try/except block, since load_perm_private_key can raise and p_key.private_bytes can also raise. We could just let it fail

raise ex