From f5047e0cbf4478b957ad9d7f38fcac77d1216eef Mon Sep 17 00:00:00 2001 From: bitgully <32452884+bitgully@users.noreply.github.com> Date: Fri, 2 Feb 2024 19:49:28 +0100 Subject: [PATCH 1/3] obfuscate credentials in log output --- dependency_cache.go | 25 ++++++++++++++++--------- dependency_cache_test.go | 23 +++++++++++++++++++++++ 2 files changed, 39 insertions(+), 9 deletions(-) diff --git a/dependency_cache.go b/dependency_cache.go index 7453ad5..fd805e7 100644 --- a/dependency_cache.go +++ b/dependency_cache.go @@ -26,6 +26,7 @@ import ( "net/url" "os" "path/filepath" + "regexp" "strconv" "strings" "time" @@ -181,10 +182,10 @@ func (d *DependencyCache) Artifact(dependency BuildpackDependency, mods ...Reque d.Logger.Headerf("%s Dependency has no SHA256. Skipping cache.", color.New(color.FgYellow, color.Bold).Sprint("Warning:")) - d.Logger.Bodyf("%s from %s", color.YellowString("Downloading"), uri) + d.Logger.Bodyf("%s from %s", color.YellowString("Downloading"), sanitizeUri(uri)) artifact = filepath.Join(d.DownloadPath, filepath.Base(uri)) if err := d.download(uri, artifact, mods...); err != nil { - return nil, fmt.Errorf("unable to download %s\n%w", uri, err) + return nil, fmt.Errorf("unable to download %s\n%w", sanitizeUri(uri), err) } return os.Open(artifact) @@ -218,10 +219,10 @@ func (d *DependencyCache) Artifact(dependency BuildpackDependency, mods ...Reque return os.Open(filepath.Join(d.DownloadPath, dependency.SHA256, filepath.Base(uri))) } - d.Logger.Bodyf("%s from %s", color.YellowString("Downloading"), uri) + d.Logger.Bodyf("%s from %s", color.YellowString("Downloading"), sanitizeUri(uri)) artifact = filepath.Join(d.DownloadPath, dependency.SHA256, filepath.Base(uri)) if err := d.download(uri, artifact, mods...); err != nil { - return nil, fmt.Errorf("unable to download %s\n%w", uri, err) + return nil, fmt.Errorf("unable to download %s\n%w", sanitizeUri(uri), err) } d.Logger.Body("Verifying checksum") @@ -250,7 +251,7 @@ func (d *DependencyCache) Artifact(dependency BuildpackDependency, mods ...Reque func (d DependencyCache) download(uri string, destination string, mods ...RequestModifierFunc) error { url, err := url.Parse(uri) if err != nil { - return fmt.Errorf("unable to parse URI %s\n%w", uri, err) + return fmt.Errorf("unable to parse URI %s\n%w", sanitizeUri(uri), err) } if url.Scheme == "file" { @@ -287,7 +288,7 @@ func (d DependencyCache) downloadFile(source string, destination string, mods .. func (d DependencyCache) downloadHttp(uri string, destination string, mods ...RequestModifierFunc) error { req, err := http.NewRequest("GET", uri, nil) if err != nil { - return fmt.Errorf("unable to create new GET request for %s\n%w", uri, err) + return fmt.Errorf("unable to create new GET request for %s\n%w", sanitizeUri(uri), err) } if d.UserAgent != "" { @@ -315,12 +316,12 @@ func (d DependencyCache) downloadHttp(uri string, destination string, mods ...Re } resp, err := client.Do(req) if err != nil { - return fmt.Errorf("unable to request %s\n%w", uri, err) + return fmt.Errorf("unable to request %s\n%w", sanitizeUri(uri), err) } defer resp.Body.Close() if resp.StatusCode < 200 || resp.StatusCode > 299 { - return fmt.Errorf("could not download %s: %d", uri, resp.StatusCode) + return fmt.Errorf("could not download %s: %d", sanitizeUri(uri), resp.StatusCode) } if err := os.MkdirAll(filepath.Dir(destination), 0755); err != nil { @@ -334,12 +335,18 @@ func (d DependencyCache) downloadHttp(uri string, destination string, mods ...Re defer out.Close() if _, err := io.Copy(out, resp.Body); err != nil { - return fmt.Errorf("unable to copy from %s to %s\n%w", uri, destination, err) + return fmt.Errorf("unable to copy from %s to %s\n%w", sanitizeUri(uri), destination, err) } return nil } +func sanitizeUri(uri string) string { + uriCreds := regexp.MustCompile(`:\/\/.*:.*@`) + uriCredsRepl := "://***:***@" + return uriCreds.ReplaceAllString(uri, uriCredsRepl) +} + func (DependencyCache) verify(path string, expected string) error { s := sha256.New() diff --git a/dependency_cache_test.go b/dependency_cache_test.go index e9e99c9..32f5c5c 100644 --- a/dependency_cache_test.go +++ b/dependency_cache_test.go @@ -17,9 +17,11 @@ package libpak_test import ( + "bytes" "fmt" "io" "net/http" + "net/url" "os" "path/filepath" "testing" @@ -32,6 +34,7 @@ import ( "github.com/sclevine/spec" "github.com/paketo-buildpacks/libpak" + "github.com/paketo-buildpacks/libpak/bard" ) func testDependencyCache(t *testing.T, context spec.G, it spec.S) { @@ -344,5 +347,25 @@ func testDependencyCache(t *testing.T, context spec.G, it spec.S) { Expect(io.ReadAll(a)).To(Equal([]byte("test-fixture"))) }) + + it("hide uri credentials from log", func() { + server.AppendHandlers(ghttp.CombineHandlers( + ghttp.RespondWith(http.StatusOK, "test-fixture"), + )) + + url, err := url.Parse(dependency.URI) + Expect(err).NotTo(HaveOccurred()) + credentials := "basic-username:basic-password" + urlWithBasicCreds := url.Scheme + "://" + credentials + "@" + url.Hostname() + ":" + url.Port() + url.Path + dependency.URI = urlWithBasicCreds + + var logBuffer bytes.Buffer + captureLogger := bard.NewLogger(&logBuffer) + dependencyCache.Logger = captureLogger + a, err := dependencyCache.Artifact(dependency) + Expect(err).NotTo(HaveOccurred()) + Expect(a).NotTo(BeNil()) + Expect(logBuffer.String()).NotTo(ContainSubstring(credentials)) + }) }) } From 49ca89433e263e8beaee0362cd1b84101415c873 Mon Sep 17 00:00:00 2001 From: bitgully <32452884+bitgully@users.noreply.github.com> Date: Thu, 15 Feb 2024 13:43:56 +0100 Subject: [PATCH 2/3] url sanitation using regex replaced with parse and redact --- carton/package.go | 3 ++- dependency_cache.go | 55 ++++++++++++++++++---------------------- dependency_cache_test.go | 27 +++++++++++++------- layer.go | 3 ++- 4 files changed, 47 insertions(+), 41 deletions(-) diff --git a/carton/package.go b/carton/package.go index f6251b7..f58b3bf 100644 --- a/carton/package.go +++ b/carton/package.go @@ -185,7 +185,8 @@ func (p Package) Create(options ...Option) { f, err := cache.Artifact(dep, n.BasicAuth) if err != nil { - config.exitHandler.Error(fmt.Errorf("unable to download %s\n%w", dep.URI, err)) + logger.Debugf("fetching dependency %s failed\n%w", dep.Name, err) + config.exitHandler.Error(fmt.Errorf("unable to download %s. see DEBUG log level", dep.Name)) return } if err = f.Close(); err != nil { diff --git a/dependency_cache.go b/dependency_cache.go index fd805e7..a905204 100644 --- a/dependency_cache.go +++ b/dependency_cache.go @@ -26,7 +26,6 @@ import ( "net/url" "os" "path/filepath" - "regexp" "strconv" "strings" "time" @@ -169,6 +168,7 @@ func (d *DependencyCache) Artifact(dependency BuildpackDependency, mods ...Reque artifact string file string uri = dependency.URI + urlP *url.URL ) for d, u := range d.Mappings { @@ -178,14 +178,20 @@ func (d *DependencyCache) Artifact(dependency BuildpackDependency, mods ...Reque } } + urlP, err := url.Parse(uri) + if err != nil { + d.Logger.Debugf("URI format invalid\n%w", err) + return nil, fmt.Errorf("unable to parse URI. see DEBUG log level") + } + if dependency.SHA256 == "" { d.Logger.Headerf("%s Dependency has no SHA256. Skipping cache.", color.New(color.FgYellow, color.Bold).Sprint("Warning:")) - d.Logger.Bodyf("%s from %s", color.YellowString("Downloading"), sanitizeUri(uri)) - artifact = filepath.Join(d.DownloadPath, filepath.Base(uri)) - if err := d.download(uri, artifact, mods...); err != nil { - return nil, fmt.Errorf("unable to download %s\n%w", sanitizeUri(uri), err) + d.Logger.Bodyf("%s from %s", color.YellowString("Downloading"), urlP.Redacted()) + artifact = filepath.Join(d.DownloadPath, filepath.Base(urlP.Path)) + if err := d.download(urlP, artifact, mods...); err != nil { + return nil, fmt.Errorf("unable to download %s\n%w", urlP.Redacted(), err) } return os.Open(artifact) @@ -202,7 +208,7 @@ func (d *DependencyCache) Artifact(dependency BuildpackDependency, mods ...Reque if dependency.Equals(actual) { d.Logger.Bodyf("%s cached download from buildpack", color.GreenString("Reusing")) - return os.Open(filepath.Join(d.CachePath, dependency.SHA256, filepath.Base(uri))) + return os.Open(filepath.Join(d.CachePath, dependency.SHA256, filepath.Base(urlP.Path))) } file = filepath.Join(d.DownloadPath, fmt.Sprintf("%s.toml", dependency.SHA256)) @@ -216,13 +222,13 @@ func (d *DependencyCache) Artifact(dependency BuildpackDependency, mods ...Reque if dependency.Equals(actual) { d.Logger.Bodyf("%s previously cached download", color.GreenString("Reusing")) - return os.Open(filepath.Join(d.DownloadPath, dependency.SHA256, filepath.Base(uri))) + return os.Open(filepath.Join(d.DownloadPath, dependency.SHA256, filepath.Base(urlP.Path))) } - d.Logger.Bodyf("%s from %s", color.YellowString("Downloading"), sanitizeUri(uri)) - artifact = filepath.Join(d.DownloadPath, dependency.SHA256, filepath.Base(uri)) - if err := d.download(uri, artifact, mods...); err != nil { - return nil, fmt.Errorf("unable to download %s\n%w", sanitizeUri(uri), err) + d.Logger.Bodyf("%s from %s", color.YellowString("Downloading"), urlP.Redacted()) + artifact = filepath.Join(d.DownloadPath, dependency.SHA256, filepath.Base(urlP.Path)) + if err := d.download(urlP, artifact, mods...); err != nil { + return nil, fmt.Errorf("unable to download %s\n%w", urlP.Redacted(), err) } d.Logger.Body("Verifying checksum") @@ -248,17 +254,12 @@ func (d *DependencyCache) Artifact(dependency BuildpackDependency, mods ...Reque return os.Open(artifact) } -func (d DependencyCache) download(uri string, destination string, mods ...RequestModifierFunc) error { - url, err := url.Parse(uri) - if err != nil { - return fmt.Errorf("unable to parse URI %s\n%w", sanitizeUri(uri), err) - } - +func (d DependencyCache) download(url *url.URL, destination string, mods ...RequestModifierFunc) error { if url.Scheme == "file" { return d.downloadFile(url.Path, destination, mods...) } - return d.downloadHttp(uri, destination, mods...) + return d.downloadHttp(url, destination, mods...) } func (d DependencyCache) downloadFile(source string, destination string, mods ...RequestModifierFunc) error { @@ -285,10 +286,10 @@ func (d DependencyCache) downloadFile(source string, destination string, mods .. return nil } -func (d DependencyCache) downloadHttp(uri string, destination string, mods ...RequestModifierFunc) error { - req, err := http.NewRequest("GET", uri, nil) +func (d DependencyCache) downloadHttp(url *url.URL, destination string, mods ...RequestModifierFunc) error { + req, err := http.NewRequest("GET", url.String(), nil) if err != nil { - return fmt.Errorf("unable to create new GET request for %s\n%w", sanitizeUri(uri), err) + return fmt.Errorf("unable to create new GET request for %s\n%w", url.Redacted(), err) } if d.UserAgent != "" { @@ -316,12 +317,12 @@ func (d DependencyCache) downloadHttp(uri string, destination string, mods ...Re } resp, err := client.Do(req) if err != nil { - return fmt.Errorf("unable to request %s\n%w", sanitizeUri(uri), err) + return fmt.Errorf("unable to request %s\n%w", url.Redacted(), err) } defer resp.Body.Close() if resp.StatusCode < 200 || resp.StatusCode > 299 { - return fmt.Errorf("could not download %s: %d", sanitizeUri(uri), resp.StatusCode) + return fmt.Errorf("could not download %s: %d", url.Redacted(), resp.StatusCode) } if err := os.MkdirAll(filepath.Dir(destination), 0755); err != nil { @@ -335,18 +336,12 @@ func (d DependencyCache) downloadHttp(uri string, destination string, mods ...Re defer out.Close() if _, err := io.Copy(out, resp.Body); err != nil { - return fmt.Errorf("unable to copy from %s to %s\n%w", sanitizeUri(uri), destination, err) + return fmt.Errorf("unable to copy from %s to %s\n%w", url.Redacted(), destination, err) } return nil } -func sanitizeUri(uri string) string { - uriCreds := regexp.MustCompile(`:\/\/.*:.*@`) - uriCredsRepl := "://***:***@" - return uriCreds.ReplaceAllString(uri, uriCredsRepl) -} - func (DependencyCache) verify(path string, expected string) error { s := sha256.New() diff --git a/dependency_cache_test.go b/dependency_cache_test.go index 32f5c5c..41b9601 100644 --- a/dependency_cache_test.go +++ b/dependency_cache_test.go @@ -353,19 +353,28 @@ func testDependencyCache(t *testing.T, context spec.G, it spec.S) { ghttp.RespondWith(http.StatusOK, "test-fixture"), )) - url, err := url.Parse(dependency.URI) + url, err := url.ParseRequestURI(dependency.URI) Expect(err).NotTo(HaveOccurred()) - credentials := "basic-username:basic-password" - urlWithBasicCreds := url.Scheme + "://" + credentials + "@" + url.Hostname() + ":" + url.Port() + url.Path - dependency.URI = urlWithBasicCreds + credentials := "username:password" + uriWithBasicCreds := url.Scheme + "://" + credentials + "@" + url.Hostname() + ":" + url.Port() + url.Path + dependency.URI = uriWithBasicCreds var logBuffer bytes.Buffer - captureLogger := bard.NewLogger(&logBuffer) - dependencyCache.Logger = captureLogger - a, err := dependencyCache.Artifact(dependency) - Expect(err).NotTo(HaveOccurred()) + dependencyCache.Logger = bard.NewLogger(&logBuffer) + + // Make sure the password is not part of the log output. + a, errA := dependencyCache.Artifact(dependency) + Expect(errA).NotTo(HaveOccurred()) Expect(a).NotTo(BeNil()) - Expect(logBuffer.String()).NotTo(ContainSubstring(credentials)) + Expect(logBuffer.String()).NotTo(ContainSubstring("password")) + logBuffer.Reset() + + // Make sure the password is not part of the log output when an error occurs. + dependency.URI = "foo://username:password@acme.com" + b, errB := dependencyCache.Artifact(dependency) + Expect(errB).To(HaveOccurred()) + Expect(b).To(BeNil()) + Expect(logBuffer.String()).NotTo(ContainSubstring("password")) }) }) } diff --git a/layer.go b/layer.go index 6eefa05..ae3b7a3 100644 --- a/layer.go +++ b/layer.go @@ -291,7 +291,8 @@ func (d *DependencyLayerContributor) Contribute(layer libcnb.Layer, f Dependency return lc.Contribute(layer, func() (libcnb.Layer, error) { artifact, err := d.DependencyCache.Artifact(d.Dependency, d.RequestModifierFuncs...) if err != nil { - return libcnb.Layer{}, fmt.Errorf("unable to get dependency %s\n%w", d.Dependency.ID, err) + d.Logger.Debugf("fetching dependency %s failed\n%w", d.Dependency.Name, err) + return libcnb.Layer{}, fmt.Errorf("unable to get dependency %s. see DEBUG log level", d.Dependency.Name) } defer artifact.Close() From bae311cf1369ddb80cb7f28b2b6d79497c3e2a11 Mon Sep 17 00:00:00 2001 From: Markus Krumpschmid <32452884+bitgully@users.noreply.github.com> Date: Thu, 15 Feb 2024 16:56:39 +0100 Subject: [PATCH 3/3] Apply suggestions from code review Co-authored-by: Daniel Mikusa --- dependency_cache.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dependency_cache.go b/dependency_cache.go index a905204..55471df 100644 --- a/dependency_cache.go +++ b/dependency_cache.go @@ -189,7 +189,7 @@ func (d *DependencyCache) Artifact(dependency BuildpackDependency, mods ...Reque color.New(color.FgYellow, color.Bold).Sprint("Warning:")) d.Logger.Bodyf("%s from %s", color.YellowString("Downloading"), urlP.Redacted()) - artifact = filepath.Join(d.DownloadPath, filepath.Base(urlP.Path)) + artifact = filepath.Join(d.DownloadPath, filepath.Base(uri)) if err := d.download(urlP, artifact, mods...); err != nil { return nil, fmt.Errorf("unable to download %s\n%w", urlP.Redacted(), err) } @@ -226,7 +226,7 @@ func (d *DependencyCache) Artifact(dependency BuildpackDependency, mods ...Reque } d.Logger.Bodyf("%s from %s", color.YellowString("Downloading"), urlP.Redacted()) - artifact = filepath.Join(d.DownloadPath, dependency.SHA256, filepath.Base(urlP.Path)) + artifact = filepath.Join(d.DownloadPath, dependency.SHA256, filepath.Base(uri)) if err := d.download(urlP, artifact, mods...); err != nil { return nil, fmt.Errorf("unable to download %s\n%w", urlP.Redacted(), err) }