Skip to content

feat: adds mtls to all providers (#2794) #2799

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
jamadeo merged 1 commit into from
Aug 20, 2025
Merged

feat: adds mtls to all providers (#2794) #2799

jamadeo merged 1 commit into from
Aug 20, 2025

Conversation

@myaple
Copy link
Contributor

@myaple myaple commented Jun 5, 2025 •
edited
Loading

This PR adds mutual TLS support to all the providers in goose. The default behavior remains unchanged, with not providing any certificates just skipping the appropriate sections of the client builder.

@zanesq zanesq requested review from baxen and salman1993 June 18, 2025 20:43
@myaple myaple force-pushed the myaple/mtls branch 3 times, most recently from 80e3442 to e478792 Compare July 19, 2025 12:19
@myaple myaple changed the title feat: adds mtls to openai and ollama (#2794) feat: adds mtls to openai, litellm, and ollama (#2794) Jul 19, 2025
@myaple
Copy link
Contributor Author

myaple commented Jul 19, 2025 •
edited
Loading

Added mTLS support to the new liteLLM client since that has a similar use case to a custom OpenAI or ollama URL.

Also removed changes from the goose-llm crate since I'm not sure that's relevant to the overall goal of adding mTLS to goose cli.

@myaple myaple force-pushed the myaple/mtls branch 2 times, most recently from 8312cb9 to a80c6f7 Compare July 26, 2025 19:51
@DOsinga
Copy link
Collaborator

DOsinga commented Aug 2, 2025

sorry for the non response for so long here! thanks for doing this. I'd be happy to merge as is, but I think we could do better here if we made this more general.

extract build_client to a common file, I think we want to do this anyway. call this one from all the providers. and then use a variable like: TLS_CERT_FILE / TLS_KEY_FILE / TLS_CA_FILE for all of them

what do you think? we need to get the specific provider handling under control

myaple
myaple commented Aug 2, 2025
View reviewed changes
@myaple
Copy link
Contributor Author

myaple commented Aug 2, 2025 •
edited
Loading

@DOsinga no worries! thanks for looking at it. I've unified the approach across some all of the providers I'll continue working to integrate the rest of them where possible. I had limited it just to the most common providers for this sort of enterprise access control originally, which is why I chose OpenAI compatible interfaces and Ollama, but I'm happy to make it more generic.

I asked one question about docs above, only other question I have is would you like me to make a global provider timeout config instead of having things like OLLAMA_TIMEOUT and a bunch of hardcoded 600 second timeouts? It feels awkward to just smash in hardcoded timeouts when there's already at least one configuration.

myaple
myaple commented Aug 2, 2025
View reviewed changes
@myaple myaple force-pushed the myaple/mtls branch 3 times, most recently from 161f736 to 0e985a2 Compare August 4, 2025 20:38
@myaple
Copy link
Contributor Author

myaple commented Aug 4, 2025

@DOsinga no worries! thanks for looking at it. I've unified the approach across some all of the providers I'll continue working to integrate the rest of them where possible. I had limited it just to the most common providers for this sort of enterprise access control originally, which is why I chose OpenAI compatible interfaces and Ollama, but I'm happy to make it more generic.

I asked one question about docs above, only other question I have is would you like me to make a global provider timeout config instead of having things like OLLAMA_TIMEOUT and a bunch of hardcoded 600 second timeouts? It feels awkward to just smash in hardcoded timeouts when there's already at least one configuration.

I pulled out the docs, it was in a totally wrong format, not even worth it. If docs are needed, I'll start from scratch.

@DOsinga
Copy link
Collaborator

DOsinga commented Aug 5, 2025

sorry to keep banging on this - we just merged a refactor on how the providers work and they now all use an ApiClient object - it should be easy to add that to that now

@myaple
Copy link
Contributor Author

myaple commented Aug 8, 2025 •
edited
Loading

sorry to keep banging on this - we just merged a refactor on how the providers work and they now all use an ApiClient object - it should be easy to add that to that now

its okay, I'll need some time to validate it again. with a holiday coming up, it'll probably be a week or so before it's ready again, I'll ping you when I'm ready for a review with the new builder methods. I've pushed what I've got so far, it probably won't be far off what I'll come to eventually, if you want to let me know if you're alright with it in structure.

It deviates slightly from other AuthMethods just cause mTLS doesn't use headers and such, so let me know if this looks okay and I'll finish up with writing tests.

@michaelneale
Copy link
Collaborator

michaelneale commented Aug 11, 2025

thanks @myaple did you have more you wanted to do or this is ready to try as is?

@myaple
Copy link
Contributor Author

myaple commented Aug 12, 2025

thanks @myaple did you have more you wanted to do or this is ready to try as is?

I believe this is all I wanted to do, I just haven't written tests for it as I'm on holiday without a real computer till next week. Feel free to try it out, and I'll write tests ASAP.

@myaple
Copy link
Contributor Author

myaple commented Aug 19, 2025

TODOs from testing:

  • modify ApiClient::new to build tls auth instead of in all of the with_* funcs
  • modify each client creation in the providers to detect whether or not to use mutual TLS - potentially in addition to the bearer token instead of replacing it?
  • validate env vars as GOOSE_* are picked up

DOsinga
DOsinga reviewed Aug 19, 2025
View reviewed changes
@myaple myaple force-pushed the myaple/mtls branch 2 times, most recently from 5aa2e46 to 152d5bb Compare August 19, 2025 19:41
@myaple
Copy link
Contributor Author

myaple commented Aug 20, 2025

I've tested this today and it is working. If there's anything else you want done in this PR, let me know, otherwise I think it's good to go @DOsinga @michaelneale

@jamadeo jamadeo assigned DOsinga and unassigned baxen Aug 20, 2025
@myaple myaple changed the title feat: adds mtls to openai, litellm, and ollama (#2794) feat: adds mtls to all providers (#2794) Aug 20, 2025
jamadeo
jamadeo approved these changes Aug 20, 2025
View reviewed changes
@jamadeo
Copy link
Collaborator

jamadeo commented Aug 20, 2025

fantastic, thanks for the contribution @myaple !

@jamadeo jamadeo merged commit 1dbbd91 into block:main Aug 20, 2025
10 checks passed
@myaple
Copy link
Contributor Author

myaple commented Aug 20, 2025

fantastic, thanks for the contribution @myaple !

For sure, thanks for working with me to get it across the line!

ayax79 pushed a commit to ayax79/goose that referenced this pull request Aug 21, 2025
@myaple
feat: adds mtls to all providers (block#2794) (block#2799)
e9bd414 
Signed-off-by: Matt Yaple <[email protected]>
Signed-off-by: Jack Wright <[email protected]>
michaelneale added a commit that referenced this pull request Aug 21, 2025
@michaelneale
Merge branch 'main' into spence/chatrework
d125cff 
* main:
  docs: add figma tutorial (#4231)
  Add Nix flake for reproducible builds (#4213)
  Enhanced onboarding page visual design (#4156)
  feat: adds mtls to all providers (#2794) (#2799)
  Don't show a confirm dialog for quitting (#4225)
  Fix: Missing smart_approve in CLI /mode help text and error message (#4132)
  Clean up langfuse docs and scripts (#4220)
  feat: add remark-breaks plugin to preserve single newlines in markdown (#4217)
  feat(mcp/developer): accept -1 for insert_line number (#4112)
  Remove dead code and old settings migration (#4180)
  removed tests from lint-staged (#4203)
  docs: openrouter and ollama easy desktop setup (#4195)
  Custom providers update (#4099)
  docs: goose_terminal env var (#4205)
  Desktop alerts when suspicious unicode characters found in Recipe (#4080)
  chore: remove the google drive built-in extension (#4187)
  Move out app init (#4185)
michaelneale added a commit that referenced this pull request Aug 21, 2025
@michaelneale
Merge branch 'main' into jackamadeo/extensions-concurrent
cbc6c67 
* main:
  Add PKCE support for Tetrate Agent Router Service (#4165)
  Read AGENTS.md by default (#4232)
  docs: configure provider and model (#4235)
  docs: add figma tutorial (#4231)
  Add Nix flake for reproducible builds (#4213)
  Enhanced onboarding page visual design (#4156)
  feat: adds mtls to all providers (#2794) (#2799)
katzdave added a commit that referenced this pull request Aug 21, 2025
@katzdave
Merge branch 'main' of github.com:block/goose into dkatz/fast-summarize2
a60b0fd 
* 'main' of github.com:block/goose:
  chore: upgrade rmcp to 0.6.0 (#4243)
  doc: uvx not npx (#4240)
  Add PKCE support for Tetrate Agent Router Service (#4165)
  Read AGENTS.md by default (#4232)
  docs: configure provider and model (#4235)
  docs: add figma tutorial (#4231)
  Add Nix flake for reproducible builds (#4213)
  Enhanced onboarding page visual design (#4156)
  feat: adds mtls to all providers (#2794) (#2799)
  Don't show a confirm dialog for quitting (#4225)
  Fix: Missing smart_approve in CLI /mode help text and error message (#4132)
lifeizhou-ap added a commit that referenced this pull request Aug 22, 2025
@lifeizhou-ap
Merge branch 'main' into lifei/read-upper-goosehints
7d38c5e 
* main: (108 commits)
  Remove unused game (#4226)
  fix issue where app redirects to home after initialization but user has already started a chat (#4260)
  Feat: Let providers configure a fast model for summarization (#4228)
  docs: update tool selection strategy (#4258)
  feat: upgrade `@mcp-ui/client` package and improve UI message handling (#4164)
  stop replacing chat window when changing working directory (#4200)
  Only fetch session tokens when chat state is idle to avoid resetting during streaming (#4104)
  bump timeouts for e2e tests (#4251)
  docs: custom context files improvements (#4096)
  chore: upgrade rmcp to 0.6.0 (#4243)
  doc: uvx not npx (#4240)
  Add PKCE support for Tetrate Agent Router Service (#4165)
  Read AGENTS.md by default (#4232)
  docs: configure provider and model (#4235)
  docs: add figma tutorial (#4231)
  Add Nix flake for reproducible builds (#4213)
  Enhanced onboarding page visual design (#4156)
  feat: adds mtls to all providers (#2794) (#2799)
  Don't show a confirm dialog for quitting (#4225)
  Fix: Missing smart_approve in CLI /mode help text and error message (#4132)
  ...
@alexhancock alexhancock mentioned this pull request Aug 22, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DOsinga DOsinga DOsinga left review comments

@jamadeo jamadeo jamadeo approved these changes

@salman1993 salman1993 Awaiting requested review from salman1993

@baxen baxen Awaiting requested review from baxen

Assignees

@DOsinga DOsinga

Labels

feature-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@myaple @DOsinga @michaelneale @jamadeo @baxen