Re: [Savannah-hackers-public] bzr commit notifications

[Bob Proulx]

Glenn Morris wrote:
> Recently Savannah bzr was updated to bzr 2.6.
> This breaks the bzr-hookless-email script that was being used for
> commit notifications. [1]
> 
> To try and fix this, I suggest installing the bzr-email plugin.
> `apt-get install bzr-email' (the version from Debian testing is the
> latest version).

Thank you for researching this problem and proposing a solution.
That's awesome!

> IIUC, historically Savannah had some theoretical security concerns
> about bzr-email. I've looked at it and IMO the one issue that there
> might be can be trivially patched away. [2]
> But I don't really know why you went with bzr-hookless-email.

This was before my time and I have no information on it.

> If you are willing to install it, I can test it.

I am willing to give it a try.  Here are my comments.  There will be
enough delay that if there are other comments I will hear them before
I get this done.  But otherwise I will implement your plan as filed.

> [1] If you want the details,
> https://lists.ubuntu.com/archives/bazaar/2013q2/075520.html

With the exception of your comment there that the 2.6 wasn't in the
then stable Squeeze 6.  It is in the now stable Wheezy 7.  But vcs.sv
is still at this instant running Squeeze 6 and would normally have had
bzr 2.1.2.  The bzr that was installed on vcs.sv was not from the
Debian packages but from local compilation.

> [2] The only issue I can see is this:
> Anyone with write access to a bzr branch can set plugin options in
> .bzr/branch/branch.conf (this is actually good, because it means we
> will be able to control our own commit notifications without needing
> to bug Savannah admins).

If those are simply options I don't see the issue.  But with security
concerns it was probably possible to set configurations that executed
commands.  I assume.  I haven't looked.

> One option is "post_commit_mailer".
> This is either 'smtplib' (an internal Python library), or an external
> command like "/bin/mail".
> (I am assuming smtplib will be the correct option for Savannah.)

I don't think it matters.  My personal preference is to use the local
command.  I would use /usr/sbin/sendmail as a general local mailer
because that takes no configuration.  But /bin/mail or /usr/bin/mailx
would be okay too.  In that role they are effectively simply a wrapper
around /usr/sbin/sendmail.

Editorial comment: I think the general preference that we often see in
documentation for smtp is that people developing on an MS or Apple
client don't usually have an mta running.  Using smtp they can point
off to their mail relay and it works for them.  Using
/usr/sbin/sendmail only works on GNU/BSD/Unix machines.  But we have
one of those.  :-)

> Someone could try and set this to something nasty like "rm -rf /".
> So all we need to do is hard-code that option to "smtplib".
> Patching emailer.py is one trivial way to do that (see end).

That would be unpleasant.  Talking out the problem....  It would run
on the server with the user:group of the person who is running the
commit, who is probably not the person who set up the attack.  The
permissions would be similar though.  It would take quite a long time
to traverse the entire filesystem slowing things down for a long time.
It would eventually remove the project files that are accessible by
that user:group.  It wouldn't have permission to remove other projects
files or anything else on the server.  Worse would be if a single user
were in several groups because then it would have permission to remove
all of the files in all of the groups of that user.

> Alternatively, I am told that options set in ~/.bzr/locations.conf
> will take precedence over branch options. If bzr on Savannah runs
> under a single user, that could be a better way to do it.

AFAIK write access is through ssh+bzr invoking sv_membersh which is a
restricted shell which screens incoming actions.  Each process is run
as a specific uid:gid and access is allowed or denied by group control.
There isn't a single process owner.  It is compartmentalized into
individually owned processes.

> If you want to review it, the code is at
> https://launchpad.net/bzr-email

If a python person would look that over it would be great.  I am not a
python person so must rely upon it being "obvious".

> I suggest a patch something like:
> 
> ***************
> *** 206,212 ****
>               if mailer == 'smtplib':
>                   self._send_using_smtplib()
>               else:
> !                 self._send_using_process()
>           finally:
>               self.repository.unlock()
>               self.branch.unlock()
> --- 206,213 ----
>               if mailer == 'smtplib':
>                   self._send_using_smtplib()
>               else:
> !                 raise errors.BzrError("Bad value for post_commit_mailer")
> !                 # self._send_using_process()
>           finally:
>               self.repository.unlock()
>               self.branch.unlock()

Seems okay.  Throw an error if not one of the predefined possibilites.

> ***************
> *** 303,309 ****
>   opt_post_commit_to = ListOption('post_commit_to',
>       help='Address to send commit emails to.')
>   opt_post_commit_mailer = Option('post_commit_mailer',
> !     help='Mail client to use.', default='mail')
>   opt_post_commit_url = Option('post_commit_url',
>       help='URL to mention for branch in post commit messages.')
>   opt_revision_mail_headers = ListOption('revision_mail_headers',
> --- 304,310 ----
>   opt_post_commit_to = ListOption('post_commit_to',
>       help='Address to send commit emails to.')
>   opt_post_commit_mailer = Option('post_commit_mailer',
> !     help='Mail client to use.', default='smtplib')
>   opt_post_commit_url = Option('post_commit_url',
>       help='URL to mention for branch in post commit messages.')
>   opt_revision_mail_headers = ListOption('revision_mail_headers',

Change the default to smtplib.

> > (I am assuming smtplib will be the correct option for Savannah.)
> 
> PS if not, patch would look like this. Replace /usr/bin/mail with
> whatever the correct value is for Savannah.
> 
>               if mailer == 'smtplib':
>                   self._send_using_smtplib()
> !             elif mailer == '/usr/bin/mail':
>                   self._send_using_process()
> +             else:
> +                 raise errors.BzrError("Bad value for mailer option")

I think this allows /usr/bin/mail as one of the allowable options.
Which is fine.  Require that mailer be one or the other and throw an
exception otherwise.  Looks okay to me.

Because mailx is POSIX I think that should be /usr/bin/mailx and not
/usr/bin/mail.  But practically they are the same since the bsd-mail
package is providing both.

  vcs:~# update-alternatives --display mailx
  mailx - auto mode
    link currently points to /usr/bin/bsd-mailx
  /usr/bin/bsd-mailx - priority 50
    slave Mail.1.gz: /usr/share/man/man1/bsd-mailx.1.gz
    slave mail.1.gz: /usr/share/man/man1/bsd-mailx.1.gz
    slave mailx.1.gz: /usr/share/man/man1/bsd-mailx.1.gz
    slave mail: /usr/bin/bsd-mailx
    slave Mail: /usr/bin/bsd-mailx
  Current 'best' version is '/usr/bin/bsd-mailx'.

As a practical matter on other systems /usr/bin/mail is usually the
old /bin/mail program and doesn't support the -s subject option.  But
/usr/bin/mailx is the BSD traditional /bin/Mail program and does
support the -s subject option.  Of course GNU is not Unix and so this
difference is really just one of those portability issues that never
fully got converged everywhere regardless of POSIX standardizing on
the SysV flavor of it.  In SysV /bin/Mail with a capital would be out
of place so they went with the /usr/bin/mailx name for it.

Bob