Page MenuHomePhabricator
Create Task
Maniphest T10095

provide administrators more hints to track down initial failed logins
Closed, WontfixPublic
Actions

Description

We use ldap auth (so accounts are created on demand after a successful login). When an existing user fails to login in it shows up in people/logs/. However, if a user has never logged in successfully before there is no entry in people/logs/. If one digs around on the server there is an access log entry:

[Thu, 07 Jan 2016 10:19:30 -0500]	6876	hostname.example.org	10.2.1.74	-	PhabricatorAuthLoginController	-	/auth/login/ldap%3Aself/	-	200	253854

But that doesn't show which username was used either. As an administrator I can't tell (from phabricator) if the user is using the wrong username, the wrong password, or is so confused they were trying to log into something else and just blamed phabricator. If someone was trying to login as non-existant accounts like admin,root,etc it might be useful to surface that.

Related Objects

Mentioned Here
P1916 failed_logins.diff

Event Timeline

cburroughs created this task.Jan 7 2016, 3:32 PM
chad added a project: Auth.Jan 7 2016, 4:18 PM
epriestley added a subscriber: epriestley.Jan 7 2016, 6:22 PM

This is straightforward, but I'm hesitant about it because users can use email addresses to log in, and we do not currently expose email addresses to administrators. Adding this information to the activity log could reveal lots of near-misses of email addresses. While providing this information would help administrators with users who are extremely creative in their use of computers, it seems slightly worse for the bulk of normal users from a privacy/security point of view.

Perhaps you can examine logs on the LDAP server instead?

(Vaguely related story: circa 2007, Facebook hashed user passwords mostly-adequately, but had a "bad login" table which stored usernames, etc., for failed login attempts. "etc." included the plaintext of the failing password, so you could look up a username and see a lot of strings that were not their password, like "quarts123", "quart123", "quartz12", "quartz123", "QUARTZ123", and so on. Guess what that user's password is?)

Here's a patch to show it in the Activity Logs:

P1916 failed_logins.diff
1diff --git a/src/applications/auth/provider/PhabricatorLDAPAuthProvider.php b/src/applications/auth/provider/PhabricatorLDAPAuthProvider.php
2index 013cd21..3a8aa93 100644
3--- a/src/applications/auth/provider/PhabricatorLDAPAuthProvider.php
4+++ b/src/applications/auth/provider/PhabricatorLDAPAuthProvider.php
5@@ -144,31 +144,31 @@ final class PhabricatorLDAPAuthProvider extends PhabricatorAuthProvider {
6 $has_password = strlen($password);
7 $password = new PhutilOpaqueEnvelope($password);
8
9- if (!strlen($username) || !$has_password) {
10- $response = $controller->buildProviderPageResponse(
11- $this,
12- $this->renderLoginForm($request, 'login'));
13- return array($account, $response);
14- }
15-
16 if ($request->isFormPost()) {
17 try {
18- if (strlen($username) && $has_password) {
19- $adapter = $this->getAdapter();
20- $adapter->setLoginUsername($username);
21- $adapter->setLoginPassword($password);
22-
23- // TODO: This calls ldap_bind() eventually, which dumps cleartext
24- // passwords to the error log. See note in PhutilLDAPAuthAdapter.
25- // See T3351.
26-
27- DarkConsoleErrorLogPluginAPI::enableDiscardMode();
28- $account_id = $adapter->getAccountID();
29- DarkConsoleErrorLogPluginAPI::disableDiscardMode();
30- } else {
31- throw new Exception(pht('Username and password are required!'));
32+ if (!strlen($username) || !$has_password) {
33+ throw new PhutilAuthCredentialException();
34 }
35+
36+ $adapter = $this->getAdapter();
37+ $adapter->setLoginUsername($username);
38+ $adapter->setLoginPassword($password);
39+
40+ // TODO: This calls ldap_bind() eventually, which dumps cleartext
41+ // passwords to the error log. See note in PhutilLDAPAuthAdapter.
42+ // See T3351.
43+
44+ DarkConsoleErrorLogPluginAPI::enableDiscardMode();
45+ $account_id = $adapter->getAccountID();
46+ DarkConsoleErrorLogPluginAPI::disableDiscardMode();
47 } catch (PhutilAuthCredentialException $ex) {
48+ $log = PhabricatorUserLog::initializeNewLog(
49+ null,
50+ null,
51+ PhabricatorUserLog::ACTION_LOGIN_FAILURE);
52+ $log->setFailedUsername($username);
53+ $log->save();
54+
55 $response = $controller->buildProviderPageResponse(
56 $this,
57 $this->renderLoginForm($request, 'login'));
58diff --git a/src/applications/auth/provider/PhabricatorPasswordAuthProvider.php b/src/applications/auth/provider/PhabricatorPasswordAuthProvider.php
59index 68dbf1e..72bb95c 100644
60--- a/src/applications/auth/provider/PhabricatorPasswordAuthProvider.php
61+++ b/src/applications/auth/provider/PhabricatorPasswordAuthProvider.php
62@@ -270,9 +270,9 @@ final class PhabricatorPasswordAuthProvider extends PhabricatorAuthProvider {
63 $account = null;
64 $log_user = null;
65
66+ $username_or_email = $request->getStr('username');
67 if ($request->isFormPost()) {
68 if (!$require_captcha || $captcha_valid) {
69- $username_or_email = $request->getStr('username');
70 if (strlen($username_or_email)) {
71 $user = id(new PhabricatorUser())->loadOneWhere(
72 'username = %s',
73@@ -313,6 +313,7 @@ final class PhabricatorPasswordAuthProvider extends PhabricatorAuthProvider {
74 null,
75 $log_user ? $log_user->getPHID() : null,
76 PhabricatorUserLog::ACTION_LOGIN_FAILURE);
77+ $log->setFailedUsername($username_or_email);
78 $log->save();
79 }
80
81diff --git a/src/applications/people/storage/PhabricatorUserLog.php b/src/applications/people/storage/PhabricatorUserLog.php
82index 5939778..37968bc 100644
83--- a/src/applications/people/storage/PhabricatorUserLog.php
84+++ b/src/applications/people/storage/PhabricatorUserLog.php
85@@ -122,6 +122,15 @@ final class PhabricatorUserLog extends PhabricatorUserDAO
86 time() - $timespan);
87 }
88
89+ public function setFailedUsername($username) {
90+ $this->details['fail.username'] = $username;
91+ return $this;
92+ }
93+
94+ public function getFailedUsername() {
95+ return idx($this->details, 'fail.username');
96+ }
97+
98 public function save() {
99 $this->details['host'] = php_uname('n');
100 $this->details['user_agent'] = AphrontRequest::getHTTPHeader('User-Agent');
101diff --git a/src/applications/people/view/PhabricatorUserLogView.php b/src/applications/people/view/PhabricatorUserLogView.php
102index 12bcee9..e4bc027 100644
103--- a/src/applications/people/view/PhabricatorUserLogView.php
104+++ b/src/applications/people/view/PhabricatorUserLogView.php
105@@ -55,6 +55,16 @@ final class PhabricatorUserLogView extends AphrontView {
106 $action = $log->getAction();
107 $action_name = idx($action_map, $action, $action);
108
109+ $user = null;
110+ if ($log->getUserPHID()) {
111+ $username = $handles[$log->getUserPHID()]->getName();
112+ } else if (strlen($log->getFailedUsername())) {
113+ $username = $log->getFailedUsername();
114+ } else {
115+ $username = null;
116+ }
117+
118+
119 $rows[] = array(
120 phabricator_date($log->getDateCreated(), $viewer),
121 phabricator_time($log->getDateCreated(), $viewer),
122@@ -62,7 +72,7 @@ final class PhabricatorUserLogView extends AphrontView {
123 $log->getActorPHID()
124 ? $handles[$log->getActorPHID()]->getName()
125 : null,
126- $handles[$log->getUserPHID()]->getName(),
127+ $username,
128 $ip,
129 $session,
130 );

...which does this:

badlogin.png (1×1 px, 196 KB)

...but I don't really want to upstream it because I'm not thrilled about epreitsly@phaciliity.ocm, etc., showing up in the log for all admins.

epriestley triaged this task as Wishlist priority.Jan 7 2016, 6:30 PM

I'd be fine with exposing this information from some bin/auth help me with computer since anyone with CLI access has other means available to read email addresses, but we don't currently have such a command and I don't think there are any adjacent requests to motivate building one.

I'd be slightly more comfortable putting it in the access log than the web UI, but I don't think being able to read the access log is quite the same as having CLI access (e.g., access logs may be streamed to analytics services with wider access) and it's a sort of weird fit for the access log.

While I'm sympathetic to the plight of administrators, it also seems like this is a case where it's not unreasonable to ask the user what they typed if everything else fails? Even the most stalwart champions among a userbase should usually be able to answer that correctly some of the time? I hope?

eadler added a project: Restricted Project.Jan 8 2016, 10:24 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Jan 8 2016, 10:41 PM
cburroughs added a project: Restricted Project.Apr 20 2016, 3:54 PM
Herald added a subscriber: eadler. · View Herald TranscriptApr 20 2016, 3:54 PM
cburroughs moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Apr 20 2016, 3:55 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Jul 4 2016, 9:06 PM
epriestley closed this task as Wontfix.Dec 12 2018, 8:01 PM
epriestley claimed this task.

This doesn't seem to be cropping up terribly often and I think this use case is fairly weak.

I'm not entirely opposed to adding some sort of raw per-provider auth attempt log, but I think this request isn't strong enough to justify it on its own because it's so narrow (basically just LDAP logins by users who are very resistant to begin helped).

Phabricator · Built by Phacility