~sircmpwn/aerc

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
1

[PATCH] swap out local keyring for system gpg

Mark Wilkerson <mark@markhuge.com>
Details
Message ID
<20211204035706.105981-1-mark@markhuge.com>
DKIM signature
missing
Download raw message
Patch: +17 -40
/x/crypto/openpgp has been deprecated by the golang team. Given the
current state of the lib, support for things like smartcards may not be
possible.

See: https://github.com/golang/go/issues/44226

This patch allows for decrypting with existing keyrings and smartcards, but 
doesn't yet populate the fields for signature verification. I haven't been able
to come up with a solution that doesn't involve hacky parsing of the gpg
log output (which is also annoyingly sent to stderr). Parsing this output also
breaks the idea of having user specified pgp commands in the config.

The right way to do this is probably to interface with gpg-agent, but
that's a rabbit hole I don't have the bandwidth to sign up for.

Open to suggestions!

---
 aerc.go                  |  4 ----
 commands/account/view.go |  2 +-
 commands/msg/delete.go   |  2 +-
 commands/msgview/next.go |  2 +-
 lib/messageview.go       | 20 +++++++++++++-------
 widgets/aerc.go          | 25 -------------------------
 widgets/msglist.go       |  2 +-
 7 files changed, 17 insertions(+), 40 deletions(-)

diff --git a/aerc.go b/aerc.go
index b3338ba..dd08619 100644
--- a/aerc.go
+++ b/aerc.go
@@ -187,10 +187,6 @@ func main() {
		ui.EnableMouse()
	}

	logger.Println("Initializing PGP keyring")
	lib.InitKeyring()
	defer lib.UnlockKeyring()

	logger.Println("Starting Unix server")
	as, err := lib.StartServer(logger)
	if err != nil {
diff --git a/commands/account/view.go b/commands/account/view.go
index 4f59d94..48c7bfc 100644
--- a/commands/account/view.go
+++ b/commands/account/view.go
@@ -42,7 +42,7 @@ func (ViewMessage) Execute(aerc *widgets.Aerc, args []string) error {
		aerc.PushError(msg.Error.Error())
		return nil
	}
	lib.NewMessageStoreView(msg, store, aerc.DecryptKeys,
	lib.NewMessageStoreView(msg, store,
		func(view lib.MessageView, err error) {
			if err != nil {
				aerc.PushError(err.Error())
diff --git a/commands/msg/delete.go b/commands/msg/delete.go
index 34eac72..e9dbd93 100644
--- a/commands/msg/delete.go
+++ b/commands/msg/delete.go
@@ -68,7 +68,7 @@ func (Delete) Execute(aerc *widgets.Aerc, args []string) error {
				acct.Messages().Invalidate()
				return nil
			}
			lib.NewMessageStoreView(next, store, aerc.DecryptKeys,
			lib.NewMessageStoreView(next, store,
				func(view lib.MessageView, err error) {
					if err != nil {
						aerc.PushError(err.Error())
diff --git a/commands/msgview/next.go b/commands/msgview/next.go
index 4291a6a..d61300e 100644
--- a/commands/msgview/next.go
+++ b/commands/msgview/next.go
@@ -37,7 +37,7 @@ func (NextPrevMsg) Execute(aerc *widgets.Aerc, args []string) error {
		aerc.RemoveTab(mv)
		return nil
	}
	lib.NewMessageStoreView(nextMsg, store, aerc.DecryptKeys,
	lib.NewMessageStoreView(nextMsg, store,
		func(view lib.MessageView, err error) {
			if err != nil {
				aerc.PushError(err.Error())
diff --git a/lib/messageview.go b/lib/messageview.go
index 532d2c8..740ce3a 100644
--- a/lib/messageview.go
+++ b/lib/messageview.go
@@ -4,10 +4,10 @@ import (
	"bytes"
	"io"
	"io/ioutil"
	"os/exec"

	"github.com/emersion/go-message"
	_ "github.com/emersion/go-message/charset"
	"github.com/emersion/go-pgpmail"
	"golang.org/x/crypto/openpgp"

	"git.sr.ht/~rjarry/aerc/models"
@@ -30,6 +30,7 @@ type MessageView interface {
	// Fetches a specific body part for this message
	FetchBodyPart(part []int, cb func(io.Reader))

	// TODO figure out what parts we actually need
	PGPDetails() *openpgp.MessageDetails
}

@@ -56,13 +57,12 @@ type MessageStoreView struct {
	messageInfo   *models.MessageInfo
	messageStore  *MessageStore
	message       []byte
	details       *openpgp.MessageDetails
	details       *openpgp.MessageDetails // TODO
	bodyStructure *models.BodyStructure
}

func NewMessageStoreView(messageInfo *models.MessageInfo,
	store *MessageStore, decryptKeys openpgp.PromptFunction,
	cb func(MessageView, error)) {
	store *MessageStore, cb func(MessageView, error)) {

	msv := &MessageStoreView{messageInfo, store,
		nil, nil, messageInfo.BodyStructure}
@@ -70,12 +70,18 @@ func NewMessageStoreView(messageInfo *models.MessageInfo,
	if usePGP(messageInfo.BodyStructure) {
		store.FetchFull([]uint32{messageInfo.Uid}, func(fm *types.FullMessage) {
			reader := fm.Content.Reader
			pgpReader, err := pgpmail.Read(reader, Keyring, decryptKeys, nil)
			cmd := exec.Command("gpg", "-d")
			cmd.Stdin = reader
			out, err := cmd.StdoutPipe()

			if err != nil {
				cb(nil, err)
				return
			}
			msv.message, err = ioutil.ReadAll(pgpReader.MessageDetails.UnverifiedBody)
			// TODO should handle this error
			go cmd.Run()

			msv.message, err = ioutil.ReadAll(out)
			if err != nil {
				cb(nil, err)
				return
@@ -91,7 +97,7 @@ func NewMessageStoreView(messageInfo *models.MessageInfo,
				return
			}
			msv.bodyStructure = bs
			msv.details = pgpReader.MessageDetails
			msv.details = &openpgp.MessageDetails{}
			cb(msv, nil)
		})
	} else {
diff --git a/widgets/aerc.go b/widgets/aerc.go
index cbde56c..5fcf922 100644
--- a/widgets/aerc.go
+++ b/widgets/aerc.go
@@ -12,7 +12,6 @@ import (
	"github.com/emersion/go-message/mail"
	"github.com/gdamore/tcell/v2"
	"github.com/google/shlex"
	"golang.org/x/crypto/openpgp"

	"git.sr.ht/~rjarry/aerc/config"
	"git.sr.ht/~rjarry/aerc/lib"
@@ -613,30 +612,6 @@ func (aerc *Aerc) Initialize(ui *ui.UI) {
	aerc.ui = ui
}

func (aerc *Aerc) DecryptKeys(keys []openpgp.Key, symmetric bool) (b []byte, err error) {
	for _, key := range keys {
		ident := key.Entity.PrimaryIdentity()
		chPass, chErr := aerc.GetPassword("Decrypt PGP private key",
			fmt.Sprintf("Enter password for %s (%8X)\nPress <ESC> to cancel",
				ident.Name, key.PublicKey.KeyId))

		for {
			select {
			case err = <-chErr:
				if err != nil {
					return nil, err
				}
				pass := <-chPass
				err = key.PrivateKey.Decrypt([]byte(pass))
				return nil, err
			default:
				aerc.ui.Tick()
			}
		}
	}
	return nil, err
}

// errorScreen is a widget that draws an error in the middle of the context
func errorScreen(s string, conf config.UIConfig) ui.Drawable {
	errstyle := conf.GetStyle(config.STYLE_ERROR)
diff --git a/widgets/msglist.go b/widgets/msglist.go
index 6163d0e..07c0bfa 100644
--- a/widgets/msglist.go
+++ b/widgets/msglist.go
@@ -296,7 +296,7 @@ func (ml *MessageList) MouseEvent(localX int, localY int, event tcell.Event) {
				if msg == nil {
					return
				}
				lib.NewMessageStoreView(msg, store, ml.aerc.DecryptKeys,
				lib.NewMessageStoreView(msg, store,
					func(view lib.MessageView, err error) {
						if err != nil {
							ml.aerc.PushError(err.Error())
-- 
2.34.0
Details
Message ID
<CG8377ATQJS3.1Y858XHAMWNFE@diabtop>
In-Reply-To
<20211204035706.105981-1-mark@markhuge.com> (view parent)
DKIM signature
missing
Download raw message
Hi Mark,

The project has been forked a while ago. In the future, could you please
send patches to the new mailing list as well?

  ~rjarry/aerc-devel@lists.sr.ht

Mark Wilkerson, Dec 04, 2021 at 04:57:
> /x/crypto/openpgp has been deprecated by the golang team. Given the
> current state of the lib, support for things like smartcards may not
> be possible.
>
> See: https://github.com/golang/go/issues/44226

We should seriously think about dropping this dependency from aerc.

> This patch allows for decrypting with existing keyrings and
> smartcards, but doesn't yet populate the fields for signature
> verification. I haven't been able to come up with a solution that
> doesn't involve hacky parsing of the gpg log output (which is also
> annoyingly sent to stderr). Parsing this output also breaks the idea
> of having user specified pgp commands in the config.
>
> The right way to do this is probably to interface with gpg-agent, but
> that's a rabbit hole I don't have the bandwidth to sign up for.
>
> Open to suggestions!

I was afraid of this. However it does not look like we have a chance
since none of the other implementations of pgp do support smart cards.
I don't know gpg-agent well enough to evaluate how much work would be
required.

Would parsing gpg commands be so bad? I know it prevents users from
specifying their own commands but only signature verification would be
affected as far as I can tell. And maybe, it would be enough if we since
gpg exits with a non-zero error code on verification failure:

robin@diabtop:~/git/aerc master$ gpg --verify gpg.patch.asc gpg.patch
gpg: Signature made Mon 06 Dec 2021 09:31:22 AM CET
gpg:                using EDDSA key D8B6A9907C3AB720428B606862718E0D666FC335
gpg: checking the trustdb
gpg: marginals needed: 3  completes needed: 1  trust model: pgp
gpg: depth: 0  valid:   1  signed:   0  trust: 0-, 0q, 0n, 0m, 0f, 1u
gpg: Good signature from "Robin Jarry <robin@jarry.cc>" [ultimate]
Primary key fingerprint: DC07 18E3 22E2 C760 5EBD  C831 4695 7EC0 8FD0 FE90
     Subkey fingerprint: D8B6 A990 7C3A B720 428B  6068 6271 8E0D 666F C335
robin@diabtop:~/aerc master$ echo $?
0
robin@diabtop:~/git/aerc master$ echo xxxxxxxxxxxxxxxxxxxxxx>> pouet.patch
robin@diabtop:~/git/aerc master$ gpg --verify pouet.patch.asc pouet.patch
gpg: Signature made Mon 06 Dec 2021 09:31:22 AM CET
gpg:                using EDDSA key D8B6A9907C3AB720428B606862718E0D666FC335
gpg: BAD signature from "Robin Jarry <robin@jarry.cc>" [ultimate]
robin@diabtop:~/git/aerc master$ echo $?
1

I know this does not provide any info from the signature and it is not
really suitable for automation. Did you look at gpgv(1)?

For other operations (signature, decryption, encryption), I don't think
any output parsing would be required.

> diff --git a/aerc.go b/aerc.go
> index b3338ba..dd08619 100644
> --- a/aerc.go
> +++ b/aerc.go
> @@ -187,10 +187,6 @@ func main() {
>  		ui.EnableMouse()
>  	}
>  
> -	logger.Println("Initializing PGP keyring")
> -	lib.InitKeyring()
> -	defer lib.UnlockKeyring()
> -

If you are dropping the keyring, I think more cleanup is needed (see
lib/keystore.go).

Also, since this is a major (breaking?) change, explicit mention in the
docs are mandatory.

> diff --git a/lib/messageview.go b/lib/messageview.go
> index 532d2c8..740ce3a 100644
> --- a/lib/messageview.go
> +++ b/lib/messageview.go
[snip]
> @@ -70,12 +70,18 @@ func NewMessageStoreView(messageInfo *models.MessageInfo,
>  	if usePGP(messageInfo.BodyStructure) {
>  		store.FetchFull([]uint32{messageInfo.Uid}, func(fm *types.FullMessage) {
>  			reader := fm.Content.Reader
> -			pgpReader, err := pgpmail.Read(reader, Keyring, decryptKeys, nil)
> +			cmd := exec.Command("gpg", "-d")
> +			cmd.Stdin = reader
> +			out, err := cmd.StdoutPipe()
> +

For now, no custom commands. Do you plan on adding these in a separate
patch?

>  			if err != nil {
>  				cb(nil, err)
>  				return
>  			}
> -			msv.message, err = ioutil.ReadAll(pgpReader.MessageDetails.UnverifiedBody)
> +			// TODO should handle this error
> +			go cmd.Run()
> +
> +			msv.message, err = ioutil.ReadAll(out)
>  			if err != nil {
>  				cb(nil, err)
>  				return
> @@ -91,7 +97,7 @@ func NewMessageStoreView(messageInfo *models.MessageInfo,
>  				return
>  			}
>  			msv.bodyStructure = bs
> -			msv.details = pgpReader.MessageDetails
> +			msv.details = &openpgp.MessageDetails{}
>  			cb(msv, nil)
>  		})
>  	} else {

Exit code should be checked here I guess.

Thanks a lot for working on this!
Reply to thread Export thread (mbox)