feat(mcp): implement logging capability with error handling and secret redaction#629
Conversation
|
|
||
| func sanitizeMessage(msg string) string { | ||
| // JSON/YAML field patterns (indices 0-6) - preserve field name | ||
| for i := 0; i < 7 && i < len(sensitivePatterns); i++ { |
There was a problem hiding this comment.
I guess there hard coded checks seem a bit funky ? 😅
There was a problem hiding this comment.
yeah, I feel the same, but couldn't think of something better, will take another pass at it
There was a problem hiding this comment.
Would this not be one of the sanitizations that the MCP gateway would handle? (Although I get that that depends on a specific deployment topology, so maybe not the best fix for here)
There was a problem hiding this comment.
The spec states that the server should take care of this: https://modelcontextprotocol.io/specification/2025-11-25/server/utilities/logging#implementation-considerations
However, I believe this should be part of the SDK itself, in addition to our own implementation, we might want to open an issue for this.
It's my understanding that most servers will deal with the same kind of logic to remove sensitive information.
There was a problem hiding this comment.
created an issue modelcontextprotocol/go-sdk#748
|
I noticed that for instance on Would this be a follow up? |
|
Overall |
I probably missed that one, will fix |
pkg/mcplog/log.go
Outdated
| // SendMCPLog sends a log notification to the MCP client and server logs. | ||
| // Uses dedicated "mcp" named logger. Message is automatically sanitized. | ||
| // Level: "debug", "info", "notice", "warning", "error", "critical", "alert", "emergency" | ||
| func SendMCPLog(ctx context.Context, level, message string) { |
There was a problem hiding this comment.
Maybe we can use a specific type and some constants to make avoid errors
type Level int
const (
Error Level = iota
Critical
Alert
Emergency
)
For the levels, maybe reproduce whatever the protocol defines: https://modelcontextprotocol.io/specification/2025-11-25/server/utilities/logging#log-levels
pkg/mcplog/log.go
Outdated
| // This provides complete separation from server logs | ||
| mcpLogger logr.Logger = klog.NewKlogr().WithName("mcp") | ||
|
|
||
| // Patterns for redacting sensitive data from industry-standard secret detection tools |
There was a problem hiding this comment.
This comment states that these patterns are from industry-standard secret detection tools. It'd be better if we could include references for maintenance and future updates
There was a problem hiding this comment.
its not coming from one place, I kind of looked in multiple places plus did some research, so don't really have one reference to link here. will remove the comment so its not misleading, but will mention the sdk issue as a future replacement
| } | ||
| eventMap, err := kubernetes.NewCore(params).EventsList(params, namespace.(string)) | ||
| if err != nil { | ||
| mcplog.HandleK8sError(params.Context, err, "events listing") |
There was a problem hiding this comment.
This pattern seems brittle.
Implementors need to take care of handling the error which might be eventually forgotten.
As a follow up we should try to find a (decoupled) way of integrating the logging directly in the Kubernetes package or some other alternative that would not require the extra-handling for each tool function
…t redaction Add comprehensive MCP logging support to enable debug information flow to clients while maintaining security. Signed-off-by: Nader Ziada <nziada@redhat.com>
nader-ziada
force-pushed
the
mcp-logging
branch
from
96e263b to
e34389b
Compare
|
LGTM! |
4 participants
Add MCP logging support to enable debug information flow to clients while maintaining security.