Page MenuHomePhabricator

Add logging to OATHAuth
Closed, ResolvedPublicSecurity

Description

Potential things that should be logged (including information about clientip:

  • User enrolling
  • User un-enrolling
  • Wrong OTP entered
  • Scratch code used
  • Login using OTP

It would also be advantageous if we emailed a user when they enrolled and un-enrolled from 2FA, suggesting they contact the wiki administrators (or similar) if they didn't perform the action that is being reported

Event Timeline

Reedy created this task.Nov 18 2016, 12:41 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 18 2016, 12:41 AM
Tgr added a subscriber: Tgr.Nov 18 2016, 12:50 AM

Ideally, no OTP entered (see also T150903: Alert ops/security on many 2FA failures). Not sure how we would do that though.

tstarling updated the task description. (Show Details)Nov 18 2016, 12:52 AM
dpatrick triaged this task as High priority.
dpatrick added a parent task: Restricted Task.
Reedy merged a task: Restricted Task.Oct 26 2018, 1:06 PM
Reedy updated the task description. (Show Details)Oct 26 2018, 1:09 PM

Change 469878 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/OATHAuth@master] Add some logging of OATHAuth actions

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

chasemp added a project: Restricted Project.Oct 26 2018, 6:20 PM
chasemp changed the subtype of this task from "Task" to "Security Issue".Oct 26 2018, 7:13 PM
chasemp raised the priority of this task from High to Unbreak Now!.Oct 26 2018, 7:42 PM
chasemp moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.
Restricted Application added subscribers: Liuxinyu970226, TerraCodes. · View Herald TranscriptOct 26 2018, 7:42 PM
Tgr added a comment.Oct 29 2018, 8:01 PM

Some of our security-related logging adds whether the targeted user is privileged (is an admin, steward, admin at another wiki etc), other (like these) doesn't. Not much of a problem in the short term, as unprivileged users cannot currently use OATHAuth, but eventually it should be improved IMO. Either we should move the logic into User::getPrivilegedGroups (with some way for auth extensions like CentralAuth to hook into it), or the isPrivileged and privilegedGroups fields should be added in a custom log processor added in our config, so they are present in all log events.

chasemp lowered the priority of this task from Unbreak Now! to High.Oct 31 2018, 7:49 PM
Reedy added a comment.Nov 1 2018, 10:23 PM

Any chance someone can review my patch please?

I know a few bits of it are a bit clunky

Tgr added a comment.Nov 15 2018, 8:01 PM

Any objections to making this task public? There is nothing sensitive about logging, and private patches tend to linger forever without getting review / getting deployed.

This task already is public.

I

Reedy added a comment.Nov 15 2018, 8:04 PM

This task already is public.

And the patch is in gerrit

Tgr added a comment.Nov 15 2018, 8:10 PM

Eh, sorry. This new task type got me all confused.

Change 469878 merged by jenkins-bot:
[mediawiki/extensions/OATHAuth@master] Add some logging of OATHAuth actions

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

Tgr closed this task as Resolved.Nov 16 2018, 6:54 PM
Tgr moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.
Reedy reopened this task as Open.Nov 29 2018, 6:16 AM

Reverted due to some monolog (or our additional functions) issues

Where does that is supposed to log to? Logstash? On-wiki log that is accessible to functionaries (if we want to do T210919 and T209749)? If so, then the messages shall be localizable and not be hardcoded in code.

Reedy added a comment.Dec 16 2018, 4:01 PM

This is for server side logging

Tgr added a comment.Dec 17 2018, 11:54 PM

I checked out the patch to mwdebug1002 and had no problem adding, removing or using a 2FA key on Wikitech. Also the error seems not at all related to the patch. I'm not sure what to do here, short of re-deploying and live-debugging if the problems reoccur.

I checked out the patch to mwdebug1002 and had no problem adding, removing or using a 2FA key on Wikitech. Also the error seems not at all related to the patch. I'm not sure what to do here, short of re-deploying and live-debugging if the problems reoccur.

Seems sane to me. Could be something specific to the host(s) running wikitech

Change 480274 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/OATHAuth@master] Re-instate "Add some logging of OATHAuth actions"

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

Tgr added a comment.Dec 17 2018, 11:57 PM

OTOH it does not seem it logged anything, either. Maybe I messed up. I'll retry the test later today.

Tgr added a comment.Dec 18 2018, 2:56 AM

So it does not log anything because X-Wikimedia-Debug does not work on wikitech. Duhh. On checkuserwiki when I try to login with the patch checked out on the debug host I get Call to a member function info() on a non-object (null) for the $this->logger->info() call. WTF?

Tgr added a comment.Dec 18 2018, 3:50 AM

In CLI mode the config seems OK:

tgr@mwdebug1002:~$ PHP=php7.0 mwscript shell.php --wiki=checkuserwiki
>>> $k = OATHAuthKey::newFromRandom()                                                                                                                                                                       => OATHAuthKey {#2452}
>>> sudo $k->logger->name
=> "authentication"
>>> sudo $k->logger->handlers
=> [
     Monolog\Handler\WhatFailureGroupHandler {#168},
   ]
>>> sudo $k->logger->handlers[0]->handlers
=> [
     MediaWiki\Logger\Monolog\LegacyHandler {#169},
     MediaWiki\Logger\Monolog\SyslogHandler {#170},
   ]
>>> sudo $k->logger->info( 'test', [ 'test-foo' => 'bar' ] )
=> true

...and that last test does show up in Logstash.

The null error is deterministic. (actually it's not, now I get the Argument must be an instance of array, null given error instead) When I replace $this with \MediaWiki\Logger\LoggerFactory::getInstance( 'authentication' ), (which is the value it should be set to in the constructor) things work (and logs show up in Logstash).

Tgr added a comment.Dec 18 2018, 3:55 AM

See T210696#4830215.

In hindsight that should have been obvious: if a property is null even though absolutely no code path leaves it null, chances are it disappears during serialization. I got too invested into an alternative theory (that there is some non-fatal error PHP recovers from by exiting the method without returning anything, and since this happens during logger creation logging gets messed up and the error is not logged) to think of that.

Change 480274 merged by jenkins-bot:
[mediawiki/extensions/OATHAuth@master] Re-instate "Add some logging of OATHAuth actions"

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

Reedy closed this task as Resolved.Dec 18 2018, 6:38 PM
Reedy removed a project: Patch-For-Review.