From 34567d93ee949b16172d370aac9443e74fb81990 Mon Sep 17 00:00:00 2001 From: Dominik Schulz Date: Wed, 13 Mar 2024 14:26:06 +0100 Subject: [PATCH] [bugfix] Bring back audit summary (#2820) * [bugfix] Bring back audit summary This PR brings back the audit summary view and displays only that by default. This restores the old behaviour before we refactored the audit implementation. The new view is still available with the new --full flag. Fixes #2816 Signed-off-by: Dominik Schulz * Fix tests. Signed-off-by: Dominik Schulz * Fix integration test Signed-off-by: Dominik Schulz --------- Signed-off-by: Dominik Schulz --- internal/action/audit.go | 22 ++++++++-- internal/action/audit_test.go | 6 +-- internal/action/commands.go | 9 +++- internal/audit/output.go | 51 +++++++++++++++++++++- internal/audit/report.go | 57 ++++++++++++++++++++++++- internal/backend/storage/fs/rcs.go | 2 +- internal/backend/storage/fs/rcs_test.go | 2 +- internal/store/leaf/convert.go | 6 ++- internal/store/leaf/rcs_test.go | 4 +- internal/store/root/rcs_test.go | 2 +- main_test.go | 1 - tests/audit_test.go | 3 +- zsh.completion | 2 +- 13 files changed, 145 insertions(+), 22 deletions(-) diff --git a/internal/action/audit.go b/internal/action/audit.go index d02da8ffa6..577a0676fb 100644 --- a/internal/action/audit.go +++ b/internal/action/audit.go @@ -71,12 +71,26 @@ func (s *Action) Audit(c *cli.Context) error { case "csv": return saveReport(ctx, r.RenderCSV, c.String("output-file"), "csv") default: - if err := r.PrintResults(ctx); err != nil { - return err + var err error + if c.Bool("full") { + debug.Log("Printing full report") + err = r.PrintResults(ctx) + } + if c.Bool("summary") { + debug.Log("Printing summary") + + nerr := r.PrintSummary(ctx) + // do not overwrite err if it is already set + if err == nil { + err = nerr + } + } + if !c.Bool("full") && !c.Bool("summary") { + out.Warning(ctx, "No output format specified. Use `--full` or `--summary` to specify.") } - } - return nil + return err + } } func saveReport(ctx context.Context, f func(io.Writer) error, path, suffix string) error { diff --git a/internal/action/audit_test.go b/internal/action/audit_test.go index 85f982822a..a10548a4af 100644 --- a/internal/action/audit_test.go +++ b/internal/action/audit_test.go @@ -33,17 +33,17 @@ func TestAudit(t *testing.T) { stdout = os.Stdout }() - t.Run("expect audit complaints on very weak passwords", func(t *testing.T) { + t.Run("expect audit to complains on very weak passwords", func(t *testing.T) { sec := secrets.NewAKV() sec.SetPassword("123") require.NoError(t, act.Store.Set(ctx, "bar", sec)) require.NoError(t, act.Store.Set(ctx, "baz", sec)) - require.Error(t, act.Audit(gptest.CliCtx(ctx, t))) + require.Error(t, act.Audit(gptest.CliCtxWithFlags(ctx, t, map[string]string{"full": "true"})), buf.String()) buf.Reset() }) - t.Run("test with filter and very passwords", func(t *testing.T) { + t.Run("test with filter", func(t *testing.T) { c := gptest.CliCtx(ctx, t, "foo") require.Error(t, act.Audit(c)) buf.Reset() diff --git a/internal/action/commands.go b/internal/action/commands.go index 7a49dd7c3c..a68003b70c 100644 --- a/internal/action/commands.go +++ b/internal/action/commands.go @@ -97,8 +97,13 @@ func (s *Action) GetCommands() []*cli.Command { Usage: "HTML template. If not set use the built-in default.", }, &cli.BoolFlag{ - Name: "failed", - Usage: "Report only entries that failed validation. Default: false (reports all)", + Name: "full", + Usage: "Print full details of all findings. Default: false", + }, + &cli.BoolFlag{ + Name: "summary", + Usage: "Print a summary of the audit results. Default: true (print summary)", + Value: true, }, }, }, diff --git a/internal/audit/output.go b/internal/audit/output.go index b682ac50ba..e9051534ce 100644 --- a/internal/audit/output.go +++ b/internal/audit/output.go @@ -7,6 +7,7 @@ import ( "io" "os" "sort" + "strings" "text/template" "time" @@ -23,17 +24,35 @@ func (r *Report) PrintResults(ctx context.Context) error { return nil } + debug.Log("Printing results for %d secrets", len(r.Secrets)) + var failed bool for _, name := range set.SortedKeys(r.Secrets) { s := r.Secrets[name] - out.Printf(ctx, "%s (age: %s)", name, s.Age.String()) + var sb strings.Builder + sb.WriteString(fmt.Sprintf("%s (age: %s) ", name, s.HumanizeAge())) + if !s.HasFindings() { + sb.WriteString("OK") + out.OK(ctx, sb.String()) + + continue + } + sb.WriteString("Potentially weak. ") for k, v := range s.Findings { if v.Severity == "error" || v.Severity == "warning" { failed = true } - out.Errorf(ctx, "[%s] %s: %s", v.Severity, k, v.Message) + switch v.Severity { + case "error": + fallthrough + case "warning": + sb.WriteString(fmt.Sprintf("%s: %s. ", k, v.Message)) + default: + continue + } } + out.Warning(ctx, sb.String()) } if failed { @@ -43,6 +62,34 @@ func (r *Report) PrintResults(ctx context.Context) error { return nil } +func (r *Report) PrintSummary(ctx context.Context) error { + if r == nil { + out.Warning(ctx, "Empty report") + + return nil + } + + debug.Log("Printing summary for %d findings", len(r.Findings)) + + for _, name := range set.SortedKeys(r.Findings) { + f := r.Findings[name] + if f.Len() < 1 { + continue + } + // TODO add details about the analyzer, not just the name + out.Printf(ctx, "Analyzer %s found issues: ", name) + for _, v := range set.Sorted(f.Elements()) { + out.Printf(ctx, "- %s", v) + } + } + + if len(r.Findings) > 0 { + return fmt.Errorf("weak password or duplicates detected") + } + + return nil +} + func (r *Report) RenderCSV(w io.Writer) error { cw := csv.NewWriter(w) diff --git a/internal/audit/report.go b/internal/audit/report.go index 6db3fe72de..3402e64491 100644 --- a/internal/audit/report.go +++ b/internal/audit/report.go @@ -7,6 +7,7 @@ import ( "github.com/gopasspw/gopass/internal/hashsum" "github.com/gopasspw/gopass/internal/set" + "github.com/gopasspw/gopass/pkg/debug" ) type Finding struct { @@ -15,11 +16,39 @@ type Finding struct { } type SecretReport struct { - Name string + Name string + // analyzer -> finding details Findings map[string]Finding Age time.Duration } +func (s *SecretReport) HasFindings() bool { + for _, f := range s.Findings { + if f.Severity != "none" { + return true + } + } + + return false +} + +func (s *SecretReport) HumanizeAge() string { + if s.Age < 24*time.Hour { + return fmt.Sprintf("%d hours", int(s.Age.Hours())) + } + days := int(s.Age.Hours() / 24) + if days < 30 { + return fmt.Sprintf("%d days", days) + } + months := days / 30 + if months < 12 { + return fmt.Sprintf("%d months", months) + } + years := months / 12 + + return fmt.Sprintf("%d years", years) +} + func errors(e []error) []string { s := make([]string, 0, len(e)) for _, es := range e { @@ -30,15 +59,25 @@ func errors(e []error) []string { } type Report struct { - Secrets map[string]SecretReport + // secret name -> report + Secrets map[string]SecretReport + + // finding -> secrets + Findings map[string]set.Set[string] + Template string Duration time.Duration } type ReportBuilder struct { + // protects all below sync.Mutex + // secret name -> report secrets map[string]SecretReport + // finding -> secrets + findings map[string]set.Set[string] + // SHA512(password) -> secret names duplicates map[string]set.Set[string] @@ -76,6 +115,7 @@ func (r *ReportBuilder) AddFinding(secret, finding, message, severity string) { r.Lock() defer r.Unlock() + // record individual findings s := r.secrets[secret] s.Name = secret if s.Findings == nil { @@ -86,6 +126,11 @@ func (r *ReportBuilder) AddFinding(secret, finding, message, severity string) { f.Severity = severity s.Findings[finding] = f r.secrets[secret] = s + + // record secrets per finding, for the summary + ss := r.findings[finding] + ss.Add(secret) + r.findings[finding] = ss } func (r *ReportBuilder) SetAge(name string, age time.Duration) { @@ -105,6 +150,7 @@ func (r *ReportBuilder) SetAge(name string, age time.Duration) { func newReport() *ReportBuilder { return &ReportBuilder{ secrets: make(map[string]SecretReport, 512), + findings: make(map[string]set.Set[string], 512), duplicates: make(map[string]set.Set[string], 512), sha1sums: make(map[string]set.Set[string], 512), t0: time.Now().UTC(), @@ -134,6 +180,7 @@ func (r *ReportBuilder) Finalize() *Report { ret := &Report{ Secrets: make(map[string]SecretReport, len(r.secrets)), + Findings: make(map[string]set.Set[string], len(r.findings)), Duration: time.Since(r.t0), } @@ -141,5 +188,11 @@ func (r *ReportBuilder) Finalize() *Report { ret.Secrets[k] = r.secrets[k] } + for k := range r.findings { + ret.Findings[k] = r.findings[k] + } + + debug.Log("Finalized report: %d secrets, %d findings", len(ret.Secrets), len(ret.Findings)) + return ret } diff --git a/internal/backend/storage/fs/rcs.go b/internal/backend/storage/fs/rcs.go index 169a1c04f0..a0e710ffd6 100644 --- a/internal/backend/storage/fs/rcs.go +++ b/internal/backend/storage/fs/rcs.go @@ -75,7 +75,7 @@ func (s *Store) Revisions(context.Context, string) ([]backend.Revision, error) { Hash: "latest", Date: time.Now(), }, - }, nil + }, backend.ErrNotSupported } // GetRevision only supports getting the latest revision. diff --git a/internal/backend/storage/fs/rcs_test.go b/internal/backend/storage/fs/rcs_test.go index 94c2b6d7f0..d32ed14425 100644 --- a/internal/backend/storage/fs/rcs_test.go +++ b/internal/backend/storage/fs/rcs_test.go @@ -28,7 +28,7 @@ func TestRCS(t *testing.T) { assert.Equal(t, "fs", g.Name()) require.Error(t, g.AddRemote(ctx, "foo", "bar")) revs, err := g.Revisions(ctx, "foo") - require.NoError(t, err) + require.Error(t, err) assert.Len(t, revs, 1) body, err := g.GetRevision(ctx, "foo", "latest") require.Error(t, err) diff --git a/internal/store/leaf/convert.go b/internal/store/leaf/convert.go index c04f9ad194..5c0e8b02ac 100644 --- a/internal/store/leaf/convert.go +++ b/internal/store/leaf/convert.go @@ -2,6 +2,7 @@ package leaf import ( "context" + "errors" "fmt" "os" "path/filepath" @@ -95,7 +96,10 @@ func (s *Store) Convert(ctx context.Context, cryptoBe backend.CryptoBackend, sto debug.Log("converting %s", e) revs, err := s.ListRevisions(ctx, e) if err != nil { - return fmt.Errorf("failed to list revision of %s: %w", e, err) + // the fs backend does not support revisions. but we can still convert the "latest" (only) revision. + if !errors.Is(err, backend.ErrNotSupported) || len(revs) < 1 { + return fmt.Errorf("failed to list revision of %s: %w", e, err) + } } sort.Sort(sort.Reverse(backend.Revisions(revs))) diff --git a/internal/store/leaf/rcs_test.go b/internal/store/leaf/rcs_test.go index f62ef6b2ee..6466e9307c 100644 --- a/internal/store/leaf/rcs_test.go +++ b/internal/store/leaf/rcs_test.go @@ -46,8 +46,8 @@ func TestGitRevisions(t *testing.T) { require.NoError(t, s.Storage().InitConfig(ctx, "foo", "bar@baz.com")) revs, err := s.ListRevisions(ctx, "foo") - require.NoError(t, err) - assert.Len(t, revs, 1) + require.Error(t, err) // not supported by the fs backend + assert.Len(t, revs, 1) // but it will still give a fake "latest" rev sec, err := s.GetRevision(ctx, "foo", "latest") require.Error(t, err) diff --git a/internal/store/root/rcs_test.go b/internal/store/root/rcs_test.go index 51177de55a..b4efed35ce 100644 --- a/internal/store/root/rcs_test.go +++ b/internal/store/root/rcs_test.go @@ -25,6 +25,6 @@ func TestRCS(t *testing.T) { require.Error(t, rs.RCSStatus(ctx, "")) revs, err := rs.ListRevisions(ctx, "foo") - require.NoError(t, err) + require.Error(t, err) assert.Len(t, revs, 1) } diff --git a/main_test.go b/main_test.go index 2fe9033686..331c3856e4 100644 --- a/main_test.go +++ b/main_test.go @@ -60,7 +60,6 @@ var commandsWithError = set.Map([]string{ ".alias.add", ".alias.remove", ".alias.delete", - ".audit", ".cat", ".clone", ".copy", diff --git a/tests/audit_test.go b/tests/audit_test.go index f93b3f311c..538e8c9b56 100644 --- a/tests/audit_test.go +++ b/tests/audit_test.go @@ -17,6 +17,7 @@ func TestAudit(t *testing.T) { t.Run("audit the test store", func(t *testing.T) { out, err := ts.run("audit") require.Error(t, err) - assert.Contains(t, out, "weak password") + assert.Contains(t, out, "crunchy") + assert.Contains(t, out, "zxcvbn") }) } diff --git a/zsh.completion b/zsh.completion index f28f81a504..4a02eb37bb 100644 --- a/zsh.completion +++ b/zsh.completion @@ -24,7 +24,7 @@ _gopass () { ;; audit) - _arguments : "--format[Output format. text, csv or html. Default: text]" "--output-file[Output filename. Used for csv and html]" "--template[HTML template. If not set use the built-in default.]" "--failed[Report only entries that failed validation. Default: false (reports all)]" + _arguments : "--format[Output format. text, csv or html. Default: text]" "--output-file[Output filename. Used for csv and html]" "--template[HTML template. If not set use the built-in default.]" "--full[Print full details of all findings. Default: false]" "--summary[Print a summary of the audit results. Default: true (print summary)]" ;;