-
Notifications
You must be signed in to change notification settings - Fork 51
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.
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
feat: secrets configuration for mcp server #565
base: v1.0
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -1,3 +1,5 @@ | ||||
use std::collections::HashMap; | ||||
|
||||
use mcp_client::client::Error as ClientError; | ||||
use serde::{Deserialize, Serialize}; | ||||
use thiserror::Error; | ||||
|
@@ -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 | ||||
#[serde(default)] | ||||
#[serde(flatten)] | ||||
map: HashMap<String, String>, | ||||
} | ||||
|
||||
impl Secrets { | ||||
pub fn new(map: HashMap<String, String>) -> Self { | ||||
Self { map } | ||||
} | ||||
|
||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated this to be set envs in the spawned process |
||||
} | ||||
} | ||||
} | ||||
|
||||
/// Represents the different types of MCP systems that can be added to the manager | ||||
#[derive(Debug, Clone, Deserialize, Serialize)] | ||||
#[serde(tag = "type")] | ||||
pub enum SystemConfig { | ||||
/// Server-sent events client with a URI endpoint | ||||
Sse { uri: String }, | ||||
Sse { | ||||
uri: String, | ||||
secrets: Option<Secrets>, | ||||
}, | ||||
/// Standard I/O client with command and arguments | ||||
Stdio { cmd: String, args: Vec<String> }, | ||||
Stdio { | ||||
cmd: String, | ||||
args: Vec<String>, | ||||
secrets: Option<Secrets>, | ||||
}, | ||||
} | ||||
|
||||
impl SystemConfig { | ||||
pub fn sse<S: Into<String>>(uri: S) -> Self { | ||||
Self::Sse { uri: uri.into() } | ||||
Self::Sse { | ||||
uri: uri.into(), | ||||
secrets: None, | ||||
} | ||||
} | ||||
|
||||
pub fn stdio<S: Into<String>>(cmd: S) -> Self { | ||||
Self::Stdio { | ||||
cmd: cmd.into(), | ||||
args: vec![], | ||||
secrets: None, | ||||
} | ||||
} | ||||
|
||||
|
@@ -43,20 +76,29 @@ impl SystemConfig { | |||
S: Into<String>, | ||||
{ | ||||
match self { | ||||
Self::Stdio { cmd, .. } => Self::Stdio { | ||||
Self::Stdio { cmd, secrets, .. } => Self::Stdio { | ||||
cmd, | ||||
secrets, | ||||
args: args.into_iter().map(Into::into).collect(), | ||||
}, | ||||
other => other, | ||||
} | ||||
} | ||||
|
||||
/// Returns a reference to the secrets in this config, if any | ||||
pub fn secrets(&self) -> Option<&Secrets> { | ||||
match self { | ||||
Self::Sse { secrets, .. } => secrets.as_ref(), | ||||
Self::Stdio { secrets, .. } => secrets.as_ref(), | ||||
} | ||||
} | ||||
} | ||||
|
||||
impl std::fmt::Display for SystemConfig { | ||||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||||
match self { | ||||
SystemConfig::Sse { uri } => write!(f, "SSE({})", uri), | ||||
SystemConfig::Stdio { cmd, args } => write!(f, "Stdio({} {})", cmd, args.join(" ")), | ||||
SystemConfig::Sse { uri, .. } => write!(f, "SSE({})", uri), | ||||
SystemConfig::Stdio { cmd, args, .. } => write!(f, "Stdio({} {})", cmd, args.join(" ")), | ||||
} | ||||
} | ||||
} | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 vargoose/crates/goose/src/key_manager.rs
Line 45 in 3e0ce19