[PATCH] gpg-agent storage - add support for /run based sockets V2

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] gpg-agent storage - add support for /run based sockets V2

Lukas Jirkovsky
New version. Instead of reimplementing the gpg-agent logic, "gpgconf
--list-dir agent-socket" is used. If the gpgconf fails, the code falls
back to the original solution to stay compatible with old Gnupg
versions.

[[[
Find gpg-agent socket using gpgconf if possible.
This allows detection of socket with Gnupg >= 2.1.13
which changed the default socket path to /run/user/UID/gnupg

* subversion/libsvn_subr/gpg_agent.c
    (find_gpgconf_agent_socket): new function to find
    gpg-agent socket using gpgconf
    (find_running_gpg_agent): use find_gpgconf_agent_socket
    to detect socket when possible.
]]]

gpg_agent_socket_paths_v2.diff (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] gpg-agent storage - add support for /run based sockets V2

Stefan Sperling
On Wed, May 03, 2017 at 10:05:01PM +0200, Lukas Jirkovsky wrote:
> New version. Instead of reimplementing the gpg-agent logic, "gpgconf
> --list-dir agent-socket" is used. If the gpgconf fails, the code falls
> back to the original solution to stay compatible with old Gnupg
> versions.

Thanks Lukas, this patch looks good to me.

I have just one minor stylistic nit:
The local scratch_pool created in your new function is not necessary.
This code could be simplified further by just using 'pool' throughout.

In any case, I would not object to having your patch committed as it is.
The scratch_pool is just a small detail.

It would be good if you could send patches as a unidiff in the future.
'svn diff' produces such output by default, and 'diff -u' works, too.

Regards,
Stefan
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] gpg-agent storage - add support for /run based sockets V2

Lukas Jirkovsky
On 3 May 2017 at 22:35, Stefan Sperling <[hidden email]> wrote:

> On Wed, May 03, 2017 at 10:05:01PM +0200, Lukas Jirkovsky wrote:
>> New version. Instead of reimplementing the gpg-agent logic, "gpgconf
>> --list-dir agent-socket" is used. If the gpgconf fails, the code falls
>> back to the original solution to stay compatible with old Gnupg
>> versions.
>
> Thanks Lukas, this patch looks good to me.
>
> I have just one minor stylistic nit:
> The local scratch_pool created in your new function is not necessary.
> This code could be simplified further by just using 'pool' throughout.
Hello Stefan,
thank you for comments. Attached is the updated version without scratch_pool.

> In any case, I would not object to having your patch committed as it is.
> The scratch_pool is just a small detail.
>
> It would be good if you could send patches as a unidiff in the future.
> 'svn diff' produces such output by default, and 'diff -u' works, too.

No problem, will do. I was just blindly following the Patch submission
guidelines from the Community Guide. I like the unified diff output
more as well.

gpg_agent_socket_paths_v3.diff (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] gpg-agent storage - add support for /run based sockets V2

James McCoy-3
On Wed, May 03, 2017 at 11:03:42PM +0200, Lukas Jirkovsky wrote:
> On 3 May 2017 at 22:35, Stefan Sperling <[hidden email]> wrote:
> > On Wed, May 03, 2017 at 10:05:01PM +0200, Lukas Jirkovsky wrote:
> >> New version. Instead of reimplementing the gpg-agent logic, "gpgconf
> >> --list-dir agent-socket" is used. If the gpgconf fails, the code falls
> >> back to the original solution to stay compatible with old Gnupg
> >> versions.

I apologize for not looking into this more before recommending it.  The
ability to specify a name to --list-dir is very recent (only since
2.1.14).  All the other 2.1.x releases, and the entire 2.0.x series,
don't support an argument to --list-dir.  They just print out all the
items.

$ gpgconf --list-dir
...
agent-socket:/run/user/1000/gnupg/S.gpg-agent
...

It's still a stable, easy to parse format, but it does require parsing
instead of just using the output directly.

Cheers,
--
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7  2D23 DFE6 91AE 331B A3DB
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] gpg-agent storage - add support for /run based sockets V2

Daniel Shahaf-2
In reply to this post by Lukas Jirkovsky
I have nothing to add to Stefan and James' review of functionality, so
here are a couple of style suggestions:

Lukas Jirkovsky wrote on Wed, May 03, 2017 at 23:03:42 +0200:
> +++ subversion/libsvn_subr/gpg_agent.c (working copy)
> @@ -225,6 +228,46 @@
> +find_gpgconf_agent_socket(apr_pool_t *pool)
> +{
> +  apr_proc_t proc;
> +  svn_stringbuf_t *line;
> +  svn_error_t *err;
> +  const char *gpgargv[] = { "gpgconf", "--list-dir", "agent-socket", NULL };
> +

You could add the second const here (after the asterisk) to remove the
need for the cast?

> +  /* execute "gpgconf --list-dir agent-socket" */
> +  err = svn_io_start_cmd3(&proc, NULL, "gpgconf", (const char* const*)gpgargv,
> +                          NULL, TRUE, FALSE, NULL, TRUE, NULL, FALSE, NULL,
> +                          pool);

Add "/* inherit */", "/* infile_pipe */", etc comments after the
booleans?  (That's how we simulate Python's keyword arguments in C.)

Not trying to nitpick; just to make the code easier to read.

Cheers,

Daniel
Loading...