Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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 "<unparseable>"
}
u.User = nil
u.RawQuery = ""
u.Fragment = ""
return u.String()
}

type revalClient struct {
endpoint string
host string
Expand Down Expand Up @@ -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")
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment thread
mesutoezdil marked this conversation as resolved.

// 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.
Expand All @@ -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"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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: "<unparseable>",
},
}
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) {
Expand Down
Loading