diff --git a/log/log.go b/log/log.go index de2f171b2..da1d7622e 100644 --- a/log/log.go +++ b/log/log.go @@ -27,14 +27,16 @@ var redacted = &Hook{ AcceptedLevels: logrus.AllLevels, RedactionList: []string{ // Keys from the config - "(ApiKey:\")[\\w]*", - "(Secret:\")[\\w]*", + "(ApiKey:[\\s]*\")[\\w]*", + "(Secret:[\\s]*\")[\\w]*", "(PasswordEncryptionKey:[\\s]*\")[^\"]*", "(UserHeader:[\\s]*\")[^\"]*", "(TrustedSources:[\\s]*\")[^\"]*", "(MetricsPath:[\\s]*\")[^\"]*", "(DevAutoCreateAdminPassword:[\\s]*\")[^\"]*", "(DevAutoLoginUsername:[\\s]*\")[^\"]*", + // Prometheus.Password. Any character is allowed, so skip escaped quotes in the value + `(Password:[\s]*")(?:[^"\\]|\\.)*`, // UI appConfig "(subsonicToken:)[\\w]+(\\s)", diff --git a/log/log_test.go b/log/log_test.go index 82207c672..184ff57db 100644 --- a/log/log_test.go +++ b/log/log_test.go @@ -9,6 +9,7 @@ import ( "testing" "time" + "github.com/kr/pretty" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/sirupsen/logrus" @@ -94,7 +95,7 @@ var _ = Describe("Logger", func() { SetLogSourceLine(true) Error("A crash happened") // NOTE: This assertion breaks if the line number above changes - Expect(hook.LastEntry().Data[" source"]).To(ContainSubstring("/log/log_test.go:95")) + Expect(hook.LastEntry().Data[" source"]).To(ContainSubstring("/log/log_test.go:96")) Expect(hook.LastEntry().Message).To(Equal("A crash happened")) }) @@ -291,5 +292,69 @@ var _ = Describe("Logger", func() { Expect(got).ToNot(ContainSubstring("secret")) Expect(got).To(ContainSubstring(`"User-Agent":["Finamp/1.0"]`)) }) + + // https://github.com/navidrome/navidrome/discussions/6232 + DescribeTable("redacts config keys in the startup Configuration dump", + func(line, expected string) { + Expect(Redact(line)).To(Equal(expected)) + }, + Entry("unpadded ApiKey", `ApiKey:"0123456789abcdef0123456789abcdef"`, `ApiKey:"[REDACTED]"`), + Entry("unpadded Secret", `Secret:"fedcba9876543210fedcba9876543210"`, `Secret:"[REDACTED]"`), + Entry("padded ApiKey", ` ApiKey: "0123456789abcdef0123456789abcdef",`, + ` ApiKey: "[REDACTED]",`), + Entry("padded Secret", ` Secret: "fedcba9876543210fedcba9876543210",`, + ` Secret: "[REDACTED]",`), + Entry("unpadded Prometheus Password", `Password:"p@ss w0rd!"`, `Password:"[REDACTED]"`), + Entry("padded Prometheus Password", ` Password: "p@ss w0rd!",`, ` Password: "[REDACTED]",`), + Entry("Prometheus Password with escaped quotes", ` Password: "a\"b\\\"c",`, + ` Password: "[REDACTED]",`), + ) + + It("redacts secrets in a pretty-printed config struct", func() { + // Mirrors conf.lastfmOptions and conf.prometheusOptions (conf imports log, so it can't be + // used here). pretty only breaks a struct into padded lines when it is long enough, so + // keep all the fields. + type lastfmOptions struct { + Enabled bool + ApiKey string + Secret string + Language string + ScrobbleFirstArtistOnly bool + Languages []string + } + type prometheusOptions struct { + Enabled bool + MetricsPath string + Password string + } + type configOptions struct { + Address string + LastFM lastfmOptions + Prometheus prometheusOptions + } + cfg := configOptions{ + Address: "0.0.0.0", + LastFM: lastfmOptions{ //nolint:gosec + Enabled: true, + ApiKey: "0123456789abcdef0123456789abcdef", + Secret: "fedcba9876543210fedcba9876543210", + Language: "en", + Languages: []string{"en"}, + }, + Prometheus: prometheusOptions{ //nolint:gosec + Enabled: true, + MetricsPath: "/metrics", + Password: `prom"pass-tail`, + }, + } + dump := pretty.Sprintf("Configuration: %# v", cfg) + Expect(dump).To(MatchRegexp(`ApiKey:\s{2,}"`), "the dump must use the padded layout") + + got := Redact(dump) + Expect(got).ToNot(ContainSubstring(cfg.LastFM.ApiKey)) + Expect(got).ToNot(ContainSubstring(cfg.LastFM.Secret)) + Expect(got).ToNot(ContainSubstring("pass-tail")) + Expect(got).To(ContainSubstring(`"en"`)) + }) }) })