Page MenuHomePhabricator

centralauthtoken should be redacted in logs (including hadoop wmf_raw.apiaction)
Closed, ResolvedPublic

Description

centralauthtoken, although is only alive for a short time, is an auth token, and its probably best not to log auth tokens (imo). similar to how passwords are treated.

Event Timeline

Bawolff created this task.Oct 24 2018, 2:13 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 24 2018, 2:13 AM
Anomie added a subscriber: Anomie.

For the logs generated by the API itself (e.g. api.log on mwlog1001) you'd want to change this code to set PARAM_SENSITIVE (added in MW 1.29), like

$params['centralauthtoken'] = [
    ApiBase::PARAM_TYPE => 'string',
    ApiBase::PARAM_SENSITIVE => true,
];

I have no idea about hadoop or other logging external to MediaWiki.

Currently, apiaction is generated by Mediawiki directly via monolog and the Kafka/Avro producer. I don't know much about how this works, but to stop logging it it needs to be removed from logging in MW.

Ottomata moved this task from Incoming to Radar on the Analytics board.Nov 1 2018, 4:20 PM

I guess this would help:

I guess this would help:

Code-Review+1

Ladsgroup added a subscriber: Reedy.Dec 3 2018, 1:32 PM

ping @Bawolff and @Reedy. Given the security deployment window is close by.

Another ping :)

@Ladsgroup - this looks fine to me. Were you pinging @Bawolff and @Reedy to deploy this during the security window (2200 UTC Mondays) or did you want to deploy? I'd guess either would most likely be fine.

@Ladsgroup - this looks fine to me. Were you pinging @Bawolff and @Reedy to deploy this during the security window (2200 UTC Mondays) or did you want to deploy? I'd guess either would most likely be fine.

Hey, either is fine for me. I can try it SWAT today (with all of the needed measures for security patches)

SWAT today sounds fine to me.

It's depoyed and the patch also got merged in gerrit as well. I wait until I can make sure it's actually redacted before closing this.

It seems it doesn't fix it. select * from wmf_raw.apiaction where year = 2018 and month = 12 and day = 19 and params['centralauthtoken'] is not null and hour is not NULL order by hour desc limit 50; still gives out centralauthtoken.

Ah, I see why. We'd also need to remove the && $flags from CentralAuthTokenSessionProvider.php line 200.

Now we have a weird situation. The main patch is in gerrit but it's not public that it's not working. Is it okay if I make a security patch and deploy it today?

@Ladsgroup - I think the Security-Team would be fine with that, thanks.

@Anomie: Hey can you take a look at this?

Anomie added a comment.EditedJan 10 2019, 2:05 PM

@Anomie: Hey can you take a look at this?

Code-Review+1 That looks like it should do it. Virtual +2 for you to do the security patch as mentioned in T207814#4835169 and approved in T207814#4838417.

The patch is deployed on wmf.13 and wmf.14

This query used to give out the actual tokens but now returns this:

select params['centralauthtoken'], hour, ts from wmf_raw.apiaction where year = 2019 and month = 1 and day = 23 and params['centralauthtoken'] is not null and hour is not NULL order by ts desc limit 50;
....
_c0	hour	ts
[redacted]	10	1548241199
[redacted]	10	1548241197
[redacted]	10	1548241195
[redacted]	10	1548241194
[redacted]	10	1548241194
[redacted]	10	1548241189
[redacted]	10	1548241185
[redacted]	10	1548241182
[redacted]	10	1548241182
[redacted]	10	1548241180
[redacted]	10	1548241180
[redacted]	10	1548241180
[redacted]	10	1548241180
[redacted]	10	1548241180
[redacted]	10	1548241178
[redacted]	10	1548241177
[redacted]	10	1548241176
[redacted]	10	1548241174
[redacted]	10	1548241173
[redacted]	10	1548241172
[redacted]	10	1548241172
[redacted]	10	1548241172
[redacted]	10	1548241160
[redacted]	10	1548241158
[redacted]	10	1548241158
[redacted]	10	1548241158
[redacted]	10	1548241157
[redacted]	10	1548241155
[redacted]	10	1548241150
[redacted]	10	1548241149
[redacted]	10	1548241146
[redacted]	10	1548241146
[redacted]	10	1548241146
[redacted]	10	1548241146
[redacted]	10	1548241145
[redacted]	10	1548241145
[redacted]	10	1548241144
[redacted]	10	1548241143
[redacted]	10	1548241119
[redacted]	10	1548241119
[redacted]	10	1548241119
[redacted]	10	1548241119
[redacted]	10	1548241117
[redacted]	10	1548241117
[redacted]	10	1548241112
[redacted]	10	1548241109
[redacted]	10	1548241104
[redacted]	10	1548241104
[redacted]	10	1548241104
[redacted]	10	1548241104
50 rows selected (46.093 seconds)
Ladsgroup closed this task as Resolved.Jan 23 2019, 11:36 AM
Ladsgroup claimed this task.

Given that CentralAuth is not part of the bundle, I just made the patch and +2'd it.

Restricted Application added a project: User-Ladsgroup. · View Herald TranscriptJan 23 2019, 11:36 AM
Ladsgroup changed the visibility from "Custom Policy" to "Public (No Login Required)".Jan 23 2019, 11:37 AM

Change 486063 merged by jenkins-bot:
[mediawiki/extensions/CentralAuth@master] Security: Remove $flags in conditions when adding centralauthtoken param

https://gerrit.wikimedia.org/r/486063