Add optional ssl config flag #1802
Conversation
szaffarano
force-pushed
the
szaffarano/ssl-ca
branch
from
81fddaa to
746ce80
Compare
📝 WalkthroughWalkthroughAdds SSL context handling to Slack clients and a Config flag controlling system CA usage. Threads an optional Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (monitor/send_report)
participant Config as Config
participant App as App/Service
participant SlackClient as SlackClient
participant SlackSDK as Slack SDK
CLI->>Config: construct Config(use_system_ca_files)
Config->>App: provide config
App->>SlackClient: initialize(tracking, ssl_context=None)
Note right of SlackClient: derive ssl_context based on Config.use_system_ca_files\n(may call certifi.where() to build SSLContext)
alt use_system_ca_files == True
SlackClient->>System: use system CA files (ssl_context=None)
else
SlackClient->>Certifi: certifi.where()
SlackClient->>SSL: create SSLContext(using certifi bundle)
end
SlackClient->>SlackSDK: instantiate WebClient/WebhookClient(ssl=ssl_context, token/webhook)
SlackSDK-->>SlackClient: client ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
elementary/clients/slack/client.py(5 hunks)elementary/config/config.py(2 hunks)elementary/monitor/cli.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
elementary/clients/slack/client.py (1)
elementary/monitor/data_monitoring/alerts/integrations/slack/slack.py (1)
_initial_client(87-93)
| 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 | ||
| ) |
There was a problem hiding this comment.
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.
| 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 | |
| ) |
There was a problem hiding this comment.
Hmm this issue seems to suggest that slack doesn't depend on Certifi by default:
slackapi/python-slack-sdk#1491
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
python-slack-sdk issue 1491 certifi SSL certificates
💡 Result:
python-slack-sdk itself doesn’t bundle CA roots; SSL verification ultimately depends on your Python/OpenSSL trust store (and sometimes certifi). When you see errors like CERTIFICATE_VERIFY_FAILED / “unable to get local issuer certificate”, the common fixes are:
- Provide an explicit CA bundle to the Slack client (sync WebClient supports a custom
sslcontext)
The SDK documents passing a custom SSL context via thessl=option. (github.com)
Example (syncWebClient):
import ssl, certifi
from slack_sdk import WebClient
ssl_context = ssl.create_default_context(cafile=certifi.where())
client = WebClient(token="xoxb-...", ssl=ssl_context)(certifi.where() is the standard way to locate certifi’s CA bundle.) (stackoverflow.com)
-
If this started “suddenly” in CI/Docker: install OS CA certs in the image
In slim containers, addingca-certificatesand updating them often resolves “local issuer” failures. (community.shopify.com) -
Adjust
certifiversion (upgrade or downgrade) depending on the breakage
Updatingcertififrequently fixes verification failures. (askssl.com)
But there have also been incidents where a newcertifirelease triggered widespread failures behind certain CDNs, with a workaround of pinning/downgradingcertifito an earlier version. (community.shopify.com)
If you paste the exact traceback + your environment (OS, Python version, slack-sdk version, whether Docker/proxy is involved), I can map you to the most likely of the three paths above.
Citations:
- 1: https://github.com/slackapi/python-slack-sdk?utm_source=openai
- 2: https://stackoverflow.com/questions/52030010/getting-certificate-verify-failed-when-trying-to-use-slackclient-and-python-2-7?utm_source=openai
- 3: https://community.shopify.com/c/shopify-discussions/ssl-certificate-verification-failed-when-calling-webhooks-api/m-p/3037960/highlight/true?utm_source=openai
- 4: https://www.askssl.com/python-ssl-certificate_verify_failed/?utm_source=openai
- 5: https://community.shopify.com/c/shopify-discussions/ssl-certificate-verification-failed-when-calling-webhooks-api/m-p/3037960/highlight/true?utm_source=openai
🌐 Web query:
site:github.com/slackapi/python-slack-sdk issues 1491
💡 Result:
I couldn’t find any publicly accessible page for slackapi/python-slack-sdk Issue #1491 via search (it doesn’t show up in the repo’s issues listing/search results). (github.com)
Likely explanations:
- the issue was deleted, transferred to another repo, or made private/internal (so it won’t be indexable/searchable publicly).
How to double-check on GitHub:
- Use the repo issue search:
https://github.com/slackapi/python-slack-sdk/issues?q=1491(github.com) - Or try opening directly while logged into GitHub:
https://github.com/slackapi/python-slack-sdk/issues/1491(if it exists but is access-restricted, GitHub will tell you).
Citations:
- 1: https://github.com/slackapi/python-slack-sdk/issues?utm_source=openai
- 2: https://github.com/slackapi/python-slack-sdk/issues?utm_source=openai
🏁 Script executed:
#!/bin/bash
# Fetch the GitHub issue directly to see what it says
curl -s "https://api.github.com/repos/slackapi/python-slack-sdk/issues/1491"
Length of output: 5893
🏁 Script executed:
#!/bin/bash
# Fetch the comments on the GitHub issue to understand the resolution
curl -s "https://api.github.com/repos/slackapi/python-slack-sdk/issues/1491/comments"
Length of output: 279
🌐 Web query:
python slack_sdk WebClient ssl parameter None default behavior
💡 Result:
In slack_sdk.WebClient, the constructor’s ssl parameter defaults to None (ssl: ssl.SSLContext | None = None) and is stored as self.ssl. When sending requests, the SDK passes it straight through to Python’s urllib layer:
- No proxy:
urllib.request.urlopen(req, context=self.ssl, ...)(socontext=Nonewhen you didn’t supplyssl=) (docs.slack.dev) - With proxy: it builds an
HTTPSHandler(context=self.ssl)(docs.slack.dev)
Therefore, ssl=None means “don’t use a custom SSLContext; use urllib’s default HTTPS/TLS behavior” (i.e., normal certificate verification using the platform/default trust store). urllib.request.urlopen(..., context=None) only uses a custom context if one is provided. (docs.python.org)
Citations:
- 1: https://docs.slack.dev/tools/python-slack-sdk/reference/web/base_client.html
- 2: https://docs.slack.dev/tools/python-slack-sdk/reference/web/base_client.html
- 3: https://docs.python.org/3.12/library/urllib.request.html?utm_source=openai
@haritamar You're absolutely right, and thank you for catching this!
I've reviewed the issue you linked and confirmed through the Slack SDK documentation that when ssl=None is passed to WebClient or WebhookClient, it uses urllib's default behavior, which relies on the platform's system trust store (not certifi).
This means the current implementation in your PR is correct:
- ✅
use_system_ca_files=True→ssl_context=None→ Slack SDK uses system CA certificates - ✅
use_system_ca_files=False→ssl_context=ssl.create_default_context(cafile=certifi.where())→ Slack SDK uses certifi bundle
My previous suggestion to pass ssl.create_default_context() when use_system_ca_files=True was incorrect. The code as written properly achieves the goal stated in issue #1801.
I apologize for the confusion in my initial review.
🧬 Code Graph Analysis Results
elementary/clients/slack/client.py
- Summary: SlackClient.create_client(config, tracking) selects between SlackWebClient (token-based) and SlackWebhookClient (webhook) depending on config.has_slack and whether slack_token or slack_webhook is provided. It configures an SSL context based on config.use_system_ca_files:
- If using system CA files (use_system_ca_files is True), ssl_context is None.
- If not using system CA files (use_system_ca_files is False), ssl_context is ssl.create_default_context(cafile=certifi.where()).
- This context is then passed to the concrete Slack client (SlackWebClient or SlackWebhookClient).
elementary/clients/slack/client.py
- Summary: SlackWebClient._initial_client(ssl_context) constructs the Slack WebClient with the provided token and the given ssl_context (passed as ssl).
elementary/clients/slack/client.py
- Summary: SlackWebhookClient._initial_client(ssl_context) behavior depends on is_workflow:
- If is_workflow is True, returns a plain requests.Session() (which uses the default CA bundle, certifi).
- Otherwise, returns a Slack WebhookClient configured with url=self.webhook and ssl=ssl_context.
Notes:
- The use of certifi is conditional based on the config.use_system_ca_files flag in create_client and is passed through to the underlying Slack client initialization when appropriate.
- This aligns with the referenced GitHub issue discussion by showing that default CA handling depends on the system CA decision (certifi vs system CA files) rather than Slack SDK defaulting to certifi in all cases.
✏️ Learnings added
Learnt from: haritamar
Repo: elementary-data/elementary PR: 1802
File: elementary/clients/slack/client.py:48-71
Timestamp: 2026-02-09T22:58:17.878Z
Learning: In the Slack SDK (slack_sdk), when the `ssl` parameter is set to `None` for `WebClient` or `WebhookClient`, the underlying `urllib` uses the platform's system default trust store for certificate verification, not certifi. To force the use of certifi's CA bundle, an explicit SSL context must be created with `ssl.create_default_context(cafile=certifi.where())`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| 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) |
There was a problem hiding this comment.
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.
| 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.
|
This pull request is stale because it has been open for too long with no activity. |
haritamar
force-pushed
the
szaffarano/ssl-ca
branch
from
5e7da25 to
572e790
Compare
haritamar
temporarily deployed
to
elementary_test_env
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@elementary/clients/slack/client.py`:
- Around line 263-271: The _initial_client method currently returns a bare
requests.Session() when self.is_workflow is True, which ignores the ssl_context;
update _initial_client so that when is_workflow is True you either configure the
returned requests.Session to honor ssl_context (e.g., set session.verify to the
provided SSLContext or CA bundle path, or attach a TransportAdapter that uses
it) or emit a clear warning via the existing logger explaining that
--use-system-ca-files/ssl_context will be used/ignored for workflow webhooks;
reference _initial_client, self.is_workflow, ssl_context, requests.Session, and
WebhookClient to locate and change the behavior.
- Update _initial_client in SlackWebhookClient to configure requests.Session with SSL verification - When ssl_context is provided, set session.verify to certifi CA bundle - Add warning when workflow webhooks cannot fully honor --use-system-ca-files setting - Ensures SSL context is respected for workflow webhook requests
haritamar
temporarily deployed
to
elementary_test_env
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@elementary/clients/slack/client.py`:
- Around line 264-276: The warning and comment around requests.Session SSL
handling are incorrect and the use_system_ca_files flag isn't honored; update
the workflow webhook branch in the method that references is_workflow and
ssl_context so that when ssl_context is None (meaning use_system_ca_files=True)
you explicitly set session.verify to the OS trust store path (or load OS CAs via
a library like truststore) and only log a warning if you fail and must fall back
to certifi.where(); when ssl_context is not None set session.verify =
certifi.where() and change the warning text to reflect that certifi is being
used (not that it's overriding a system-ca preference). Ensure the comment is
corrected to state requests.Session defaults to certifi.where() and document
which branch uses the OS CA file vs certifi.
2 participants
To address #1801 I'm proposing to add a new CLI boolean flag flag
--use-system-ca-files/--no-use-system-ca-filesto let the user decide when to use the system root CAs or the certifi ones.The default value is
trueto keep it backward compatible.Summary by CodeRabbit