Generate passwords and confirm PROD updates in service_update_password - #250
Conversation
Askir
left a comment
There was a problem hiding this comment.
I will say I am not too fond of the elicitation stuff yet basically haven't used it ever but maybe i just use MCPs too little.
But generally looks good 👍
| // setWithPasswordSchemaProperties sets common with_password schema properties | ||
| func setWithPasswordSchemaProperties(schema *jsonschema.Schema) { | ||
| schema.Properties["with_password"].Description = "Whether to include the password in the response and connection string. NEVER set to true unless the user explicitly asks for the password." | ||
| schema.Properties["with_password"].Description = "Whether to include the password in the response. NEVER set to true unless the user explicitly asks for the password." |
There was a problem hiding this comment.
I think the other tools with this flag actually still output the connection string 🤔
There was a problem hiding this comment.
Yes, the other tools do still output the connection string, but the connection string is part of "the response", so the message is still accurate imo. I just wanted to make it generic enough to also be used for tools that don't output a connection string. For the tools that do, the connection string field description itself carries an explanation of how the with_password parameter affects it (see here), so I don't think we're really losing any clarity here.
Yeah, it's a tough call, and I frankly have mixed feelings too. We decided to add it as an extra safety precaution for PROD services, because we previously couldn't even come to agreement about whether it made sense to provide a Fwiw, we have also seen real issues with agents abusing these tools: we had an internal report of someone's agent deciding to autonomously change a service's password when it failed to connect to it, which could have been very bad for a production service. So adding the same confirmation prompt for PROD password updates likewise felt like a valuable safeguard. The flip side is that we do already have read-only mode, which protects people's production services, and most coding agent harnesses likewise ask for confirmation before executing MCP tools (though most people are probably just using auto-mode these days, which I suspect would let these kinds of calls through). The MCP tools are also marked as "destructive", which many harnesses use to determine if they should ask for additional confirmation before proceeding. So in some ways, it feels like we're re-implementing features that belong in the coding agent/harness, rather than in the MCP server, and aren't trusting people to make their own choices about the level of safety they want. Still, doing it in the MCP server is the only way we can guarantee there's always a human-in-loop for these kinds of destructive actions, which I think is worthwhile from a product safety perspective. Additionally, I don't expect people to be deleting production services, or rotating their passwords, very often (and DEV services do not have the same confirmation requirement), so I don't expect it to be a major source of friction for most agentic workflows. It's still definitely something we should keep an eye on, though. |
Makes the
service_update_passwordMCP tool safer by having it generate passwords itself and confirm PROD updates with the user.passwordparameter; the tool now always generates the password. A password passed as an argument is either invented by the model or typed into the chat by the user, so it ends up in the model's context. Collecting it through an elicitation form instead would still expose it to session transcripts and the MCP client, and the MCP spec says servers MUST NOT request passwords through form elicitation. Users who want a specific password can still set one withtiger service update-password.with_passwordoption to include the generated password in the result, matchingservice_createandservice_get. It's off by default. When it's off and the new password isn't saved, because saving fails or password storage is disabled, the result warns that nobody has the password.service_delete: the user types the service ID back to confirm. A client that can't prompt gets an error telling the agent to ask the user to run the CLI command instead.internal/mcp/elicitation.go, so all tools share them.GenerateSecurePasswordfrominternal/utiltointernal/common, since it's specific to database passwords rather than a generic helper (likeGenerateServiceName). It's also now a var so tests can stub it.GetServicealso moved frominternal/common/replica.gointointernal/common/service.go, where it belongs.