Skip to content

Add optional ssl config flag #1802

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.

Sign up for GitHub

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

Open
szaffarano wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add optional ssl config flag #1802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
746ce80
Add optional ssl config flag
szaffarano Feb 5, 2025
5e7da25
Merge branch 'master' into szaffarano/ssl-ca
szaffarano Feb 10, 2025
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
51 changes: 39 additions & 12 deletions elementary/clients/slack/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import json
import ssl
from abc import ABC, abstractmethod
from typing import Dict, List, Optional, Tuple

import certifi
from ratelimit import limits, sleep_and_retry
from slack_sdk import WebClient, WebhookClient
from slack_sdk.errors import SlackApiError
Expand All @@ -24,8 +26,9 @@ class SlackClient(ABC):
def __init__(
self,
tracking: Optional[Tracking] = None,
ssl_context: Optional[ssl.SSLContext] = None,
):
self.client = self._initial_client()
self.client = self._initial_client(ssl_context)
self.tracking = tracking
self._initial_retry_handlers()
self.email_to_user_id_cache: Dict[str, str] = {}
Expand All @@ -37,15 +40,35 @@ def create_client(
if not config.has_slack:
return None
if config.slack_token:
logger.debug("Creating Slack client with token.")
return SlackWebClient(token=config.slack_token, tracking=tracking)
logger.debug(
"Creating Slack client with token (system CA? = %s).",
config.use_system_ca_files,
)
ssl_context = (
None
if config.use_system_ca_files
else ssl.create_default_context(cafile=certifi.where())
)
return SlackWebClient(
token=config.slack_token, tracking=tracking, ssl_context=ssl_context
)
elif config.slack_webhook:
logger.debug("Creating Slack client with webhook.")
return SlackWebhookClient(webhook=config.slack_webhook, tracking=tracking)
logger.debug(
"Creating Slack client with webhook (system CA? = %s).",
config.use_system_ca_files,
)
ssl_context = (
ssl.create_default_context(cafile=certifi.where())
if not config.use_system_ca_files
else None
)
return SlackWebhookClient(
webhook=config.slack_webhook, tracking=tracking, ssl_context=ssl_context
)
Comment on lines +47 to +67
Copy link

@coderabbitai coderabbitai bot Nov 11, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Create an explicit system CA SSLContext

When use_system_ca_files is True, ssl_context is set to None, which leaves the Slack SDK on its certifi-backed default. The flag never enables system trust roots. Instead, build a context with ssl.create_default_context() for the system store, and keep the certifi override only when the flag is False.

-            ssl_context = (
-                None
-                if config.use_system_ca_files
-                else ssl.create_default_context(cafile=certifi.where())
-            )
+            ssl_context = (
+                ssl.create_default_context()
+                if config.use_system_ca_files
+                else ssl.create_default_context(cafile=certifi.where())
+            )
@@
-            ssl_context = (
-                ssl.create_default_context(cafile=certifi.where())
-                if not config.use_system_ca_files
-                else None
-            )
+            ssl_context = (
+                ssl.create_default_context()
+                if config.use_system_ca_files
+                else ssl.create_default_context(cafile=certifi.where())
+            )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
ssl_context = (
None
if config.use_system_ca_files
else ssl.create_default_context(cafile=certifi.where())
)
return SlackWebClient(
token=config.slack_token, tracking=tracking, ssl_context=ssl_context
)
elif config.slack_webhook:
logger.debug("Creating Slack client with webhook.")
return SlackWebhookClient(webhook=config.slack_webhook, tracking=tracking)
logger.debug(
"Creating Slack client with webhook (system CA? = %s).",
config.use_system_ca_files,
)
ssl_context = (
ssl.create_default_context(cafile=certifi.where())
if not config.use_system_ca_files
else None
)
return SlackWebhookClient(
webhook=config.slack_webhook, tracking=tracking, ssl_context=ssl_context
)
ssl_context = (
ssl.create_default_context()
if config.use_system_ca_files
else ssl.create_default_context(cafile=certifi.where())
)
return SlackWebClient(
token=config.slack_token, tracking=tracking, ssl_context=ssl_context
)
elif config.slack_webhook:
logger.debug(
"Creating Slack client with webhook (system CA? = %s).",
config.use_system_ca_files,
)
ssl_context = (
ssl.create_default_context()
if config.use_system_ca_files
else ssl.create_default_context(cafile=certifi.where())
)
return SlackWebhookClient(
webhook=config.slack_webhook, tracking=tracking, ssl_context=ssl_context
)

return None

@abstractmethod
def _initial_client(self):
def _initial_client(self, ssl_context: Optional[ssl.SSLContext]):
raise NotImplementedError

def _initial_retry_handlers(self):
Expand Down Expand Up @@ -79,12 +102,13 @@ def __init__(
self,
token: str,
tracking: Optional[Tracking] = None,
ssl_context: Optional[ssl.SSLContext] = None,
):
self.token = token
super().__init__(tracking)
super().__init__(tracking, ssl_context)

def _initial_client(self):
return WebClient(token=self.token)
def _initial_client(self, ssl_context: Optional[ssl.SSLContext]):
return WebClient(token=self.token, ssl=ssl_context)

@sleep_and_retry
@limits(calls=1, period=ONE_SECOND)
Expand Down Expand Up @@ -224,13 +248,16 @@ def __init__(
self,
webhook: str,
tracking: Optional[Tracking] = None,
ssl_context: Optional[ssl.SSLContext] = None,
):
self.webhook = webhook
super().__init__(tracking)
super().__init__(tracking, ssl_context)

def _initial_client(self):
def _initial_client(self, ssl_context: Optional[ssl.SSLContext]):
return WebhookClient(
url=self.webhook, default_headers={"Content-type": "application/json"}
url=self.webhook,
default_headers={"Content-type": "application/json"},
ssl=ssl_context,
)

@sleep_and_retry
Expand Down
3 changes: 3 additions & 0 deletions elementary/config/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ def __init__(
env: str = DEFAULT_ENV,
run_dbt_deps_if_needed: Optional[bool] = None,
project_name: Optional[str] = None,
use_system_ca_files: bool = True,
):
Comment on lines +76 to 77
Copy link

@coderabbitai coderabbitai bot Nov 11, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Restore use_system_ca_files default to False

Line 76 flips the default to True, so every existing Config(...) call silently swaps from the certifi bundle to system trust stores. That breaks backward compatibility and contradicts the PR description (“default value is false”). Set the default back to False so legacy behavior remains unchanged.

-        use_system_ca_files: bool = True,
+        use_system_ca_files: bool = False,
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
use_system_ca_files: bool = True,
):
use_system_ca_files: bool = False,
):
🤖 Prompt for AI Agents 
In elementary/config/config.py around lines 76 to 77, the parameter default for
use_system_ca_files was changed to True which breaks backward compatibility;
change its default back to False so Config(...) continues to use the certifi
bundle by default and preserve legacy behavior. Update the function/class
signature to set use_system_ca_files: bool = False and run tests or type checks
to ensure no other call sites rely on the flipped default.

self.config_dir = config_dir
self.profiles_dir = profiles_dir
Expand Down Expand Up @@ -204,6 +205,8 @@ def __init__(
"disable_elementary_version_check", False
)

self.use_system_ca_files = use_system_ca_files

def _load_configuration(self) -> dict:
if not os.path.exists(self.config_dir):
os.makedirs(self.config_dir)
Expand Down
9 changes: 9 additions & 0 deletions elementary/monitor/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,11 @@ def decorator(func):
default=None,
help="The Slack token for your workspace.",
)(func)
func = click.option(
"--use-system-ca-files/--no-use-system-ca-files",
default=True,
help="Whether to use the system CA files for SSL connections or the ones provided by certify (see https://pypi.org/project/certifi).",
)(func)
Comment on lines +76 to +80
Copy link

@coderabbitai coderabbitai bot Nov 11, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Keep the CLI flag default aligned with legacy behavior

The new click option currently defaults to True, undoing the certifi-based behavior for anyone who doesn’t pass the flag. Align it with the intended default (False) so --use-system-ca-files opt-in matches the Config change.

-            func = click.option(
-                "--use-system-ca-files/--no-use-system-ca-files",
-                default=True,
+            func = click.option(
+                "--use-system-ca-files/--no-use-system-ca-files",
+                default=False,
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func = click.option(
"--use-system-ca-files/--no-use-system-ca-files",
default=True,
help="Whether to use the system CA files for SSL connections or the ones provided by certify (see https://pypi.org/project/certifi).",
)(func)
func = click.option(
"--use-system-ca-files/--no-use-system-ca-files",
default=False,
help="Whether to use the system CA files for SSL connections or the ones provided by certify (see https://pypi.org/project/certifi).",
)(func)
🤖 Prompt for AI Agents 
In elementary/monitor/cli.py around lines 76 to 80, the Click option for
--use-system-ca-files currently defaults to True which reverses the intended
certifi-based behavior; change the option's default from True to False so that
system CA files are only used when the flag is explicitly passed, keeping the
CLI default aligned with the Config change.

if cmd in (Command.REPORT, Command.SEND_REPORT):
func = click.option(
"--exclude-elementary-models",
Expand Down Expand Up @@ -304,6 +309,7 @@ def monitor(
report_url,
filters,
teams_webhook,
use_system_ca_files,
):
"""
Get alerts on failures in dbt jobs.
Expand Down Expand Up @@ -335,6 +341,7 @@ def monitor(
slack_group_alerts_by=group_by,
report_url=report_url,
teams_webhook=teams_webhook,
use_system_ca_files=use_system_ca_files,
)
anonymous_tracking = AnonymousCommandLineTracking(config)
anonymous_tracking.set_env("use_select", bool(select))
Expand Down Expand Up @@ -652,6 +659,7 @@ def send_report(
disable,
include,
target_path,
use_system_ca_files,
):
"""
Generate and send the report to an external platform.
Expand Down Expand Up @@ -693,6 +701,7 @@ def send_report(
report_url=report_url,
env=env,
project_name=project_name,
use_system_ca_files=use_system_ca_files,
)
anonymous_tracking = AnonymousCommandLineTracking(config)
anonymous_tracking.set_env("use_select", bool(select))
Expand Down