From e22cc36e7435e386556b405337e027510a0f18a1 Mon Sep 17 00:00:00 2001 From: mesutoezdil Date: Fri, 17 Jul 2026 09:31:40 +0200 Subject: [PATCH 1/3] fix(nvca): stop logging ReVal request credentials Render() logged the full HelmReValRenderInput at V(2), including APIKey, HelmRegistryAuthConfig, and ImageRegistryAuthConfig, all of which carry credentials for the ReVal service and image/chart registries. Log a safe subset of fields instead. The struct is still marshaled as-is into the actual HTTP request body, which is correct; only the debug log line changed. Verified with go build, go vet, go test, and golangci-lint --enable-only gosec, all clean. NO-REF Signed-off-by: mesutoezdil --- .../nvca/internal/miniservice/reval_client.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/compute-plane-services/nvca/internal/miniservice/reval_client.go b/src/compute-plane-services/nvca/internal/miniservice/reval_client.go index 6f10aac1c..0dd66b5bc 100644 --- a/src/compute-plane-services/nvca/internal/miniservice/reval_client.go +++ b/src/compute-plane-services/nvca/internal/miniservice/reval_client.go @@ -92,7 +92,15 @@ func (c *revalClient) Render(ctx context.Context, input HelmReValRenderInput) (H ) log.V(1).Info("Do ReVal request") - log.V(2).WithValues("input", input).Info("Payload") + // input.APIKey, input.HelmRegistryAuthConfig, and input.ImageRegistryAuthConfig + // carry credentials and must not be logged. + log.V(2).WithValues( + "helmChart", input.HelmChartURL, + "releaseName", input.ReleaseName, + "instanceType", input.InstanceType, + "gpu", input.GPUName, + "k8sVersion", input.K8sVersion, + ).Info("Payload") // httpCode tracks the label value for the metric. It defaults to "error" (network failure), // is set to stage-specific values on pre-call failures, and to the HTTP status code on success. @@ -111,6 +119,7 @@ func (c *revalClient) Render(ctx context.Context, input HelmReValRenderInput) (H return HelmReValRenderOutput{}, err } + //nolint:gosec // input.APIKey belongs in this request body, sent to the ReVal service itself payload, err := json.Marshal(input) if err != nil { httpCode = "marshal_error" From c0a9689433e90ba902ef51b63195c7edf070e963 Mon Sep 17 00:00:00 2001 From: mesutoezdil Date: Fri, 17 Jul 2026 10:09:00 +0200 Subject: [PATCH 2/3] fix(nvca): redact HelmChartURL in ReVal payload log, add tests Address CodeRabbit review on this PR: - HelmChartURL can carry userinfo or a signed query string, so strip both before logging it, keeping only scheme/host/path. - Add TestReValClientPayloadLogRedactsCredentials, which renders with a chart URL, API key, and registry auth all containing distinct secret markers, captures the V(2) log output via a funcr logger, and asserts none of the secret markers appear while the safe fields do. - Add table tests for the new redactedHelmChartURL helper. Updated internal/miniservice/BUILD.bazel with the new go-logr/logr/funcr test dep. NO-REF Signed-off-by: mesutoezdil --- .../nvca/internal/miniservice/BUILD.bazel | 1 + .../nvca/internal/miniservice/reval_client.go | 19 ++++- .../internal/miniservice/reval_client_test.go | 82 +++++++++++++++++++ 3 files changed, 100 insertions(+), 2 deletions(-) diff --git a/src/compute-plane-services/nvca/internal/miniservice/BUILD.bazel b/src/compute-plane-services/nvca/internal/miniservice/BUILD.bazel index 61aafe0e8..fb8dce6c8 100644 --- a/src/compute-plane-services/nvca/internal/miniservice/BUILD.bazel +++ b/src/compute-plane-services/nvca/internal/miniservice/BUILD.bazel @@ -179,6 +179,7 @@ go_test( "//vendor/github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/icms-translate/translate/task", "//vendor/github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/types/nvca/config", "//vendor/github.com/bombsimon/logrusr/v4:logrusr", + "//vendor/github.com/go-logr/logr/funcr", "//vendor/github.com/google/go-cmp/cmp", "//vendor/github.com/prometheus/client_golang/prometheus", "//vendor/github.com/prometheus/client_model/go", diff --git a/src/compute-plane-services/nvca/internal/miniservice/reval_client.go b/src/compute-plane-services/nvca/internal/miniservice/reval_client.go index 0dd66b5bc..2b0d43dfa 100644 --- a/src/compute-plane-services/nvca/internal/miniservice/reval_client.go +++ b/src/compute-plane-services/nvca/internal/miniservice/reval_client.go @@ -25,6 +25,7 @@ import ( "fmt" "io" "net/http" + neturl "net/url" "github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/core" "github.com/hashicorp/go-retryablehttp" @@ -43,6 +44,19 @@ type tokenFetcher interface { FetchToken(ctx context.Context) (string, error) } +// redactedHelmChartURL strips userinfo and the query string from a Helm +// chart URL so it is safe to log; neither is needed to identify the chart. +func redactedHelmChartURL(raw string) string { + u, err := neturl.Parse(raw) + if err != nil { + return "" + } + u.User = nil + u.RawQuery = "" + u.Fragment = "" + return u.String() +} + type revalClient struct { endpoint string host string @@ -93,9 +107,10 @@ func (c *revalClient) Render(ctx context.Context, input HelmReValRenderInput) (H log.V(1).Info("Do ReVal request") // input.APIKey, input.HelmRegistryAuthConfig, and input.ImageRegistryAuthConfig - // carry credentials and must not be logged. + // carry credentials and must not be logged. HelmChartURL is redacted too, in + // case it embeds userinfo or a signed query string. log.V(2).WithValues( - "helmChart", input.HelmChartURL, + "helmChart", redactedHelmChartURL(input.HelmChartURL), "releaseName", input.ReleaseName, "instanceType", input.InstanceType, "gpu", input.GPUName, diff --git a/src/compute-plane-services/nvca/internal/miniservice/reval_client_test.go b/src/compute-plane-services/nvca/internal/miniservice/reval_client_test.go index f22d34449..b00375bba 100644 --- a/src/compute-plane-services/nvca/internal/miniservice/reval_client_test.go +++ b/src/compute-plane-services/nvca/internal/miniservice/reval_client_test.go @@ -26,11 +26,14 @@ import ( "strings" "testing" + "github.com/go-logr/logr/funcr" "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + logf "sigs.k8s.io/controller-runtime/pkg/log" "github.com/NVIDIA/nvcf/src/compute-plane-services/nvca/internal/metrics" + "github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/icms-translate/translate/common" ) func TestReValClientHostHeader(t *testing.T) { @@ -82,6 +85,85 @@ func TestReValClientHostHeader(t *testing.T) { } } +func TestReValClientPayloadLogRedactsCredentials(t *testing.T) { + const ( + apiKey = "super-secret-api-key" + registryAuth = "leaked-registry-auth" + urlPassword = "hunter2" + signedToken = "leak-me" + wantHelmChart = "https://charts.example.invalid/chart.tgz" + ) + + var lines []string + logger := funcr.New(func(prefix, args string) { + lines = append(lines, prefix+" "+args) + }, funcr.Options{Verbosity: 2}) + ctx := logf.IntoContext(t.Context(), logger) + + httpClient := &http.Client{ + Transport: roundTripFunc(func(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + Header: make(http.Header), + Body: io.NopCloser(strings.NewReader(`{"valid":true}`)), + Request: req, + }, nil + }), + } + + client := NewReValClient("http://reval.example.invalid", testTokenFetcher{}, httpClient, nil) + _, err := client.Render(ctx, HelmReValRenderInput{ + NCAID: "test-nca", + HelmChartURL: fmt.Sprintf("https://user:%s@charts.example.invalid/chart.tgz?token=%s", urlPassword, signedToken), + ReleaseName: "my-release", + APIKey: apiKey, + HelmRegistryAuthConfig: common.RegistryAuthConfig{ + K8sSecrets: []common.RegistryAuthSecret{{ + Auths: map[string]common.RegistryAuth{"registry.example.invalid": {Auth: registryAuth}}, + }}, + }, + }) + require.NoError(t, err) + + output := strings.Join(lines, "\n") + assert.NotContains(t, output, apiKey) + assert.NotContains(t, output, registryAuth) + assert.NotContains(t, output, urlPassword) + assert.NotContains(t, output, signedToken) + + assert.Contains(t, output, "my-release") + assert.Contains(t, output, wantHelmChart) +} + +func TestRedactedHelmChartURL(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + { + name: "strips userinfo and query", + in: "https://user:pass@charts.example.invalid/chart.tgz?token=secret", + want: "https://charts.example.invalid/chart.tgz", + }, + { + name: "leaves a plain URL unchanged", + in: "oci://registry.example.invalid/charts/foo:1.0.0", + want: "oci://registry.example.invalid/charts/foo:1.0.0", + }, + { + name: "falls back on unparseable input", + in: "://not a url", + want: "", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, redactedHelmChartURL(tt.in)) + }) + } +} + type roundTripFunc func(req *http.Request) (*http.Response, error) func (f roundTripFunc) RoundTrip(req *http.Request) (*http.Response, error) { From 4eb7d197e1ab4b62027e43a4e788a354f9fc4592 Mon Sep 17 00:00:00 2001 From: mesutoezdil Date: Fri, 17 Jul 2026 18:24:48 +0200 Subject: [PATCH 3/3] test(nvca): cover ImageRegistryAuthConfig in ReVal payload redaction test Extend the existing credential-redaction regression test to also set ImageRegistryAuthConfig (previously only HelmRegistryAuthConfig was covered) and assert the retained metadata fields, closing the gap CodeRabbit flagged on PR #229. JIRA: NO-REF NVBug: none Signed-off-by: mesutoezdil --- .../internal/miniservice/reval_client_test.go | 29 ++++++++++++++----- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/src/compute-plane-services/nvca/internal/miniservice/reval_client_test.go b/src/compute-plane-services/nvca/internal/miniservice/reval_client_test.go index b00375bba..2107238c4 100644 --- a/src/compute-plane-services/nvca/internal/miniservice/reval_client_test.go +++ b/src/compute-plane-services/nvca/internal/miniservice/reval_client_test.go @@ -87,11 +87,12 @@ func TestReValClientHostHeader(t *testing.T) { func TestReValClientPayloadLogRedactsCredentials(t *testing.T) { const ( - apiKey = "super-secret-api-key" - registryAuth = "leaked-registry-auth" - urlPassword = "hunter2" - signedToken = "leak-me" - wantHelmChart = "https://charts.example.invalid/chart.tgz" + apiKey = "super-secret-api-key" + helmRegistryAuth = "leaked-helm-registry-auth" + imageRegistryAuth = "leaked-image-registry-auth" + urlPassword = "hunter2" + signedToken = "leak-me" + wantHelmChart = "https://charts.example.invalid/chart.tgz" ) var lines []string @@ -116,22 +117,36 @@ func TestReValClientPayloadLogRedactsCredentials(t *testing.T) { NCAID: "test-nca", HelmChartURL: fmt.Sprintf("https://user:%s@charts.example.invalid/chart.tgz?token=%s", urlPassword, signedToken), ReleaseName: "my-release", + InstanceType: "gpu.small", + GPUName: "A100", + K8sVersion: "1.30", APIKey: apiKey, HelmRegistryAuthConfig: common.RegistryAuthConfig{ K8sSecrets: []common.RegistryAuthSecret{{ - Auths: map[string]common.RegistryAuth{"registry.example.invalid": {Auth: registryAuth}}, + Auths: map[string]common.RegistryAuth{"registry.example.invalid": {Auth: helmRegistryAuth}}, + }}, + }, + ImageRegistryAuthConfig: common.RegistryAuthConfig{ + K8sSecrets: []common.RegistryAuthSecret{{ + Auths: map[string]common.RegistryAuth{"registry.example.invalid": {Auth: imageRegistryAuth}}, }}, }, }) require.NoError(t, err) output := strings.Join(lines, "\n") + // Credentials must never appear in the logged payload. assert.NotContains(t, output, apiKey) - assert.NotContains(t, output, registryAuth) + assert.NotContains(t, output, helmRegistryAuth) + assert.NotContains(t, output, imageRegistryAuth) assert.NotContains(t, output, urlPassword) assert.NotContains(t, output, signedToken) + // Non-sensitive metadata must still be logged for debuggability. assert.Contains(t, output, "my-release") + assert.Contains(t, output, "gpu.small") + assert.Contains(t, output, "A100") + assert.Contains(t, output, "1.30") assert.Contains(t, output, wantHelmChart) }