Repository: getsops/sops (language: python)

Issue:
sops 3.8.0 doesn't work with qubes split gpg
Cross posting from https://github.com/getsops/sops/pull/1054#issuecomment-1722630295

After upgrading to the new release, qubes split gpg (https://www.qubes-os.org/doc/split-gpg/ / https://github.com/QubesOS/qubes-app-linux-split-gpg) is no longer working.
I think this is due to the new release passing `--no-default-keyring` and `--homedir` which are both broken with qubes split gpg for different reasons:
  - `--no-default-keyring` is handled [here](https://github.com/QubesOS/qubes-app-linux-split-gpg/blob/ef64f62a882a2e62f31c06ab5e02cbe7e0dd1643/gpg-client-wrapper#L39-L40)
  - `--homedir` isn't handled at all, and it fails with `qubes-gpg-client: unrecognized option '--homedir'`


Also filed this issue with qubes: https://github.com/QubesOS/qubes-issues/issues/8523


Discussion / hints:
SOPS is built with the official GnuPG binary in mind, and not with a wrapper offering a selective feature set. In my opinion, this should thus be solved on the Qubes side, as it would be hard for us to navigate every selective wrapper correctly.
From the linked qubes issue (https://github.com/QubesOS/qubes-issues/issues/8523#issuecomment-1722669637): 

> The option combination used by sops here looks weird - it adds `--no-default-keyring`, but does not specify any other keyring (so, no keyring should be used at all? decryption would fail then...). And then it specifies explicitly `--homedir`, to the value that is in fact the default one already.

@hiddeco are you able to explain what these arguments are meant to do?
After reading the man page again, it may look like we are able to drop `--no-default-keyring` but would continue to use `--homedir` (but potentially in combination with `--keyring`).

I wrote this code a long time ago, while it took some time to actually release it once we had settled stewardship. But they are intended to prevent reaching out to unexpected locations when a GnuPG home is set up for isolation: https://github.com/getsops/sops/blob/main/pgp/keysource.go#L114-L123
> After reading the man page again, it may look like we are able to drop `--no-default-keyring` but would continue to use `--homedir` (but potentially in combination with `--keyring`).

At least right now, it seems to pass my normal gnupg homedir: `--homedir /home/user/.gnupg`.
Why specify the `--homedir` argument when it already matches the default?

> they are intended to prevent reaching out to unexpected locations when a GnuPG home is set up for isolation: https://github.com/getsops/sops/blob/main/pgp/keysource.go#L114-L123

How does one activate that code path? At least here in the github UI I'm not seeing how that function gets called.
SOPS can be used as a binary and as a SDK, which means we have code paths that are not intended to necessarily serve the binary. That's the case here.
As you seem to have run into a dead end on the other side as well. I will see what we can do about preferring plain `gpg` calls without flags as much as possible from the CLI, but this will take a bit of time.

Fix patch:
diff --git a/pgp/keysource.go b/pgp/keysource.go
index b6c77bde0..b968a3046 100644
--- a/pgp/keysource.go
+++ b/pgp/keysource.go
@@ -318,7 +318,7 @@ func (key *MasterKey) encryptWithGnuPG(dataKey []byte) error {
 		fingerprint,
 		"--no-encrypt-to",
 	}
-	err, stdout, stderr := gpgExec(key.gnuPGHome(), args, bytes.NewReader(dataKey))
+	err, stdout, stderr := gpgExec(key.gnuPGHomeDir, args, bytes.NewReader(dataKey))
 	if err != nil {
 		return fmt.Errorf("failed to encrypt sops data key with pgp: %s", strings.TrimSpace(stderr.String()))
 	}
@@ -407,7 +407,7 @@ func (key *MasterKey) decryptWithGnuPG() ([]byte, error) {
 	args := []string{
 		"-d",
 	}
-	err, stdout, stderr := gpgExec(key.gnuPGHome(), args, strings.NewReader(key.EncryptedKey))
+	err, stdout, stderr := gpgExec(key.gnuPGHomeDir, args, strings.NewReader(key.EncryptedKey))
 	if err != nil {
 		return nil, fmt.Errorf("failed to decrypt sops data key with pgp: %s",
 			strings.TrimSpace(stderr.String()))
@@ -436,27 +436,6 @@ func (key MasterKey) ToMap() map[string]interface{} {
 	return out
 }
 
-// gnuPGHome determines the GnuPG home directory for the MasterKey, and returns
-// its path. In order of preference:
-//  1. MasterKey.gnuPGHomeDir
-//  2. $GNUPGHOME
-//  3. user.Current().HomeDir/.gnupg
-//  4. $HOME/.gnupg
-func (key *MasterKey) gnuPGHome() string {
-	if key.gnuPGHomeDir == "" {
-		dir := os.Getenv("GNUPGHOME")
-		if dir == "" {
-			usr, err := user.Current()
-			if err != nil {
-				return filepath.Join(os.Getenv("HOME"), ".gnupg")
-			}
-			return filepath.Join(usr.HomeDir, ".gnupg")
-		}
-		return dir
-	}
-	return key.gnuPGHomeDir
-}
-
 // retrievePubKey attempts to retrieve the public key from the public keyring
 // by Fingerprint.
 func (key *MasterKey) retrievePubKey() (openpgp.Entity, error) {
@@ -479,7 +458,7 @@ func (key *MasterKey) retrievePubKey() (openpgp.Entity, error) {
 func (key *MasterKey) getPubRing() (openpgp.EntityList, error) {
 	path := key.pubRing
 	if path == "" {
-		path = filepath.Join(key.gnuPGHome(), defaultPubRing)
+		path = filepath.Join(gnuPGHome(key.gnuPGHomeDir), defaultPubRing)
 	}
 	return loadRing(path)
 }
@@ -490,7 +469,7 @@ func (key *MasterKey) getPubRing() (openpgp.EntityList, error) {
 func (key *MasterKey) getSecRing() (openpgp.EntityList, error) {
 	path := key.secRing
 	if path == "" {
-		path = filepath.Join(key.gnuPGHome(), defaultSecRing)
+		path = filepath.Join(gnuPGHome(key.gnuPGHomeDir), defaultSecRing)
 	}
 	if _, err := os.Lstat(path); err != nil {
 		if !os.IsNotExist(err) {
@@ -583,11 +562,11 @@ func fingerprintIndex(ring openpgp.EntityList) map[string]openpgp.Entity {
 }
 
 // gpgExec runs the provided args with the gpgBinary, while restricting it to
-// gnuPGHome. Stdout and stderr can be read from the returned buffers.
-// When the command fails, an error is returned.
-func gpgExec(gnuPGHome string, args []string, stdin io.Reader) (err error, stdout bytes.Buffer, stderr bytes.Buffer) {
-	if gnuPGHome != "" {
-		args = append([]string{"--no-default-keyring", "--homedir", gnuPGHome}, args...)
+// homeDir when provided. Stdout and stderr can be read from the returned
+// buffers. When the command fails, an error is returned.
+func gpgExec(homeDir string, args []string, stdin io.Reader) (err error, stdout bytes.Buffer, stderr bytes.Buffer) {
+	if homeDir != "" {
+		args = append([]string{"--homedir", homeDir}, args...)
 	}
 
 	cmd := exec.Command(gpgBinary(), args...)
@@ -609,6 +588,28 @@ func gpgBinary() string {
 	return binary
 }
 
+// gnuPGHome determines the GnuPG home directory, and returns its path.
+// In order of preference:
+//  1. customPath
+//  2. $GNUPGHOME
+//  3. user.Current().HomeDir/.gnupg
+//  4. $HOME/.gnupg
+func gnuPGHome(customPath string) string {
+	if customPath != "" {
+		return customPath
+	}
+
+	dir := os.Getenv("GNUPGHOME")
+	if dir == "" {
+		usr, err := user.Current()
+		if err != nil {
+			return filepath.Join(os.Getenv("HOME"), ".gnupg")
+		}
+		return filepath.Join(usr.HomeDir, ".gnupg")
+	}
+	return dir
+}
+
 // shortenFingerprint returns the short ID of the given fingerprint.
 // This is mostly used for compatability reasons, as older versions of GnuPG
 // do not always like long IDs.


Test patch:
diff --git a/pgp/keysource_test.go b/pgp/keysource_test.go
index 58cb32040..187a75b1d 100644
--- a/pgp/keysource_test.go
+++ b/pgp/keysource_test.go
@@ -271,7 +271,7 @@ func TestMasterKey_encryptWithGnuPG(t *testing.T) {
 		args := []string{
 			"-d",
 		}
-		err, stdout, stderr := gpgExec(key.gnuPGHome(), args, strings.NewReader(key.EncryptedKey))
+		err, stdout, stderr := gpgExec(key.gnuPGHomeDir, args, strings.NewReader(key.EncryptedKey))
 		assert.NoError(t, err, stderr.String())
 		assert.Equal(t, data, stdout.Bytes())
 	})
@@ -529,24 +529,6 @@ func TestMasterKey_ToMap(t *testing.T) {
 	}, key.ToMap())
 }
 
-func TestMasterKey_gnuPGHome(t *testing.T) {
-	key := &MasterKey{}
-
-	usr, err := user.Current()
-	if err == nil {
-		assert.Equal(t, filepath.Join(usr.HomeDir, ".gnupg"), key.gnuPGHome())
-	} else {
-		assert.Equal(t, filepath.Join(os.Getenv("HOME"), ".gnupg"), key.gnuPGHome())
-	}
-
-	gnupgHome := "/overwrite/home"
-	t.Setenv("GNUPGHOME", gnupgHome)
-	assert.Equal(t, gnupgHome, key.gnuPGHome())
-
-	key.gnuPGHomeDir = "/home/dir/overwrite"
-	assert.Equal(t, key.gnuPGHomeDir, key.gnuPGHome())
-}
-
 func TestMasterKey_retrievePubKey(t *testing.T) {
 	t.Run("existing fingerprint", func(t *testing.T) {
 		key := NewMasterKeyFromFingerprint(mockFingerprint)
@@ -671,6 +653,22 @@ func Test_gpgBinary(t *testing.T) {
 	assert.Equal(t, overwrite, gpgBinary())
 }
 
+func Test_gnuPGHome(t *testing.T) {
+	usr, err := user.Current()
+	if err == nil {
+		assert.Equal(t, filepath.Join(usr.HomeDir, ".gnupg"), gnuPGHome(""))
+	} else {
+		assert.Equal(t, filepath.Join(os.Getenv("HOME"), ".gnupg"), gnuPGHome(""))
+	}
+
+	gnupgHome := "/overwrite/home"
+	t.Setenv("GNUPGHOME", gnupgHome)
+	assert.Equal(t, gnupgHome, gnuPGHome(""))
+
+	customP := "/home/dir/overwrite"
+	assert.Equal(t, customP, gnuPGHome(customP))
+}
+
 func Test_shortenFingerprint(t *testing.T) {
 	shortId := shortenFingerprint(mockFingerprint)
 	assert.Equal(t, "9732075EA221A7EA", shortId)