Re: Subversion fails to checkout new working set when $HOME is automounted

[Daniel Shahaf]

Joerg Wunsch wrote on Fri, 24 Jan 2020 08:23 +0100:
> As Daniel Shahaf wrote:
> > Could we look for a solution that doesn't involve explicit user input?
> > For example, given «svn co $URL $dir», we could search only in
> > ancestors that are writable by the current user (in the sense of
> > access(W_OK)).  That would generally cause the $HOME behaviour Jörg
> > proposed, but without special-casing $HOME.  
> 
> Sounds reasonable to me.

Moving to dev@ [please drop users@ from follow-ups].

The thread on users@ covered a few topics.  The ones I'd like to here
concern «svn checkout $URL $dir»'s behaviour of checking whether $dir is
part of an existing working copy.  It has been suggested that the
crawl-up should stop earlier (with "Don't crawl above $HOME" being
named as a use-case; see the quoted paragraph for a proposal to achieve
that) and that if the crawl-up does find that the checkout would cause
an obstruction in an ancestor working copy, it should not error out but
merely warn,

Thoughts?

Cheers,

Daniel

Re: Subversion fails to checkout new working set when $HOME is automounted

[Joerg Wunsch]

As Daniel Shahaf wrote:

> Joerg Wunsch wrote on Thu, 23 Jan 2020 22:18 +0100:
> > Would having at least an option to "svn co" that does just not
> > traverse upwards (for those who know what they are doing) be a
> > compromise?
> 
> In general, we don't like adding options, [...]

Understood.

> Could we look for a solution that doesn't involve explicit user input?
> For example, given «svn co $URL $dir», we could search only in
> ancestors that are writable by the current user (in the sense of
> access(W_OK)).  That would generally cause the $HOME behaviour Jörg
> proposed, but without special-casing $HOME.

Sounds reasonable to me.

-- 
cheers, Joerg   .-.-.   --... ...--   -.. .  DL8DTL

http://www.sax.de/~joerg/
Never trust an operating system you don't have sources for. ;-)

Re: Subversion fails to checkout new working set when $HOME is automounted

[Daniel Shahaf]

Joerg Wunsch wrote on Thu, 23 Jan 2020 22:18 +0100:
> Would having at least an option to "svn co" that does just not
> traverse upwards (for those who know what they are doing) be a
> compromise?

In general, we don't like adding options, because every option added is
another variable to account for in future maintenance, testing, user
support, etc..  I'd rather figure out a new, different behaviour that
we can roll out for everyone.

> That way, using the default behaviour:
> 
> > ./subversion/libsvn_client/checkout.c:173: 
> > (apr_err=SVN_ERR_WC_OBSTRUCTED_UPDATE)
> > svn: E155000: '/home/daniel/src/svn/t1/notes' is already a working copy 
> > for a different URL  
> 
> This would still trigger that error, but one could override it for
> situations where traversing beyond the current directory is just not
> wanted - for whatever reason.  For mortal users, traversing into the
> OS domain beyond $HOME is usually not useful anyway,

Agreed.

> but dragging the concept of a $HOME directory into subversion is
> certainly going too far.

Agreed.

> Of course, if considering $HOME as a search boundary sounds really
> more reasonable, that entire directory traversal might be made more
> flexible.  Suppose adding an option --topmost-search-directory, it
> could then default to $HOME. *) One could then completely avoid the
> traversal by using --topmost-search-directory=., or alternatively,
> allow for traversing the entire hierarchy with
> --topmost-search-directory=/.

Could we look for a solution that doesn't involve explicit user input?
For example, given «svn co $URL $dir», we could search only in
ancestors that are writable by the current user (in the sense of
access(W_OK)).  That would generally cause the $HOME behaviour Jörg
proposed, but without special-casing $HOME.

As a separate question, I think the fatal error behaviour is
questionable.  A user has any number of ways to obstruct their own
files, so why should «svn checkout» be the exception that goes out of
its way to prevent that outcome?  Would it be reasonable to downgrade
the error to a warning?

Cheers,

Daniel

Re: Subversion fails to checkout new working set when $HOME is automounted

[Joerg Wunsch]

As Daniel Shahaf wrote:

> Well, I don't know whether those semantics are configurable, but you
> might be able to duct tape around them by creating /home/.svn as a local
> directory:

Hmm.  Looks strange. ;-)

> > But that's another point. I was more suprised about "svn checkout"
> > traversing upwards at all, as I think this violates the principle of
> > least astonishment.

> For context, one of the reason it does this is to make the following
> error possible:

There are arguments for that, I see.

Would having at least an option to "svn co" that does just not
traverse upwards (for those who know what they are doing) be a
compromise? That way, using the default behaviour:

> ./subversion/libsvn_client/checkout.c:173: 
> (apr_err=SVN_ERR_WC_OBSTRUCTED_UPDATE)
> svn: E155000: '/home/daniel/src/svn/t1/notes' is already a working copy 
> for a different URL

This would still trigger that error, but one could override it for
situations where traversing beyond the current directory is just not
wanted - for whatever reason.  For mortal users, traversing into the
OS domain beyond $HOME is usually not useful anyway, but dragging the
concept of a $HOME directory into subversion is certainly going too
far.

Of course, if considering $HOME as a search boundary sounds really
more reasonable, that entire directory traversal might be made more
flexible.  Suppose adding an option --topmost-search-directory, it
could then default to $HOME. *) One could then completely avoid the
traversal by using --topmost-search-directory=., or alternatively,
allow for traversing the entire hierarchy with
--topmost-search-directory=/.

*) Sure, for "modern" root accounts, this would limit all SVN searches
to /root.  However, it can be argued that root always needs to know
what she is doing so the entire traversal is not necessary for root. :)

-- 
cheers, Joerg   .-.-.   --... ...--   -.. .  DL8DTL

http://www.sax.de/~joerg/
Never trust an operating system you don't have sources for. ;-)

Re: Subversion fails to checkout new working set when $HOME is automounted

[Daniel Shahaf]

Joerg Wunsch wrote on Thu, 23 Jan 2020 21:16 +0100:
> As Daniel Shahaf wrote:
> > However, on FreeBSD a plain «stat /nonexistent/foo/bar»
> > returns ENOENT, not ENOTDIR…  
> 
> The semantics of that automounter are, indeed, a bit strange.  I would
> have expected an ENOENT for ../.svn (the NFS server in question does
> not provide the respective directory). I'm not sure whether this would
> be difficult to fix or not.

Well, I don't know whether those semantics are configurable, but you
might be able to duct tape around them by creating /home/.svn as a local
directory:
.
% r=`mktemp -d`
% wc=`mktemp -d`
% svnadmin create $r
% svn co --depth=empty file://$r $wc 
% mkdir /home/.svn
% mount … /home/.svn # bypass the automounter for .svn/
% mv $wc/.svn/* /home/.svn/*
% rm -rf $d $wc

This fakes a valid working copy.

> But that's another point. I was more suprised about "svn checkout"
> traversing upwards at all, as I think this violates the principle of
> least astonishment.

For context, one of the reason it does this is to make the following
error possible:

% svn info --show-item=url 
https://svn.apache.org/repos/asf/subversion/trunk
% svn ls | grep notes
notes/
% rm -rf notes
% mkdir notes
% cd notes
% svn co -q https://svn.freebsd.org/base/head/usr.bin/passwd ./ 
./subversion/svn/checkout-cmd.c:175,
./subversion/libsvn_client/checkout.c:228,
./subversion/libsvn_client/checkout.c:173: 
(apr_err=SVN_ERR_WC_OBSTRUCTED_UPDATE)
svn: E155000: '/home/daniel/src/svn/t1/notes' is already a working copy for 
a different URL
zsh: exit 1
% 

The error is saying that the checkout would cause the notes/
subdirectory of the parent working copy to be obstructed (status '~').
We could make the failure mode less fatal in some cases: for example,
when the parent working copy's .svn directory is inaccessible, we could
issue a warning and continue.

I'm not sure whether there are other reasons for the upwards scan.

> > However, there might be other things we could do.  First, it is possible
> > to create nested checkouts in general, so perhaps the "Are we already
> > inside a working copy?" check is superfluous.  That is, perhaps «svn co
> > $URL $dir» shouldn't check $dir's ancestors for .svn/ subdirectories.
> > (Checking $dir/.svn is probably fine.)  
> 
> I'd vote for that.
> 
> FWIW, this is also in a line what e.g. git does: if I'm inside a git
> cloned repository / working copy, and perform another "git clone", I
> get a new copy inside the already existing one.

To be clear, Subversion behaves this way too.

Cheers,

Daniel

Re: Subversion fails to checkout new working set when $HOME is automounted

[Joerg Wunsch]

As Daniel Shahaf wrote:

> However, there might be other things we could do.  First, it is possible
> to create nested checkouts in general, so perhaps the "Are we already
> inside a working copy?" check is superfluous.  That is, perhaps «svn co
> $URL $dir» shouldn't check $dir's ancestors for .svn/ subdirectories.
> (Checking $dir/.svn is probably fine.)

I'd vote for that.

FWIW, this is also in a line what e.g. git does: if I'm inside a git
cloned repository / working copy, and perform another "git clone", I
get a new copy inside the already existing one.

For everything else except "checkout", I completely agree that
traversing upwards needs to be done.

> However, on FreeBSD a plain «stat /nonexistent/foo/bar»
> returns ENOENT, not ENOTDIR…

The semantics of that automounter are, indeed, a bit strange.  I would
have expected an ENOENT for ../.svn (the NFS server in question does
not provide the respective directory). I'm not sure whether this would
be difficult to fix or not.

But that's another point. I was more suprised about "svn checkout"
traversing upwards at all, as I think this violates the principle of
least astonishment.

-- 
cheers, Joerg   .-.-.   --... ...--   -.. .  DL8DTL

http://www.sax.de/~joerg/
Never trust an operating system you don't have sources for. ;-)

Re: Subversion fails to checkout new working set when $HOME is automounted

[Daniel Shahaf]

Vincent Lefevre wrote on Thu, 23 Jan 2020 15:50 +0100:
> On 2020-01-23 12:44:02 +0100, Joerg Wunsch wrote:
> > If the automounter already yields ENOENT for the ../.svn directory
> > probe, everything is not going to be a problem. I think the point here
> > is the automounter (eventually, after "thinking" about it for about 1
> > s) offers a successful stat() result for ../.svn (probably because
> > that directory *might be* a possible mount point for the automounter)
> > but then yields EIO when trying to access anything within that
> > ficticous directory (because nothing is actually mounted there).  
> 
> Do you mean that Subversion tries to go higher in the hierarchy
> without checking the owner of the directory? If it does, this is
> a security issue.

How so?  What's the attacker model?  What can someone leverage this
feature of Subversion to do that they couldn't do without it?

Re: Subversion fails to checkout new working set when $HOME is automounted

[Daniel Shahaf]

Joerg Wunsch wrote on Thu, 23 Jan 2020 12:44 +0100:
> My entire point is: when getting any error for ..*/.svn/wc.db, just
> stop traversing there, and proceed with the checkout (in its own
> new directory).

This change is not likely to be accepted.

As your ktrace shows, Subversion does a stat("…/.svn") which succeeds
followed by a stat("…/.svn/wc.db") which fails.  One possible cause
for this behaviour is that "…/.svn/wc.db" does in fact exist, but cannot
be accessed, for whatever reason.  In that case, pretending that we are
not actually inside a working copy would be the Wrong Thing to do.

However, there might be other things we could do.  First, it is possible
to create nested checkouts in general, so perhaps the "Are we already
inside a working copy?" check is superfluous.  That is, perhaps «svn co
$URL $dir» shouldn't check $dir's ancestors for .svn/ subdirectories.
(Checking $dir/.svn is probably fine.)

Second, the error code you got was EIO.  While I think bailing out on
EIO is reasonable, there's room to argue that the second stat() call
should have returned ENOTDIR (since the path refers to a file inside
a non-existent directory) and that Subversion should have treated that
as harmless.  However, on FreeBSD a plain «stat /nonexistent/foo/bar»
returns ENOENT, not ENOTDIR…

> You could construct a similar testcase without involving an automounter
> by creating a directory /.svn as root which is not accessible by any
> mere user:
⋮
> In other words, the sys admin, by creating that directory, prevents
> *any* user of the system from checking out any (new) SVN repository in
> the system. I don't believe this is intented. ;-)

The fact that a sysadmin can shoot his users in the foot is not a bug.
It's simply how Unix works.  root can prevent jrandom from creating
working copies in any number of ways.

As to the fact that when you do «svn co $URL $dir» Subversion checks not
only whether $dir/.svn exists but also whether $dir is inside a working
copy, _that_ we might be able to do something about.

Thanks for including the ktrace output; it was very helpful.

Cheers,

Daniel

Re: Subversion fails to checkout new working set when $HOME is automounted

[Vincent Lefevre]

On 2020-01-23 12:44:02 +0100, Joerg Wunsch wrote:
> If the automounter already yields ENOENT for the ../.svn directory
> probe, everything is not going to be a problem. I think the point here
> is the automounter (eventually, after "thinking" about it for about 1
> s) offers a successful stat() result for ../.svn (probably because
> that directory *might be* a possible mount point for the automounter)
> but then yields EIO when trying to access anything within that
> ficticous directory (because nothing is actually mounted there).

Do you mean that Subversion tries to go higher in the hierarchy
without checking the owner of the directory? If it does, this is
a security issue.

-- 
Vincent Lefèvre  - Web: 
100% accessible validated (X)HTML - Blog: 
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: Subversion fails to checkout new working set when $HOME is automounted

[Joerg Wunsch]

As Paul Hammant wrote:

> Works for me (svn, version 1.13.0 (r1867053) on Mac) 
> 
> $ svn co https://svn.freebsd.org/base/head/usr.bin/passwd freebsdTest
> AfreebsdTest/Makefile
> AfreebsdTest/passwd.c
> AfreebsdTest/Makefile.depend
> AfreebsdTest/passwd.1
> Checked out revision 357040.

What automounter is below?

If the automounter already yields ENOENT for the ../.svn directory
probe, everything is not going to be a problem. I think the point here
is the automounter (eventually, after "thinking" about it for about 1
s) offers a successful stat() result for ../.svn (probably because
that directory *might be* a possible mount point for the automounter)
but then yields EIO when trying to access anything within that
ficticous directory (because nothing is actually mounted there).

My entire point is: when getting any error for ..*/.svn/wc.db, just
stop traversing there, and proceed with the checkout (in its own
new directory).

You could construct a similar testcase without involving an automounter
by creating a directory /.svn as root which is not accessible by any
mere user:

j@uriah 53% su
Password:
root@uriah:/home/joerg # mkdir /.svn
root@uriah:/home/joerg # chmod 0700 /.svn
root@uriah:/home/joerg # exit
exit
j@uriah 54% svn co https://svn.freebsd.org/base/head/usr.bin/passwd freebsdTest
svn: E13: Can't check path '/.svn/wc.db': Permission denied

In other words, the sys admin, by creating that directory, prevents
*any* user of the system from checking out any (new) SVN repository in
the system. I don't believe this is intented. ;-)

-- 
cheers, Joerg   .-.-.   --... ...--   -.. .  DL8DTL

http://www.sax.de/~joerg/
Never trust an operating system you don't have sources for. ;-)

Re: Subversion fails to checkout new working set when $HOME is automounted

[Paul Hammant]

Works for me (svn, version 1.13.0 (r1867053) on Mac) 

$ svn co https://svn.freebsd.org/base/head/usr.bin/passwd freebsdTest
AfreebsdTest/Makefile
AfreebsdTest/passwd.c
AfreebsdTest/Makefile.depend
AfreebsdTest/passwd.1
Checked out revision 357040.