[PATCH] cli: crypto: tell gmime to use gpg-agent

[PATCH] cli: crypto: tell gmime to use gpg-agent

[Jani Nikula]

For decryption, we expect there to be a functioning gpg-agent, and we
want gpg to talk to it for any needed credentials. There's a gmime
function to declare that: g_mime_gpg_context_set_use_agent() [1], [2].
Start using it.

I had gpg-agent running, but gpg "use-agent" configuration option
disabled. This resulted in an error message from 'notmuch show':

  Failed to decrypt part: Canceled.

and json had this:

  "encstatus" : [ { "status" : "bad" } ]

One could argue the "use-agent" option should be enabled, but I'd like
to use the agent only as a last resort. I think that's irrelevant
though. There's a gmime function to declare what we expect, so we
should use it. Conveniently it also fixes the problem in a user
friendly way.

[1] http://git.gnome.org/browse/gmime/commit/?id=ed985397843a9da3745a8b5de3d1d652acd24724
[2] https://bugzilla.gnome.org/show_bug.cgi?id=651826
---
 crypto.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/crypto.c b/crypto.c
index fbe5aeb..cb361e1 100644
--- a/crypto.c
+++ b/crypto.c
@@ -45,6 +45,9 @@ notmuch_crypto_get_context (notmuch_crypto_t *crypto, const char *protocol)
 	    g_object_unref (session);
 #endif
 	    if (crypto->gpgctx) {
+#ifdef GMIME_ATLEAST_26
+		g_mime_gpg_context_set_use_agent ((GMimeGpgContext*) crypto->gpgctx, TRUE);
+#endif
 		g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) crypto->gpgctx, FALSE);
 	    } else {
 		fprintf (stderr, "Failed to construct gpg context.\n");
-- 
1.7.10.4

Re: [PATCH] cli: crypto: tell gmime to use gpg-agent

[Tomi Ollila]

On Wed, Feb 27 2013, Jani Nikula <jani@nikula.org> wrote:

> For decryption, we expect there to be a functioning gpg-agent, and we
> want gpg to talk to it for any needed credentials. There's a gmime
> function to declare that: g_mime_gpg_context_set_use_agent() [1], [2].
> Start using it.
>
> I had gpg-agent running, but gpg "use-agent" configuration option
> disabled. This resulted in an error message from 'notmuch show':
>
>   Failed to decrypt part: Canceled.
>
> and json had this:
>
>   "encstatus" : [ { "status" : "bad" } ]
>
> One could argue the "use-agent" option should be enabled, but I'd like
> to use the agent only as a last resort. I think that's irrelevant
> though. There's a gmime function to declare what we expect, so we
> should use it. Conveniently it also fixes the problem in a user
> friendly way.

I agree fully. The code looks good to me.

Tomi

>
> [1] http://git.gnome.org/browse/gmime/commit/?id=ed985397843a9da3745a8b5de3d1d652acd24724
> [2] https://bugzilla.gnome.org/show_bug.cgi?id=651826
> ---
>  crypto.c |    3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/crypto.c b/crypto.c
> index fbe5aeb..cb361e1 100644
> --- a/crypto.c
> +++ b/crypto.c
> @@ -45,6 +45,9 @@ notmuch_crypto_get_context (notmuch_crypto_t *crypto, const char *protocol)
>  	    g_object_unref (session);
>  #endif
>  	    if (crypto->gpgctx) {
> +#ifdef GMIME_ATLEAST_26
> +		g_mime_gpg_context_set_use_agent ((GMimeGpgContext*) crypto->gpgctx, TRUE);
> +#endif
>  		g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) crypto->gpgctx, FALSE);
>  	    } else {
>  		fprintf (stderr, "Failed to construct gpg context.\n");
> -- 
> 1.7.10.4

Re: [PATCH] cli: crypto: tell gmime to use gpg-agent

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 1617 bytes --]

On Tue, Feb 26 2013, Jani Nikula <jani@nikula.org> wrote:
> For decryption, we expect there to be a functioning gpg-agent, and we
> want gpg to talk to it for any needed credentials. There's a gmime
> function to declare that: g_mime_gpg_context_set_use_agent() [1], [2].
> Start using it.
>
> I had gpg-agent running, but gpg "use-agent" configuration option
> disabled. This resulted in an error message from 'notmuch show':
>
>   Failed to decrypt part: Canceled.
>
> and json had this:
>
>   "encstatus" : [ { "status" : "bad" } ]
>
> One could argue the "use-agent" option should be enabled, but I'd like
> to use the agent only as a last resort. I think that's irrelevant
> though. There's a gmime function to declare what we expect, so we
> should use it. Conveniently it also fixes the problem in a user
> friendly way.

I will argue that the "use-agent" option should be enabled.  If we force
use of gpg-agent, then we don't allow people to opt out of using it.
That's not very user friendly, particularly if someone has not enabled
it for a specific reason.

But I think more to the point we need a little bit of due diligence of
the effects of this before we enable it.  What happens if gpg-agent is
not available?  What happens if there is no X session?  Tests that probe
the various circumstances would be useful.

I do note, though, that the error messages are not very useful.  It
would be nice if could figure out that the decryption failed because of
lack of agent and inform the user of that.

We should probably also update the show man page to make explicit that
an agent may be required.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] cli: crypto: tell gmime to use gpg-agent

[David Bremner]

Jameson Graef Rollins <jrollins@finestructure.net> writes:

> I will argue that the "use-agent" option should be enabled.  If we force
> use of gpg-agent, then we don't allow people to opt out of using it.
> That's not very user friendly, particularly if someone has not enabled
> it for a specific reason.

But right now we force people to enable the agent globally via use-agent
if they want to decrypt mail in notmuch-cli/emacs. The proposed change
allows them to use the agent only for notmuch.

> But I think more to the point we need a little bit of due diligence of
> the effects of this before we enable it.  What happens if gpg-agent is
> not available?  What happens if there is no X session?  Tests that probe
> the various circumstances would be useful.

I don't think we should directly care about the presence of an X session
or not; the agent protocol doesn't depend on how the agent was started
afaik. 

> I do note, though, that the error messages are not very useful.  It
> would be nice if could figure out that the decryption failed because of
> lack of agent and inform the user of that.

Yes, it would be nice to detect a missing/non-responsive agent. And that
could be used by tests.

> We should probably also update the show man page to make explicit that
> an agent may be required.

We probably need to word it more strongly than that.  If the user wants
decryption then notmuch requires an agent; if they want encryption or
signing then message-mode (really probably easypg) requires an agent.
I'm not sure how it manages it, but according to Jani's experiments it
seems that message mode already uses the agent independently of the
user's config; perhaps via the '--use-agent' argument to gpg.  So the
current situation is unfortunately asymmetric.

Of course it would help if there was documentation for the emacs
interface that we could update.

Re: [PATCH] cli: crypto: tell gmime to use gpg-agent

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 776 bytes --]

On Wed, Feb 27 2013, David Bremner <david@tethera.net> wrote:
> But right now we force people to enable the agent globally via use-agent
> if they want to decrypt mail in notmuch-cli/emacs. The proposed change
> allows them to use the agent only for notmuch.

Doesn't the proposed change actually *force* the user to use gpg-agent?
How can the user opt out?

> I don't think we should directly care about the presence of an X session
> or not; the agent protocol doesn't depend on how the agent was started
> afaik. 

Maybe, but I would like some example of what happens if you force usage
of an agent and the agent is not present or there is no X session.

All I'm saying is that I would like to see how this patch interacts with
these situations.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] cli: crypto: tell gmime to use gpg-agent

[Jani Nikula]

On Wed, 27 Feb 2013, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> On Wed, Feb 27 2013, David Bremner <david@tethera.net> wrote:
>> But right now we force people to enable the agent globally via use-agent
>> if they want to decrypt mail in notmuch-cli/emacs. The proposed change
>> allows them to use the agent only for notmuch.
>
> Doesn't the proposed change actually *force* the user to use gpg-agent?
> How can the user opt out?

If the user wants to have decryption in notmuch, the user *must* use
gpg-agent, regardless of this change or the "use-agent" configuration
option. There is no opt out if one wants to have decryption in notmuch,
regardless of this change.

The proposed change gives the user the possibility to opt out of
*globally* using gpg-agent for everything, and still have decryption in
notmuch.

The proposed change merely passes the --use-agent option to gpg. It does
not *force* anything. It tells gpg to *try* to connect to the gpg-agent
before it asks for a passphrase. (Except that notmuch will never ask for
a passphrase. It will fail if it can't connect to the gpg-agent. Without
--use-agent or "use-agent" option it will unconditionally fail.)

When I use gpg on the command line, I want it to prompt for the
passphrase on the command line instead of popping up a gpg-agent
dialog. I don't think that is unreasonable. To achieve that I have
disabled the "use-agent" configuration option. Without the proposed
change, if I still wanted to have this *and* decryption in notmuch, I
would have to pass --no-use-agent on the gpg command line. I think that
*is* unreasonable.

>> I don't think we should directly care about the presence of an X session
>> or not; the agent protocol doesn't depend on how the agent was started
>> afaik. 
>
> Maybe, but I would like some example of what happens if you force usage
> of an agent and the agent is not present or there is no X session.

There is no force anything. It tries to connect to the agent, and if one
is not present, decryption fails like it would have failed without this
change.

Finally, look up the references I provided. The whole function in gmime
was provided *exactly* for situations like we have: the caller will fail
without the agent, so have a tiny bit of sanity and see if it's there
before failing.


BR,
Jani.

Re: [PATCH] cli: crypto: tell gmime to use gpg-agent

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 664 bytes --]

On Wed, Feb 27 2013, Jani Nikula <jani@nikula.org> wrote:
> If the user wants to have decryption in notmuch, the user *must* use
> gpg-agent, regardless of this change or the "use-agent" configuration
> option. There is no opt out if one wants to have decryption in notmuch,
> regardless of this change.

I think this point makes sense.  I guess my main worry was that querying
the agent would have unintended consequences for someone who is not
expecting it or specifically does not want it to be queried.  But maybe
the point is that if they don't want that to happen, then they should
just not call notmuch with --decrypt.  I suppose that's reasonable.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] cli: crypto: tell gmime to use gpg-agent

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 538 bytes --]

On 02/28/2013 04:10 PM, Jameson Graef Rollins wrote:
> I think this point makes sense.  I guess my main worry was that querying
> the agent would have unintended consequences for someone who is not
> expecting it or specifically does not want it to be queried.  But maybe
> the point is that if they don't want that to happen, then they should
> just not call notmuch with --decrypt.  I suppose that's reasonable.

I agree with this analysis.  automatically asking gmime to invoke the
agent is the right thing to do.

	--dkg


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 1027 bytes --]

Re: [PATCH] cli: crypto: tell gmime to use gpg-agent

[Tomi Ollila]

On Fri, Mar 01 2013, Daniel Kahn Gillmor <dkg@fifthhorseman.net> wrote:

> On 02/28/2013 04:10 PM, Jameson Graef Rollins wrote:
>> I think this point makes sense.  I guess my main worry was that querying
>> the agent would have unintended consequences for someone who is not
>> expecting it or specifically does not want it to be queried.  But maybe
>> the point is that if they don't want that to happen, then they should
>> just not call notmuch with --decrypt.  I suppose that's reasonable.
>
> I agree with this analysis.  automatically asking gmime to invoke the
> agent is the right thing to do.
>
> 	--dkg

4(5?) in favo(u)r, 0 against. dropped needs-review. Speak up if you disagree :D

Tomi

[PATCH] man: show and reply --decrypt option requires gpg-agent

[Jani Nikula]

---
 man/man1/notmuch-reply.1 |    9 ++++++---
 man/man1/notmuch-show.1  |    9 +++++++--
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/man/man1/notmuch-reply.1 b/man/man1/notmuch-reply.1
index 454bdee..bf2021f 100644
--- a/man/man1/notmuch-reply.1
+++ b/man/man1/notmuch-reply.1
@@ -89,9 +89,12 @@ user's addresses.
 
 Decrypt any MIME encrypted parts found in the selected content
 (ie. "multipart/encrypted" parts). Status of the decryption will be
-reported (currently only supported with --format=json and --format=sexp)
-and the multipart/encrypted part will be replaced by the decrypted
-content.
+reported (currently only supported with --format=json and
+--format=sexp) and on successful decryption the multipart/encrypted
+part will be replaced by the decrypted content.
+
+Decryption expects a functioning \fBgpg-agent\fR(1) to provide any
+needed credentials. Without one, the decryption will fail.
 .RE
 
 See \fBnotmuch-search-terms\fR(7)
diff --git a/man/man1/notmuch-show.1 b/man/man1/notmuch-show.1
index 8be9eae..7697dfc 100644
--- a/man/man1/notmuch-show.1
+++ b/man/man1/notmuch-show.1
@@ -163,8 +163,13 @@ will be replaced by the signed data.
 Decrypt any MIME encrypted parts found in the selected content
 (ie. "multipart/encrypted" parts). Status of the decryption will be
 reported (currently only supported with --format=json and
---format=sexp) and the multipart/encrypted part will be replaced
-by the decrypted content.  Implies --verify.
+--format=sexp) and on successful decryption the multipart/encrypted
+part will be replaced by the decrypted content.
+
+Decryption expects a functioning \fBgpg-agent\fR(1) to provide any
+needed credentials. Without one, the decryption will fail.
+
+Implies --verify.
 .RE
 
 .RS 4
-- 
1.7.10.4

Re: [PATCH] man: show and reply --decrypt option requires gpg-agent

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 2001 bytes --]

On Fri, Mar 01 2013, Jani Nikula <jani@nikula.org> wrote:
> ---
>  man/man1/notmuch-reply.1 |    9 ++++++---
>  man/man1/notmuch-show.1  |    9 +++++++--
>  2 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/man/man1/notmuch-reply.1 b/man/man1/notmuch-reply.1
> index 454bdee..bf2021f 100644
> --- a/man/man1/notmuch-reply.1
> +++ b/man/man1/notmuch-reply.1
> @@ -89,9 +89,12 @@ user's addresses.
>  
>  Decrypt any MIME encrypted parts found in the selected content
>  (ie. "multipart/encrypted" parts). Status of the decryption will be
> -reported (currently only supported with --format=json and --format=sexp)
> -and the multipart/encrypted part will be replaced by the decrypted
> -content.
> +reported (currently only supported with --format=json and
> +--format=sexp) and on successful decryption the multipart/encrypted
> +part will be replaced by the decrypted content.
> +
> +Decryption expects a functioning \fBgpg-agent\fR(1) to provide any
> +needed credentials. Without one, the decryption will fail.
>  .RE
>  
>  See \fBnotmuch-search-terms\fR(7)
> diff --git a/man/man1/notmuch-show.1 b/man/man1/notmuch-show.1
> index 8be9eae..7697dfc 100644
> --- a/man/man1/notmuch-show.1
> +++ b/man/man1/notmuch-show.1
> @@ -163,8 +163,13 @@ will be replaced by the signed data.
>  Decrypt any MIME encrypted parts found in the selected content
>  (ie. "multipart/encrypted" parts). Status of the decryption will be
>  reported (currently only supported with --format=json and
> ---format=sexp) and the multipart/encrypted part will be replaced
> -by the decrypted content.  Implies --verify.
> +--format=sexp) and on successful decryption the multipart/encrypted
> +part will be replaced by the decrypted content.
> +
> +Decryption expects a functioning \fBgpg-agent\fR(1) to provide any
> +needed credentials. Without one, the decryption will fail.
> +
> +Implies --verify.
>  .RE
>  
>  .RS 4
> -- 
> 1.7.10.4

LGTM.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] cli: crypto: tell gmime to use gpg-agent

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> For decryption, we expect there to be a functioning gpg-agent, and we
> want gpg to talk to it for any needed credentials. There's a gmime
> function to declare that: g_mime_gpg_context_set_use_agent() [1], [2].
> Start using it.

Pushed this, and the followup man page patch.