From 017e7faa730ade9cb2ce5db805097dc18c9ab720 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 2 Apr 2026 05:47:47 +0000 Subject: [PATCH 1/3] Initial plan From e33c8d13bbad09e0792a631a7e8e9c1cdb914e94 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 2 Apr 2026 05:59:23 +0000 Subject: [PATCH 2/3] Fix logs forwarding: security, error handling, and observability improvements Agent-Logs-Url: https://github.com/interlink-hq/interLink/sessions/a4cec72d-3048-4b9f-a71a-721bcb5af05d Co-authored-by: dciangot <4144326+dciangot@users.noreply.github.com> --- pkg/interlink/api/func.go | 4 +- pkg/interlink/api/handler.go | 17 ++++---- pkg/interlink/api/logs.go | 75 ++++++++++++++++++++++++----------- pkg/virtualkubelet/execute.go | 12 +++--- 4 files changed, 67 insertions(+), 41 deletions(-) diff --git a/pkg/interlink/api/func.go b/pkg/interlink/api/func.go index 68c90cb4..7fffb78e 100644 --- a/pkg/interlink/api/func.go +++ b/pkg/interlink/api/func.go @@ -33,7 +33,7 @@ func getData(ctx context.Context, config types.Config, pod types.PodCreateReques for _, container := range pod.Pod.Spec.InitContainers { startContainer := time.Now().UnixMicro() - log.G(ctx).Info("- Retrieving Secrets and ConfigMaps for the Docker Sidecar. InitContainer: " + container.Name) + log.G(ctx).Info("- Retrieving Secrets and ConfigMaps for the Sidecar. InitContainer: " + container.Name) log.G(ctx).Debug(container.VolumeMounts) data, err := retrieveData(ctx, config, pod, container) if err != nil { @@ -50,7 +50,7 @@ func getData(ctx context.Context, config types.Config, pod types.PodCreateReques for _, container := range pod.Pod.Spec.Containers { startContainer := time.Now().UnixMicro() - log.G(ctx).Info("- Retrieving Secrets and ConfigMaps for the Docker Sidecar. Container: " + container.Name) + log.G(ctx).Info("- Retrieving Secrets and ConfigMaps for the Sidecar. Container: " + container.Name) log.G(ctx).Debug(container.VolumeMounts) data, err := retrieveData(ctx, config, pod, container) if err != nil { diff --git a/pkg/interlink/api/handler.go b/pkg/interlink/api/handler.go index 52d6c8c3..2a4fe6f7 100644 --- a/pkg/interlink/api/handler.go +++ b/pkg/interlink/api/handler.go @@ -11,7 +11,6 @@ import ( "io" "net/http" "net/url" - "strings" "github.com/containerd/containerd/log" "github.com/google/uuid" @@ -21,7 +20,13 @@ import ( "github.com/interlink-hq/interlink/pkg/interlink" ) -// isSafeURL checks for SSRF by allowing only http(s) URLs and blocking localhost/internal addresses. +// isSafeURL validates that a URL uses only http or https schemes. +// It blocks non-http(s) schemes (e.g. file://, ftp://) to prevent unexpected +// protocol usage. Localhost, loopback addresses, and private IP ranges are +// intentionally allowed because the sidecar plugin routinely runs on the same +// host or on internal/private network addresses in HPC and cluster environments. +// These URLs originate from trusted operator configuration (config files), +// not from user-controlled input, so private IPs are valid. func isSafeURL(rawurl string) bool { u, err := url.Parse(rawurl) if err != nil { @@ -30,10 +35,6 @@ func isSafeURL(rawurl string) bool { if u.Scheme != "http" && u.Scheme != "https" { return false } - host := u.Hostname() - if host == "localhost" || host == "127.0.0.1" || host == "::1" || strings.HasSuffix(host, ".internal") { - return false - } return true } @@ -117,10 +118,6 @@ func ReqWithError( // Add session number for end-to-end trace AddSessionContext(req, sessionContext) - if !isSafeURL(req.URL.String()) { - return nil, fmt.Errorf("potential SSRF detected: %s", req.URL.String()) - } - // SSRF protection: ensure URL is safe before making the request if !isSafeURL(req.URL.String()) { return nil, fmt.Errorf("potential SSRF detected: %s", req.URL.String()) } diff --git a/pkg/interlink/api/logs.go b/pkg/interlink/api/logs.go index 4183df94..903377a8 100644 --- a/pkg/interlink/api/logs.go +++ b/pkg/interlink/api/logs.go @@ -6,6 +6,7 @@ import ( "errors" "io" "net/http" + "regexp" "time" "github.com/containerd/containerd/log" @@ -17,6 +18,33 @@ import ( trace "go.opentelemetry.io/otel/trace" ) +// containerNameRegexp validates that a container name contains only safe characters. +// Kubernetes container names follow RFC 1123 subdomain rules: lowercase alphanumeric +// characters, '-', and must start and end with an alphanumeric character. +var containerNameRegexp = regexp.MustCompile(`^[a-z0-9][a-z0-9\-]*[a-z0-9]$|^[a-z0-9]$`) + +// namespaceRegexp validates Kubernetes namespace names (RFC 1123 label). +var namespaceRegexp = regexp.MustCompile(`^[a-z0-9][a-z0-9\-]*[a-z0-9]$|^[a-z0-9]$`) + +// podUIDRegexp validates Kubernetes pod UIDs (standard UUID format). +var podUIDRegexp = regexp.MustCompile(`^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$`) + +// validateLogRequest checks that the log request fields are well-formed and +// safe to use in file path construction. This prevents path traversal attacks +// when the sidecar plugin builds file paths from these values. +func validateLogRequest(req types.LogStruct) error { + if !namespaceRegexp.MatchString(req.Namespace) { + return errors.New("invalid namespace: must be a valid DNS label") + } + if !podUIDRegexp.MatchString(req.PodUID) { + return errors.New("invalid pod UID: must be a valid UUID") + } + if req.ContainerName != "" && !containerNameRegexp.MatchString(req.ContainerName) { + return errors.New("invalid container name: must be a valid DNS label") + } + return nil +} + // GetLogsHandler handles HTTP GET requests to retrieve container logs. // This endpoint streams container logs from the sidecar plugin to the client, // supporting various log retrieval options such as tailing, following, and filtering. @@ -30,7 +58,8 @@ import ( // // HTTP Status Codes: // - 200: Log retrieval successful (may be empty if no logs available) -// - 500: Internal server error (parameter conflicts, sidecar communication failures) +// - 400: Bad request (invalid or conflicting parameters) +// - 500: Internal server error (sidecar communication failures) func (h *InterLinkHandler) GetLogsHandler(w http.ResponseWriter, r *http.Request) { start := time.Now().UnixMicro() tracer := otel.Tracer("interlink-API") @@ -44,7 +73,6 @@ func (h *InterLinkHandler) GetLogsHandler(w http.ResponseWriter, r *http.Request sessionContext := GetSessionContext(r) sessionContextMessage := GetSessionContextMessage(sessionContext) - var statusCode int log.G(h.Ctx).Info(sessionContextMessage, "InterLink: received GetLogs call") bodyBytes, err := io.ReadAll(r.Body) if err != nil { @@ -55,8 +83,7 @@ func (h *InterLinkHandler) GetLogsHandler(w http.ResponseWriter, r *http.Request var req2 types.LogStruct // incoming request. To be used in interlink API. req is directly forwarded to sidecar err = json.Unmarshal(bodyBytes, &req2) if err != nil { - statusCode = http.StatusInternalServerError - w.WriteHeader(statusCode) + w.WriteHeader(http.StatusInternalServerError) log.G(h.Ctx).Error(sessionContextMessage, err) return } @@ -74,48 +101,48 @@ func (h *InterLinkHandler) GetLogsHandler(w http.ResponseWriter, r *http.Request ) log.G(h.Ctx).Info(sessionContextMessage, "InterLink: new GetLogs podUID: now ", req2.PodUID) - if (req2.Opts.Tail != 0 && req2.Opts.LimitBytes != 0) || (req2.Opts.SinceSeconds != 0 && !req2.Opts.SinceTime.IsZero()) { - statusCode = http.StatusInternalServerError - w.WriteHeader(statusCode) - - if req2.Opts.Tail != 0 && req2.Opts.LimitBytes != 0 { - _, err = w.Write([]byte("Both Tail and LimitBytes set. Set only one of them")) - if err != nil { - log.G(h.Ctx).Error(errors.New(sessionContextMessage + "Failed to write to http buffer")) - } - return + + if err := validateLogRequest(req2); err != nil { + w.WriteHeader(http.StatusBadRequest) + if _, werr := w.Write([]byte(err.Error())); werr != nil { + log.G(h.Ctx).Error(errors.New(sessionContextMessage + "Failed to write to http buffer")) } + return + } - _, err = w.Write([]byte("Both SinceSeconds and SinceTime set. Set only one of them")) - if err != nil { + if req2.Opts.Tail != 0 && req2.Opts.LimitBytes != 0 { + w.WriteHeader(http.StatusBadRequest) + if _, werr := w.Write([]byte("Both Tail and LimitBytes set. Set only one of them")); werr != nil { log.G(h.Ctx).Error(errors.New(sessionContextMessage + "Failed to write to http buffer")) } + return + } + if req2.Opts.SinceSeconds != 0 && !req2.Opts.SinceTime.IsZero() { + w.WriteHeader(http.StatusBadRequest) + if _, werr := w.Write([]byte("Both SinceSeconds and SinceTime set. Set only one of them")); werr != nil { + log.G(h.Ctx).Error(errors.New(sessionContextMessage + "Failed to write to http buffer")) + } + return } log.G(h.Ctx).Info(sessionContextMessage, "InterLink: marshal GetLogs request ") bodyBytes, err = json.Marshal(req2) if err != nil { - statusCode = http.StatusInternalServerError - w.WriteHeader(statusCode) + w.WriteHeader(http.StatusInternalServerError) log.G(h.Ctx).Error(err) return } reader := bytes.NewReader(bodyBytes) log.G(h.Ctx).Info("Sending log request to: ", h.SidecarEndpoint) - req, err := http.NewRequest(http.MethodGet, h.SidecarEndpoint+"/getLogs", reader) + req, err := http.NewRequestWithContext(r.Context(), http.MethodGet, h.SidecarEndpoint+"/getLogs", reader) if err != nil { log.G(h.Ctx).Fatal(err) } req.Header.Set("Content-Type", "application/json") - // logTransport := http.DefaultTransport.(*http.Transport).Clone() - // // logTransport.DisableKeepAlives = true - // // logTransport.MaxIdleConnsPerHost = -1 - // var logHTTPClient = &http.Client{Transport: logTransport} - log.G(h.Ctx).Info(sessionContextMessage, "InterLink: forwarding GetLogs call to sidecar") _, err = ReqWithError(h.Ctx, req, w, start, span, true, false, sessionContext, h.ClientHTTP) if err != nil { diff --git a/pkg/virtualkubelet/execute.go b/pkg/virtualkubelet/execute.go index 72459dec..40bc515a 100644 --- a/pkg/virtualkubelet/execute.go +++ b/pkg/virtualkubelet/execute.go @@ -31,7 +31,13 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -// isSafeURL checks for SSRF by allowing only http(s) URLs and blocking localhost/internal addresses. +// isSafeURL validates that a URL uses only http or https schemes. +// It blocks non-http(s) schemes (e.g. file://, ftp://) to prevent unexpected +// protocol usage. Localhost, loopback addresses, and private IP ranges are +// intentionally allowed because both the interLink API and sidecar plugin +// routinely run on internal/private network addresses in HPC and cluster +// environments. These URLs originate from trusted operator configuration +// (config files), not from user-controlled input, so private IPs are valid. func isSafeURL(rawurl string) bool { u, err := url.Parse(rawurl) if err != nil { @@ -40,10 +46,6 @@ func isSafeURL(rawurl string) bool { if u.Scheme != "http" && u.Scheme != "https" { return false } - host := u.Hostname() - if host == "localhost" || host == "127.0.0.1" || host == "::1" || strings.HasSuffix(host, ".internal") { - return false - } return true } From ac1f36490707c9058baf9efa2557c62ab944669b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 2 Apr 2026 06:00:43 +0000 Subject: [PATCH 3/3] Add tests for validateLogRequest, GetLogsHandler validation, and isSafeURL Agent-Logs-Url: https://github.com/interlink-hq/interLink/sessions/a4cec72d-3048-4b9f-a71a-721bcb5af05d Co-authored-by: dciangot <4144326+dciangot@users.noreply.github.com> --- pkg/interlink/api/logs_test.go | 305 +++++++++++++++++++++++++++++++++ 1 file changed, 305 insertions(+) create mode 100644 pkg/interlink/api/logs_test.go diff --git a/pkg/interlink/api/logs_test.go b/pkg/interlink/api/logs_test.go new file mode 100644 index 00000000..749b570f --- /dev/null +++ b/pkg/interlink/api/logs_test.go @@ -0,0 +1,305 @@ +package api + +import ( + "bytes" + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/sdk/trace" + "go.opentelemetry.io/otel/sdk/trace/tracetest" + + types "github.com/interlink-hq/interlink/pkg/interlink" +) + +func setupLogsTestTracer() (*trace.TracerProvider, func()) { + exporter := tracetest.NewInMemoryExporter() + tp := trace.NewTracerProvider( + trace.WithSyncer(exporter), + ) + otel.SetTracerProvider(tp) + cleanup := func() { + if err := tp.Shutdown(context.Background()); err != nil { + panic(err) + } + } + return tp, cleanup +} + +func TestValidateLogRequest(t *testing.T) { + tests := []struct { + name string + req types.LogStruct + wantErr bool + errSubstr string + }{ + { + name: "valid request", + req: types.LogStruct{ + Namespace: "default", + PodUID: "12345678-1234-1234-1234-123456789012", + ContainerName: "my-container", + }, + wantErr: false, + }, + { + name: "valid request without container name", + req: types.LogStruct{ + Namespace: "kube-system", + PodUID: "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee", + }, + wantErr: false, + }, + { + name: "single character namespace", + req: types.LogStruct{ + Namespace: "a", + PodUID: "12345678-1234-1234-1234-123456789012", + }, + wantErr: false, + }, + { + name: "invalid namespace - contains path separator", + req: types.LogStruct{ + Namespace: "default/../../etc", + PodUID: "12345678-1234-1234-1234-123456789012", + }, + wantErr: true, + errSubstr: "invalid namespace", + }, + { + name: "invalid namespace - contains uppercase", + req: types.LogStruct{ + Namespace: "Default", + PodUID: "12345678-1234-1234-1234-123456789012", + }, + wantErr: true, + errSubstr: "invalid namespace", + }, + { + name: "invalid namespace - starts with hyphen", + req: types.LogStruct{ + Namespace: "-default", + PodUID: "12345678-1234-1234-1234-123456789012", + }, + wantErr: true, + errSubstr: "invalid namespace", + }, + { + name: "invalid pod UID - not UUID format", + req: types.LogStruct{ + Namespace: "default", + PodUID: "not-a-uuid", + }, + wantErr: true, + errSubstr: "invalid pod UID", + }, + { + name: "invalid pod UID - path traversal", + req: types.LogStruct{ + Namespace: "default", + PodUID: "../../etc/passwd", + }, + wantErr: true, + errSubstr: "invalid pod UID", + }, + { + name: "invalid container name - contains path separator", + req: types.LogStruct{ + Namespace: "default", + PodUID: "12345678-1234-1234-1234-123456789012", + ContainerName: "container/../../../etc/passwd", + }, + wantErr: true, + errSubstr: "invalid container name", + }, + { + name: "invalid container name - contains uppercase", + req: types.LogStruct{ + Namespace: "default", + PodUID: "12345678-1234-1234-1234-123456789012", + ContainerName: "MyContainer", + }, + wantErr: true, + errSubstr: "invalid container name", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateLogRequest(tt.req) + if tt.wantErr { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.errSubstr) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestGetLogsHandler_Validation(t *testing.T) { + _, cleanup := setupLogsTestTracer() + defer cleanup() + + // Create a mock sidecar that returns OK + sidecar := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("log output")) + })) + defer sidecar.Close() + + handler := &InterLinkHandler{ + Ctx: context.Background(), + SidecarEndpoint: sidecar.URL, + ClientHTTP: sidecar.Client(), + } + + tests := []struct { + name string + req types.LogStruct + expectedStatus int + expectedBody string + }{ + { + name: "valid request forwarded to sidecar", + req: types.LogStruct{ + Namespace: "default", + PodUID: "12345678-1234-1234-1234-123456789012", + ContainerName: "my-container", + PodName: "my-pod", + }, + expectedStatus: http.StatusOK, + }, + { + name: "invalid namespace rejected", + req: types.LogStruct{ + Namespace: "../etc", + PodUID: "12345678-1234-1234-1234-123456789012", + ContainerName: "my-container", + }, + expectedStatus: http.StatusBadRequest, + expectedBody: "invalid namespace", + }, + { + name: "invalid pod UID rejected", + req: types.LogStruct{ + Namespace: "default", + PodUID: "../../etc/passwd", + ContainerName: "my-container", + }, + expectedStatus: http.StatusBadRequest, + expectedBody: "invalid pod UID", + }, + { + name: "invalid container name rejected", + req: types.LogStruct{ + Namespace: "default", + PodUID: "12345678-1234-1234-1234-123456789012", + ContainerName: "../../../etc", + }, + expectedStatus: http.StatusBadRequest, + expectedBody: "invalid container name", + }, + { + name: "both Tail and LimitBytes set", + req: types.LogStruct{ + Namespace: "default", + PodUID: "12345678-1234-1234-1234-123456789012", + Opts: types.ContainerLogOpts{ + Tail: 10, + LimitBytes: 1024, + }, + }, + expectedStatus: http.StatusBadRequest, + expectedBody: "Both Tail and LimitBytes set", + }, + { + name: "both SinceSeconds and SinceTime set", + req: types.LogStruct{ + Namespace: "default", + PodUID: "12345678-1234-1234-1234-123456789012", + Opts: types.ContainerLogOpts{ + SinceSeconds: 60, + SinceTime: time.Now(), + }, + }, + expectedStatus: http.StatusBadRequest, + expectedBody: "Both SinceSeconds and SinceTime set", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + body, err := json.Marshal(tt.req) + require.NoError(t, err) + + r := httptest.NewRequest(http.MethodGet, "/getLogs", bytes.NewReader(body)) + w := httptest.NewRecorder() + + handler.GetLogsHandler(w, r) + + assert.Equal(t, tt.expectedStatus, w.Code) + if tt.expectedBody != "" { + assert.Contains(t, w.Body.String(), tt.expectedBody) + } + }) + } +} + +func TestIsSafeURL(t *testing.T) { + tests := []struct { + name string + url string + expected bool + }{ + { + name: "valid http URL", + url: "http://example.com/path", + expected: true, + }, + { + name: "valid https URL", + url: "https://example.com/path", + expected: true, + }, + { + name: "localhost http allowed (valid sidecar endpoint)", + url: "http://localhost:8080", + expected: true, + }, + { + name: "127.0.0.1 http allowed (valid sidecar endpoint)", + url: "http://127.0.0.1:8080", + expected: true, + }, + { + name: "file scheme blocked", + url: "file:///etc/passwd", + expected: false, + }, + { + name: "ftp scheme blocked", + url: "ftp://example.com", + expected: false, + }, + { + name: "invalid URL blocked", + url: "not-a-url", + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := isSafeURL(tt.url) + assert.Equal(t, tt.expected, result) + }) + } +}