diff --git a/src/compute-plane-services/nvca/internal/miniservice/BUILD.bazel b/src/compute-plane-services/nvca/internal/miniservice/BUILD.bazel index 03dc7e749..a1315a960 100644 --- a/src/compute-plane-services/nvca/internal/miniservice/BUILD.bazel +++ b/src/compute-plane-services/nvca/internal/miniservice/BUILD.bazel @@ -181,6 +181,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 6f10aac1c..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 @@ -92,7 +106,16 @@ 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. HelmChartURL is redacted too, in + // case it embeds userinfo or a signed query string. + log.V(2).WithValues( + "helmChart", redactedHelmChartURL(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 +134,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" 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..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 @@ -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,100 @@ func TestReValClientHostHeader(t *testing.T) { } } +func TestReValClientPayloadLogRedactsCredentials(t *testing.T) { + const ( + 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 + 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", + 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: 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, 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) +} + +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) {