Patchwork [4,of,4] bundle2-push: provide transaction to reply unbundler

login
register
mail settings
Submitter Eric Sumner
Date Nov. 25, 2014, 6:26 p.m.
Message ID <99c668568ef7ecc0f6fd.1416939989@dev911.prn1.facebook.com>
Download mbox | patch
Permalink /patch/6856/
State Accepted
Delegated to: Pierre-Yves David
Headers show

Comments

Eric Sumner - Nov. 25, 2014, 6:26 p.m.
# HG changeset patch
# User Eric Sumner <ericsumner@fb.com>
# Date 1416613838 28800
#      Fri Nov 21 15:50:38 2014 -0800
# Node ID 99c668568ef7ecc0f6fd1ca7a3f9dc6de91d176e
# Parent  46994128747aa4bc3057c7fbdbe19f4734cd48ab
bundle2-push: provide transaction to reply unbundler

This patch series is intended to allow bundle2 push reply part handlers to
make changes to the local repository; it has been developed in parallel with
an extension that allows the server to rebase incoming changesets while applying
them.

This diff adds an experimental config option "bundle2.pushback" which provides
a transaction to the reply unbundler during a push operation.  This behavior is
opt-in because of potential security issues: the response can contain any part
type that has a handler defined, allowing the server to make arbitrary changes
to the local repository.
Pierre-Yves David - Nov. 25, 2014, 11:50 p.m.
On 11/25/2014 10:26 AM, Eric Sumner wrote:
> # HG changeset patch
> # User Eric Sumner <ericsumner@fb.com>
> # Date 1416613838 28800
> #      Fri Nov 21 15:50:38 2014 -0800
> # Node ID 99c668568ef7ecc0f6fd1ca7a3f9dc6de91d176e
> # Parent  46994128747aa4bc3057c7fbdbe19f4734cd48ab
> bundle2-push: provide transaction to reply unbundler

This series  looks fine to me.

I'll lets someone else push it since I was deeply involved in its creation.
Durham Goode - Dec. 2, 2014, 12:34 a.m.
On 11/25/14 10:26 AM, Eric Sumner wrote:
> # HG changeset patch
> # User Eric Sumner <ericsumner@fb.com>
> # Date 1416613838 28800
> #      Fri Nov 21 15:50:38 2014 -0800
> # Node ID 99c668568ef7ecc0f6fd1ca7a3f9dc6de91d176e
> # Parent  46994128747aa4bc3057c7fbdbe19f4734cd48ab
> bundle2-push: provide transaction to reply unbundler
>
> This patch series is intended to allow bundle2 push reply part handlers to
> make changes to the local repository; it has been developed in parallel with
> an extension that allows the server to rebase incoming changesets while applying
> them.
>
> This diff adds an experimental config option "bundle2.pushback" which provides
> a transaction to the reply unbundler during a push operation.  This behavior is
> opt-in because of potential security issues: the response can contain any part
> type that has a handler defined, allowing the server to make arbitrary changes
> to the local repository.
>
> +
> +  $ cat >> $HGRCPATH <<EOF
> +  > [ui]
> +  > ssh = python "$TESTDIR/dummyssh"
> +  > username = nobody <no.reply@example.com>
> +  > EOF
> +  $ alias commit='hg commit -d "0 0" -A -m'
> +  $ alias log='hg log -G -T "{desc} [{phase}:{node|short}]"'
> +

Generally looks good.  Pushed to clowncopter with some changes to these 
uses of 'alias' since they cause test-check-code-hg failures.
Augie Fackler - Dec. 2, 2014, 3:11 p.m.
On Tue, Nov 25, 2014 at 10:26:29AM -0800, Eric Sumner wrote:
> # HG changeset patch
> # User Eric Sumner <ericsumner@fb.com>
> # Date 1416613838 28800
> #      Fri Nov 21 15:50:38 2014 -0800
> # Node ID 99c668568ef7ecc0f6fd1ca7a3f9dc6de91d176e
> # Parent  46994128747aa4bc3057c7fbdbe19f4734cd48ab
> bundle2-push: provide transaction to reply unbundler
>
> This patch series is intended to allow bundle2 push reply part handlers to
> make changes to the local repository; it has been developed in parallel with
> an extension that allows the server to rebase incoming changesets while applying
> them.
>
> This diff adds an experimental config option "bundle2.pushback" which provides
> a transaction to the reply unbundler during a push operation.  This behavior is
> opt-in because of potential security issues: the response can contain any part
> type that has a handler defined, allowing the server to make arbitrary changes
> to the local repository.

Security-wise, what are we worried about? The server destroying all
your draft-phase changes?

Is there a way for the server to reject pushes from clients that don't
support pushback (I've got some cleverness in mind for review tooling
some day)?


>
> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
> --- a/mercurial/bundle2.py
> +++ b/mercurial/bundle2.py
> @@ -877,7 +877,7 @@
>                  'b2x:remote-changegroup': ('http', 'https'),
>                 }
>
> -def getrepocaps(repo):
> +def getrepocaps(repo, allowpushback=False):
>      """return the bundle2 capabilities for a given repo
>
>      Exists to allow extensions (like evolution) to mutate the capabilities.
> @@ -887,6 +887,8 @@
>      if obsolete.isenabled(repo, obsolete.exchangeopt):
>          supportedformat = tuple('V%i' % v for v in obsolete.formats)
>          caps['b2x:obsmarkers'] = supportedformat
> +    if allowpushback:
> +        caps['b2x:pushback'] = ()
>      return caps
>
>  def bundle2caps(remote):
> diff --git a/mercurial/exchange.py b/mercurial/exchange.py
> --- a/mercurial/exchange.py
> +++ b/mercurial/exchange.py
> @@ -572,8 +572,12 @@
>      The only currently supported type of data is changegroup but this will
>      evolve in the future."""
>      bundler = bundle2.bundle20(pushop.ui, bundle2.bundle2caps(pushop.remote))
> +    pushback = (pushop.trmanager
> +                and pushop.ui.configbool('experimental', 'bundle2.pushback'))
> +
>      # create reply capability
> -    capsblob = bundle2.encodecaps(bundle2.getrepocaps(pushop.repo))
> +    capsblob = bundle2.encodecaps(bundle2.getrepocaps(pushop.repo,
> +                                                      allowpushback=pushback))
>      bundler.newpart('b2x:replycaps', data=capsblob)
>      replyhandlers = []
>      for partgenname in b2partsgenorder:
> @@ -590,7 +594,10 @@
>      except error.BundleValueError, exc:
>          raise util.Abort('missing support for %s' % exc)
>      try:
> -        op = bundle2.processbundle(pushop.repo, reply)
> +        trgetter = None
> +        if pushback:
> +            trgetter = pushop.trmanager.transaction
> +        op = bundle2.processbundle(pushop.repo, reply, trgetter)
>      except error.BundleValueError, exc:
>          raise util.Abort('missing support for %s' % exc)
>      for rephand in replyhandlers:
> diff --git a/tests/test-bundle2-pushback.t b/tests/test-bundle2-pushback.t
> new file mode 100644
> --- /dev/null
> +++ b/tests/test-bundle2-pushback.t
> @@ -0,0 +1,110 @@
> +  $ cat > bundle2.py << EOF
> +  > """A small extension to test bundle2 pushback parts.
> +  > Current bundle2 implementation doesn't provide a way to generate those
> +  > parts, so they must be created by extensions.
> +  > """
> +  > from mercurial import bundle2, pushkey, exchange, util
> +  > def _newhandlechangegroup(op, inpart):
> +  >     """This function wraps the changegroup part handler for getbundle.
> +  >     It issues an additional b2x:pushkey part to send a new
> +  >     bookmark back to the client"""
> +  >     result = bundle2.handlechangegroup(op, inpart)
> +  >     if 'b2x:pushback' in op.reply.capabilities:
> +  >         params = {'namespace': 'bookmarks',
> +  >                   'key': 'new-server-mark',
> +  >                   'old': '',
> +  >                   'new': 'tip'}
> +  >         encodedparams = [(k, pushkey.encode(v)) for (k,v) in params.items()]
> +  >         op.reply.newpart('b2x:pushkey', mandatoryparams=encodedparams)
> +  >     else:
> +  >         op.reply.newpart('b2x:output', data='pushback not enabled')
> +  >     return result
> +  > _newhandlechangegroup.params = bundle2.handlechangegroup.params
> +  > bundle2.parthandlermapping['b2x:changegroup'] = _newhandlechangegroup
> +  > EOF
> +
> +  $ cat >> $HGRCPATH <<EOF
> +  > [ui]
> +  > ssh = python "$TESTDIR/dummyssh"
> +  > username = nobody <no.reply@example.com>
> +  > EOF
> +  $ alias commit='hg commit -d "0 0" -A -m'
> +  $ alias log='hg log -G -T "{desc} [{phase}:{node|short}]"'
> +
> +Set up server repository
> +
> +  $ hg init server
> +  $ cd server
> +  $ echo c0 > f0
> +  $ commit 0
> +  adding f0
> +
> +Set up client repository
> +
> +  $ cd ..
> +  $ hg clone ssh://user@dummy/server client -q
> +  $ cd client
> +
> +Enable extension
> +  $ cat >> $HGRCPATH <<EOF
> +  > [extensions]
> +  > bundle2=$TESTTMP/bundle2.py
> +  > [experimental]
> +  > bundle2-exp = True
> +  > EOF
> +
> +Without config
> +
> +  $ cd ../client
> +  $ echo c1 > f1
> +  $ commit 1
> +  adding f1
> +  $ hg push
> +  pushing to ssh://user@dummy/server
> +  searching for changes
> +  remote: pushback not enabled
> +  remote: adding changesets
> +  remote: adding manifests
> +  remote: adding file changes
> +  remote: added 1 changesets with 1 changes to 1 files
> +  $ hg bookmark
> +  no bookmarks set
> +
> +  $ cd ../server
> +  $ log
> +  o  1 [public:2b9c7234e035]
> +  |
> +  @  0 [public:6cee5c8f3e5b]
> +
> +
> +
> +
> +With config
> +
> +  $ cd ../client
> +  $ echo '[experimental]' >> .hg/hgrc
> +  $ echo 'bundle2.pushback = True' >> .hg/hgrc
> +  $ echo c2 > f2
> +  $ commit 2
> +  adding f2
> +  $ hg push
> +  pushing to ssh://user@dummy/server
> +  searching for changes
> +  remote: adding changesets
> +  remote: adding manifests
> +  remote: adding file changes
> +  remote: added 1 changesets with 1 changes to 1 files
> +  $ hg bookmark
> +     new-server-mark           2:0a76dfb2e179
> +
> +  $ cd ../server
> +  $ log
> +  o  2 [public:0a76dfb2e179]
> +  |
> +  o  1 [public:2b9c7234e035]
> +  |
> +  @  0 [public:6cee5c8f3e5b]
> +
> +
> +
> +
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Eric Sumner - Dec. 2, 2014, 6:44 p.m.
On 12/2/14, 7:11 AM, "Augie Fackler" <raf@durin42.com> wrote:

>>This diff adds an experimental config option "bundle2.pushback" which
>>provides
>>a transaction to the reply unbundler during a push operation.  This
>>behavior is
>>opt-in because of potential security issues: the response can contain
>>any part
>>type that has a handler defined, allowing the server to make arbitrary
>>changes
>>to the local repository.
>
>Security-wise, what are we worried about? The server destroying all
>your draft-phase changes?

That¹s basically right.  I don¹t think there are any actual security
issues with this, but it does let an operation alter your local repository
that didn¹t before, which is something to be cautious about.

>
>Is there a way for the server to reject pushes from clients that don't
>support pushback (I've got some cleverness in mind for review tooling
>some day)?
>

The client adds Œb2x:pushback¹ to the reply capabilities when it sends the
initial request, so the server knows whether or not it can send pushback
parts; an extension could decide to abort the push if the capability is
absent.

  ‹ Eric

Patch

diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
--- a/mercurial/bundle2.py
+++ b/mercurial/bundle2.py
@@ -877,7 +877,7 @@ 
                 'b2x:remote-changegroup': ('http', 'https'),
                }
 
-def getrepocaps(repo):
+def getrepocaps(repo, allowpushback=False):
     """return the bundle2 capabilities for a given repo
 
     Exists to allow extensions (like evolution) to mutate the capabilities.
@@ -887,6 +887,8 @@ 
     if obsolete.isenabled(repo, obsolete.exchangeopt):
         supportedformat = tuple('V%i' % v for v in obsolete.formats)
         caps['b2x:obsmarkers'] = supportedformat
+    if allowpushback:
+        caps['b2x:pushback'] = ()
     return caps
 
 def bundle2caps(remote):
diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -572,8 +572,12 @@ 
     The only currently supported type of data is changegroup but this will
     evolve in the future."""
     bundler = bundle2.bundle20(pushop.ui, bundle2.bundle2caps(pushop.remote))
+    pushback = (pushop.trmanager
+                and pushop.ui.configbool('experimental', 'bundle2.pushback'))
+
     # create reply capability
-    capsblob = bundle2.encodecaps(bundle2.getrepocaps(pushop.repo))
+    capsblob = bundle2.encodecaps(bundle2.getrepocaps(pushop.repo,
+                                                      allowpushback=pushback))
     bundler.newpart('b2x:replycaps', data=capsblob)
     replyhandlers = []
     for partgenname in b2partsgenorder:
@@ -590,7 +594,10 @@ 
     except error.BundleValueError, exc:
         raise util.Abort('missing support for %s' % exc)
     try:
-        op = bundle2.processbundle(pushop.repo, reply)
+        trgetter = None
+        if pushback:
+            trgetter = pushop.trmanager.transaction
+        op = bundle2.processbundle(pushop.repo, reply, trgetter)
     except error.BundleValueError, exc:
         raise util.Abort('missing support for %s' % exc)
     for rephand in replyhandlers:
diff --git a/tests/test-bundle2-pushback.t b/tests/test-bundle2-pushback.t
new file mode 100644
--- /dev/null
+++ b/tests/test-bundle2-pushback.t
@@ -0,0 +1,110 @@ 
+  $ cat > bundle2.py << EOF
+  > """A small extension to test bundle2 pushback parts.
+  > Current bundle2 implementation doesn't provide a way to generate those
+  > parts, so they must be created by extensions.
+  > """
+  > from mercurial import bundle2, pushkey, exchange, util
+  > def _newhandlechangegroup(op, inpart):
+  >     """This function wraps the changegroup part handler for getbundle.
+  >     It issues an additional b2x:pushkey part to send a new
+  >     bookmark back to the client"""
+  >     result = bundle2.handlechangegroup(op, inpart)
+  >     if 'b2x:pushback' in op.reply.capabilities:
+  >         params = {'namespace': 'bookmarks',
+  >                   'key': 'new-server-mark',
+  >                   'old': '',
+  >                   'new': 'tip'}
+  >         encodedparams = [(k, pushkey.encode(v)) for (k,v) in params.items()]
+  >         op.reply.newpart('b2x:pushkey', mandatoryparams=encodedparams)
+  >     else:
+  >         op.reply.newpart('b2x:output', data='pushback not enabled')
+  >     return result
+  > _newhandlechangegroup.params = bundle2.handlechangegroup.params
+  > bundle2.parthandlermapping['b2x:changegroup'] = _newhandlechangegroup
+  > EOF
+
+  $ cat >> $HGRCPATH <<EOF
+  > [ui]
+  > ssh = python "$TESTDIR/dummyssh"
+  > username = nobody <no.reply@example.com>
+  > EOF
+  $ alias commit='hg commit -d "0 0" -A -m'
+  $ alias log='hg log -G -T "{desc} [{phase}:{node|short}]"'
+
+Set up server repository
+
+  $ hg init server
+  $ cd server
+  $ echo c0 > f0
+  $ commit 0
+  adding f0
+
+Set up client repository
+
+  $ cd ..
+  $ hg clone ssh://user@dummy/server client -q
+  $ cd client
+
+Enable extension
+  $ cat >> $HGRCPATH <<EOF
+  > [extensions]
+  > bundle2=$TESTTMP/bundle2.py
+  > [experimental]
+  > bundle2-exp = True
+  > EOF
+
+Without config
+
+  $ cd ../client
+  $ echo c1 > f1
+  $ commit 1
+  adding f1
+  $ hg push
+  pushing to ssh://user@dummy/server
+  searching for changes
+  remote: pushback not enabled
+  remote: adding changesets
+  remote: adding manifests
+  remote: adding file changes
+  remote: added 1 changesets with 1 changes to 1 files
+  $ hg bookmark
+  no bookmarks set
+
+  $ cd ../server
+  $ log
+  o  1 [public:2b9c7234e035]
+  |
+  @  0 [public:6cee5c8f3e5b]
+  
+
+
+
+With config
+
+  $ cd ../client
+  $ echo '[experimental]' >> .hg/hgrc
+  $ echo 'bundle2.pushback = True' >> .hg/hgrc
+  $ echo c2 > f2
+  $ commit 2
+  adding f2
+  $ hg push
+  pushing to ssh://user@dummy/server
+  searching for changes
+  remote: adding changesets
+  remote: adding manifests
+  remote: adding file changes
+  remote: added 1 changesets with 1 changes to 1 files
+  $ hg bookmark
+     new-server-mark           2:0a76dfb2e179
+
+  $ cd ../server
+  $ log
+  o  2 [public:0a76dfb2e179]
+  |
+  o  1 [public:2b9c7234e035]
+  |
+  @  0 [public:6cee5c8f3e5b]
+  
+
+
+