feat: make TLS certificate verification configurable#1893
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a new configuration setting, insecure_skip_tls_verify, and a corresponding environment variable, DEEPSEEK_INSECURE_SKIP_TLS_VERIFY, to allow users to bypass TLS certificate verification for outbound HTTPS requests. This change affects multiple modules, including the CLI update mechanism, skill management, MCP connections, and various network-dependent tools. The review feedback identifies several locations where the skip_verify flag is not properly propagated to internal function calls, specifically regarding mirror-based updates and skill registry synchronization. Furthermore, the reviewer suggests centralizing the environment variable parsing logic to eliminate redundancy and ensure consistent boolean evaluation across the codebase.
| fn fetch_latest_release(skip_verify: bool) -> Result<Release> { | ||
| if let Some(base_url) = release_base_url_from_env() { | ||
| let version = update_version_from_env().unwrap_or_else(|| env!("CARGO_PKG_VERSION").into()); | ||
| return Ok(release_from_mirror_base_url( |
There was a problem hiding this comment.
The skip_verify flag is not propagated to release_from_mirror_base_url. If a user has configured a mirror via environment variables and that mirror uses a self-signed certificate, the update check will fail even if TLS verification is disabled.
| let (network, _max_size, registry_url, _skip_verify) = installer_settings(app); | ||
| let registry = run_async(async move { install::fetch_registry(&network, ®istry_url).await }); |
There was a problem hiding this comment.
The skip_verify flag obtained from installer_settings is explicitly ignored here (as _skip_verify) and not passed to install::fetch_registry. This will cause skill listing to fail when using a registry with an untrusted certificate, despite the configuration setting.
| if let Ok(value) = std::env::var("DEEPSEEK_INSECURE_SKIP_TLS_VERIFY") { | ||
| let val = value.trim().to_ascii_lowercase(); | ||
| config.insecure_skip_tls_verify = | ||
| Some(matches!(val.as_str(), "1" | "true" | "yes" | "on")); | ||
| } |
There was a problem hiding this comment.
This boolean parsing logic is duplicated from crates/cli/src/lib.rs. It is recommended to use a centralized helper (like the one used in crates/config) to ensure consistent handling of truthy values (e.g., "1", "true", "yes", "on") across the entire application.
wavezhang
force-pushed
the
feat/insecure-skip-tls-verify-config
branch
2 times, most recently
from
120404f to
804815d
Compare
Add `insecure_skip_tls_verify` configuration option (default `false`) to allow users to disable TLS certificate verification for outbound HTTPS requests. This affects the API client, tools (fetch, search, web_run, finance, image_analyze), skill installer, MCP connections, sandbox, config UI polling, CLI self-updater, and webhook hooks. **Configuration** - TOML: `insecure_skip_tls_verify = true` - Environment variable: `DEEPSEEK_INSECURE_SKIP_TLS_VERIFY=true` **Key design decisions** - The flag is threaded through constructors and execution contexts rather than read from global config inside HTTP client builders, keeping the dependency graph clean (hooks crate does not depend on config). - `ConfigToml::resolve_insecure_skip_tls_verify()` centralizes env-var override resolution so callers don't duplicate parsing logic. - `parse_bool` in `crates/config` is now `pub` for shared boolean parsing. **Tests added** - Config TOML deserialization for `insecure_skip_tls_verify` - Env override application in TUI Config - HTTP client builder behavior with skip_verify true/false **Security note** This commit does not introduce or expose any credentials, tokens, or secrets. All changes are strictly structural (replacing hardcoded `danger_accept_invalid_certs(true)` with a configurable flag). No API keys, passwords, or personal access tokens are present in the diff.
wavezhang
force-pushed
the
feat/insecure-skip-tls-verify-config
branch
from
804815d to
f60c0fd
Compare
|
Thanks for the review. Here is the status of each comment:
All fixes have been squashed into the latest commit. |
1 participant
Add
insecure_skip_tls_verifyconfiguration option (defaultfalse) to allow users to disable TLS certificate verification.Configuration:
insecure_skip_tls_verify = trueDEEPSEEK_INSECURE_SKIP_TLS_VERIFY=trueChanges:
.danger_accept_invalid_certs(true)across the workspace with configurable flagsdeepseek-configand TUIConfigNote: TLS verification is enabled by default. Only disable when connecting to servers with self-signed or untrusted certificates.