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

[James McCoy]

On Wed, May 03, 2017 at 11:03:42PM +0200, Lukas Jirkovsky wrote:
> On 3 May 2017 at 22:35, Stefan Sperling  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

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

[Lukas Jirkovsky]

On 3 May 2017 at 22:35, Stefan Sperling  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.
Index: subversion/libsvn_subr/gpg_agent.c
===
--- subversion/libsvn_subr/gpg_agent.c  (revision 1793701)
+++ subversion/libsvn_subr/gpg_agent.c  (working copy)
@@ -64,10 +64,13 @@
 #include 
 #include 
 
+#include 
 #include 
+#include 
 #include "svn_auth.h"
 #include "svn_config.h"
 #include "svn_error.h"
+#include "svn_io.h"
 #include "svn_pools.h"
 #include "svn_cmdline.h"
 #include "svn_checksum.h"
@@ -225,6 +228,46 @@
   close(sd);
 }
 
+/* Find gpg-agent socket location using gpgconf. Returns the path to socket, or
+ * NULL if the socket path cannot be determined using gpgconf.
+ */
+static const char *
+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 };
+
+  /* execute "gpgconf --list-dir agent-socket" */
+  err = svn_io_start_cmd3(, NULL, "gpgconf", (const char* const*)gpgargv,
+  NULL, TRUE, FALSE, NULL, TRUE, NULL, FALSE, NULL,
+  pool);
+  if (err != SVN_NO_ERROR)
+{
+  svn_error_clear(err);
+  return NULL;
+}
+
+  /* read the gpgconf output */
+  err = svn_io_file_readline(proc.out, , NULL, NULL, APR_SIZE_MAX,
+ pool, pool);
+  if (err != SVN_NO_ERROR)
+{
+  svn_error_clear(err);
+  return NULL;
+}
+  apr_file_close(proc.out);
+  err = svn_io_wait_for_cmd(, "gpgconf", NULL, NULL, pool);
+  if (err != SVN_NO_ERROR)
+{
+  svn_error_clear(err);
+  return NULL;
+}
+
+  return line->data;
+}
+
 /* Locate a running GPG Agent, and return an open file descriptor
  * for communication with the agent in *NEW_SD. If no running agent
  * can be found, set *NEW_SD to -1. */
@@ -242,37 +285,43 @@
 
   *new_sd = -1;
 
-  /* This implements the method of finding the socket as described in
-   * the gpg-agent man page under the --use-standard-socket option.
-   * The manage page says the standard socket is "named 'S.gpg-agent' located
-   * in the home directory."  GPG's home directory is either the directory
-   * specified by $GNUPGHOME or ~/.gnupg. */
-  gpg_agent_info = getenv("GPG_AGENT_INFO");
-  if (gpg_agent_info != NULL)
-{
-  apr_array_header_t *socket_details;
+  /* Query socket location using gpgconf if possible */
+  socket_name = find_gpgconf_agent_socket(pool);
 
-  /* For reference GPG_AGENT_INFO consists of 3 : separated fields.
-   * The path to the socket, the pid of the gpg-agent process and
-   * finally the version of the protocol the agent talks. */
-  socket_details = svn_cstring_split(gpg_agent_info, ":", TRUE,
- pool);
-  socket_name = APR_ARRAY_IDX(socket_details, 0, const char *);
-}
-  else if ((gnupghome = getenv("GNUPGHOME")) != NULL)
-{
-  const char *homedir = svn_dirent_canonicalize(gnupghome, pool);
-  socket_name = svn_dirent_join(homedir, "S.gpg-agent", pool);
-}
-  else
+  /* fallback to the old method used with Gnupg 1.x */
+  if (socket_name == NULL)
 {
-  const char *homedir = svn_user_get_homedir(pool);
+  /* This implements the method of finding the socket as described in
+   * the gpg-agent man page under the --use-standard-socket option.
+   * The manage page says the standard socket is "named 'S.gpg-agent' 
located
+   * in the home directory."  GPG's home directory is either the directory
+   * specified by $GNUPGHOME or ~/.gnupg. */
+  if ((gpg_agent_info = getenv("GPG_AGENT_INFO")) != NULL)
+{
+  apr_array_header_t *socket_details;
 
-  if (!homedir)
-return SVN_NO_ERROR;
+  /* For reference GPG_AGENT_INFO consists of 3 : separated fields.
+   * The path to the socket, the pid of the gpg-agent 

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

[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.
]]]
Index: libsvn_subr/gpg_agent.c
===
*** libsvn_subr/gpg_agent.c (revision 1793701)
--- libsvn_subr/gpg_agent.c (working copy)
***
*** 64,73 
--- 64,76 
  #include 
  #include 
  
+ #include 
  #include 
+ #include 
  #include "svn_auth.h"
  #include "svn_config.h"
  #include "svn_error.h"
+ #include "svn_io.h"
  #include "svn_pools.h"
  #include "svn_cmdline.h"
  #include "svn_checksum.h"
*** bye_gpg_agent(int sd)
*** 225,230 
--- 228,280 
close(sd);
  }
  
+ /* Find gpg-agent socket location using gpgconf. Returns the path to socket, 
or
+  * NULL if the socket path cannot be determined using gpgconf.
+  */
+ static const char *
+ find_gpgconf_agent_socket(apr_pool_t *pool)
+ {
+   apr_pool_t *scratch_pool;
+   apr_proc_t proc;
+   svn_stringbuf_t *line;
+   svn_error_t *err;
+   const char *gpgargv[] = { "gpgconf", "--list-dir", "agent-socket", NULL };
+ 
+   scratch_pool = svn_pool_create(pool);
+ 
+   /* execute "gpgconf --list-dir agent-socket" */
+   err = svn_io_start_cmd3(, NULL, "gpgconf", (const char* const*)gpgargv,
+   NULL, TRUE, FALSE, NULL, TRUE, NULL, FALSE, NULL,
+   scratch_pool);
+   if (err != SVN_NO_ERROR)
+ {
+   svn_pool_destroy(scratch_pool);
+   svn_error_clear(err);
+   return NULL;
+ }
+ 
+   /* read the gpgconf output */
+   err = svn_io_file_readline(proc.out, , NULL, NULL, APR_SIZE_MAX, pool,
+  scratch_pool);
+   if (err != SVN_NO_ERROR)
+ {
+   svn_pool_destroy(scratch_pool);
+   svn_error_clear(err);
+   return NULL;
+ }
+   apr_file_close(proc.out);
+   err = svn_io_wait_for_cmd(, "gpgconf", NULL, NULL, scratch_pool);
+   if (err != SVN_NO_ERROR)
+ {
+   svn_pool_destroy(scratch_pool);
+   svn_error_clear(err);
+   return NULL;
+ }
+ 
+   svn_pool_destroy(scratch_pool);
+   return line->data;
+ }
+ 
  /* Locate a running GPG Agent, and return an open file descriptor
   * for communication with the agent in *NEW_SD. If no running agent
   * can be found, set *NEW_SD to -1. */
*** find_running_gpg_agent(int *new_sd, apr_
*** 242,278 
  
*new_sd = -1;
  
!   /* This implements the method of finding the socket as described in
!* the gpg-agent man page under the --use-standard-socket option.
!* The manage page says the standard socket is "named 'S.gpg-agent' located
!* in the home directory."  GPG's home directory is either the directory
!* specified by $GNUPGHOME or ~/.gnupg. */
!   gpg_agent_info = getenv("GPG_AGENT_INFO");
!   if (gpg_agent_info != NULL)
! {
!   apr_array_header_t *socket_details;
  
!   /* For reference GPG_AGENT_INFO consists of 3 : separated fields.
!* The path to the socket, the pid of the gpg-agent process and
!* finally the version of the protocol the agent talks. */
!   socket_details = svn_cstring_split(gpg_agent_info, ":", TRUE,
!  pool);
!   socket_name = APR_ARRAY_IDX(socket_details, 0, const char *);
! }
!   else if ((gnupghome = getenv("GNUPGHOME")) != NULL)
! {
!   const char *homedir = svn_dirent_canonicalize(gnupghome, pool);
!   socket_name = svn_dirent_join(homedir, "S.gpg-agent", pool);
! }
!   else
  {
!   const char *homedir = svn_user_get_homedir(pool);
  
!   if (!homedir)
! return SVN_NO_ERROR;
  
!   socket_name = svn_dirent_join_many(pool, homedir, ".gnupg",
!  "S.gpg-agent", SVN_VA_NULL);
  }
  
if (socket_name != NULL)
--- 292,334 
  
*new_sd = -1;
  
!   /* Query socket location using gpgconf if possible */
!   socket_name = find_gpgconf_agent_socket(pool);
  
!   /* fallback to the old method used with Gnupg 1.x */
!   if (socket_name == NULL)
  {
!   /* This implements the method of finding the socket as described in
!* the gpg-agent man page under the --use-standard-socket option.
!* The manage page says the standard socket is "named 'S.gpg-agent' 
located
!* in the home directory."  GPG's home directory is either the directory
!* specified by $GNUPGHOME or ~/.gnupg. */
!   if ((gpg_agent_info = getenv("GPG_AGENT_INFO")) != NULL)
! 

Re: wildcard authz docs question

[Doug Robinson]

Daniel:

On Mon, May 1, 2017 at 2:05 PM, Daniel Shahaf 
wrote:

> Doug Robinson wrote on Mon, May 01, 2017 at 14:20:16 +:
> > On Mon, Apr 17, 2017 at 21:13 Daniel Shahaf 
> wrote:
> > > Stefan Fuhrmann wrote on Mon, Apr 17, 2017 at 22:22:33 +0200:
> > > > On 15.03.2017 10:55, Daniel Shahaf wrote:
> > > > >>From the 1.10 draft release notes:
> > > > >
> > > > >>All wildcards apply to full path segments only, i.e. * never
> matches
> > > > >>/, except for the case where /**/ matches zero or more path
> segments.
> > > > >>For example, /*/**/* will match any path which contains at least
> > > > >>2 segments and is equivalent to /**/*/* as well as /*/*/**.
> > > > >Are «/*/**/*» «/**/*/*» «/*/*/**» really equivalent?  I would have
> > > > >expected the first two to match any node except / and /'s immediate
> > > > >children, but I wouldn't expect the third form to match /trunk/iota
> > > > >where iota is a file, since the pattern has a trailing slash after
> the
> > > > >non-optional second component.
> > > > How do you know that /trunk/iota is a file?
> > >
> > > I was reviewing the API docs as a black box, i.e., from a user
> > > (repository admin) perspective, not from an implementation perspective.
> > >
> > >  From that perspective, I would say that having a [/trunk/iota/**]
> > > stanza to apply to a /trunk/iota file violates the principle of least
> > > surprise.
> >
> >
> > From a very critical point of view I agree.
> >
> > However, the point of wildcards is to easily reserve a complete
> namespace.
>
> Sure, that's a valid use-case.
>
> I was envisioning that, if a [/trunk/iota/**] stanza were present, then
> an authz query for a /trunk/iota file would return either "No access" or
> a parse error.  This would reserve the namespace, wouldn't it?  Referring
> to your next paragraph, this logic would neither leak the contents of
> the file nor require multiple stanzas.
>

For an AuthZ check the answer is either Yes or No, not "parser error",
right?

And it really can't be a "parser error" (invalidating the AuthZ file
entirely) since
in some other revision that "file" could be a "directory".  So either the
stanza
gets skipped as "not applicable" (and therefore not reserving the namespace)
or it gets entered as if the file were a directory and we're back to the
behavior
that I am expecting.


> > If we do not apply that stanza apply to the file means requiring 2
> stanzas
> > to cover the space entirely. That's both expensive and brittle (2X
> stanzas
> > and requires remembering to treat them in pairs - both when adding and
> when
> > removing).
> >
> > And I think the "surprise" will be very short-lived if at all.
> >
> > From a cost/benefit standpoint I think it is extremely positive.
>
> Well, if a common task requires two stanzas, then _of course_ we'll find
> an easier way for users to spell it.  For example, we could invent some
> new "reserve prefix" stanza syntax, or pass to
> svn_repos_authz_check_access()
> the svn_node_kind_t of the path it checks access to, or any number of
> other solutions.
>
> In short: there might well be a design that meets both of our criteria:
> principle of least surprise _and_ namespace reservation.
>

Not seeing it - at least not yet.  In Perl the RE needed to handle this
would
be one of the duals, e.g. "/trunk/iota(|/.*)" - the either/or with nothing
on the left
and "/.*" on the right.  It really is a dual case.  I know of no better
syntax.  Since
we're working on this as a wildcard I don't see an alternative.

As I said, I think the surprise, if any (none if we document it well) will
be
very short-lived.

Cheers.

Doug

-- 
*DOUGLAS B ROBINSON* SENIOR PRODUCT MANAGER

T +1 925 396 1125
*E* doug.robin...@wandisco.com

-- 


World Leader in Active Data Replication™
*Find out more wandisco.com *

THIS MESSAGE AND ANY ATTACHMENTS ARE CONFIDENTIAL, PROPRIETARY AND MAY BE 
PRIVILEGED

If this message was misdirected, WANdisco, Inc. and its subsidiaries, 
("WANdisco") does not waive any confidentiality or privilege. If you are 
not the intended recipient, please notify us immediately and destroy the 
message without disclosing its contents to anyone. Any distribution, use or 
copying of this email or the information it contains by other than an 
intended recipient is unauthorized. The views and opinions expressed in 
this email message are the author's own and may not reflect the views and 
opinions of WANdisco, unless the author is authorized by WANdisco to 
express such views or opinions on its behalf. All email sent to or from 
this address is subject to electronic storage and review by WANdisco. 
Although WANdisco operates anti-virus programs, it does not accept 
responsibility for any damage whatsoever caused by viruses being passed.

Umlauts on OS X

[Thomas Singer]

Hi all,

There is the well-known umlaut bug on OS X in SVN since 2005



Are there any plans to finally solve this topic somehow in the near 
future (e.g. for 1.10) or are there too much concerns regarding 
upgrading existing repositories and breaking something?


Maybe it would be a good time to treat 1.10 as a large bug-fix release 
(duplicate hash fix, OS X umlaut fix, ...)?


--
Best regards,
Thomas Singer
smartsvn GmbH