You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ja...@apache.org on 2017/05/08 17:56:35 UTC
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:
=====================
Re: svn commit: r1794433 - /subversion/branches/1.9.x/STATUS
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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
Re: svn commit: r1794433 - /subversion/branches/1.9.x/STATUS
Posted by James McCoy <ja...@jamessan.com>.
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
Re: svn commit: r1794433 - /subversion/branches/1.9.x/STATUS
Posted by James McCoy <ja...@jamessan.com>.
On Tue, May 09, 2017 at 09:36:18AM -0400, Mark Phippard wrote:
> On Tue, May 9, 2017 at 8:02 AM, James McCoy <ja...@jamessan.com> 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
Re: svn commit: r1794433 - /subversion/branches/1.9.x/STATUS
Posted by Mark Phippard <ma...@gmail.com>.
On Tue, May 9, 2017 at 8:02 AM, James McCoy <ja...@jamessan.com> 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.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
Re: svn commit: r1794433 - /subversion/branches/1.9.x/STATUS
Posted by James McCoy <ja...@jamessan.com>.
On Tue, May 09, 2017 at 01:00:00PM +0200, Bert Huijben wrote:
> > -----Original Message-----
> > From: Stefan Sperling [mailto:stsp@elego.de]
> > Sent: dinsdag 9 mei 2017 11:26
> > To: Bert Huijben <be...@qqmail.nl>
> > Cc: dev@subversion.apache.org
> > 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
Re: svn commit: r1794433 - /subversion/branches/1.9.x/STATUS
Posted by Stefan Sperling <st...@elego.de>.
On Tue, May 09, 2017 at 01:00:00PM +0200, Bert Huijben wrote:
>
>
> > -----Original Message-----
> > From: Stefan Sperling [mailto:stsp@elego.de]
> > Sent: dinsdag 9 mei 2017 11:26
> > To: Bert Huijben <be...@qqmail.nl>
> > Cc: dev@subversion.apache.org
> > 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.
RE: svn commit: r1794433 - /subversion/branches/1.9.x/STATUS
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Stefan Sperling [mailto:stsp@elego.de]
> Sent: dinsdag 9 mei 2017 11:26
> To: Bert Huijben <be...@qqmail.nl>
> Cc: dev@subversion.apache.org
> 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
Re: svn commit: r1794433 - /subversion/branches/1.9.x/STATUS
Posted by Stefan Sperling <st...@elego.de>.
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.
RE: svn commit: r1794433 - /subversion/branches/1.9.x/STATUS
Posted by Bert Huijben <be...@qqmail.nl>.
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: jamessan@apache.org
Sent: maandag 8 mei 2017 19:56
To: commits@subversion.apache.org
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:
=====================