I’m not sure how to continue with this task. Is it okay to add all the maintainers of the affected tools as subscribers? That would let e.g. the wikiusos maintainer steal the brazilianlaws OAuth credentials if they see the notification before the brazilianlaws maintainer has had the opportunity to protect their file…

(wikiusos and brazilianlaws actually happen to have the same maintainer, but not all of the listed tools are by the same user.)

I've reviewed secrets in this task, and it looks it is OAuth-related tokens for the consumer or the SECRET_KEY flask config variable. I revoked all the OAuth secrets and sent an email to the tool's maintainer (via the default tools.XXX@tools.wmflabs.org mail alias). This should be good enough for now, I think.

I also fixed the two tools I maintain, ipwatcher and massmailer. ipwatcher is pending T286423 to be considered fully done.

In T286416#7203593, @LucasWerkmeister wrote:

Is it okay to add all the maintainers of the affected tools as subscribers?

I'm not certain there's a better way of getting various project-owners' attentions regarding this issue, aside from breaking this task out into individual tasks for each project, which seems a bit onerous. If anyone was at all curious/nefarious, the first question they'd likely ask after realizing their config.yaml was world-readable would be "wait, are other config files world-readable on toolforge?" so I'm not sure that more burdensome approach would even provide much of an advantage.

The other option I can think of is that a Toolforge admin chmods the files preemptively and then we invite the maintainers here. But if @Urbanecm already revoked the OAuth secrets then adding them all directly is probably fine.

In T286416#7206845, @LucasWerkmeister wrote:

The other option I can think of is that a Toolforge admin chmods the files preemptively and then we invite the maintainers here.

Done. I think we need to make our documentation much more clear on file permissions, these kinds of issues are unfortunately common.

In T286416#7206845, @LucasWerkmeister wrote:

[...] But if @Urbanecm already revoked the OAuth secrets then adding them all directly is probably fine.

I confirm I revoked the OAuth secrets used in the files, and verified via a SQL query that I didn't miss anything.

I think we need to make our documentation much more clear on file permissions, these kinds of issues are unfortunately common.

Yeah, I’m thinking of adding something to cookiecutter-toolforge too. It already instructs users to chmod the file, but it should probably come with boilerplate code to either chmod the file on startup or refuse to start if it’s world-readable.

Anyways, adding the maintainers now. Welcome, everyone! See the task description for context :)

@Majavah Previous versions of the tutorial Django documentation left out the step of chmodding the file (resulting in a lot of occurrences of this for settings.py...which is why we added that to it for the documentation pieces I could find at the time). It's documented in the Flask one (though I might suggest making it a bigger deal than just one command in the list):
https://wikitech.wikimedia.org/wiki/Help:Toolforge/My_first_Flask_OAuth_tool#How_to_add_a_configuration_file

There is no real mention of such concerns in the Pywikibot one https://wikitech.wikimedia.org/wiki/Help:Toolforge/Pywikibot

Unfortunately, these issues are common in all environments, including things like Amazon S3. It requires docs, monitoring and guard rails like cookiecutters to fix.

I wonder if https://github.com/lucaswerkmeister/cookiecutter-toolforge is used in any tutorial docs now? The whole set-up needs more guard rails still to help people make good choices.

I would by far prefer if people used the replica.my.cnf file directly instead (for the wikireplicas part...doesn't help with OAUTH). It's not hard to read an ini file in with python, but our docs don't seem to suggest it.

In T286416#7207048, @Bstorm wrote:

I would by far prefer if people used the replica.my.cnf file directly instead (for the wikireplicas part...doesn't help with OAUTH). It's not hard to read an ini file in with python, but our docs don't seem to suggest it.

@Legoktm’s toolforge library has a toolsdb function that’ll read the replica.my.cnf, so that would be a good thing to advertise. (Currently I don’t have any database functions in cookiecutter-toolforge because I rarely use ToolsDB, but maybe I should add something there.)

Hello to all! I can confirm that I skiped this line of code in my first application, then copy+pasted to the other ones. Sorry about that.

I followed the instructions @Urbanecm gave me by email and hope everything is ok now.

The applications I mantain and did this update to are: brazilianlaws, wikibrasoes, wikimarcas, wikimotivos, wikiquantos, wikiroupas and wikiusos.

Good contributions.

In T286416#7206991, @LucasWerkmeister wrote:

I think we need to make our documentation much more clear on file permissions, these kinds of issues are unfortunately common.

Yeah, I’m thinking of adding something to cookiecutter-toolforge too. It already instructs users to chmod the file, but it should probably come with boilerplate code to either chmod the file on startup or refuse to start if it’s world-readable.

cookiecutter-toolforge now comes with some boilerplate code that throws an exception if the config file is group- or world-readable (commit).

Is there anything left to do in this task before we close it and make it public? There aren’t any world-readable files with secrets left:

lucaswerkmeister@tools-sgebastion-07:~$ for file in /data/project/*/www/python/src/config.yaml; do if grep -qi -e secret_key -e oauth "$file" 2>/dev/null; then printf '%s\n' "$file"; fi; done
lucaswerkmeister@tools-sgebastion-07:~$ ls -l /data/project/clpo13-flask/www/python/src/config.yaml # example from task description
-rw------- 1 tools.clpo13-flask tools.clpo13-flask 270 Dez 21  2018 /data/project/clpo13-flask/www/python/src/config.yaml

In T286416#7487295, @LucasWerkmeister wrote:

Is there anything left to do in this task before we close it and make it public? There aren’t any world-readable files with secrets left:

I'll move this back into our incoming column to reassess at the Security-Team's clinic today. If @Dsharpe / WMF-Legal do not have any issues then it should be fine to make this public at this point. The search patterns within your above bash obviously do not cover every type of "secret", but I personally think there's been enough due diligence for the issues surfaced on this task.

As a follow up: https://wikitech.wikimedia.org/w/index.php?title=Help%3AToolforge%2FMy_first_Flask_OAuth_tool&type=revision&diff=1970023&oldid=1949040

In T286416#7862557, @Zabe wrote:

As a follow up: https://wikitech.wikimedia.org/w/index.php?title=Help%3AToolforge%2FMy_first_Flask_OAuth_tool&type=revision&diff=1970023&oldid=1949040

Okay, but if you’re chmoding the file after writing credentials to it, you’re doing it wrong, because then you still left open a window where the file was world-readable and contained credentials. What you want to do is make the file private before writing to it – and this is exactly what the documentation already tells you to do:

$ touch $HOME/www/python/src/config.yaml
$ chmod u=rw,go= $HOME/www/python/src/config.yaml
$ cat > $HOME/www/python/src/config.yaml << EOF
GREETING: Goodnight moon!
EOF

So I would just revert that documentation edit, to be honest.

(Side note: technically, chmoding the file after creating it still leaves a small vulnerability, because someone else can open() the file while it’s still world-readable, and then read() from it later after it’s been made private and credentials have been written to it: permissions are only checked when a file is opened, not each time it is read. So really you want to create with the correct permissions right away, e.g. by adjusting the umask or using install. Edit: cookiecutter-toolforge fix)

In T286416#7862577, @LucasWerkmeister wrote:

In T286416#7862557, @Zabe wrote:

As a follow up: https://wikitech.wikimedia.org/w/index.php?title=Help%3AToolforge%2FMy_first_Flask_OAuth_tool&type=revision&diff=1970023&oldid=1949040

Okay, but if you’re chmoding the file after writing credentials to it, you’re doing it wrong, because then you still left open a window where the file was world-readable and contained credentials. What you want to do is make the file private before writing to it – and this is exactly what the documentation already tells you to do:

$ touch $HOME/www/python/src/config.yaml
$ chmod u=rw,go= $HOME/www/python/src/config.yaml
$ cat > $HOME/www/python/src/config.yaml << EOF
GREETING: Goodnight moon!
EOF

Makes sense.

So I would just revert that documentation edit, to be honest.

Done, I did not notice that it was already documented.

(Side note: technically, chmoding the file after creating it still leaves a small vulnerability, because someone else can open() the file while it’s still world-readable, and then read() from it later after it’s been made private and credentials have been written to it: permissions are only checked when a file is opened, not each time it is read. So really you want to create with the correct permissions right away, e.g. by adjusting the umask or using install. Edit: cookiecutter-toolforge fix)