Skip to content

Commit

Permalink
Fix insecure request warning (#146)
Browse files Browse the repository at this point in the history
### Summary

Fixed the insecure request warning by ensuring ssl verification is
enabled in all of our api calls. In addition, I've added a new config
option called verify_ssl so that this behaviour is configurable by the
user.

### Description

The new config option is set to true by default. I've added this option
to the Parameters class, which we already pass to each api call, instead
of passing ssl_verify as a separate option.

### Test Results

Ran all functional tests against Dremio Cloud:

- Basic tests were flaky at first with one random test failing each
time. But eventually all tests passed. The flaky tests may be related to
#91
- All other func tests passed without an issue

### Changelog

-   [x] Added a summary of what this PR accomplishes to CHANGELOG.md

### Related Issue

#142
  • Loading branch information
ravjotbrar authored Mar 14, 2023
1 parent 0988fce commit bd6ded4
Show file tree
Hide file tree
Showing 10 changed files with 115 additions and 46 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
## Features

## Fixes
- [#142](https://github.com/dremio/dbt-dremio/issues/142) Ensure ssl verification is enabled in all api calls. Also added an option called `verify_ssl` so it can be disabled in necessary circumstances.

## Under the Hood

Expand Down
11 changes: 8 additions & 3 deletions dbt/adapters/dremio/api/authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,17 @@
@dataclass
class DremioAuthentication:
username: Optional[str] = None
verify_ssl: bool = True

@classmethod
def build(cls, username: None, password: None, pat: None):
def build(cls, username: None, password: None, pat: None, verify_ssl: True):
if password is not None:
return DremioPasswordAuthentication(username, password, token=None)
return DremioPatAuthentication(username, pat)
return DremioPasswordAuthentication(
username=username, verify_ssl=verify_ssl, password=password, token=None
)
return DremioPatAuthentication(
username=username, verify_ssl=verify_ssl, pat=pat
)

@classmethod
def build_headers(cls, authorization_field):
Expand Down
11 changes: 3 additions & 8 deletions dbt/adapters/dremio/api/cursor.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,7 @@ def execute(self, sql, bindings=None):
if bindings is None:
self._initialize()

json_payload = sql_endpoint(
self._parameters, sql, context=None, ssl_verify=True
)
json_payload = sql_endpoint(self._parameters, sql, context=None)

self._job_id = json_payload["id"]

Expand Down Expand Up @@ -120,7 +118,7 @@ def _populate_rowcount(self):
job_id = self._job_id

last_job_state = ""
job_status_response = job_status(self._parameters, job_id, ssl_verify=True)
job_status_response = job_status(self._parameters, job_id)
job_status_state = job_status_response["jobState"]

while True:
Expand All @@ -134,7 +132,7 @@ def _populate_rowcount(self):
if job_status_state == "COMPLETED" or job_status_state == "CANCELLED":
break
last_job_state = job_status_state
job_status_response = job_status(self._parameters, job_id, ssl_verify=True)
job_status_response = job_status(self._parameters, job_id)
job_status_state = job_status_response["jobState"]

# this is done as job status does not return a rowCount if there are no rows affected (even in completed job_state)
Expand All @@ -148,15 +146,13 @@ def _populate_rowcount(self):

self._rowcount = rows


def _populate_job_results(self, row_limit=100):
if self._job_results == None:
combined_job_results = job_results(
self._parameters,
self._job_id,
offset=0,
limit=row_limit,
ssl_verify=True,
)
total_row_count = combined_job_results["rowCount"]
current_row_count = len(combined_job_results["rows"])
Expand All @@ -168,7 +164,6 @@ def _populate_job_results(self, row_limit=100):
self._job_id,
offset=current_row_count,
limit=row_limit,
ssl_verify=True,
)["rows"]
)
current_row_count += row_limit
Expand Down
25 changes: 15 additions & 10 deletions dbt/adapters/dremio/api/parameters.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,21 +47,25 @@ def build(cls, credentials: DremioCredentials):
and credentials.cloud_project_id is not None
):
return CloudParametersBuilder(
cls._build_dremio_authentication(credentials=credentials),
credentials.cloud_host,
credentials.cloud_project_id,
authentication=cls._build_dremio_authentication(
credentials=credentials
),
cloud_host=credentials.cloud_host,
cloud_project_id=credentials.cloud_project_id,
)
elif (
if (
credentials.software_host is not None
and credentials.cloud_host is None
and credentials.port is not None
and credentials.use_ssl is not None
):
return SoftwareParametersBuilder(
cls._build_dremio_authentication(credentials=credentials),
credentials.software_host,
credentials.port,
credentials.use_ssl,
authentication=cls._build_dremio_authentication(
credentials=credentials
),
software_host=credentials.software_host,
port=credentials.port,
use_ssl=credentials.use_ssl,
)
raise ValueError("Credentials match neither Cloud nor Software")

Expand All @@ -76,7 +80,7 @@ def get_parameters(self) -> Parameters:
@classmethod
def _build_dremio_authentication(self, credentials: DremioCredentials):
return DremioAuthentication.build(
credentials.UID, credentials.PWD, credentials.pat
credentials.UID, credentials.PWD, credentials.pat, credentials.verify_ssl
)


Expand Down Expand Up @@ -113,5 +117,6 @@ def build_base_url(self):

def get_parameters(self) -> Parameters:
return SoftwareParameters(
base_url=self.build_base_url(), authentication=self.authentication
base_url=self.build_base_url(),
authentication=self.authentication,
)
44 changes: 24 additions & 20 deletions dbt/adapters/dremio/api/rest/endpoints.py
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ def _check_error(response, details=""):
raise DremioException("Unknown error", error)


def login(api_parameters: Parameters, timeout=10, verify=True):
def login(api_parameters: Parameters, timeout=10):

if isinstance(api_parameters.authentication, DremioPatAuthentication):
return api_parameters
Expand All @@ -160,42 +160,44 @@ def login(api_parameters: Parameters, timeout=10, verify=True):
"password": api_parameters.authentication.password,
},
timeout=timeout,
ssl_verify=verify,
ssl_verify=api_parameters.authentication.verify_ssl,
)

api_parameters.authentication.token = response["token"]

return api_parameters


def sql_endpoint(api_parameters: Parameters, query, context=None, ssl_verify=True):
def sql_endpoint(api_parameters: Parameters, query, context=None):
url = UrlBuilder.sql_url(api_parameters)
return _post(
url,
api_parameters.authentication.get_headers(),
ssl_verify=ssl_verify,
ssl_verify=api_parameters.authentication.verify_ssl,
json={"sql": query, "context": context},
)


def job_status(api_parameters: Parameters, job_id, ssl_verify=True):
def job_status(api_parameters: Parameters, job_id):
url = UrlBuilder.job_status_url(api_parameters, job_id)
return _get(url, api_parameters.authentication.get_headers(), ssl_verify=ssl_verify)
return _get(
url,
api_parameters.authentication.get_headers(),
ssl_verify=api_parameters.authentication.verify_ssl,
)


def job_cancel_api(api_parameters: Parameters, job_id, ssl_verify=True):
def job_cancel_api(api_parameters: Parameters, job_id):
url = UrlBuilder.job_cancel_url(api_parameters, job_id)
return _post(
url,
api_parameters.authentication.get_headers(),
json=None,
ssl_verify=ssl_verify,
ssl_verify=api_parameters.authentication.verify_ssl,
)


def job_results(
api_parameters: Parameters, job_id, offset=0, limit=100, ssl_verify=True
):
def job_results(api_parameters: Parameters, job_id, offset=0, limit=100):
url = UrlBuilder.job_results_url(
api_parameters,
job_id,
Expand All @@ -205,23 +207,21 @@ def job_results(
return _get(
url,
api_parameters.authentication.get_headers(),
ssl_verify=ssl_verify,
ssl_verify=api_parameters.authentication.verify_ssl,
)


def create_catalog_api(api_parameters, json, ssl_verify=True):
def create_catalog_api(api_parameters, json):
url = UrlBuilder.catalog_url(api_parameters)
return _post(
url,
api_parameters.authentication.get_headers(),
json=json,
ssl_verify=ssl_verify,
ssl_verify=api_parameters.authentication.verify_ssl,
)


def get_catalog_item(
api_parameters, catalog_id=None, catalog_path=None, ssl_verify=True
):
def get_catalog_item(api_parameters, catalog_id=None, catalog_path=None):
if catalog_id is None and catalog_path is None:
raise TypeError("both id and path can't be None for a catalog_item call")

Expand All @@ -236,13 +236,17 @@ def get_catalog_item(
api_parameters,
catalog_id,
)
return _get(url, api_parameters.authentication.get_headers(), ssl_verify=ssl_verify)
return _get(
url,
api_parameters.authentication.get_headers(),
ssl_verify=api_parameters.authentication.verify_ssl,
)


def delete_catalog(api_parameters, cid, ssl_verify=True):
def delete_catalog(api_parameters, cid):
url = UrlBuilder.delete_catalog_url(api_parameters, cid)
return _delete(
url,
api_parameters.authentication.get_headers(),
ssl_verify=ssl_verify,
ssl_verify=api_parameters.authentication.verify_ssl,
)
7 changes: 3 additions & 4 deletions dbt/adapters/dremio/connections.py
Original file line number Diff line number Diff line change
Expand Up @@ -199,13 +199,12 @@ def drop_catalog(self, database, schema):
api_parameters,
catalog_id=None,
catalog_path=path_list,
ssl_verify=False,
)
except DremioNotFoundException:
logger.debug("Catalog not found. Returning")
return

delete_catalog(api_parameters, catalog_info["id"], ssl_verify=False)
delete_catalog(api_parameters, catalog_info["id"])

def create_catalog(self, database, schema):
thread_connection = self.get_thread_connection()
Expand All @@ -232,7 +231,7 @@ def _make_new_folder_json(self, path) -> json:
def _create_space(self, database, api_parameters):
space_json = self._make_new_space_json(database)
try:
create_catalog_api(api_parameters, space_json, False)
create_catalog_api(api_parameters, space_json)
except DremioAlreadyExistsException:
logger.debug(f"Database {database} already exists. Creating folders only.")

Expand All @@ -242,7 +241,7 @@ def _create_folders(self, database, schema, api_parameters):
temp_path_list.append(folder)
folder_json = self._make_new_folder_json(temp_path_list)
try:
create_catalog_api(api_parameters, folder_json, False)
create_catalog_api(api_parameters, folder_json)
except DremioAlreadyExistsException:
logger.debug(f"Folder {folder} already exists.")

Expand Down
1 change: 1 addition & 0 deletions dbt/adapters/dremio/credentials.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ class DremioCredentials(Credentials):
software_host: Optional[str] = None
port: Optional[int] = 9047 # for rest endpoint
use_ssl: Optional[bool] = True
verify_ssl: Optional[bool] = True

_ALIASES = {
# Only terms on right-side will be used going forward.
Expand Down
1 change: 1 addition & 0 deletions tests/component/test_profile_template.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ class TestProfileTemplate:
"dremio_space": "@user",
"dremio_space_folder": "no_schema",
"threads": 1,
"verify_ssl": True,
}
_DREMIO_CLOUD_PROFILE_SPECIFIC_OPTIONS_WITH_DEFAULTS = {
"cloud_host": "api.dremio.cloud",
Expand Down
58 changes: 58 additions & 0 deletions tests/functional/adapter/dremio_specific/test_verify_ssl.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
# Copyright (C) 2022 Dremio Corporation

# Licensed 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.

import pytest
import warnings
from urllib3.connectionpool import InsecureRequestWarning
import yaml
from dbt.tests.util import (
run_dbt,
read_file,
write_file,
)
from tests.fixtures.profiles import unique_schema, dbt_profile_data

verify_ssl_model = """
select * from Samples."samples.dremio.com"."NYC-taxi-trips-iceberg" limit 10
"""


class TestVerifyCertificateDremio:
@pytest.fixture(scope="class")
def models(self):
return {
"test_verify_ssl.sql": verify_ssl_model,
}

def update_config_file(self, updates, *paths):
current_yaml = read_file(*paths)
config = yaml.safe_load(current_yaml)
config["test"]["outputs"]["default"].update(updates)
new_yaml = yaml.safe_dump(config)
write_file(new_yaml, *paths)

@pytest.mark.filterwarnings("error:InsecureRequestWarning")
def test_insecure_request_warning_not_exist(self, project):
run_dbt(["run"])

@pytest.mark.filterwarnings("ignore:InsecureRequestWarning")
def test_insecure_request_warning_exists(self, project):
with pytest.warns(InsecureRequestWarning):
self.update_config_file(
{"verify_ssl": False},
project.profiles_dir,
"profiles.yml",
)

run_dbt(["run"])
2 changes: 1 addition & 1 deletion tests/functional/adapter/grants/test_invalid_grants.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,4 @@ def grantee_does_not_exist_error(self):
return "StatusRuntimeException: INTERNAL"

def privilege_does_not_exist_error(self):
return "Failure parsing the query."
return 'Encountered "fake_privilege"'

0 comments on commit bd6ded4

Please sign in to comment.