Skip to content

feat: add optional native-tls support as alternative to rustls#8037

Open
r0x0d wants to merge 3 commits intoblock:mainfrom
r0x0d:native-tls-support
Open

feat: add optional native-tls support as alternative to rustls#8037
r0x0d wants to merge 3 commits intoblock:mainfrom
r0x0d:native-tls-support

Conversation

@r0x0d
Copy link
Contributor

@r0x0d r0x0d commented Mar 20, 2026

Summary

Add a native-tls Cargo feature across all crates as an alternative to the default rustls TLS backend. This enables building goose against the platform's native TLS stack (OpenSSL on Linux, Secure Transport on macOS, SChannel on Windows), which is required for Fedora/RHEL packaging where vendoring rustls/aws-lc-rs is problematic.

Changes by crate:

goose (core):

  • Add default-tls and native-tls feature groups gating reqwest, rmcp, sqlx, jsonwebtoken, and oauth2 TLS backends
  • Set reqwest, rmcp, sqlx, oauth2 to default-features = false so TLS backend is selected purely via features
  • Switch jsonwebtoken to default-features = false with explicit use_pem; gate aws_lc_rs (default-tls) vs rust_crypto (native-tls)
  • Conditionally compile Identity::from_pem (rustls) vs Identity::from_pkcs8_pem (native-tls) in api_client.rs

goose-server:

  • Add default-tls and native-tls feature groups gating reqwest, tokio-tungstenite, axum-server, rustls, aws-lc-rs, and openssl
  • Make rustls, aws-lc-rs optional (default-tls); add openssl optional (native-tls)
  • Refactor tls.rs: extract generate_self_signed_cert() and sha256_fingerprint() helpers; use cfg-gated TlsConfig type alias (RustlsConfig vs OpenSSLConfig)
  • Cfg-gate rustls crypto provider initialization in agent.rs and lapstone.rs
  • Cfg-gate axum_server::bind_rustls vs bind_openssl in agent.rs
  • Add TLS integration tests in tests/tls_test.rs

goose-cli:

  • Add default-tls and native-tls feature groups gating reqwest and sigstore-verification TLS backends, forwarding to goose and goose-mcp

goose-mcp:

  • Add default-tls and native-tls features gating reqwest TLS backend; remove hardcoded rustls feature from reqwest dep

goose-acp:

  • Add default-tls and native-tls feature forwarding to goose dep

CI and build:

  • Remove scripts/check-no-native-tls.sh (no longer applicable)
  • Remove banned TLS crate check from ci.yml and Justfile

Usage:

Default build (rustls, unchanged behavior) cargo build

Build with native-tls (OpenSSL) cargo build --no-default-features --features native-tls,code-mode

Made-with: Cursor

Testing

Tested by running cargo test locally and verifying that native-tls was being used, both with default features and without it.

Related Issues

Relates to #ISSUE_ID
Discussion: LINK (if any)

Screenshots/Demos (for UX changes)

Before:

After:

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ad16ab9720

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 59bc733759

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3049f47a47

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@DOsinga DOsinga requested a review from jamadeo March 20, 2026 17:58
@DOsinga DOsinga added the needs_human label to set when a robot looks at a PR and can't handle it label Mar 23, 2026
Copy link
Collaborator

@jamadeo jamadeo left a comment

Choose a reason for hiding this comment

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

This makes sense to me and thank you for taking it on!

My main question is whether this should be two features or one. With both default-tls/native-tls features, we now have a third state (assuming we keep them mutually exclusive) where neither are enabled, and I'm not sure that would ever produce a valid build (?).

If that's true I'd imagine we'd want a single new feature, native-tls, and when this is off, use what we're calling here default-tls.

@jh-block
Copy link
Collaborator

jh-block commented Mar 23, 2026

@jamadeo I think it's okay for --no-default-features without a TLS feature specified to fail to build. Cargo features are supposed to be strictly additive; in theory enabling a feature (e.g. native-tls) shouldn't remove functionality (rustls support). We can make it fail with a sensible compile error by doing:

#[cfg(not(any(feature = "default-tls", feature = "native-tls")))]
compile_error!("At least one of the default-tls and native-tls features must be enabled");

Arguably we should make it exactly one, and add a similar gate for an error when more than one is enabled; an alternative is to allow specifying both and selecting the implementation at runtime, but that seems overly complex and I don't know what the use case would be.

I think I'd prefer rustls-tls rather than default-tls though, wdyt?

@r0x0d r0x0d force-pushed the native-tls-support branch from 3049f47 to 76def0c Compare March 23, 2026 17:51
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 76def0c909

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".


[features]
default = ["code-mode", "local-inference"]
default = ["code-mode", "local-inference", "rustls-tls"]

Choose a reason for hiding this comment

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

P1 Badge Make goose native-tls replace the default rustls backend

When a downstream crate opts into goose's new native-tls feature in the normal Cargo way (features = ["native-tls"]), line 11 still keeps rustls-tls in the default set, so both TLS features become active. That now trips the mutual-exclusion compile_error! in crates/goose/src/lib.rs:1-5, which means the advertised alternative backend is unusable for consumers unless they also know to add default-features = false to their dependency spec.

Useful? React with 👍 / 👎.

Copy link
Collaborator

@jh-block jh-block Mar 24, 2026

Choose a reason for hiding this comment

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

Hmm, thinking about this more, this means that if any one crate in the dependency tree doesn't disable default features on goose, you can't switch to native-tls without getting a compile error.

Hot take: What if we just didn't have any default features (everything is opt-in)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, thinking about this more, this means that if any one crate in the dependency tree doesn't disable default features on goose, you can't switch to native-tls without getting a compile error.

Hot take: What if we just didn't have any default features (everything is opt-in)?

Hm, I don't think so... In the downstream builds, we have a few crates that can't switch to native-tls due to them not having native-tls present, but the build still works fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What I mean is that if you write a crate A that depends on goose, and then one of your dependencies B also depends on goose, if B doesn't disable default-features then A has no choice but to use rustls even if it wanted native-tls (because it's impossible to turn off a Cargo feature that another -- even transitive -- dependency has turned on)

Possible solutions are to not fail the build if both are enabled (make the non-default tls backend take precedence over the default one), or to not have any default at all and make people specify the Cargo feature. I actually prefer the no default approach -- I think default features are fundamentally flawed because of this issue -- but it does create a tiny bit more work when someone is adding goose as a dependency or building it from source.

@jamadeo
Copy link
Collaborator

jamadeo commented Mar 23, 2026

@jh-block makes sense to me -- you're right I had forgotten the strictly additive aspect. Agreed that rustls-tls is probably the better name too.

r0x0d and others added 3 commits March 24, 2026 09:10
Add a `native-tls` Cargo feature across all crates as an alternative to
the default `rustls` TLS backend. This enables building goose against
the platform's native TLS stack (OpenSSL on Linux, Secure Transport on
macOS, SChannel on Windows), which is required for Fedora/RHEL packaging
where vendoring rustls/aws-lc-rs is problematic.

Changes by crate:

goose (core):
- Add `default-tls` and `native-tls` feature groups gating reqwest,
  rmcp, sqlx, jsonwebtoken, and oauth2 TLS backends
- Set reqwest, rmcp, sqlx, oauth2 to default-features = false so TLS
  backend is selected purely via features
- Switch jsonwebtoken to default-features = false with explicit use_pem;
  gate aws_lc_rs (default-tls) vs rust_crypto (native-tls)
- Conditionally compile Identity::from_pem (rustls) vs
  Identity::from_pkcs8_pem (native-tls) in api_client.rs

goose-server:
- Add `default-tls` and `native-tls` feature groups gating reqwest,
  tokio-tungstenite, axum-server, rustls, aws-lc-rs, and openssl
- Make rustls, aws-lc-rs optional (default-tls); add openssl optional
  (native-tls)
- Refactor tls.rs: extract generate_self_signed_cert() and
  sha256_fingerprint() helpers; use cfg-gated TlsConfig type alias
  (RustlsConfig vs OpenSSLConfig)
- Cfg-gate rustls crypto provider initialization in agent.rs and
  lapstone.rs
- Cfg-gate axum_server::bind_rustls vs bind_openssl in agent.rs
- Add TLS integration tests in tests/tls_test.rs

goose-cli:
- Add `default-tls` and `native-tls` feature groups gating reqwest and
  sigstore-verification TLS backends, forwarding to goose and goose-mcp

goose-mcp:
- Add `default-tls` and `native-tls` features gating reqwest TLS
  backend; remove hardcoded rustls feature from reqwest dep

goose-acp:
- Add `default-tls` and `native-tls` feature forwarding to goose dep

CI and build:
- Remove scripts/check-no-native-tls.sh (no longer applicable)
- Remove banned TLS crate check from ci.yml and Justfile

Usage:
  # Default build (rustls, unchanged behavior)
  cargo build

  # Build with native-tls (OpenSSL)
  cargo build --no-default-features --features native-tls,code-mode

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>
Made-with: Cursor
Propagate TLS feature selection from goose-acp to goose-mcp so standalone
goose-acp-server builds retain HTTPS support for builtin extensions.

Use openssl to convert any PEM key format (PKCS#1, SEC1, PKCS#8) to PKCS#8
in the native-tls path, so existing mTLS setups with traditional RSA or EC
keys continue to work.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>
…itation

Change all rustls cfg guards from `not(feature = "native-tls")` to
`feature = "default-tls"` so the rustls path does not compile when
neither TLS backend is enabled (e.g. --no-default-features). Gate the
tls module and its consumers on having at least one TLS backend, and
return a clear error if TLS is requested at runtime without one.

Add module-level documentation to tls.rs explaining that the native-tls
feature uses OpenSSL for the server listener via axum-server.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>
@r0x0d r0x0d force-pushed the native-tls-support branch from 76def0c to 182acff Compare March 24, 2026 12:19
@r0x0d
Copy link
Contributor Author

r0x0d commented Mar 24, 2026

@jamadeo / @jh-block, rebased to fix the conflicts.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 182acff496

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +13 to +15
[features]
rustls-tls = ["reqwest/rustls"]
native-tls = ["reqwest/native-tls"]

Choose a reason for hiding this comment

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

P2 Badge Add default TLS feature back to goose-mcp

goose-mcp now defines rustls-tls/native-tls but no default feature, while its reqwest dependency has default-features = false. That makes standalone goose-mcp builds (or downstream users that don’t explicitly pick a TLS feature) compile without any HTTPS backend, so tools like web_scrape fail for normal https:// URLs at runtime. Before this change, rustls was always enabled; restoring a default TLS feature preserves that behavior.

Useful? React with 👍 / 👎.

Comment on lines +32 to +34
native-tls = [
"dep:openssl",
"reqwest/native-tls",

Choose a reason for hiding this comment

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

P2 Badge Remove OpenSSL hard dependency from native-tls mode

The native-tls feature now unconditionally enables dep:openssl, so selecting native TLS always pulls in openssl-sys even on macOS/Windows where the expected backend is Security.framework/SChannel. In environments that don’t provide OpenSSL development libs, cargo build --no-default-features --features native-tls fails before runtime, which makes the advertised alternative backend unusable there.

Useful? React with 👍 / 👎.

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, dep:openssl here seems wrong -- openssl is not the native TLS on mac or windows

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs_human label to set when a robot looks at a PR and can't handle it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants