[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 <[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]>
gopasspw · Mar 13, 2024 · 34567d9 · 34567d9
1 parent 0562045
commit 34567d9
Show file tree

Hide file tree

Showing 13 changed files with 145 additions and 22 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
 }
diff --git 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
@@ -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
@@ -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
@@ -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)

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