RE: svn commit: r1794433 - /subversion/branches/1.9.x/STATUS

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

RE: svn commit: r1794433 - /subversion/branches/1.9.x/STATUS

Bert Huijben-5

I haven’t investigated this any further, but do we now try to start the gpg-agent on every invocation of a command just to poll if we perhaps have a GPG agent running, and might want to use that authentication option?

 

I don’t think we want to do that as a simple replacement of a cheap check of an environment variable as we did before. There is also a long list of applications where just executing a program by name (without path or anything) is called a security problem.

 

Bert

 

Sent from Mail for Windows 10

 

From: [hidden email]
Sent: maandag 8 mei 2017 19:56
To: [hidden email]
Subject: svn commit: r1794433 - /subversion/branches/1.9.x/STATUS

 

Author: jamessan

Date: Mon May  8 17:56:35 2017

New Revision: 1794433

 

URL: http://svn.apache.org/viewvc?rev=1794433&view=rev

Log:

* STATUS: Nominate r1794166.

 

Modified:

    subversion/branches/1.9.x/STATUS

 

Modified: subversion/branches/1.9.x/STATUS

URL: http://svn.apache.org/viewvc/subversion/branches/1.9.x/STATUS?rev=1794433&r1=1794432&r2=1794433&view=diff

==============================================================================

--- subversion/branches/1.9.x/STATUS (original)

+++ subversion/branches/1.9.x/STATUS Mon May  8 17:56:35 2017

@@ -89,6 +89,14 @@ Candidate changes:

    Votes:

      +1: stefan2, rhuijben

+ * r1794166

+   Find gpg-agent socket using gpgconf, if possible.

+   Justification:

+     Improves GPG socket detection when $GPG_AGENT_INFO is unset or when gpg

+     >= 2.1.13 is used

+   Votes:

+     +1: jamessan

+

Veto-blocked changes:

=====================

 

 

 

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

Re: svn commit: r1794433 - /subversion/branches/1.9.x/STATUS

Stefan Sperling
On Tue, May 09, 2017 at 09:13:57AM +0200, Bert Huijben wrote:
> I haven’t investigated this any further, but do we now try to start the
> gpg-agent on every invocation of a command just to poll if we perhaps have a
> GPG agent running, and might want to use that authentication option?
 
No. gpgconf is not gpg-agent.
gpgconf is a tool for querying configuration parameters of gnupg.
https://www.gnupg.org/documentation/manuals/gnupg/gpgconf.html#gpgconf

No agent is started when I run this:
$ gpgconf --list-dir agent-socket
/home/stsp/.gnupg/S.gpg-agent

In the source code [1] it looks like what is printed is the path
to the configuration directory (/run/user/UID/gnupg on Linux) and
the static string "/S.gpg-agent" (a gnupg compile-time default).

[1] https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=tools/gpgconf.c;h=2236555671b2f33b36e6985ded93ebfcafef344d;hb=refs/heads/master

So gpgconf does the same as this part of our code except it does not
need the GNUPGHOME environment variable:

      else if ((gnupghome = getenv("GNUPGHOME")) != NULL)
        {
          const char *homedir = svn_dirent_canonicalize(gnupghome, pool);
          socket_name = svn_dirent_join(homedir, "S.gpg-agent", pool);
        }

> I don’t think we want to do that as a simple replacement of a cheap check of
> an environment variable as we did before. There is also a long list of
> applications where just executing a program by name (without path or
> anything) is called a security problem.

Well, it seems that GnuPG 2.x has changed the way this works and we
don't have much choice if we want to keep supporting gpg-agent.

I am fine with restricting the PATH if that's a concern. Not sure what
this would look like on Windows but we could probably restrict it to
something like "/usr/bin:/usr/local/bin" on Unix-like systems without
much risk of breaking this functionality.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: svn commit: r1794433 - /subversion/branches/1.9.x/STATUS

Bert Huijben-5


> -----Original Message-----
> From: Stefan Sperling [mailto:[hidden email]]
> Sent: dinsdag 9 mei 2017 11:26
> To: Bert Huijben <[hidden email]>
> Cc: [hidden email]
> Subject: Re: svn commit: r1794433 - /subversion/branches/1.9.x/STATUS
>
> On Tue, May 09, 2017 at 09:13:57AM +0200, Bert Huijben wrote:
> > I haven’t investigated this any further, but do we now try to start the
> > gpg-agent on every invocation of a command just to poll if we perhaps
> have a
> > GPG agent running, and might want to use that authentication option?
>
> No. gpgconf is not gpg-agent.
> gpgconf is a tool for querying configuration parameters of gnupg.
> https://www.gnupg.org/documentation/manuals/gnupg/gpgconf.html#gpg
> conf
>
> No agent is started when I run this:
> $ gpgconf --list-dir agent-socket
> /home/stsp/.gnupg/S.gpg-agent

But 'gpgpconf' is started.

The problem is that we just start external code... Which executable doesn't matter that much.

Subversion is a library and we should be very careful about this. I think this code is by default left out on Windows, but there are tons of cert reports where just loading a library dynamically to test something is a security problem, and just running an executable is far worse.

I don't see a problem with enabling this if we know the user uses gpg, but doing this on every auth request just to see if gpg can theoretically be used as backend is too much for me.


The function to test if there is a gpg store becomes several orders of magnitude slower, while we don't even cache the result... because the code used to be blazingly fast

The code forks the process, which may have severe consequences in certain environments involving threads (running inside Eclipse?)



This is no longer some small trivial change... It changes outside dependencies and security boundaries.


        Bert

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

Re: svn commit: r1794433 - /subversion/branches/1.9.x/STATUS

Daniel Shahaf-2
In reply to this post by Stefan Sperling
Stefan Sperling wrote on Tue, May 09, 2017 at 11:26:26 +0200:
> I am fine with restricting the PATH if that's a concern. Not sure what
> this would look like on Windows but we could probably restrict it to
> something like "/usr/bin:/usr/local/bin" on Unix-like systems without
> much risk of breaking this functionality.

I'm pretty sure there is abstraction for "default path" which we should
use rather than hardcode a PATH string.  (getconf(3) or so?)

Hopefully it's already wrapped in APR, too, which would address the
windows part of your question.

Cheers,

Daniel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1794433 - /subversion/branches/1.9.x/STATUS

Stefan Sperling
In reply to this post by Bert Huijben-5
On Tue, May 09, 2017 at 01:00:00PM +0200, Bert Huijben wrote:

>
>
> > -----Original Message-----
> > From: Stefan Sperling [mailto:[hidden email]]
> > Sent: dinsdag 9 mei 2017 11:26
> > To: Bert Huijben <[hidden email]>
> > Cc: [hidden email]
> > Subject: Re: svn commit: r1794433 - /subversion/branches/1.9.x/STATUS
> >
> > On Tue, May 09, 2017 at 09:13:57AM +0200, Bert Huijben wrote:
> > > I haven’t investigated this any further, but do we now try to start the
> > > gpg-agent on every invocation of a command just to poll if we perhaps
> > have a
> > > GPG agent running, and might want to use that authentication option?
> >
> > No. gpgconf is not gpg-agent.
> > gpgconf is a tool for querying configuration parameters of gnupg.
> > https://www.gnupg.org/documentation/manuals/gnupg/gpgconf.html#gpg
> > conf
> >
> > No agent is started when I run this:
> > $ gpgconf --list-dir agent-socket
> > /home/stsp/.gnupg/S.gpg-agent
>
> But 'gpgpconf' is started.
>
> The problem is that we just start external code... Which executable doesn't
> matter that much.

> This is no longer some small trivial change... It changes outside
> dependencies and security boundaries.

OK, I see your point.

It seems the original motivation for this change was that gpg-agent
has now moved its socket to /run/user/UID/gnupg in the Linux world.
We could add custom code which looks for a socket there instead of
calling out to a binary. The only downside is that if Linux again changes
its current way of working, we'll have to keep piling on hacks to keep
things working rather than relying on gpgconf as an abstraction.
But that's probably just the way it is with Linux.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1794433 - /subversion/branches/1.9.x/STATUS

James McCoy-3
In reply to this post by Bert Huijben-5
On Tue, May 09, 2017 at 01:00:00PM +0200, Bert Huijben wrote:

> > -----Original Message-----
> > From: Stefan Sperling [mailto:[hidden email]]
> > Sent: dinsdag 9 mei 2017 11:26
> > To: Bert Huijben <[hidden email]>
> > Cc: [hidden email]
> > Subject: Re: svn commit: r1794433 - /subversion/branches/1.9.x/STATUS
> >
> > On Tue, May 09, 2017 at 09:13:57AM +0200, Bert Huijben wrote:
> > > I haven’t investigated this any further, but do we now try to start the
> > > gpg-agent on every invocation of a command just to poll if we perhaps
> > have a
> > > GPG agent running, and might want to use that authentication option?
> >
> > No. gpgconf is not gpg-agent.
> > gpgconf is a tool for querying configuration parameters of gnupg.
> > https://www.gnupg.org/documentation/manuals/gnupg/gpgconf.html#gpg
> > conf
> >
> > No agent is started when I run this:
> > $ gpgconf --list-dir agent-socket
> > /home/stsp/.gnupg/S.gpg-agent
>
> But 'gpgpconf' is started.
>
> The problem is that we just start external code... Which executable doesn't matter that much.
>
> Subversion is a library and we should be very careful about this. I think this code is by default left out on Windows, but there are tons of cert reports where just loading a library dynamically to test something is a security problem, and just running an executable is far worse.
>
> I don't see a problem with enabling this if we know the user uses gpg, but doing this on every auth request just to see if gpg can theoretically be used as backend is too much for me.

Unfortunately, with newer gnupg there isn't always an agent running.
It's started on-demand, if needed.  That means we may not have
$GPG_AGENT_INFO to check or an existing socket that we can use.

> The function to test if there is a gpg store becomes several orders of magnitude slower, while we don't even cache the result... because the code used to be blazingly fast

Would it be amenable to cache the value, similarly to what's being done
for kwallet/gnome-keyring?  Isn't that cache only live for the duration
of the client process?  How typicaly is it to actually need to re-auth
so the cache is re-used?

I saw this as a stop gap measure to help people using newer GnuPG, until
I have time to look at using gpgme instead.

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: svn commit: r1794433 - /subversion/branches/1.9.x/STATUS

Mark Phippard-3
On Tue, May 9, 2017 at 8:02 AM, James McCoy <[hidden email]> wrote:
> Subversion is a library and we should be very careful about this. I think this code is by default left out on Windows, but there are tons of cert reports where just loading a library dynamically to test something is a security problem, and just running an executable is far worse.
>
> I don't see a problem with enabling this if we know the user uses gpg, but doing this on every auth request just to see if gpg can theoretically be used as backend is too much for me.

Unfortunately, with newer gnupg there isn't always an agent running.
It's started on-demand, if needed.  That means we may not have
$GPG_AGENT_INFO to check or an existing socket that we can use.

> The function to test if there is a gpg store becomes several orders of magnitude slower, while we don't even cache the result... because the code used to be blazingly fast

Would it be amenable to cache the value, similarly to what's being done
for kwallet/gnome-keyring?  Isn't that cache only live for the duration
of the client process?  How typicaly is it to actually need to re-auth
so the cache is re-used?

I saw this as a stop gap measure to help people using newer GnuPG, until
I have time to look at using gpgme instead.


I would expect a feature like this to at least require some kind of opt-in mechanism.  In this case, it should require some setting in config that is not on by default.  I get that we just want to make things work for users as easily as possible but just blindly launching an executable does not seem like the correct approach to me.
 

--
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1794433 - /subversion/branches/1.9.x/STATUS

James McCoy-3
On Tue, May 09, 2017 at 09:36:18AM -0400, Mark Phippard wrote:

> On Tue, May 9, 2017 at 8:02 AM, James McCoy <[hidden email]> wrote:
>
>     > Subversion is a library and we should be very careful about this. I think
>     this code is by default left out on Windows, but there are tons of cert
>     reports where just loading a library dynamically to test something is a
>     security problem, and just running an executable is far worse.
>     >
>     > I don't see a problem with enabling this if we know the user uses gpg,
>     but doing this on every auth request just to see if gpg can theoretically
>     be used as backend is too much for me.
>
>     Unfortunately, with newer gnupg there isn't always an agent running.
>     It's started on-demand, if needed.  That means we may not have
>     $GPG_AGENT_INFO to check or an existing socket that we can use.
>    
>     > The function to test if there is a gpg store becomes several orders of
>     magnitude slower, while we don't even cache the result... because the code
>     used to be blazingly fast
>
>     Would it be amenable to cache the value, similarly to what's being done
>     for kwallet/gnome-keyring?  Isn't that cache only live for the duration
>     of the client process?  How typicaly is it to actually need to re-auth
>     so the cache is re-used?
>
>     I saw this as a stop gap measure to help people using newer GnuPG, until
>     I have time to look at using gpgme instead.
>
>
>
> I would expect a feature like this to at least require some kind of opt-in
> mechanism.  In this case, it should require some setting in config that is not
> on by default.  I get that we just want to make things work for users as easily
> as possible but just blindly launching an executable does not seem like the
> correct approach to me.

ACK.  I'll commit an earlier version of Lukas' patch, which hard-coded
the paths, to trunk and update the nomination after that.

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: svn commit: r1794433 - /subversion/branches/1.9.x/STATUS

James McCoy-3
On Wed, May 10, 2017 at 08:34:59AM -0400, James McCoy wrote:
> On Tue, May 09, 2017 at 09:36:18AM -0400, Mark Phippard wrote:
> > I would expect a feature like this to at least require some kind of opt-in
> > mechanism.  In this case, it should require some setting in config that is not
> > on by default.  I get that we just want to make things work for users as easily
> > as possible but just blindly launching an executable does not seem like the
> > correct approach to me.
>
> ACK.  I'll commit an earlier version of Lukas' patch, which hard-coded
> the paths, to trunk and update the nomination after that.

Done in trunk with r1795087.  Backport branch and STATUS were also
updated.

Cheers,
--
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7  2D23 DFE6 91AE 331B A3DB
Loading...