Add optional ssl config flag #1802
Conversation
szaffarano
force-pushed
the
szaffarano/ssl-ca
branch
from
81fddaa to
746ce80
Compare
WalkthroughThe changes add SSL context handling to Slack clients and introduce a system CA file configuration option. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 | |
| ) |
| use_system_ca_files: bool = True, | ||
| ): |
There was a problem hiding this comment.
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.
| 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.
| 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.
1 participant
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
falseto keep it backward compatible.Summary by CodeRabbit
--use-system-ca-filesCLI flag (default enabled) to allow users to choose between system or bundled certificate authorities for secure connections.