[bugfix] Do not remove unused keys on import by default (#2657)

The cleanup during import is currently buggy on some scenarios
so as a workaround we'll disable auto-cleanup by default and
introduce `recipients.remove-extra-keys` to allow users to turn
it back on.

See GH-2620

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

Author: dominikschulz

docs/config.md

@@ -115,6 +115,7 @@ This is a list of available options:
 | `mounts.path`          | `string` | Path to the root store. | `$XDG_DATA_HOME/gopass/stores/root` |
 | `recipients.check`     | `bool`   | Check recipients hash. | `false` |
 | `recipients.hash`      | `string` | SHA256 hash of the recipients file. Used to notify the user when the recipients files change. | `` |
+| `recipients.remove-extra-keys` | `bool` | Remove extra recipients during key import. | `false` |
 | `show.post-hook` | `string` | This hook is run right after displaying a secret with `gopass show` | `None` |
 | `updater.check`        | `bool`   | Check for updates when running `gopass version` | `true` |
 | `output.internal-pager` | `bool` | Use the internal pager `ov` |  `false` |

internal/store/leaf/recipients.go

@@ -327,7 +327,42 @@ func (s *Store) UpdateExportedPublicKeys(ctx context.Context, rs []string) (bool
 	}
 
 	// add any missing keys
+	failed, exported := s.addMissingKeys(ctx, exp, recipients)
+
+	// remove any extra key files
+	// TODO(GH-2620): Temporarily disabled by default until we fix the
+	// key cleanup.
+	if config.Bool(ctx, "recipients.remove-extra-keys") {
+		f, e := s.removeExtraKeys(ctx, recipients)
+		failed = failed || f
+		exported = exported || e
+	}
+
+	if exported && ctxutil.IsGitCommit(ctx) {
+		if err := s.storage.Commit(ctx, "Updated exported Public Keys"); err != nil {
+			switch {
+			case errors.Is(err, store.ErrGitNothingToCommit):
+				debug.Log("nothing to commit: %s", err)
+			case errors.Is(err, store.ErrGitNotInit):
+				debug.Log("git not initialized: %s", err)
+			default:
+				failed = true
+
+				out.Errorf(ctx, "Failed to git commit: %s", err)
+			}
+		}
+	}
+
+	if failed {
+		return exported, fmt.Errorf("some keys failed")
+	}
+
+	return exported, nil
+}
+
+func (s *Store) addMissingKeys(ctx context.Context, exp keyExporter, recipients map[string]bool) (bool, bool) {
 	var failed, exported bool
+
 	for r := range recipients {
 		if r == "" {
 			continue
@@ -358,7 +393,12 @@ func (s *Store) UpdateExportedPublicKeys(ctx context.Context, rs []string) (bool
 		}
 	}
 
-	// remove any extra key files
+	return failed, exported
+}
+
+func (s *Store) removeExtraKeys(ctx context.Context, recipients map[string]bool) (bool, bool) {
+	var failed, exported bool
+
 	keys, err := s.storage.List(ctx, keyDir)
 	if err != nil {
 		failed = true
@@ -397,26 +437,7 @@ func (s *Store) UpdateExportedPublicKeys(ctx context.Context, rs []string) (bool
 		debug.Log("Removed extra key %s", key)
 	}
 
-	if exported && ctxutil.IsGitCommit(ctx) {
-		if err := s.storage.Commit(ctx, "Updated exported Public Keys"); err != nil {
-			switch {
-			case errors.Is(err, store.ErrGitNothingToCommit):
-				debug.Log("nothing to commit: %s", err)
-			case errors.Is(err, store.ErrGitNotInit):
-				debug.Log("git not initialized: %s", err)
-			default:
-				failed = true
-
-				out.Errorf(ctx, "Failed to git commit: %s", err)
-			}
-		}
-	}
-
-	if failed {
-		return exported, fmt.Errorf("some keys failed")
-	}
-
-	return exported, nil
+	return failed, exported
 }
 
 type recipientMarshaler interface {