diff --git a/cmd/notation/internal/plugin/plugin.go b/cmd/notation/internal/plugin/plugin.go index 1bcdec693..191875a2c 100644 --- a/cmd/notation/internal/plugin/plugin.go +++ b/cmd/notation/internal/plugin/plugin.go @@ -68,7 +68,7 @@ func DownloadPluginFromURL(ctx context.Context, pluginURL string, tmpFile io.Wri defer resp.Body.Close() // Check server response if resp.StatusCode != http.StatusOK { - return fmt.Errorf("%s %q: https response bad status: %s", resp.Request.Method, resp.Request.URL, resp.Status) + return fmt.Errorf("%s %q: https response bad status: %s", resp.Request.Method, resp.Request.URL.Redacted(), resp.Status) } // Write the body to file lr := &io.LimitedReader{ @@ -80,7 +80,7 @@ func DownloadPluginFromURL(ctx context.Context, pluginURL string, tmpFile io.Wri return err } if lr.N == 0 { - return fmt.Errorf("%s %q: https response reached the %d MiB size limit", resp.Request.Method, resp.Request.URL, MaxPluginSourceBytes/1024/1024) + return fmt.Errorf("%s %q: https response reached the %d MiB size limit", resp.Request.Method, resp.Request.URL.Redacted(), MaxPluginSourceBytes/1024/1024) } return nil } diff --git a/cmd/notation/internal/plugin/plugin_test.go b/cmd/notation/internal/plugin/plugin_test.go new file mode 100644 index 000000000..31ca59f7b --- /dev/null +++ b/cmd/notation/internal/plugin/plugin_test.go @@ -0,0 +1,42 @@ +// Copyright The Notary Project Authors. +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package plugin + +import ( + "context" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +func TestDownloadPluginFromURL_BadStatusRedactsCredentials(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.NotFound(w, nil) + })) + defer server.Close() + pluginURL := strings.Replace(server.URL, "http://", "http://user:s3cret@", 1) + "/plugin.tar.gz" + + err := DownloadPluginFromURL(context.Background(), pluginURL, io.Discard) + if err == nil { + t.Fatal("DownloadPluginFromURL() expected an error, but got nil") + } + if !strings.Contains(err.Error(), "https response bad status: 404 Not Found") { + t.Errorf("DownloadPluginFromURL() error = %q, want the bad status", err) + } + if strings.Contains(err.Error(), "s3cret") { + t.Errorf("DownloadPluginFromURL() error contains the password: %q", err) + } +} diff --git a/cmd/notation/plugin/install.go b/cmd/notation/plugin/install.go index a0849c42d..757a9e0d6 100644 --- a/cmd/notation/plugin/install.go +++ b/cmd/notation/plugin/install.go @@ -132,7 +132,12 @@ func install(command *cobra.Command, opts *pluginInstallOpts) error { } pluginURL, err := url.Parse(opts.pluginSource) if err != nil { - return fmt.Errorf("failed to parse plugin download URL %s with error: %w", opts.pluginSource, err) + // the error of url.Parse contains the URL, which can have credentials + var urlErr *url.Error + if errors.As(err, &urlErr) { + err = urlErr.Err + } + return fmt.Errorf("failed to parse plugin download URL with error: %w", err) } if pluginURL.Scheme != "https" { return fmt.Errorf("failed to download plugin from URL: only the HTTPS scheme is supported, but got %s", pluginURL.Scheme) @@ -143,10 +148,12 @@ func install(command *cobra.Command, opts *pluginInstallOpts) error { } defer os.Remove(tmpFile.Name()) defer tmpFile.Close() - fmt.Printf("Downloading plugin from %s\n", opts.pluginSource) + // do not show the password of the URL in the output + redactedURL := pluginURL.Redacted() + fmt.Printf("Downloading plugin from %s\n", redactedURL) err = notationplugin.DownloadPluginFromURL(ctx, opts.pluginSource, tmpFile) if err != nil { - return fmt.Errorf("failed to download plugin from URL %s with error: %w", opts.pluginSource, err) + return fmt.Errorf("failed to download plugin from URL %s with error: %w", redactedURL, err) } fmt.Println("Download completed") if err := installPlugin(ctx, tmpFile.Name(), opts.inputChecksum, opts.force); err != nil { diff --git a/cmd/notation/plugin/install_test.go b/cmd/notation/plugin/install_test.go index d739ef8d3..c7a26a164 100644 --- a/cmd/notation/plugin/install_test.go +++ b/cmd/notation/plugin/install_test.go @@ -33,13 +33,52 @@ func TestInstall(t *testing.T) { inputChecksum: "dummy", pluginSource: "http://[::1]/%", } - expectedErrMsg := `failed to parse plugin download URL http://[::1]/% with error: parse "http://[::1]/%": invalid URL escape "%"` + expectedErrMsg := `failed to parse plugin download URL with error: invalid URL escape "%"` err := install(&cobra.Command{}, opts) if err == nil || err.Error() != expectedErrMsg { t.Fatalf("expected error %s, but got %s", expectedErrMsg, err) } }) + t.Run("invalid plugin source url with credentials", func(t *testing.T) { + opts := &pluginInstallOpts{ + pluginSourceType: notationplugin.PluginSourceTypeURL, + inputChecksum: "dummy", + pluginSource: "https://user:s3cret@localhost:badport/plugin.tar.gz", + } + err := install(&cobra.Command{}, opts) + if err == nil { + t.Fatal("install() expected an error, but got nil") + } + if !strings.Contains(err.Error(), "failed to parse plugin download URL") { + t.Errorf("install() error = %q, want a parse error", err) + } + if strings.Contains(err.Error(), "s3cret") { + t.Errorf("install() error contains the password: %q", err) + } + }) + + t.Run("failed download with credentials in the url", func(t *testing.T) { + cmd := &cobra.Command{} + cmd.SetContext(context.Background()) + // nothing listens on port 1, so the connection is refused + opts := &pluginInstallOpts{ + pluginSourceType: notationplugin.PluginSourceTypeURL, + inputChecksum: "dummy", + pluginSource: "https://user:s3cret@127.0.0.1:1/plugin.tar.gz", + } + err := install(cmd, opts) + if err == nil { + t.Fatal("install() expected an error, but got nil") + } + if want := "failed to download plugin from URL https://user:xxxxx@127.0.0.1:1/plugin.tar.gz"; !strings.Contains(err.Error(), want) { + t.Errorf("install() error = %q, want it to contain %q", err, want) + } + if strings.Contains(err.Error(), "s3cret") { + t.Errorf("install() error contains the password: %q", err) + } + }) + t.Run("unknown plugin source type", func(t *testing.T) { opts := &pluginInstallOpts{ pluginSourceType: -1,