diff --git a/pkg/cli/bicep/types.go b/pkg/cli/bicep/types.go index beaf43a58e4..2b8cd1f720a 100644 --- a/pkg/cli/bicep/types.go +++ b/pkg/cli/bicep/types.go @@ -78,7 +78,9 @@ type Impl struct { func (i *Impl) PrepareTemplate(ctx context.Context, filePath string) (map[string]any, error) { // A remote URL is downloaded to a temporary local file so it can be read or compiled like a // local template. This mirrors the behavior users expect from tools such as kubectl. - originalPath := filePath + // displayPath is the only form of the template argument that may be shown to the user, so a + // credential embedded in a remote URL is never written to the terminal or to CI logs. + displayPath := RedactTemplatePath(filePath) remote := isRemoteURL(filePath) if remote { localPath, cleanup, err := i.downloadTemplate(ctx, filePath) @@ -92,11 +94,11 @@ func (i *Impl) PrepareTemplate(ctx context.Context, filePath string) (map[string if strings.EqualFold(path.Ext(filePath), ".json") { template, err := ReadARMJSON(filePath) if err != nil && remote { - return nil, fmt.Errorf("failed to read remote template %q: %w", originalPath, err) + return nil, fmt.Errorf("failed to read remote template %q: %w", displayPath, err) } return template, err } else if !strings.EqualFold(path.Ext(filePath), ".bicep") { - return nil, fmt.Errorf("the provided file %q must be a .json or .bicep file", originalPath) + return nil, fmt.Errorf("the provided file %q must be a .json or .bicep file", displayPath) } ok, err := IsBicepInstalled() @@ -118,14 +120,14 @@ func (i *Impl) PrepareTemplate(ctx context.Context, filePath string) (map[string return nil, fmt.Errorf("could not find file: %w", err) } - step := i.Output.BeginStep("Building %s...", originalPath) + step := i.Output.BeginStep("Building %s...", displayPath) bytes, err := i.Call("build", "--stdout", filePath) if err != nil { i.Output.CompleteStep(step) if remote { // The bicep compiler prints detailed diagnostics to stderr, so keep the wrapper // error focused on identifying the remote source rather than guessing the cause. - return nil, fmt.Errorf("failed to build remote template %q: %w", originalPath, err) + return nil, fmt.Errorf("failed to build remote template %q: %w", displayPath, err) } return nil, fmt.Errorf("failed to build template: %w", err) } @@ -149,6 +151,18 @@ func isRemoteURL(filePath string) bool { return strings.HasPrefix(lower, "http://") || strings.HasPrefix(lower, "https://") } +// RedactTemplatePath returns a display-safe form of a `rad` template argument. A local file path is +// returned unchanged; an http(s) URL has its userinfo and query-parameter values redacted so that +// credentials such as SAS tokens are never written to the terminal, to CI logs, or into generated +// output. Callers that display the template argument supplied by the user should format this value +// rather than the raw argument. +func RedactTemplatePath(templatePath string) string { + if !isRemoteURL(templatePath) { + return templatePath + } + return redactURL(templatePath) +} + // redactURL returns a display-safe copy of a URL with any userinfo and query-parameter values // removed, so credentials embedded in the URL (basic-auth userinfo or signed query parameters such // as SAS tokens) are never written to logs or error messages. @@ -167,11 +181,44 @@ func redactURL(raw string) string { } parsed.RawQuery = query.Encode() } + // A fragment is never needed to fetch a template, so drop it rather than risk displaying a + // credential someone placed there. + parsed.Fragment = "" + parsed.RawFragment = "" return parsed.String() } -// urlParseReason extracts the underlying reason from a url.Parse error, dropping the raw URL string -// it embeds (which may contain credentials). +// TemplateFileName returns the bare file name of a `rad` template argument, with any URL query +// string, fragment, and userinfo removed. Use it instead of filepath.Base when the result is +// written to disk or displayed, because filepath.Base of a URL retains the query string (and any +// credential in it). +func TemplateFileName(templatePath string) string { + if !isRemoteURL(templatePath) { + return filepath.Base(templatePath) + } + parsed, err := url.Parse(templatePath) + if err != nil { + return "template" + } + return urlFileName(parsed.Path) +} + +// urlFileName extracts the final segment of a decoded URL path. Percent-decoding can turn %5C into +// a backslash, which path.Base does not treat as a separator but Windows does, so backslashes are +// normalized first. Otherwise a path such as /..%5C..%5Capp.bicep would yield "..\..\app.bicep", +// which filepath.Join would resolve outside the intended directory on Windows. A segment that is +// not usable as a file name falls back to a fixed name. +func urlFileName(urlPath string) string { + base := path.Base(strings.ReplaceAll(urlPath, `\`, "/")) + if base == "." || base == "/" || base == ".." { + return "template" + } + return base +} + +// urlParseReason extracts the underlying reason from a *url.Error, dropping the raw URL string it +// embeds (which may contain credentials). It applies to both url.Parse failures and http.Client +// transport failures, since both return *url.Error. func urlParseReason(err error) error { var urlErr *url.Error if errors.As(err, &urlErr) { @@ -230,8 +277,10 @@ func (i *Impl) downloadTemplate(ctx context.Context, templateURL string) (string _ = i.FileSystem.RemoveAll(dir) } - // Preserve the original file name so compiler diagnostics reference a recognizable file. - localPath := filepath.Join(dir, path.Base(parsed.Path)) + // Preserve the original file name so compiler diagnostics reference a recognizable file. The + // name is taken from the URL path only, so an encoded separator cannot place the file outside + // the temporary directory. + localPath := filepath.Join(dir, urlFileName(parsed.Path)) if err := i.FileSystem.WriteFile(localPath, body, 0600); err != nil { cleanup() return "", nil, fmt.Errorf("failed to write remote template to temporary file: %w", err) @@ -299,7 +348,9 @@ func (i *Impl) attemptDownload(ctx context.Context, client *http.Client, templat if ctx.Err() != nil { return nil, 0, false, fmt.Errorf("failed to download template from %q: %w", display, context.Cause(ctx)) } - return nil, 0, true, fmt.Errorf("failed to download template from %q: %w", display, err) + // http.Client returns a *url.Error whose message embeds the raw request URL, so unwrap it + // to the underlying reason rather than leaking credentials alongside the redacted display. + return nil, 0, true, fmt.Errorf("failed to download template from %q: %w", display, urlParseReason(err)) } defer resp.Body.Close() @@ -322,7 +373,7 @@ func (i *Impl) attemptDownload(ctx context.Context, client *http.Client, templat if ctx.Err() != nil { return nil, 0, false, fmt.Errorf("failed to read template from %q: %w", display, context.Cause(ctx)) } - return nil, 0, true, fmt.Errorf("failed to read template from %q: %w", display, err) + return nil, 0, true, fmt.Errorf("failed to read template from %q: %w", display, urlParseReason(err)) } if int64(len(body)) > maxRemoteTemplateSize { return nil, 0, false, fmt.Errorf("template from %q exceeds the maximum allowed size of %d bytes", display, maxRemoteTemplateSize) diff --git a/pkg/cli/bicep/types_test.go b/pkg/cli/bicep/types_test.go index 80ed69e1fda..d259f5335b0 100644 --- a/pkg/cli/bicep/types_test.go +++ b/pkg/cli/bicep/types_test.go @@ -243,6 +243,27 @@ func Test_downloadTemplate_DownloadFailure(t *testing.T) { require.Contains(t, err.Error(), "failed to download template") } +// An encoded backslash must not become a path separator: on Windows it would otherwise place the +// downloaded template outside the temporary directory. +func Test_downloadTemplate_EncodedBackslashStaysInTempDir(t *testing.T) { + content := []byte("resource foo 'Foo' = {}\n") + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write(content) + })) + defer server.Close() + + t.Chdir(t.TempDir()) + + i := newTestImpl() + localPath, cleanup, err := i.downloadTemplate(t.Context(), server.URL+"/..%5C..%5Capp.bicep") + require.NoError(t, err) + defer cleanup() + + require.Equal(t, "app.bicep", filepath.Base(localPath)) + require.NotContains(t, localPath, `\`) + require.NotContains(t, localPath, "..") +} + func Test_PrepareTemplate_RemoteJSON(t *testing.T) { template := `{"$schema":"https://schema.management.azure.com/schemas/2019-04-01/deploymentTemplate.json#","resources":[]}` server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -457,6 +478,139 @@ func Test_redactURL(t *testing.T) { } } +func Test_RedactTemplatePath(t *testing.T) { + tests := []struct { + name string + path string + expected string + notContain string + }{ + { + name: "local path unchanged", + path: "./app.bicep", + expected: "./app.bicep", + }, + { + name: "local path with query-like name unchanged", + path: "/tmp/app.bicep?notaquery", + expected: "/tmp/app.bicep?notaquery", + }, + { + name: "remote url without credentials unchanged", + path: "https://example.com/app.bicep", + expected: "https://example.com/app.bicep", + }, + { + name: "remote url with signed query redacted", + path: "https://example.com/app.bicep?sig=TOPSECRET", + expected: "https://example.com/app.bicep?sig=redacted", + notContain: "TOPSECRET", + }, + { + name: "remote url with userinfo redacted", + path: "https://user:" + "TOPSECRET" + "@example.com/app.bicep", + expected: "https://redacted@example.com/app.bicep", + notContain: "TOPSECRET", + }, + { + name: "remote url with fragment dropped", + path: "https://example.com/app.bicep#token=" + "TOPSECRET", + expected: "https://example.com/app.bicep", + notContain: "TOPSECRET", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := RedactTemplatePath(tt.path) + require.Equal(t, tt.expected, got) + if tt.notContain != "" { + require.NotContains(t, got, tt.notContain) + } + }) + } +} + +// PrepareTemplate must never surface the raw template argument, because a remote URL can carry a +// SAS token or basic-auth userinfo that would otherwise land in terminal and CI logs. +func Test_PrepareTemplate_RemoteErrorRedactsCredentials(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = io.WriteString(w, "{ not valid json") + })) + defer server.Close() + + i := newTestImpl() + _, err := i.PrepareTemplate(t.Context(), server.URL+"/template.json?sig=TOPSECRET") + require.Error(t, err) + require.Contains(t, err.Error(), "failed to read remote template") + require.NotContains(t, err.Error(), "TOPSECRET") + require.Contains(t, err.Error(), "sig=redacted") +} + +func Test_TemplateFileName(t *testing.T) { + tests := []struct { + name string + path string + expected string + }{ + {name: "local relative path", path: "./app.bicep", expected: "app.bicep"}, + {name: "local absolute path", path: "/tmp/dir/app.bicep", expected: "app.bicep"}, + {name: "remote url", path: "https://example.com/dir/app.bicep", expected: "app.bicep"}, + { + // filepath.Base would return "app.bicep?sig=abc.def" here, putting part of the + // credential into an on-disk file name. + name: "remote url with dotted query value drops the query", + path: "https://example.com/app.bicep?sig=abc.def", + expected: "app.bicep", + }, + {name: "remote url with fragment drops the fragment", path: "https://example.com/app.bicep#token=abc", expected: "app.bicep"}, + { + // %5C decodes to a backslash, which Windows treats as a path separator even though + // path.Base does not, so it must not survive into a file name. + name: "remote url with encoded backslash", + path: "https://example.com/folder%5Capp.bicep", + expected: "app.bicep", + }, + { + name: "remote url with encoded backslash traversal", + path: "https://example.com/..%5C..%5Capp.bicep", + expected: "app.bicep", + }, + { + name: "remote url with no file segment", + path: "https://example.com/", + expected: "template", + }, + {name: "malformed url", path: "https://[::1/app.bicep", expected: "template"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + require.Equal(t, tt.expected, TemplateFileName(tt.path)) + }) + } +} + +// A transport failure returns a *url.Error whose message embeds the raw request URL, so the +// wrapped error must not carry the credential that the redacted display string removed. +func Test_downloadTemplate_TransportErrorRedactsCredentials(t *testing.T) { + original := retryBaseDelay + retryBaseDelay = time.Millisecond + defer func() { retryBaseDelay = original }() + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + url := server.URL + "/app.bicep?sig=TOPSECRET" + server.Close() // Close immediately so the connection is refused. + + i := newTestImpl() + _, _, err := i.downloadTemplate(t.Context(), url) + require.Error(t, err) + require.NotContains(t, err.Error(), "TOPSECRET") + require.Contains(t, err.Error(), "sig=redacted") +} + func Test_newHTTPClient_RedirectPolicy(t *testing.T) { client := newHTTPClient() mustReq := func(rawURL string) *http.Request { diff --git a/pkg/cli/cmd/app/graph/graph.go b/pkg/cli/cmd/app/graph/graph.go index 6a7f2c42ef7..3f368f9511a 100644 --- a/pkg/cli/cmd/app/graph/graph.go +++ b/pkg/cli/cmd/app/graph/graph.go @@ -248,10 +248,11 @@ func (r *Runner) runDeployed(ctx context.Context) error { // under /app-graph.json; otherwise it is written to // ./app-graph.json in the current working directory. func (r *Runner) runModeled(ctx context.Context) error { - r.Output.LogInfo("Compiling %s", r.BicepFilePath) + displayPath := bicep.RedactTemplatePath(r.BicepFilePath) + r.Output.LogInfo("Compiling %s", displayPath) template, err := r.Bicep.PrepareTemplate(ctx, r.BicepFilePath) if err != nil { - return clierrors.Message("Failed to compile %q: %v", r.BicepFilePath, err) + return clierrors.Message("Failed to compile %q: %v", displayPath, err) } graph, err := cligraph.BuildModeledGraph(template, r.IncludeIcons) diff --git a/pkg/cli/cmd/app/graph/preview/graph.go b/pkg/cli/cmd/app/graph/preview/graph.go index 7bc2b00d192..7322386709e 100644 --- a/pkg/cli/cmd/app/graph/preview/graph.go +++ b/pkg/cli/cmd/app/graph/preview/graph.go @@ -178,10 +178,11 @@ func (r *Runner) Run(ctx context.Context) error { body.IncludeIcons = to.Ptr(true) } if r.BicepFilePath != "" { - r.Output.LogInfo("Compiling %s", r.BicepFilePath) + displayPath := bicep.RedactTemplatePath(r.BicepFilePath) + r.Output.LogInfo("Compiling %s", displayPath) template, err := r.Bicep.PrepareTemplate(ctx, r.BicepFilePath) if err != nil { - return clierrors.Message("Failed to compile %q: %v", r.BicepFilePath, err) + return clierrors.Message("Failed to compile %q: %v", displayPath, err) } // ExtractDependsOnEdges returns nil when the template has no // eligible dependsOn edges, leaving body.DependsOnEdges nil so diff --git a/pkg/cli/cmd/bicep/generatekubernetesmanifest/generatekubernetesmanifest.go b/pkg/cli/cmd/bicep/generatekubernetesmanifest/generatekubernetesmanifest.go index cf6eff3e9e9..021f7495248 100644 --- a/pkg/cli/cmd/bicep/generatekubernetesmanifest/generatekubernetesmanifest.go +++ b/pkg/cli/cmd/bicep/generatekubernetesmanifest/generatekubernetesmanifest.go @@ -141,9 +141,12 @@ func (r *Runner) Validate(cmd *cobra.Command, args []string) error { return err } - // If the destination file is not provided, use the base name of the file with a .yaml extension + // If the destination file is not provided, use the base name of the file with a .yaml extension. + // The name is derived from the template file name only, so a credential in a remote URL's query + // string is never used to build an on-disk file name. if r.DestinationFile == "" { - r.DestinationFile = strings.TrimSuffix(filepath.Base(r.FilePath), filepath.Ext(r.FilePath)) + ".yaml" + templateFileName := bicep.TemplateFileName(r.FilePath) + r.DestinationFile = strings.TrimSuffix(templateFileName, filepath.Ext(templateFileName)) + ".yaml" } if filepath.Ext(r.DestinationFile) != ".yaml" && filepath.Ext(r.DestinationFile) != ".yml" { @@ -175,7 +178,9 @@ func (r *Runner) Run(ctx context.Context) error { return err } - deploymentTemplate, err := r.generateDeploymentTemplate(filepath.Base(r.FilePath), template, r.Parameters) + // The template name is written into the generated manifest, so derive it from the template file + // name only to keep credentials in a remote URL out of the generated output. + deploymentTemplate, err := r.generateDeploymentTemplate(bicep.TemplateFileName(r.FilePath), template, r.Parameters) if err != nil { return err } diff --git a/pkg/cli/cmd/bicep/publish/publish.go b/pkg/cli/cmd/bicep/publish/publish.go index af012659143..16b2537d6b4 100644 --- a/pkg/cli/cmd/bicep/publish/publish.go +++ b/pkg/cli/cmd/bicep/publish/publish.go @@ -152,9 +152,12 @@ func (r *Runner) Validate(cmd *cobra.Command, args []string) error { // The Run function prepares a Bicep template, extracts the destination, publishes the template to the target, and logs // a success message if no errors are encountered. An error is returned if any of the steps fail. func (r *Runner) Run(ctx context.Context) error { + // Redact any credentials embedded in a remote template URL before displaying it. + displayFile := bicep.RedactTemplatePath(r.File) + template, err := r.Bicep.PrepareTemplate(ctx, r.File) if err != nil { - return clierrors.MessageWithCause(err, "Failed to prepare Bicep file %q.", r.File) + return clierrors.MessageWithCause(err, "Failed to prepare Bicep file %q.", displayFile) } r.Template = template @@ -173,13 +176,13 @@ func (r *Runner) Run(ctx context.Context) error { digest, err := r.publish(ctx) var httpErr *errcode.ErrorResponse if errors.As(err, &httpErr) { - message := fmt.Sprintf("Failed to publish Bicep file %q to %q", r.File, r.Target) + message := fmt.Sprintf("Failed to publish Bicep file %q to %q", displayFile, r.Target) return handleErrorResponse(httpErr, message) } else if err != nil { - return clierrors.MessageWithCause(err, "Failed to publish Bicep file %q to %q", r.File, r.Target) + return clierrors.MessageWithCause(err, "Failed to publish Bicep file %q to %q", displayFile, r.Target) } - r.Output.LogInfo("Successfully published Bicep file %q to %q", r.File, r.Target) + r.Output.LogInfo("Successfully published Bicep file %q to %q", displayFile, r.Target) r.Output.LogInfo("To immutably pin the artifact, use the following Recipe url: %s", computeImmutableRecipeUrl(r.Target, digest.String())) return nil diff --git a/pkg/cli/cmd/deploy/deploy.go b/pkg/cli/cmd/deploy/deploy.go index 8eddc28a21e..ac5b735b217 100644 --- a/pkg/cli/cmd/deploy/deploy.go +++ b/pkg/cli/cmd/deploy/deploy.go @@ -344,15 +344,18 @@ func (r *Runner) Run(ctx context.Context) error { } } + // Redact any credentials embedded in a remote template URL before displaying it. + displayPath := bicep.RedactTemplatePath(r.FilePath) + progressText := "" if r.ApplicationName == "" { progressText = fmt.Sprintf( "Deploying template '%v' into environment '%v' from workspace '%v'...\n\n"+ - "Deployment In Progress...", r.FilePath, r.EnvironmentNameOrID, r.Workspace.Name) + "Deployment In Progress...", displayPath, r.EnvironmentNameOrID, r.Workspace.Name) } else { progressText = fmt.Sprintf( "Deploying template '%v' for application '%v' and environment '%v' from workspace '%v'...\n\n"+ - "Deployment In Progress... ", r.FilePath, r.ApplicationName, r.EnvironmentNameOrID, r.Workspace.Name) + "Deployment In Progress... ", displayPath, r.ApplicationName, r.EnvironmentNameOrID, r.Workspace.Name) } // Before deploying, set up recipe packs for any Radius.Core environments in the @@ -446,7 +449,7 @@ func (r *Runner) reportMissingParameters(template map[string]any) error { details = append(details, fmt.Sprintf(" - %v", errors[key])) } - return clierrors.Message("The template %q could not be deployed because of the following errors:\n\n%v", r.FilePath, strings.Join(details, "\n")) + return clierrors.Message("The template %q could not be deployed because of the following errors:\n\n%v", bicep.RedactTemplatePath(r.FilePath), strings.Join(details, "\n")) } // resolvePreview reports whether the deploy command should use the Radius.Core preview diff --git a/pkg/cli/cmd/deploy/deploy_test.go b/pkg/cli/cmd/deploy/deploy_test.go index cdc31d4eac2..3b4bf88f03d 100644 --- a/pkg/cli/cmd/deploy/deploy_test.go +++ b/pkg/cli/cmd/deploy/deploy_test.go @@ -716,6 +716,55 @@ func Test_Run(t *testing.T) { require.Empty(t, outputSink.Writes) }) + t.Run("Remote template URL credentials are redacted in progress text", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + bicep := bicep.NewMockInterface(ctrl) + + workspace := &workspaces.Workspace{ + Connection: map[string]any{ + "kind": "kubernetes", + "context": "kind-kind", + }, + Name: "kind-kind", + } + provider := &clients.Providers{ + Radius: &clients.RadiusProvider{ + EnvironmentID: fmt.Sprintf("/planes/radius/local/resourceGroups/%s/providers/applications.core/environments/%s", radcli.TestEnvironmentName, radcli.TestEnvironmentName), + }, + } + + var captured deploy.Options + deployMock := deploy.NewMockInterface(ctrl) + deployMock.EXPECT(). + DeployWithProgress(gomock.Any(), gomock.Any()). + DoAndReturn(func(ctx context.Context, o deploy.Options) (clients.DeploymentResult, error) { + captured = o + return clients.DeploymentResult{}, nil + }). + Times(1) + + outputSink := &output.MockOutput{} + runner := &Runner{ + Bicep: bicep, + Deploy: deployMock, + Output: outputSink, + FilePath: "https://example.com/app.bicep?sig=TOPSECRET", + EnvironmentNameOrID: radcli.TestEnvironmentID, + Parameters: map[string]map[string]any{}, + Workspace: workspace, + Providers: provider, + Template: map[string]any{}, + } + + err := runner.Run(t.Context()) + require.NoError(t, err) + + require.NotContains(t, captured.ProgressText, "TOPSECRET") + require.Contains(t, captured.ProgressText, "sig=redacted") + }) + t.Run("Environment-scoped deployment with aws provider", func(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish()