Re: [PATCH] subrepo: config option to disable subrepositories

2017-11-04 Thread Gregory Szorc 
On Fri, Nov 3, 2017 at 11:57 PM, Yuya Nishihara  wrote:

> On Fri, 03 Nov 2017 17:28:27 -0700, Gregory Szorc wrote:
> > # HG changeset patch
> > # User Gregory Szorc 
> > # Date 1509755155 25200
> > #  Fri Nov 03 17:25:55 2017 -0700
> > # Branch stable
> > # Node ID f2390c369bfebf32f26f5a2e4aa5620224a7c8ea
> > # Parent  f445b10dc7fb3495d24d1c22b0996148864c77f7
> > subrepo: config option to disable subrepositories
>
> > +``enablesubrepos``
> > +Whether the subrepositories feature is enabled. If disabled,
> > +subrepositories are effectively ignored by the Mercurial client.
> > +(default: True)
>
> We might want to select subrepo types to be enabled since hg subrepo is
> more widely used and considered less broken.
>

I would like per-type controls as well.

I would prefer to start with a simple patch providing a master switch. We
can add per-type switches in later. But we should have a plan for the
option names so the end state has a reasonable UI.

I'll try to send out a V2 in the next few hours...


>
> > +TODO buggy because localrepository.commit() managed its file contents
> > +  $ hg commit -m 'manually add .hgsubstate'
> > +  nothing changed
> > +  [1]
> > +
> > +... but hg commit --amend works (this relies on a regression in 4.4)
> > +
> > +  $ hg commit --amend
> > +  saved backup bundle to $TESTTMP/testrepo0/.hg/strip-
> backup/addf99df3e66-ab5c9ff8-amend.hg (glob)
>
> Yeah, it's the issue5677.
>
> https://bz.mercurial-scm.org/show_bug.cgi?id=5677
>
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [PATCH] subrepo: config option to disable subrepositories

2017-11-04 Thread Adrian Buehlmann 
On 2017-11-04 07:57, Yuya Nishihara wrote:
> On Fri, 03 Nov 2017 17:28:27 -0700, Gregory Szorc wrote:
>> # HG changeset patch
>> # User Gregory Szorc 
>> # Date 1509755155 25200
>> #  Fri Nov 03 17:25:55 2017 -0700
>> # Branch stable
>> # Node ID f2390c369bfebf32f26f5a2e4aa5620224a7c8ea
>> # Parent  f445b10dc7fb3495d24d1c22b0996148864c77f7
>> subrepo: config option to disable subrepositories
> 
>> +``enablesubrepos``
>> +Whether the subrepositories feature is enabled. If disabled,
>> +subrepositories are effectively ignored by the Mercurial client.
>> +(default: True)
> 
> We might want to select subrepo types to be enabled since hg subrepo is
> more widely used and considered less broken.
> 

After looking at subrepo.gitsubrepo._checkversion in the mercurial sources:

Assuming the problem is limited to specific vulnerable versions of
external scm tools:

Perhaps, this might be narrowed to requiring specific versions of git
and svn using a config knob (which defaults to the hard-coded values).
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [PATCH] subrepo: config option to disable subrepositories

2017-11-04 Thread Yuya Nishihara 
On Fri, 03 Nov 2017 17:28:27 -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc 
> # Date 1509755155 25200
> #  Fri Nov 03 17:25:55 2017 -0700
> # Branch stable
> # Node ID f2390c369bfebf32f26f5a2e4aa5620224a7c8ea
> # Parent  f445b10dc7fb3495d24d1c22b0996148864c77f7
> subrepo: config option to disable subrepositories

> +``enablesubrepos``
> +Whether the subrepositories feature is enabled. If disabled,
> +subrepositories are effectively ignored by the Mercurial client.
> +(default: True)

We might want to select subrepo types to be enabled since hg subrepo is
more widely used and considered less broken.

> +TODO buggy because localrepository.commit() managed its file contents
> +  $ hg commit -m 'manually add .hgsubstate'
> +  nothing changed
> +  [1]
> +
> +... but hg commit --amend works (this relies on a regression in 4.4)
> +
> +  $ hg commit --amend
> +  saved backup bundle to 
> $TESTTMP/testrepo0/.hg/strip-backup/addf99df3e66-ab5c9ff8-amend.hg (glob)

Yeah, it's the issue5677.

https://bz.mercurial-scm.org/show_bug.cgi?id=5677
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [PATCH] subrepo: config option to disable subrepositories

2017-11-03 Thread Gregory Szorc 
On Fri, Nov 3, 2017 at 10:06 PM, Matt Harbison 
wrote:

> On Fri, 03 Nov 2017 20:28:27 -0400, Gregory Szorc 
> wrote:
>
> # HG changeset patch
>> # User Gregory Szorc 
>> # Date 1509755155 25200
>> #  Fri Nov 03 17:25:55 2017 -0700
>> # Branch stable
>> # Node ID f2390c369bfebf32f26f5a2e4aa5620224a7c8ea
>> # Parent  f445b10dc7fb3495d24d1c22b0996148864c77f7
>> subrepo: config option to disable subrepositories
>>
>>
> One class of problems stems from the fact that the .hgsubstate file
>> is managed automatically. This code still runs in this patch and
>> the result is the content of .hgsubstate will get nuked by a client
>> with the feature disabled. If you have this file in your repo and
>> disable the feature and commit, you lose the file. That's obviously
>> not good.
>>
>
> This seems to prevent the file from being nuked when no subrepo is present:
>
> diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
> --- a/mercurial/subrepo.py
> +++ b/mercurial/subrepo.py
> @@ -182,6 +182,9 @@
>
>  def writestate(repo, state):
>  """rewrite .hgsubstate in (outer) repo with these subrepo states"""
> +if not repo.ui.configbool('ui', 'enablesubrepos'):
> +return
> +
>  lines = ['%s %s\n' % (state[s][1], s) for s in sorted(state)
>  if state[s][1] !=
> nullstate[1]]
>  repo.wwrite('.hgsubstate', ''.join(lines), '')
> diff --git a/tests/test-subrepo-disable.t b/tests/test-subrepo-disable.t
> --- a/tests/test-subrepo-disable.t
> +++ b/tests/test-subrepo-disable.t
> @@ -123,15 +123,9 @@
>
>$ cd with-substate-disabled
>$ hg status
> -  M .hgsubstate
> -TODO buggy
>$ hg diff
> -  diff -r 7645bb5a5a99 .hgsubstate
> -  --- a/.hgsubstateThu Jan 01 00:00:00 1970 +
> -  +++ b/.hgsubstateThu Jan 01 00:00:00 1970 +
> -  @@ -1,1 +0,0 @@
> -  -45cc468b8f18bee314935a4651bad80f9cb3b540 hgsub
>$ cat .hgsubstate
> +  45cc468b8f18bee314935a4651bad80f9cb3b540 hgsub
>$ hg up
>0 files updated, 0 files merged, 0 files removed, 0 files unresolved
>
>
> What is the bug around the subrepo() revset?
>

The revset needs to read the parsed .hgsubstate file. That we can probably
fix easily by adding a flag to force parsing even if the subrepo feature is
disabled.

It is the magical .hgsubstate management that will be harder to fix with a
simple patch. I'm leaning towards putting most "do something with subrepos"
code behind an "if ui.configbool(...)" check. That is several dozen call
sites. But I think we'll need to do that in order to get the behavior we
desire.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [PATCH] subrepo: config option to disable subrepositories

2017-11-03 Thread Matt Harbison 
On Fri, 03 Nov 2017 20:28:27 -0400, Gregory Szorc  
 wrote:



# HG changeset patch
# User Gregory Szorc 
# Date 1509755155 25200
#  Fri Nov 03 17:25:55 2017 -0700
# Branch stable
# Node ID f2390c369bfebf32f26f5a2e4aa5620224a7c8ea
# Parent  f445b10dc7fb3495d24d1c22b0996148864c77f7
subrepo: config option to disable subrepositories




One class of problems stems from the fact that the .hgsubstate file
is managed automatically. This code still runs in this patch and
the result is the content of .hgsubstate will get nuked by a client
with the feature disabled. If you have this file in your repo and
disable the feature and commit, you lose the file. That's obviously
not good.


This seems to prevent the file from being nuked when no subrepo is present:

diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
--- a/mercurial/subrepo.py
+++ b/mercurial/subrepo.py
@@ -182,6 +182,9 @@

 def writestate(repo, state):
 """rewrite .hgsubstate in (outer) repo with these subrepo states"""
+if not repo.ui.configbool('ui', 'enablesubrepos'):
+return
+
 lines = ['%s %s\n' % (state[s][1], s) for s in sorted(state)
 if state[s][1] !=  
nullstate[1]]

 repo.wwrite('.hgsubstate', ''.join(lines), '')
diff --git a/tests/test-subrepo-disable.t b/tests/test-subrepo-disable.t
--- a/tests/test-subrepo-disable.t
+++ b/tests/test-subrepo-disable.t
@@ -123,15 +123,9 @@

   $ cd with-substate-disabled
   $ hg status
-  M .hgsubstate
-TODO buggy
   $ hg diff
-  diff -r 7645bb5a5a99 .hgsubstate
-  --- a/.hgsubstateThu Jan 01 00:00:00 1970 +
-  +++ b/.hgsubstateThu Jan 01 00:00:00 1970 +
-  @@ -1,1 +0,0 @@
-  -45cc468b8f18bee314935a4651bad80f9cb3b540 hgsub
   $ cat .hgsubstate
+  45cc468b8f18bee314935a4651bad80f9cb3b540 hgsub
   $ hg up
   0 files updated, 0 files merged, 0 files removed, 0 files unresolved


What is the bug around the subrepo() revset?
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

[PATCH] subrepo: config option to disable subrepositories

2017-11-03 Thread Gregory Szorc 
# HG changeset patch
# User Gregory Szorc 
# Date 1509755155 25200
#  Fri Nov 03 17:25:55 2017 -0700
# Branch stable
# Node ID f2390c369bfebf32f26f5a2e4aa5620224a7c8ea
# Parent  f445b10dc7fb3495d24d1c22b0996148864c77f7
subrepo: config option to disable subrepositories

Subrepositories are a lesser-used feature that has a history of security
vulnerabilities. Previous subrepo vulnerabilities have resulted in
arbitrary code execution during `hg clone`. This is one of the worst
kind of vulnerabilities a version control system can have.

Another security concern is that Mercurial supports non-Mercurial
subrepositories. (Git and Subversion are supported by default.) While
the Mercurial developers try to keep up with development of other
version control systems, it is effectively impossible for us to
keep tabs on all 3rd party changes and their security impact. Every
time Mercurial attempts to call out into another [version control]
tool, we run a higher risk of accidentally introducing a security
vulnerability. This is what's referred to as "surface area for
"attack" in computer security speak.

Since subrepos have a history of vulnerabilities, increase our
exposure to security issues in other tools, and aren't widely used,
or a critical feature to have enabled by default, it makes sense
for the feature to be optional.

This commit introduces a config flag to control whether subrepos are
enabled. The default of having them enabled remains unchanged.

This patch is simple and crude. As the added test demonstrates,
it does a few things sub-optimally.

One class of problems stems from the fact that the .hgsubstate file
is managed automatically. This code still runs in this patch and
the result is the content of .hgsubstate will get nuked by a client
with the feature disabled. If you have this file in your repo and
disable the feature and commit, you lose the file. That's obviously
not good.

I figure this patch is effective enough to send out, even though
it has problems. I'll continue to work on a more thorough solution...

diff --git a/mercurial/configitems.py b/mercurial/configitems.py
--- a/mercurial/configitems.py
+++ b/mercurial/configitems.py
@@ -857,6 +857,9 @@ coreconfigitem('ui', 'debugger',
 coreconfigitem('ui', 'editor',
 default=dynamicdefault,
 )
+coreconfigitem('ui', 'enablesubrepos',
+default=True,
+)
 coreconfigitem('ui', 'fallbackencoding',
 default=None,
 )
diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt
--- a/mercurial/help/config.txt
+++ b/mercurial/help/config.txt
@@ -2012,6 +2012,11 @@ User interface controls.
 ``editor``
 The editor to use during a commit. (default: ``$EDITOR`` or ``vi``)
 
+``enablesubrepos``
+Whether the subrepositories feature is enabled. If disabled,
+subrepositories are effectively ignored by the Mercurial client.
+(default: True)
+
 ``fallbackencoding``
 Encoding to try if it's not possible to decode the changelog using
 UTF-8. (default: ISO-8859-1)
diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
--- a/mercurial/subrepo.py
+++ b/mercurial/subrepo.py
@@ -85,6 +85,14 @@ def state(ctx, ui):
 to tuple: (source from .hgsub, revision from .hgsubstate, kind
 (key in types dict))
 """
+# Blank out knowledge about subrepos when the feature is disabled.
+# This effectively causes consumers of this data structure to think
+# there are no subrepos. This causes problems with the automatic
+# management of .hgsubstate (which is based on this parsed data)
+# and the subrepo() revset.
+if not ui.configbool('ui', 'enablesubrepos'):
+return {}
+
 p = config.config()
 repo = ctx.repo()
 def read(f, sections=None, remap=None):
diff --git a/tests/test-subrepo-disable.t b/tests/test-subrepo-disable.t
new file mode 100644
--- /dev/null
+++ b/tests/test-subrepo-disable.t
@@ -0,0 +1,153 @@
+  $ hg init hgsub
+  $ cd hgsub
+  $ echo sub0 > foo
+  $ hg -q commit -A -m 'subrepo initial'
+  $ hg log -T '{node}\n'
+  45cc468b8f18bee314935a4651bad80f9cb3b540
+  $ cd ..
+
+  $ hg init testrepo0
+  $ cd testrepo0
+  $ cat >> .hg/hgrc << EOF
+  > [ui]
+  > enablesubrepos = false
+  > EOF
+
+  $ echo 0 > foo
+  $ hg -q commit -A -m initial
+
+Adding a .hgsub should result in no sign of the subrepo in the working 
directory
+
+  $ cat > .hgsub << EOF
+  > hgsub = ../hgsub
+  > EOF
+
+  $ hg add .hgsub
+  $ hg commit -m 'add .hgsub'
+
+  $ hg files --subrepos
+  .hgsub
+  foo
+
+No .hgsubstate should have been created
+
+  $ hg files
+  .hgsub
+  foo
+
+Adding a nested hg repo will be treated like a normal nested hg repo - it will 
be ignored
+
+  $ hg init hgsub
+  $ hg st
+  $ rm -rf hgsub
+
+subrepo() revset still works even if feature is disabled
+
+TODO buggy
+  $ hg log -r 'subrepo()'
+
+  $ cd ..
+
+Cloning this repo won't clone subrepo because no .hgsubstate
+
+  $ hg clone --pull testrepo0 no-substate-enabled
+  requesting all changes
+  adding