Merge lp:~jml/bzr/allow-writes-change-84659 into lp:~bzr/bzr/trunk-old

This branch changes the 'serve' command to take --allow-anonymous-write-access instead of --allow-writes, which it deprecates.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jonathan Lange wrote:
> Jonathan Lange has proposed merging lp:~jml/bzr/allow-writes-change-84659 into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> This branch changes the 'serve' command to take --allow-anonymous-write-access instead of --allow-writes, which it deprecates.
>

So... I'm a bit iffy on this. For example when using "bzr+ssh" we do:

ssh host bzr serve --inet --allow-writes --directory=/

It doesn't really fit to do:

ssh host bzr serve --inet --allow-anonymous-writes --directory=/

1) It will break launchpad (until the changes are merged, and the ssh
server updated to allow that string)
2) It *is* anonymous, except that you've already authenticated...

I understand the desire behind being clear about it for "bzr serve" to
bring up the http/bzr:// access. I just worry about some of the fallout.

(I'm not blocking, just bringing up the discussion.)

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkpPorEACgkQJdeBCYSNAAM4BgCfTrNMochm9renXoWBE0Jgyhib
BhEAoL5VPhldTYA+5DhR6JHlo/9ATxI+
=jAd+
-----END PGP SIGNATURE-----

> Jonathan Lange wrote:
> > Jonathan Lange has proposed merging lp:~jml/bzr/allow-writes-change-84659
> into lp:bzr.
> >
> > Requested reviews:
> > bzr-core (bzr-core)
> >
> > This branch changes the 'serve' command to take --allow-anonymous-write-
> access instead of --allow-writes, which it deprecates.
> >
>
> So... I'm a bit iffy on this. For example when using "bzr+ssh" we do:
>
> ssh host bzr serve --inet --allow-writes --directory=/
>
> It doesn't really fit to do:
>
> ssh host bzr serve --inet --allow-anonymous-writes --directory=/
>

Agreed. I wonder if the mis-fit is all that bad.

> 1) It will break launchpad (until the changes are merged, and the ssh
> server updated to allow that string)

Well, the old option is still present & deprecated.

Also, pretty much every release of Bazaar breaks Launchpad :)

> 2) It *is* anonymous, except that you've already authenticated...
>

Yeah, this is also kind of unfortunate.

> I understand the desire behind being clear about it for "bzr serve" to
> bring up the http/bzr:// access. I just worry about some of the fallout.
>
> (I'm not blocking, just bringing up the discussion.)

So where to from here?

jml

Something that was missed I think, is that this breaks 'bzr push bzr
+ssh://' between versions. So

 review -1

We should _not_ break that fairly fundamental interface without a very
compelling reason.

> > I understand the desire behind being clear about it for "bzr serve" to
> > bring up the http/bzr:// access. I just worry about some of the fallout.
> >
> > (I'm not blocking, just bringing up the discussion.)
>
> So where to from here?

I think --allow-writes is clear and precise. I don't think we should
change it unless the change fixes a problem greater than changing it
will cause.

AIUI the problem is one of expectation? People expecting that allowing
writes implies some sort of authentication, and we *anticipate* that
users are putting live, anonymous write permitting servers on the
internet?

Seems to me that updating the docs is all that can really be done, in
the absence of deeper changes.

Perhaps:
 - add a parameter:
   --auth=(external|anonymous)
 - make --allow-writes error unless --auth= is supplied or --inet was
   supplied.

Then, for ssh:
 --inet is supplied

For http:
 --auth is required

For inetd:
 If the user starts it via the superserver, --inet is passed and its up
 to them. Otherwise --auth is required.

-Rob

2009/7/6 Robert Collins <email address hidden>:

>> > I understand the desire behind being clear about it for "bzr serve" to
>> > bring up the http/bzr:// access. I just worry about some of the fallout.
>> >
>> > (I'm not blocking, just bringing up the discussion.)
>>
>> So where to from here?
>
> I think --allow-writes is clear and precise. I don't think we should
> change it unless the change fixes a problem greater than changing it
> will cause.
>
> AIUI the problem is one of expectation? People expecting that allowing
> writes implies some sort of authentication, and we *anticipate* that
> users are putting live, anonymous write permitting servers on the
> internet?

I think that's true.

Murphy's law (correctly stated) implies and experience confirms that
if it is possible to offer anonymous write access to the whole
internet, people will do it. I feel an obligation to at least reduce
the occurrence of problems arising from it.

sshd sets some environment variables for the subprocess. On a brief
inspection this doesn't seem to help us distinguish bzr started by
"ssh host bzr --inet" and "ssh host" then restarting inetd from the
shell, but there may be more detail or it might work as a partial
protection.

Compatibility with existing clients is important.

We can take a step forward by saying that at least for server mode,
listening on --http or as a daemon, you need a special step to allow
write access. This shouldn't break normal use, and people with a
special use can think about what they want. Robert has a point with
something like a "--auth" option indicating what authentication is
acceptable.

--
Martin <http://launchpad.net/~mbp/>

Martin Pool wrote:
[...]
> Murphy's law (correctly stated) implies and experience confirms that
> if it is possible to offer anonymous write access to the whole
> internet, people will do it. I feel an obligation to at least reduce
> the occurrence of problems arising from it.

I agree, and that's why the default is read-only, and you have to use
apply a switch to enable writing at all. The hope was that at the point
the user makes a conscious decision to choose and type “--allow-writes”
they will have applied some thought to the consequences of that.

(And the default is also to only serve the cwd, not the whole
filesystem. That's not maximally paranoid by default, but it's pretty
good I think.)

> sshd sets some environment variables for the subprocess. On a brief
> inspection this doesn't seem to help us distinguish bzr started by
> "ssh host bzr --inet" and "ssh host" then restarting inetd from the
> shell, but there may be more detail or it might work as a partial
> protection.

Yeah, and if we infer things from environment variables it's easy to
imagine issues with older OpenSSH sshd versions, or non-OpenSSH sshd
implementations.

> Compatibility with existing clients is important.
>
> We can take a step forward by saying that at least for server mode,
> listening on --http or as a daemon, you need a special step to allow
> write access. This shouldn't break normal use, and people with a
> special use can think about what they want. Robert has a point with
> something like a "--auth" option indicating what authentication is
> acceptable.

Complicated rules for a command along the lines of “--auth=... is
mandatory unless --inet is provided” feel a bit messy to me. Maybe
what's needed is separate commands for “start a TCP server” and “start a
bzr+ssh” server? I'm not sure that that is the best answer, but the
different requirements for typical bzr+ssh vs. TCP are a bit suggestive
of it. And we're planning on adding --http to the mix...

-Andrew.

>>>>> "Andrew" == Andrew Bennetts <email address hidden> writes:

<snip/>

    Andrew> Complicated rules for a command along the lines of
    Andrew> “--auth=... is mandatory unless --inet is provided”
    Andrew> feel a bit messy to me. Maybe what's needed is
    Andrew> separate commands for “start a TCP server” and “start
    Andrew> a bzr+ssh” server? I'm not sure that that is the
    Andrew> best answer, but the different requirements for
    Andrew> typical bzr+ssh vs. TCP are a bit suggestive of it.
    Andrew> And we're planning on adding --http to the mix...

I agree with the feeling. I'd prefer several high level commands
with their own options that all use the same helper than a single
high-level command with many options for which some combinations
make no sense.

> Something that was missed I think, is that this breaks 'bzr push bzr
> +ssh://' between versions. So
>
> review -1
>
> We should _not_ break that fairly fundamental interface without a very
> compelling reason.
>

Fair enough.

I'll update this merge proposal and the original bug accordingly.