fix(agent): honor HERMES_TLS_MAX_VERSION to cap provider TLS handshakes - #44392
fix(agent): honor HERMES_TLS_MAX_VERSION to cap provider TLS handshakes#44392AIalliAI wants to merge 1 commit into
Conversation
✅ Code Review — CleanReviewed the full diff (3 files: What I verified:
No issues found. LGTM. |
|
Reworked in 1cb8df1 per the AGENTS.md contribution rubric ("behavioral settings go in config.yaml, not new |
|
Requesting maintainer review — this is ready to land from my side. Just merge-synced with current main (the only conflict was a trivial AUTHOR_MAP keep-both in scripts/release.py); the PR's touched test files pass locally on the merged head. Standalone fork CI is pending first-run approval here; the rollup branch in #44061 carrying this session's batch is fully green on upstream CI. |
f95ca39 to
227e805
Compare
227e805 to
c0474be
Compare
|
STALE-NEEDS-REBASE: This PR is ~27 days old and has merge conflicts with main. The fix may still be relevant, but it needs a rebase and human decision on whether to revive. Not closing -- leaving for triage. |
Some CDN edges and middleboxes accept TLS 1.2 handshakes but kill TLS 1.3 ClientHellos, surfacing as [SSL: UNEXPECTED_EOF_WHILE_READING] ~15s into every request while curl (OS TLS stack) works fine (NousResearch#44365, DeepSeek's edge). - add _get_tls_ssl_context + _apply_tls_max_version in agent/process_bootstrap.py, composing the cap onto the httpx verify value both keepalive builders already carry so per-provider CA material survives - apply the cap in AIAgent._build_keepalive_http_client (primary clients) and the module-level build_keepalive_http_client (auxiliary clients), covering direct transports, no-proxy mounts, and proxy mounts - expose network.tls_max_version in config.yaml (DEFAULT_CONFIG, _config_version 33->34), bridged to HERMES_TLS_MAX_VERSION at startup via hermes_constants.apply_tls_max_version from gateway/run.py and hermes_cli/main.py; an explicitly exported env var still wins Fixes NousResearch#44365
c0474be to
009bd23
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for carrying the config.yaml bridge and both client-builder paths. The TLS-cap premise remains present on current main: neither keepalive builder reads a cap, while both receive the resolved verify value.
Problems
agent/process_bootstrap.py:229builds a separate TLS context before inspecting an existingverifycontext. If that independent CA lookup fails (:201-208),_apply_tls_max_version()returns the supplied provider context uncapped. Current provider setup creates that context before entering the builder (agent/agent_runtime_helpers.py:1689-1691). Parse the cap independently and cap an existing context first.hermes_cli/config.py:3316bumps_config_versionfor a defaulted field, contrary toAGENTS.md:584-590; this addition deep-merges without migration.website/docs/user-guide/configuration.md:1998-2007has no documentation for the new public setting.
Suggested changes
- Add a regression for an explicit valid provider CA context plus an invalid unrelated CA environment value.
- Preserve the existing default verification/trust-store semantics when replacing
verify=True. - Remove the schema-version bump and document the setting.
This is an automated hermes-sweeper review.
|
|
||
| Returns ``verify`` unchanged when the cap is unset or invalid. | ||
| """ | ||
| capped = _get_tls_ssl_context() |
There was a problem hiding this comment.
This creates and validates a separate CA context before inspecting verify. With a valid per-provider SSLContext but an invalid unrelated HERMES_CA_BUNDLE/REQUESTS_CA_BUNDLE value, _get_tls_ssl_context() returns None and the valid supplied context is returned uncapped. Parse the version separately and cap an existing SSLContext before any fresh CA-context creation.
|
|
||
| # Config schema version - bump this when adding new required fields | ||
| "_config_version": 33, | ||
| "_config_version": 34, |
There was a problem hiding this comment.
Please leave _config_version unchanged: this is a defaulted key in an existing section, so the normal deep merge supplies it without a migration. AGENTS.md explicitly reserves version bumps for active config transformations.
Summary
#44365 reports intermittent DeepSeek connection failures on Windows desktop: the bundled Python/OpenSSL dies with
[SSL: UNEXPECTED_EOF_WHILE_READING]~15s into every TLS 1.3 handshake againstapi.deepseek.com, while a TLS 1.2-only handshake succeeds in 0.33s (andcurl/PowerShell work because they use the OS TLS stack, not OpenSSL). This is a known class of CDN-edge/middlebox behavior — the server (or something in the path) accepts TLS 1.2 ClientHellos but kills TLS 1.3 ones.This adds the escape hatch the issue asks for, as a config.yaml setting:
Implementation
hermes_cli/config.py: newnetwork.tls_max_versionkey (default""= OpenSSL default), sibling ofnetwork.force_ipv4in the existing "connectivity workarounds" section. Per the AGENTS.md contribution rubric, the user-facing surface is config.yaml — the env var below is an internal bridge.hermes_constants.py:apply_tls_max_version()bridges the config value onto the internalHERMES_TLS_MAX_VERSIONenv var (same pattern asgateway.strict→HERMES_MEDIA_DELIVERY_STRICT). The env-var hop is needed becauseagent/process_bootstrap.pyhas no config access at HTTP-client build time, and spawned agent subprocesses must inherit the cap. An explicitly exported env var wins over config.yaml, so one-off shell overrides keep working.network.force_ipv4: thehermes_cli/main.pyearly raw-yaml block (covers CLI, desktop dashboard spawns, TUI gateway — no extra config.yaml read) and thegateway/run.pybootstrap.agent/process_bootstrap.py:_get_tls_ssl_context()parses the value (accepts1.2/1.3, optionaltls/tlsvprefix, case-insensitive; yaml floats fine) and builds anssl.SSLContextwithmaximum_versioncapped. It honors the CLI's existing CA-bundle override convention (HERMES_CA_BUNDLE>REQUESTS_CA_BUNDLE>SSL_CERT_FILE, same precedence as_resolve_requests_verifyinagent/model_metadata.py). Unset/invalid values fall back to httpx defaults with a logged warning;1.0/1.1are deliberately rejected — the knob exists to dodge broken TLS 1.3 paths, not to enable deprecated protocols.run_agent.py_build_keepalive_http_client: passes the context to both the keepaliveHTTPTransport(httpx ignores client-levelverifywhen an explicittransportis passed) and thehttpx.Client(so the proxy mount built internally fromproxy=inherits the same cap — otherwise proxied users would silently keep the broken default).Tests
tests/run_agent/test_keepalive_tls_max_version.py(12 tests): parsing (unset/blank, prefixes, invalid + 1.0/1.1 rejection), integration pins that the capped context lands on the transport pool and on theHTTPProxymount whenHTTPS_PROXYis set, that the default path keepsMAXIMUM_SUPPORTED, and the config bridge (sets the env var, never overrides an explicitly exported one, no-op on empty, handles yaml-float1.2and whitespace). Adjacent suites (test_create_openai_client_proxy_env,test_openai_client_lifecycle,test_ipv4_preference,hermes_cli/test_config.py) all pass.Fixes #44365