"serve --allow-writes" allows more than you might think

Bug #84659 reported by Martin Pool on 2007-02-12
6
Affects Status Importance Assigned to Milestone
Bazaar
Medium
Unassigned

Bug Description

'bzr serve --allow-writes' is actually a bit dangerous, since it allows anonymous write access by anyone with network access to the server.

At the least, the help of the option should be updated to indicate this. Renaming the option is out of the question due to backwards compatibility constraints. Perhaps we can do something else that's more than simple documentation.

Martin Pool (mbp) on 2007-02-12
Changed in bzr:
assignee: nobody → spiv
importance: Undecided → High
John A Meinel (jameinel) on 2007-04-30
Changed in bzr:
status: Unconfirmed → Confirmed
Alexander Belchenko (bialix) wrote :

so, only changes in option name required to fix this bug?

Martin Pool (mbp) wrote :

Yes.

Isn't this option also used to enable writes when bzr serve is used over ssh ?

Jason Spashett (jspashett) wrote :

Could be. I stuck a branch up with the potential changes for this bug, I am not all all famililiar with the bzr code at all yet, but there appear to be some necessary changes for this around ssh. In contrib/bzr_ssh_path_limiter but I am not quite sure what any of it does yet.

Andrew Bennetts (spiv) wrote :

Jelmer: yes, --allow-writes is used over bzr+ssh.

Jason Spashett (jspashett) wrote :

I presume that --allow-writes will also have to be supported for a time, therefore should --allow-anonymous-write-access be implemented and presented in the help, but --allow-writes also be accepted too but not documented? Or does the documentation (bzr help + docs) need to convey this change?

Martin Pool (mbp) on 2009-04-21
Changed in bzr:
importance: High → Medium
Jonathan Lange (jml) on 2009-07-04
Changed in bzr:
assignee: Andrew Bennetts (spiv) → Jonathan Lange (jml)
status: Confirmed → Fix Committed
Jonathan Lange (jml) wrote :

As discussed on https://code.edge.launchpad.net/~jml/bzr/allow-writes-change-84659/+merge/8222, renaming this option is not acceptable, since it breaks backwards compatibility. Many other solutions to the root problem are discussed on that page.

tags: removed: easy
Changed in bzr:
status: Fix Committed → Triaged
summary: - "serve --allow-writes" needs a more obvious name
+ "serve --allow-writes" allows more than you might think
Jonathan Lange (jml) on 2009-07-06
description: updated
Jonathan Lange (jml) wrote :

Martin suggested a fix on IRC. If serve is called without --inet and with --allow-writes, issue a warning. This is easy enough, just call note() in cmd_serve or something that cmd_serve calls.

Jonathan Lange (jml) on 2009-07-07
tags: added: easy

Here is a patch that displays a warning (using 'note') when --allow-writes is used without --inet.

2009/7/20 Danny van Heumen <email address hidden>:
> Here is a patch that displays a warning (using 'note') when --allow-
> writes is used without --inet.
>
> ** Attachment added: "Patch that shows a warning (note) as suggested by Martin."
>   http://launchpadlibrarian.net/29255904/bzr.dev-bug-84659.patch

Thanks, that looks good Danny, with a few tweaks:

 * The string should be wrapped as it makes the line too long.
 * The indenting should be 4 spaces
 * You should use the trace.warning function not note
 * Ideally there would also be a test for it, along the lines of those
already in blackbox.test_serve

Could you please see about pushing a branch containing your fix to
Launchpad and then propose it for merging into trunk, then we can do a
proper review:

When this is merged it should be mentioned in NEWS as closing this bug.

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

Changed in bzr:
status: Triaged → In Progress
Jonathan Lange (jml) wrote :

I'm not going to be working on this any time soon. I'd really appreciate it if someone else could land Danny's patch though.

Changed in bzr:
assignee: Jonathan Lange (jml) → nobody
Gordon Tyler (doxxx) wrote :

I've applied Danny's patch and made the changes suggested by Martin and pushed it to lp:~doxxx/bzr/84659-allow-writes.

Gordon Tyler (doxxx) wrote :

Nevermind, I see his changes have been accepted.

Robert Collins (lifeless) wrote :

Coming back to this, I have reason /not/ to emit a warning.

We now support --http to serve http from 'bzr serve'. Its arguably sane for people to run this up behind apache with apache doing the auth, and emitting a warning there would simply be annoying.

I think the help docs on the option are really the sensible total feedback we should give on this. Dangerous options exist, but its reasonable to expect users to find out about options via the docs, where we can warn without emitting warnings in usual circumstances.

Since Robert's fix is merged, should the status be set to 'Fix Committed' or something? ('In Progress' is not really the current state ...)

Patrick Regan (patrick-regan) wrote :

>Since Robert's fix is merged, should the status be set to 'Fix Committed' or something? ('In Progress' is not really the current state ...)

That's correct. I fixed it.

Changed in bzr:
status: In Progress → Fix Released
milestone: none → 2.1.0b4
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers