-
Notifications
You must be signed in to change notification settings - Fork 7.2k
Restrict MCP servers from requirements.toml
#9101
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
Conversation
bolinfest
left a comment
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.
Hmm, so if you declare one MCP server in requirements.toml, that means an end user can't use any MCP servers of their own, correct?
Do we need a way to say, "If you use MCP server xyz, then this is how you must configure it?" I realize that's arguably what should go in /etc/config/codex.toml, so maybe that doesn't really make sense either.
I'm just having trouble envisioning an enterprise enumerating all the accepted MCP servers, though I guess we do it with Chrome extensions?
dace824 to
92c8b1d
Compare
bolinfest
left a comment
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.
I don't believe this would prevent the user from running codex mcp add, though I suppose it's fine for them to add anything they want to their config.toml: it will just blow up at runtime if they declare an out-of-bounds MCP server in there?
| fn constrain_mcp_servers( | ||
| mcp_servers: HashMap<String, McpServerConfig>, | ||
| mcp_requirements: Option<&BTreeMap<String, McpServerRequirement>>, | ||
| ) -> ConstraintResult<Constrained<HashMap<String, McpServerConfig>>> { |
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.
Should this return Constrained::allow_any() in some cases, though?
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.
Constrained::normalized is effectively allow_any when the requirements are empty. But I've added a shortcircuit for that case now.
9ec3702 to
14fd9f4
Compare
14fd9f4 to
8c19337
Compare
Enterprises want to restrict the MCP servers their users can use.
Admins can now specify an allowlist of MCPs in
requirements.toml. The MCP servers are matched on both Name and Transport (local path or HTTP URL) -- both must match to allow the MCP server. This prevents circumventing the allowlist by renaming MCP servers in user config. (It is still possible to replace the local path e.g. rewrite say/usr/local/github-mcpwith a nefarious MCP. We could allow hash pinning in the future, but that would break updates. I also think this represents a broader, out-of-scope problem.)We introduce a new field to Constrained: "normalizer". In general, it is a fn(T) -> T and applies when
Constrained<T>.set()is called. In this particular case, it disables MCP servers which do not match the allowlist. An alternative solution would remove this and instead throw a ConstraintError. That would stop Codex launching if any MCP server was configured which didn't match. I think this is bad.We currently reuse the enabled flag on MCP servers to disable them, but don't propagate any information about why they are disabled. I'd like to add that in a follow up PR, possibly by switching out enabled with an enum.
In action: