601806 - Make it less easy to divulge the schedulerdb password/improve cancellator.py

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Lukas Blakk [:lsblakk] use ?needinfo]

It's been leaked a couple of times now because we use it in the cancellator command. This can end up in wikis, bugs, irc channels too easily.  It seems like it would make the password safer if it was not part of the cancellator command but instead cancellator prompted for it, or read from file on the master.

Comment 1

[John Ford [:jhford] CET/CEST Berlin Time]

i think each master has a file, passwords.py, with a variable "BBDB_URL".  Could we install cancellator as a file in the master directory and do 

from passwords import BBDB_URL

Comment 2

[Mike Taylor [:bear]]

Attachment: modify cancellator.py to load option defaults from a config file

nothing fancy - just add a step to the loading of command line option parsing to populate a defaults dictionary with the contents of a config file.

the config file is brute-force parsed for lines that are "option=value" formatted.

Comment 3

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

(In reply to comment #3)
> It's been leaked a couple of times now because we use it in the cancellator
> command. This can end up in wikis, bugs, irc channels too easily.  It seems
> like it would make the password safer if it was not part of the cancellator
> command but instead cancellator prompted for it, or read from file on the
> master.

+1 

(I also leaked it once!)

Comment 4

[[e-mail] (:bhearsum)]

Looks like Bear is working on this!

Comment 5

[[e-mail] (:bhearsum)]

Comment on attachment 480919 [details] [diff] [review]
modify cancellator.py to load option defaults from a config file

Why not use ConfigParser? Why are you deleting the found = True line?

Comment 6

[Mike Taylor [:bear]]

I wanted to avoid ConfigParser until it is shown that a more complicated parsing requirement is present.  Getting default values for ConfigParser via a file is not, afaik, an easy task and right now we only need the password.

I removed the found=True from outside of the loop because it's immediately overwritten just at the top of the loop and to me that is wrong.  If the loop type changes to have the possibility of not being entered then it can go back - but it will always be run once so variable instantiation should be at the top of the loop IMO

Comment 7

[John Ford [:jhford] CET/CEST Berlin Time]

can't you set the defaults by passing in a dictionary of defaults into the configparser?

Comment 8

[Mike Taylor [:bear]]

sure can - I even have helper code that does that automagically for my personal projects.

but this was someone else's code and I didn't know all of the params so I wanted to get the default value from a file and not get too fancy for a bug fix.

Comment 9

[[e-mail] (:bhearsum)]

(In reply to comment #6)
> I wanted to avoid ConfigParser until it is shown that a more complicated
> parsing requirement is present.  Getting default values for ConfigParser via a
> file is not, afaik, an easy task and right now we only need the password.

Ah, fair enough. r=me.

Comment 10

[Chris AtLee [:catlee]]

Comment on attachment 480919 [details] [diff] [review]
modify cancellator.py to load option defaults from a config file

Pretty sure that you need to expand the ~ in _config_file with os.path.expanduser prior using it as a path name.

You've also got an extra parser.add_option line that appears to be doing nothing.

I find parser.set_defaults(...) cleaner than setting a default per add_option call.  Maybe parser.set_defaults(**defaults) will work?

Comment 11

[Mike Taylor [:bear]]

(In reply to comment #10)
> Comment on attachment 480919 [details] [diff] [review]
> modify cancellator.py to load option defaults from a config file
> 
> Pretty sure that you need to expand the ~ in _config_file with
> os.path.expanduser prior using it as a path name.
> 
> You've also got an extra parser.add_option line that appears to be doing
> nothing.

oops - that's what I get for doing whitespace cleanup *after* testing

> I find parser.set_defaults(...) cleaner than setting a default per add_option
> call.  Maybe parser.set_defaults(**defaults) will work?

The reason I by habit use default per add_option is that in my personal library I actually have an outside dictionary that stores everything needed for the options and then call a helper routine to build the parser.  I'll try the set_defaults() call given that your not using my helper libs :)

Comment 12

[Mike Taylor [:bear]]

Attachment: modify cancellator.py to load option defaults from a config file

changed patch to use set_defaults(), removed errant add_option and added os.path.abspath() to convert ~/ to full path

Comment 13

[Mike Taylor [:bear]]

oh poo - during dinner it hit me: it's os.path.expanduser() not abspath()

Comment 14

[Mike Taylor [:bear]]

Attachment: modify cancellator.py to load option defaults from a config file

/me writes on chalkboard...

os.path.expanduser() os.path.expanduser() os.path.expanduser() os.path.expanduser() os.path.expanduser() os.path.expanduser() os.path.expanduser() os.path.expanduser() os.path.expanduser() os.path.expanduser() os.path.expanduser() os.path.expanduser() os.path.expanduser() os.path.expanduser() os.path.expanduser() os.path.expanduser() os.path.expanduser() os.path.expanduser()

Comment 15

[Chris AtLee [:catlee]]

Comment on attachment 483380 [details] [diff] [review]
modify cancellator.py to load option defaults from a config file

>+    if os.path.isfile(os.path.expanduser(_config_file)):
>+        for line in open(_config_file, 'r').readlines():

open() needs to be using the expanded path too.

Comment 16

[Mike Taylor [:bear]]

Attachment: modify cancellator.py to load option defaults from a config file

apologies for 1) being dense and 2) lagging in making this simple change

Comment 17

[Mike Taylor [:bear]]

Comment on attachment 487548 [details] [diff] [review]
modify cancellator.py to load option defaults from a config file

buildbot-related/cancellator.py
committed changeset 22:c6985e4e645b

Comment 18

[Mike Taylor [:bear]]

production-master02 updated (after fixing two glitches found after doing sanity checks)

marking as closed

Comment 19

[Nick Thomas [:nthomas] (UTC+12)]

Fixed paths to older masters and added pm03 in 
 http://hg.mozilla.org/build/braindump/rev/e18c6225fcbd

Updated pm02.