Skip to content

fix(ssl): use default SSL context for all httpx calls#11051

Closed
phymbert wants to merge 3 commits intoOpenHands:mainfrom
phymbert:phymbert/hotfix/httpx/defaultcontext
Closed

fix(ssl): use default SSL context for all httpx calls#11051
phymbert wants to merge 3 commits intoOpenHands:mainfrom
phymbert:phymbert/hotfix/httpx/defaultcontext

Conversation

@phymbert
Copy link
Copy Markdown
Contributor

@phymbert phymbert commented Sep 20, 2025 •
edited
Loading

Summary

While testing OpenHands, I hit SSL: CERTIFICATE_VERIFY_FAILED. Many orgs install a private root CA in the OS trust store; however, several httpx clients weren’t explicitly using a default SSL context.

This PR ensures all httpx calls consistently use the expected system default SSL context. It also adds a debug-only escape hatch to disable verification via config/env vars when needed.

What’s changed

  • Introduce openhands.utils.http_session:

    • httpx_verify_option() for consistent verify= across httpx clients (async & sync).
    • Shared client management + configure_http_session(verify=...).

  • Introduce insecure_skip_verify in SecurityConfig

  • Wire default SSL behavior everywhere we create httpx clients:

    • Enterprise modules (e.g., Jira Cloud/DC, Linear), OAuth/key routes, billing, nested conversation managers, providers, storage webhooks, local runtime readiness checks, Git services, resolvers, etc.

  • Config & docs:

    • docs/usage/environment-variables.mdx: document SECURITY_INSECURE_SKIP_VERIFY.

  • Backwards compatible: by default we still verify certificates; behavior is unchanged for secure setups.

Motivation

  • Enterprises often use a self-signed corporate root CA. Using the system default SSL context lets requests succeed once that root is present in the OS trust store.
  • A opt-in (insecure_skip_verify) helps debug TLS issues without blocking critical flows, which must not be used for production.

Usage notes

  • Secure default: No change required if your environment has proper trust roots installed.
  • Temporary debug override (NOT recommended for production):
    • Env: SECURITY_INSECURE_SKIP_VERIFY=true
    • Or config.toml:
[security]
# Skip TLS certificate verification for outbound HTTP requests (NOT recommended)
insecure_skip_verify = true
  • I also touched the enterprise module (planning to try Jira integration soon), feel free to ask for or to commit additional changes.

Testing

  • Verified success against an internal endpoint with a corporate root CA installed in the OS trust store (ubuntu based docker + bitbucket endpoint).
  • Verified failure → success flip when toggling SECURITY_INSECURE_SKIP_VERIFY.

Security considerations

  • Verification remains enabled by default.
  • insecure_skip_verify is explicitly documented as NOT recommended and intended for temporary debugging only.

Thanks

Really happy to use OpenHands — huge thanks to the team and the community for the awesome project and support 🙏

@phymbert phymbert requested a review from mamoodi as a code owner September 20, 2025 20:44
@phymbert
Copy link
Copy Markdown
Contributor Author

phymbert commented Sep 20, 2025

I dont think linter issues are related to the changes, tell me if you want me to fix them.

@phymbert phymbert mentioned this pull request Sep 20, 2025
Merged
@phymbert
Copy link
Copy Markdown
Contributor Author

phymbert commented Oct 1, 2025

Is there something missing to review this change ?

I can not use openhands without these basic security fixes.

Please tell me how I can help to merge it. Thanks

@mamoodi mamoodi requested review from enyst and xingyaoww October 1, 2025 12:55
@enyst enyst changed the title fix(ssl): use default SSL context for all httpx calls; add opt-out verify for debugging fix(ssl): use default SSL context for all httpx calls Oct 1, 2025
rbren
rbren reviewed Oct 3, 2025
View reviewed changes
Comment thread openhands/utils/http_session.py Outdated
@phymbert phymbert requested a review from rbren October 5, 2025 16:07
@phymbert
Copy link
Copy Markdown
Contributor Author

phymbert commented Oct 18, 2025

Hands up 🖐

@enyst enyst requested a review from tofarr October 18, 2025 10:35
tofarr
tofarr requested changes Oct 21, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@tofarr tofarr left a comment

Choose a reason for hiding this comment

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

I find this a little worrisome - it essentially gives the ability to disable SSL for outbound requests, leaving the system vulnerable to a MITM attack. Have you tried adding the custom cert to certifi (Used by httpx) so that security is maintained?

@phymbert
Copy link
Copy Markdown
Contributor Author

phymbert commented Oct 21, 2025

  1. disabling verification is opt-in, so there is no risk at all of a Man-In-The nowhere attack except if the user wants it. This is a standard option in any platform
  2. examples you are giving is exactly what the PR does: to use OS CA bundle without code changes, this is also the industry standard

I believe you must double read the code changes?

@raymyers
Copy link
Copy Markdown
Contributor

raymyers commented Oct 21, 2025

examples you are giving is exactly what the PR does

I don't think supplying a custom cert which is checked is the same thing as allowing disabling of all checking.

no risk except if the user wants it

I agree that the risk is opt-in, and the question is whether to include that option given there could be a more secure alternative, namely custom certs.

@phymbert
Copy link
Copy Markdown
Contributor Author

phymbert commented Oct 21, 2025 •
edited
Loading

The code adds the proper SSL.create_default_context() which looks into the OS CA bundle, i.e /etc/ssl/certs/ca-certificates.crt. This is the standard way to add custom chain certs instead of modifying the code, than using the ones bundled with certify. Letting the user to specify his own CA location is dangerous as it breaks OS configuration. The proposed approach is safe and works for any container based image, even distroless.

Regarding the second code changes: the option to fully disable ssl verify, all industry tools like curl, apache offer this alternative option which is of course not recommended for production. But if you feel it is so dangerous, we can add a warning message. I thought the documentation was clear enough, isn't?

@raymyers
Copy link
Copy Markdown
Contributor

raymyers commented Oct 21, 2025

The code adds the proper SSL.create_default_context()

Thanks for expanding. I think we're all on board with that part of the change. Does your use case require the disable option in addition, or this is more about following the pattern of other tools?

@phymbert
Copy link
Copy Markdown
Contributor Author

phymbert commented Oct 21, 2025

Thanks for your time on this, appreciated.

I used it to debug at the beginning to be sure it was wrong httpx configuration, and I believe this is useful for other user to investigate in the future, but no, my use case is over secured, trust me ^)

So if you want, we can simply get rid of this option, but I think it is worth keeping

@mamoodi mamoodi removed their request for review October 22, 2025 11:17
This was referenced Oct 24, 2025
Merged
Open
@raymyers
Copy link
Copy Markdown
Contributor

raymyers commented Oct 27, 2025

Spun this off into #11505 to use the default SSL context, which has new been merged. I also think the config option has merit, and I've created issue #11506 so it can be considered separately without blocking this.

Thanks for this!

@raymyers raymyers closed this Oct 27, 2025
@phymbert phymbert deleted the phymbert/hotfix/httpx/defaultcontext branch October 28, 2025 02:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tofarr tofarr tofarr requested changes

@enyst enyst Awaiting requested review from enyst

@xingyaoww xingyaoww Awaiting requested review from xingyaoww

@rbren rbren Awaiting requested review from rbren

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@phymbert @raymyers @rbren @tofarr