You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "Fine0830 (via GitHub)" <gi...@apache.org> on 2026/03/31 10:48:03 UTC
[PR] feat: implement TLS certificate verification and add new flags [skywalking-mcp]
Fine0830 opened a new pull request, #40:
URL: https://github.com/apache/skywalking-mcp/pull/40
TLS certificate verification is now enforced for OAP connections. Added `--sw-insecure` flag to opt out (development/self-signed certs only).
* Sensitive fields (`authorization`, `password`, `token`, `secret`) are redacted in `--log-command` output.
* Environment variable references (`${VAR}`) in `--sw-username`/`--sw-password` now log a warning when the variable is not set, preventing silent unauthenticated requests.
* URL scheme validation rejects non-http/https OAP URLs.
* Regex patterns supplied to `list_mqe_metrics` are validated for complexity before compilation.
* Added `--allowed-origins` flag to `sse` and `streamable` transports for CORS/CSRF origin enforcement. Requests carrying an `Origin` header not in the allowlist receive `403 Forbidden`. When unset, all origins are permitted (backward compatible).
Signed-off-by: Qiuxia Fan <qi...@apache.org>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: implement TLS certificate verification and add new flags [skywalking-mcp]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng merged PR #40:
URL: https://github.com/apache/skywalking-mcp/pull/40
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: implement TLS certificate verification and add new flags [skywalking-mcp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #40:
URL: https://github.com/apache/skywalking-mcp/pull/40#discussion_r3015221713
##########
internal/tools/mqe.go:
##########
@@ -105,7 +122,12 @@ func executeGraphQLWithContext(ctx context.Context, query string, variables map[
req.Header.Set("Authorization", auth)
}
- client := &http.Client{Timeout: 30 * time.Second}
+ insecure := getContextBool(ctx, contextkey.Insecure{})
+ //nolint:gosec // InsecureSkipVerify is intentional and controlled by the --sw-insecure operator flag
+ transport := &http.Transport{
+ TLSClientConfig: &tls.Config{InsecureSkipVerify: insecure},
+ }
Review Comment:
The custom `http.Transport` here is created with only `TLSClientConfig` set, which drops important defaults from `http.DefaultTransport` (proxy-from-env support, dial/idle timeouts, keep-alives, HTTP/2 attempt, etc.). This can cause unexpected behavior/regressions in real deployments. Prefer cloning `http.DefaultTransport` (or using `(&http.Transport{}).Clone()` from an existing transport) and only overriding `TLSClientConfig` / `InsecureSkipVerify` as needed.
```suggestion
var transport *http.Transport
if defaultTransport, ok := http.DefaultTransport.(*http.Transport); ok {
transport = defaultTransport.Clone()
} else {
transport = &http.Transport{}
}
if transport.TLSClientConfig == nil {
transport.TLSClientConfig = &tls.Config{}
}
transport.TLSClientConfig.InsecureSkipVerify = insecure
```
##########
internal/tools/mqe.go:
##########
@@ -512,9 +539,44 @@ func validateMQEMetricsListRequest(req *MQEMetricsListRequest) error {
if err := validateMQETextField("regex", req.Regex, maxMQERegexLength); err != nil {
return err
}
- if _, err := regexp.Compile(req.Regex); err != nil {
+ if err := validateRegexComplexity(req.Regex); err != nil {
+ return err
+ }
+ return nil
+}
+
+const maxRegexNodes = 50
+
+// validateRegexComplexity rejects patterns with excessive AST node counts.
+func validateRegexComplexity(pattern string) error {
+ re, err := syntax.Parse(pattern, syntax.Perl)
+ if err != nil {
return fmt.Errorf("regex is invalid")
Review Comment:
`validateRegexComplexity` discards the underlying parse error details and always returns a generic "regex is invalid" message. Including the original error (wrapped) would make it much easier for users to correct bad patterns while still being safe.
```suggestion
return fmt.Errorf("regex is invalid: %w", err)
```
##########
internal/tools/io.go:
##########
@@ -47,7 +56,7 @@ func (l *IOLogger) Read(p []byte) (n int, err error) {
}
n, err = l.reader.Read(p)
if n > 0 {
- l.logger.Infof("[stdin]: received %d bytes: %s", n, string(p[:n]))
+ l.logger.Infof("[stdin]: received %d bytes: %s", n, redactSensitiveData(string(p[:n])))
}
Review Comment:
Redacting sensitive fields on each raw `Read()` chunk is not reliable: JSON-RPC messages can be split across multiple reads, so parts of a secret value may be logged before the regex has enough context to match (leaking data). To make the redaction guarantee accurate, consider buffering to message/line boundaries (e.g., newline-delimited JSON) or parsing JSON and re-encoding with redacted fields before logging.
##########
internal/tools/common_test.go:
##########
@@ -0,0 +1,133 @@
+// Licensed to Apache Software Foundation (ASF) under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Apache Software Foundation (ASF) licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package tools
+
+import (
+ "testing"
+ "time"
+
+ api "skywalking.apache.org/repo/goapi/query"
+)
+
+func TestFinalizeURL(t *testing.T) {
+ tests := []struct {
+ name string
+ in string
+ want string
+ }{
+ {name: "adds graphql suffix", in: "http://localhost:12800", want: "http://localhost:12800/graphql"},
+ {name: "trims trailing slash", in: "http://localhost:12800/", want: "http://localhost:12800/graphql"},
+ {name: "keeps existing graphql", in: "http://localhost:12800/graphql", want: "http://localhost:12800/graphql"},
+ }
+
+ for _, tc := range tests {
+ t.Run(tc.name, func(t *testing.T) {
+ if got := FinalizeURL(tc.in); got != tc.want {
+ t.Fatalf("FinalizeURL(%q) = %q, want %q", tc.in, got, tc.want)
+ }
+ })
+ }
+}
+
+func TestParseTimezoneOffset(t *testing.T) {
+ loc, ok := parseTimezoneOffset("+0830")
+ if !ok {
+ t.Fatal("expected timezone to parse")
+ }
+ if got := loc.String(); got != "+0830" {
+ t.Fatalf("location name = %q", got)
+ }
+
+ _, ok = parseTimezoneOffset("UTC")
+ if ok {
+ t.Fatal("expected invalid timezone offset to fail")
+ }
+}
+
+func TestParseDurationWithContextRelativeDuration(t *testing.T) {
+ now := time.Date(2026, 3, 31, 12, 0, 0, 0, time.UTC)
+ timeCtx := TimeContext{NowUTC: now, Location: time.UTC}
+
+ got := ParseDurationWithContext("-2h", false, timeCtx)
+
+ if got.Start != testTimeStart {
+ t.Fatalf("start = %q", got.Start)
+ }
+ if got.End != testTimeEnd {
Review Comment:
These tests rely on `testTimeStart`/`testTimeEnd` constants defined in a different *_test.go file (`alarm_test.go`). This creates hidden coupling between test files and makes failures harder to diagnose. Prefer defining the expected values locally in this test file (or a shared helper in a non-test file if truly shared).
```suggestion
// Expected window for a -2h relative duration from 2026-03-31 12:00 UTC.
const expectedStart = "2026-03-31 10:00"
const expectedEnd = "2026-03-31 12:00"
got := ParseDurationWithContext("-2h", false, timeCtx)
if got.Start != expectedStart {
t.Fatalf("start = %q", got.Start)
}
if got.End != expectedEnd {
```
##########
CHANGES.md:
##########
@@ -2,6 +2,20 @@
Release Notes.
+## 0.2.0
+
+### Features
+
+* TLS certificate verification is now enforced for OAP connections. Added `--sw-insecure` flag to opt out (development/self-signed certs only).
+* Sensitive fields (`authorization`, `password`, `token`, `secret`) are redacted in `--log-command` output.
+* Environment variable references (`${VAR}`) in `--sw-username`/`--sw-password` now log a warning when the variable is not set, preventing silent unauthenticated requests.
+* URL scheme validation rejects non-http/https OAP URLs.
+* Regex patterns supplied to `list_mqe_metrics` are validated for complexity before compilation.
+* Added `--allowed-origins` flag to `sse` and `streamable` transports for CORS/CSRF origin enforcement. Requests carrying an `Origin` header not in the allowlist receive `403 Forbidden`. When unset, all origins are permitted (backward compatible).
+* Implement unit tests.
+* Remove the unnecessary tool and parameter.
+* Validate properties for tools.
Review Comment:
The last three bullets in the 0.2.0 feature list are very generic ("Implement unit tests", "Remove the unnecessary tool and parameter", "Validate properties for tools") and don’t describe user-visible behavior. Consider removing them or replacing them with concrete, user-facing release notes tied to the actual changes in this version.
```suggestion
* Increased reliability of core CLI commands through expanded automated test coverage.
* Removed an unused CLI tool and its associated parameter to simplify the interface and avoid confusion.
* Added validation for tool configuration properties, returning clear errors when required values are missing or invalid.
```
##########
internal/tools/io.go:
##########
@@ -57,6 +66,6 @@ func (l *IOLogger) Write(p []byte) (n int, err error) {
if l.writer == nil {
return 0, io.ErrClosedPipe
}
- l.logger.Infof("[stdout]: sending %d bytes: %s", len(p), string(p))
+ l.logger.Infof("[stdout]: sending %d bytes: %s", len(p), redactSensitiveData(string(p)))
return l.writer.Write(p)
Review Comment:
Same issue as `Read()`: `Write()` may emit partial frames depending on the upstream writer, so redaction-by-regex on a single chunk can still leak secrets. If `--log-command` is expected to guarantee redaction, logging should happen at JSON-RPC message boundaries or after JSON parsing/redaction.
##########
README.md:
##########
@@ -44,8 +44,19 @@ Flags:
--sw-url string Specify the OAP URL to connect to (e.g. http://localhost:12800)
--sw-username string Username for basic auth to SkyWalking OAP (supports ${ENV_VAR} syntax)
--sw-password string Password for basic auth to SkyWalking OAP (supports ${ENV_VAR} syntax)
+ --sw-insecure Skip TLS certificate verification for OAP connections (use only in development)
-v, --version version for swmcp
+SSE-specific Flags:
+ --sse-address string Host and port for the SSE server (default "localhost:8000")
+ --base-path string Base path for the SSE server
+ --allowed-origins string Comma-separated list of allowed CORS origins. Empty permits all.
+
+Streamable-specific Flags:
+ --address string Host and port for the Streamable HTTP server (default "localhost:8000")
+ --endpoint-path string Endpoint path for the Streamable HTTP server (default "/mcp")
+ --allowed-origins string Comma-separated list of allowed CORS origins. Empty permits all.
Review Comment:
The flag help text says "Empty permits all", but with the current middleware an empty allowlist does not emit any `Access-Control-Allow-*` headers; browsers will still block cross-origin access. Either clarify the wording (e.g., "Empty disables origin enforcement" and document using `*` to allow all origins with CORS headers) or adjust the middleware behavior to reflect the `Origin` when the list is empty.
```suggestion
--allowed-origins string Comma-separated list of allowed CORS origins. Empty disables origin enforcement; use * to allow all origins.
Streamable-specific Flags:
--address string Host and port for the Streamable HTTP server (default "localhost:8000")
--endpoint-path string Endpoint path for the Streamable HTTP server (default "/mcp")
--allowed-origins string Comma-separated list of allowed CORS origins. Empty disables origin enforcement; use * to allow all origins.
```
##########
internal/swmcp/streamable.go:
##########
@@ -48,21 +50,39 @@ func NewStreamable() *cobra.Command {
"The host and port to start the Streamable server on")
streamableCmd.Flags().String("endpoint-path", "/mcp",
"The path for the streamable-http server")
+ streamableCmd.Flags().String("allowed-origins", "",
+ "Comma-separated list of allowed CORS origins (e.g. http://localhost:3000,https://app.example.com). Empty allows all.")
Review Comment:
This flag description implies an empty value "allows all" for CORS, but when `allowedOrigins` is empty the middleware does not set any `Access-Control-Allow-*` headers (so browsers still block cross-origin). Please clarify the help text (or change behavior) so users understand whether empty means "no enforcement" vs "CORS enabled for all".
```suggestion
"Comma-separated list of allowed CORS origins (e.g. http://localhost:3000,https://app.example.com). Empty disables CORS (no Access-Control-Allow-* headers are set).")
```
##########
internal/swmcp/server.go:
##########
@@ -130,7 +135,7 @@ func withConfiguredAuth(ctx context.Context) context.Context {
// with SkyWalking settings from the global configuration.
func EnhanceStdioContextFunc() server.StdioContextFunc {
return func(ctx context.Context) context.Context {
- ctx = WithSkyWalkingURLAndInsecure(ctx, configuredSkyWalkingURL(), false)
+ ctx = WithSkyWalkingURLAndInsecure(ctx, configuredSkyWalkingURL(), viper.GetBool("insecure"))
ctx = withConfiguredAuth(ctx)
return ctx
Review Comment:
The new `--sw-insecure` flag wiring relies on these context enrichers, but there’s no unit test asserting that `contextkey.Insecure{}` is set correctly when `viper.Set("insecure", true)` (and false by default). Adding a focused test would prevent regressions where the CLI flag is bound but not actually propagated to HTTP clients.
##########
internal/swmcp/sse.go:
##########
@@ -55,8 +55,11 @@ func NewSSEServer() *cobra.Command {
"The host and port to start the sse server on")
sseCmd.Flags().String("base-path", "",
"Base path for the sse server")
+ sseCmd.Flags().String("allowed-origins", "",
+ "Comma-separated list of allowed CORS origins (e.g. http://localhost:3000,https://app.example.com). Empty allows all.")
Review Comment:
This flag description implies an empty value "allows all" for CORS, but when `allowedOrigins` is empty the middleware does not set any `Access-Control-Allow-*` headers (so browsers still block cross-origin). Please clarify the help text (or change behavior) so users understand whether empty means "no enforcement" vs "CORS enabled for all".
```suggestion
"Comma-separated list of allowed CORS origins (e.g. http://localhost:3000,https://app.example.com). Empty disables CORS (no Access-Control-Allow-* headers are set).")
```
##########
internal/swmcp/cors.go:
##########
@@ -0,0 +1,75 @@
+// Licensed to Apache Software Foundation (ASF) under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Apache Software Foundation (ASF) licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package swmcp
+
+import (
+ "net/http"
+ "strings"
+)
+
+// corsMiddleware adds CORS response headers and enforces origin validation.
+// When allowedOrigins is empty, all origins are permitted (no restriction).
+// When populated, only listed origins receive CORS headers; requests from
+// any other origin with an Origin header receive 403 Forbidden.
+func corsMiddleware(allowedOrigins []string, next http.Handler) http.Handler {
+ return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ origin := r.Header.Get("Origin")
+ if origin != "" {
+ if isOriginAllowed(origin, allowedOrigins) {
+ w.Header().Set("Access-Control-Allow-Origin", origin)
+ w.Header().Set("Access-Control-Allow-Methods", "GET, POST, DELETE, OPTIONS")
+ w.Header().Set("Access-Control-Allow-Headers", "Content-Type, Authorization, Accept")
+ w.Header().Set("Vary", "Origin")
+ } else if len(allowedOrigins) > 0 {
Review Comment:
This introduces security-sensitive origin enforcement behavior, but there are no tests covering allowed vs. disallowed `Origin` values and preflight (`OPTIONS`) handling. Adding unit tests for `corsMiddleware` (including the empty allowlist behavior) would help prevent accidental loosening/tightening of CORS/CSRF checks.
```suggestion
// When allowedOrigins is empty, treat it as "allow all origins" for CORS purposes.
if len(allowedOrigins) == 0 || isOriginAllowed(origin, allowedOrigins) {
w.Header().Set("Access-Control-Allow-Origin", origin)
w.Header().Set("Access-Control-Allow-Methods", "GET, POST, DELETE, OPTIONS")
w.Header().Set("Access-Control-Allow-Headers", "Content-Type, Authorization, Accept")
w.Header().Set("Vary", "Origin")
} else {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org