-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
Changes from 4 commits
d0e3cad
b44a85d
924a1ae
5ffc4ee
f5afffb
9794073
268a9b8
89cf4d3
f07cd2d
bc65d2a
78f625a
e7e93b6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -136,7 +136,10 @@ websocket-client==1.2.0 | |
# via docker | ||
wrapt==1.12.1 | ||
# via astroid | ||
|
||
snowflake-connector-python==2.7.9 | ||
# via snowflake | ||
snowflake-sqlalchemy==1.2.4 | ||
# via snowflake | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is because the previous PR did imports from the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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 | ||
|
@@ -46,6 +49,8 @@ | |
"unexpected '(?P<syntax_error>.+?)'." | ||
) | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
class SnowflakeParametersSchema(Schema): | ||
username = fields.Str(required=True) | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: we can move the |
||
auth_method = encrypted_extra.pop("auth_method", None) | ||
auth_params = encrypted_extra.pop("auth_params", {}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see any reason to |
||
if not auth_method: | ||
return | ||
connect_args = params.setdefault("connect_args", {}) | ||
if auth_method == "keypair": | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should add an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added in 89cf4d3 |
||
with open(auth_params['privatekey_path'], "rb") as key: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's also support passing the |
||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
raise ex |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in 78f625a