This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6939

sidedata: apply basic but tight security around exchange
ClosedPublic

Authored by marmoute on Oct 1 2019, 12:07 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHGc17a63eb5d4c: sidedata: apply basic but tight security around exchange
Summary

We don't currently have code to deal with exchange between repository using
sidedata and repository not using sidedata. Until we implement such code (eg:
dropping side data when pushing to a non-sidedata repo) we prevent the two kind
of repo to speak to each other. This is somewhere similar to what 'treemanifest'
does.

Note that sidedata exchange is broken unless one use changegroup v3 anyway. See
next changeset for details.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

marmoute created this revision.Oct 1 2019, 12:07 PM
marmoute updated this revision to Diff 16771.Oct 2 2019, 12:08 AM
martinvonz added a subscriber: martinvonz.Oct 4 2019, 11:55 PM
martinvonz added inline comments.
mercurial/bundle2.py
1978–1979

Isn't bool() redundant here?

1981–1982

Need b'' prefix, I think (also pick one type of quotes maybe? but maybe Black will soon change this anyway...)

martinvonz added inline comments.Oct 4 2019, 11:59 PM
mercurial/bundle2.py
1981–1982

Never mind... I've just gotten used to looking at extension code where the source transformer is not run. It is run on this code, of course, so this is correct as is.

indygreg added a subscriber: indygreg.Oct 6 2019, 9:49 AM
indygreg added inline comments.
mercurial/bundle2.py
2554

Shouldn't this by exp-sidedata?

marmoute updated this revision to Diff 16943.Oct 7 2019, 8:36 PM
marmoute marked an inline comment as done.Oct 7 2019, 8:51 PM
marmoute added inline comments.
mercurial/bundle2.py
1978–1979

It is redundant, but helping Raphaël work on Rust got me scared about boolean. I'll clean it up (either resend or followup)

2554

Hum, yeah. That would be safer. Updated code coming

marmoute marked an inline comment as done.Oct 9 2019, 12:45 PM
marmoute added a commit: rHGc17a63eb5d4c: sidedata: apply basic but tight security around exchange.
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHGc17a63eb5d4c: sidedata: apply basic but tight security around exchange (authored by marmoute). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/bundle2.py (21 lines)
Mmercurial/exchange.py (5 lines)

Diff 16993

mercurial/bundle2.py

b'nbchanges', b'%d' % cg.extras[b'clcount'], mandatory=False b'nbchanges', b'%d' % cg.extras[b'clcount'], mandatory=False
) )
if opts.get(b'phases') and repo.revs( if opts.get(b'phases') and repo.revs(
b'%ln and secret()', outgoing.missingheads b'%ln and secret()', outgoing.missingheads
): ):
part.addparam( part.addparam(
b'targetphase', b'%d' % phases.secret, mandatory=False b'targetphase', b'%d' % phases.secret, mandatory=False
) )
if b'exp-sidedata-flag' in repo.requirements:
part.addparam(b'exp-sidedata', b'1')
if opts.get(b'streamv2', False): if opts.get(b'streamv2', False):
addpartbundlestream2(bundler, repo, stream=True) addpartbundlestream2(bundler, repo, stream=True)
if opts.get(b'tagsfnodescache', True): if opts.get(b'tagsfnodescache', True):
addparttagsfnodescache(repo, bundler, outgoing) addparttagsfnodescache(repo, bundler, outgoing)
if opts.get(b'revbranchcache', True): if opts.get(b'revbranchcache', True):
if changedheads > 0: if changedheads > 0:
result = 1 + changedheads result = 1 + changedheads
elif changedheads < 0: elif changedheads < 0:
result = -1 + changedheads result = -1 + changedheads
return result return result
@parthandler( @parthandler(
b'changegroup', (b'version', b'nbchanges', b'treemanifest', b'targetphase') b'changegroup',
(
b'version',
b'nbchanges',
b'exp-sidedata',
b'treemanifest',
b'targetphase',
),
) )
def handlechangegroup(op, inpart): def handlechangegroup(op, inpart):
"""apply a changegroup part on the repo """apply a changegroup part on the repo
This is a very early implementation that will massive rework before being This is a very early implementation that will massive rework before being
inflicted to any end-user. inflicted to any end-user.
""" """
from . import localrepo from . import localrepo
b"non-empty and does not use tree manifests" b"non-empty and does not use tree manifests"
) )
) )
op.repo.requirements.add(b'treemanifest') op.repo.requirements.add(b'treemanifest')
op.repo.svfs.options = localrepo.resolvestorevfsoptions( op.repo.svfs.options = localrepo.resolvestorevfsoptions(
op.repo.ui, op.repo.requirements, op.repo.features op.repo.ui, op.repo.requirements, op.repo.features
) )
op.repo._writerequirements() op.repo._writerequirements()
bundlesidedata = bool(b'exp-sidedata' in inpart.params)
reposidedata = bool(b'exp-sidedata-flag' in op.repo.requirements)
martinvonzUnsubmitted
Not Done

Isn't bool() redundant here?

martinvonz: Isn't `bool()` redundant here?
marmouteAuthorUnsubmitted
Done

It is redundant, but helping Raphaël work on Rust got me scared about boolean. I'll clean it up (either resend or followup)

marmoute: It is redundant, but helping Raphaël work on Rust got me scared about boolean. I'll clean it up…
if reposidedata and not bundlesidedata:
msg = b"repository is using sidedata but the bundle source do not"
hint = b'this is currently unsupported'
martinvonzUnsubmitted
Not Done

Need b'' prefix, I think (also pick one type of quotes maybe? but maybe Black will soon change this anyway...)

martinvonz: Need `b''` prefix, I think (also pick one type of quotes maybe? but maybe Black will soon…
martinvonzUnsubmitted
Not Done

Never mind... I've just gotten used to looking at extension code where the source transformer is not run. It is run on this code, of course, so this is correct as is.

martinvonz: Never mind... I've just gotten used to looking at extension code where the source transformer…
raise error.Abort(msg, hint=hint)
extrakwargs = {} extrakwargs = {}
targetphase = inpart.params.get(b'targetphase') targetphase = inpart.params.get(b'targetphase')
if targetphase is not None: if targetphase is not None:
extrakwargs[r'targetphase'] = int(targetphase) extrakwargs[r'targetphase'] = int(targetphase)
ret = _processchangegroup( ret = _processchangegroup(
op, op,
cg, cg,
tr, tr,
cl = repo.changelog cl = repo.changelog
for r in repo.revs(b"::%ln", common): for r in repo.revs(b"::%ln", common):
commonnodes.add(cl.node(r)) commonnodes.add(cl.node(r))
if commonnodes: if commonnodes:
# XXX: we should only send the filelogs (and treemanifest). user # XXX: we should only send the filelogs (and treemanifest). user
# already has the changelog and manifest # already has the changelog and manifest
packer = changegroup.getbundler( packer = changegroup.getbundler(
cgversion, cgversion,
repo, repo,
indygregUnsubmitted
Done

Shouldn't this by exp-sidedata?

indygreg: Shouldn't this by `exp-sidedata`?
marmouteAuthorUnsubmitted
Done

Hum, yeah. That would be safer. Updated code coming

marmoute: Hum, yeah. That would be safer. Updated code coming
oldmatcher=oldmatcher, oldmatcher=oldmatcher,
matcher=newmatcher, matcher=newmatcher,
fullnodes=commonnodes, fullnodes=commonnodes,
) )
cgdata = packer.generate( cgdata = packer.generate(
{nodemod.nullid}, {nodemod.nullid},
list(commonnodes), list(commonnodes),
False, False,
b'narrow_widen', b'narrow_widen',
changelog=False, changelog=False,
) )
part = bundler.newpart(b'changegroup', data=cgdata) part = bundler.newpart(b'changegroup', data=cgdata)
part.addparam(b'version', cgversion) part.addparam(b'version', cgversion)
if b'treemanifest' in repo.requirements: if b'treemanifest' in repo.requirements:
part.addparam(b'treemanifest', b'1') part.addparam(b'treemanifest', b'1')
if b'exp-sidedata-flag' in repo.requirements:
part.addparam(b'exp-sidedata', b'1')
return bundler return bundler

mercurial/exchange.py

cgstream = changegroup.makestream( cgstream = changegroup.makestream(
pushop.repo, pushop.outgoing, version, b'push' pushop.repo, pushop.outgoing, version, b'push'
) )
cgpart = bundler.newpart(b'changegroup', data=cgstream) cgpart = bundler.newpart(b'changegroup', data=cgstream)
if cgversions: if cgversions:
cgpart.addparam(b'version', version) cgpart.addparam(b'version', version)
if b'treemanifest' in pushop.repo.requirements: if b'treemanifest' in pushop.repo.requirements:
cgpart.addparam(b'treemanifest', b'1') cgpart.addparam(b'treemanifest', b'1')
if b'exp-sidedata-flag' in pushop.repo.requirements:
cgpart.addparam(b'exp-sidedata', b'1')
def handlereply(op): def handlereply(op):
"""extract addchangegroup returns from server reply""" """extract addchangegroup returns from server reply"""
cgreplies = op.records.getreplies(cgpart.id) cgreplies = op.records.getreplies(cgpart.id)
assert len(cgreplies[b'changegroup']) == 1 assert len(cgreplies[b'changegroup']) == 1
pushop.cgresult = cgreplies[b'changegroup'][0][b'return'] pushop.cgresult = cgreplies[b'changegroup'][0][b'return']
return handlereply return handlereply
if cgversions: if cgversions:
part.addparam(b'version', version) part.addparam(b'version', version)
part.addparam(b'nbchanges', b'%d' % len(outgoing.missing), mandatory=False) part.addparam(b'nbchanges', b'%d' % len(outgoing.missing), mandatory=False)
if b'treemanifest' in repo.requirements: if b'treemanifest' in repo.requirements:
part.addparam(b'treemanifest', b'1') part.addparam(b'treemanifest', b'1')
if b'exp-sidedata-flag' in repo.requirements:
part.addparam(b'exp-sidedata', b'1')
if ( if (
kwargs.get(r'narrow', False) kwargs.get(r'narrow', False)
and kwargs.get(r'narrow_acl', False) and kwargs.get(r'narrow_acl', False)
and (include or exclude) and (include or exclude)
): ):
# this is mandatory because otherwise ACL clients won't work # this is mandatory because otherwise ACL clients won't work
narrowspecpart = bundler.newpart(b'Narrow:responsespec') narrowspecpart = bundler.newpart(b'Narrow:responsespec')
narrowspecpart.data = b'%s\0%s' % ( narrowspecpart.data = b'%s\0%s' % (