[bugfix] Bring back audit summary (gopasspw#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 gopasspw#2816

Signed-off-by: Dominik Schulz <[email protected]>

* Fix tests.

Signed-off-by: Dominik Schulz <[email protected]>

* Fix integration test

Signed-off-by: Dominik Schulz <[email protected]>

---------

Signed-off-by: Dominik Schulz <[email protected]>

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 {

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()

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,
 				},
 			},
 		},

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)
 

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,12 +180,19 @@ 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),
 	}
 
 	for k := range r.secrets {
 		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
 }

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.

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)

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)))
 

internal/store/leaf/rcs_test.go

@@ -46,8 +46,8 @@ func TestGitRevisions(t *testing.T) {
 	require.NoError(t, s.Storage().InitConfig(ctx, "foo", "[email protected]"))
 
 	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)

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)
 }

main_test.go

@@ -60,7 +60,6 @@ var commandsWithError = set.Map([]string{
 	".alias.add",
 	".alias.remove",
 	".alias.delete",
-	".audit",
 	".cat",
 	".clone",
 	".copy",

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")
 	})
 }

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)]"
 
 
 	      ;;