feat: secrets configuration for mcp server #565
Conversation
salman1993
requested review from
michaelneale,
alexhancock,
baxen,
kalvinnchau,
wendytang and
lily-de
Desktop App for this PRThe following build is available for testing: The app is signed and notarized for macOS. After downloading, unzip the file and drag the Goose.app to your Applications folder. This link is provided by nightly.link and will work even if you're not logged into GitHub. |
| @@ -15,25 +17,56 @@ pub enum SystemError { | |||
|
|
|||
| pub type SystemResult<T> = Result<T, SystemError>; | |||
|
|
|||
| #[derive(Debug, Clone, Deserialize, Serialize)] | |||
| pub struct Secrets { | |||
| /// A map of environment variables to set, e.g. API_KEY -> some_secret | |||
There was a problem hiding this comment.
this right now stores the secret in plaintext. this can also be a map of env_var_name -> key_manager_secret_name and then we pull in the secret value and set it as env var
goose/crates/goose/src/key_manager.rs
Line 45 in 3e0ce19
crates/goose/src/agents/system.rs
Outdated
|
|
||
| pub fn set_environment_vars(&self) { | ||
| for (key, value) in &self.map { | ||
| std::env::set_var(key, value); |
There was a problem hiding this comment.
this looks like it sets the env vars for the main goose process, should we only be setting the secrets in for the subprocess when doing stdio mcps servers?
There was a problem hiding this comment.
this is a good question. there is a TODO on adding in-process transport. for now, it makes sense to set it in only in subprocess -
There was a problem hiding this comment.
updated this to be set envs in the spawned process
Desktop App for this PRThe following build is available for testing: The app is signed and notarized for macOS. After downloading, unzip the file and drag the Goose.app to your Applications folder. This link is provided by nightly.link and will work even if you're not logged into GitHub. |
| } | ||
| } | ||
|
|
||
| async fn spawn_process(&self) -> Result<(Child, ChildStdin, ChildStdout), Error> { | ||
| let mut process = Command::new(&self.command) | ||
| .envs(&self.env) |
There was a problem hiding this comment.
Will this do any validation for us to make sure the values passed are valid as environment vars? Names etc. Would be nice to get some descriptive messages if anything goes wrong.
There was a problem hiding this comment.
this totally depends on the server implementation. in the case of the Tavily example: it errors if TAVILY_API_KEY is not set but it won't error if its an invalid key
Desktop App for this PRThe following build is available for testing: The app is signed and notarized for macOS. After downloading, unzip the file and drag the Goose.app to your Applications folder. This link is provided by nightly.link and will work even if you're not logged into GitHub. |
tested with Tavily MCP Server for web search:
build and run goosed:
cargo run --bin goosed -- agentadd system with secret. you need to replace with free API key that you can get here: https://app.tavily.com/home
step (2) validation depends on MCP server implementation - eg. it fails if env var is not set but does not fail if you set an invalid key - in that case, it will fail in step (3) by replying it can't get the news, which isn't good.
We can prompt the LLM to let the user know if the system contains secrets but its tools can't be used.