Log failed login attempt

[i-amelia]

Signed-off-by: i-amelia viktoria.lyomcheva@gmail.com

Overview

What this PR does / why we need it

This PR adds logging of failed login attempts when using PasswordConnector. It will help to detect & alert brute force password attacks.

Special notes for your reviewer

Does this PR introduce a user-facing change?

NONE

[sagikazarmark]

Thanks for the contribution @i-amelia !

I have to say, I'm not a huge fan of this though. For one, a failed login attempt caused by invalid credentials is certainly not an error from the application's perspective.

Secondly, by logging failed attempts by default you open yourself up to a potential spike in the amount of logs during an attack. So enabling logging failed attempts should definitely be opt-in.

I'd say it could easily be fixed by logging these events as debug events. Then you can switch to a debug level when you need this logs and normally they wouldn't show up.

I'm open to other alternatives that consider the above issues I mentioned.

[lsangelov]

Hi sagikazarmark,

I and i-amelia are from the same team.
May be a little background will help you to understand why we need this change.
In our team we use Concourse which uses DEX for authentication.
We have one particular scenario where a local user is used and we have requirement to monitor login attempts to identify brute force attacks. It was identified that this is a weak spot in the security monitoring of our setup.

About the error message I agree that failed login attempts are not application errors. We can agree to change it to Info.
What is the your concern about more logs created as we will use these logs as a signal for brute force attack?
I would say that using debug is not an option as we won't be aware when the brute force will occur. Also other concern is that the debug log level will increase logging as a whole and logs will be flooded with excess logs which are not needed.

The best solution would be to have an automatic user locking mechanism in place but unfortunately that is not the case.

[sagikazarmark]

Hi @lsangelov !

Yeah, I guessed this might be the use case you need this feature for.

What is the your concern about more logs created as we will use these logs as a signal for brute force attack?

You answered my question in your next sentence: flooding the logs with virtually no upper bound.

The best solution would be to have an automatic user locking mechanism in place but unfortunately that is not the case.

Personally, I think IP based banning is a slightly better solution compared to account locking. That you should be able to do based on access logs.

While I understand your use case, I'm not comfortable with making this enabled by default. Unfortunately, I don't have a good solution in mind at the moment (eg. it should be some sort of config option), so I'd suggest hiding it behind a flag (eg. env var: DEX_LOG_FAILED_LOGIN_ATTEMPTS=true). We can figure out a better solution later.

Would that work for you?

[nabokihms]

From my point of view, it is better to use metrics for this kind of tracking. How many requests have been made to Dex? How many of them were errors? It is all about metrics.

Unfortunately, the number of collected metrics is minimal. The following list is an example of what you can get.

http_requests_total{code="200",handler="/approval",method="GET"} 3
http_requests_total{code="200",handler="/auth",method="GET"} 4
http_requests_total{code="200",handler="/auth/{connector}/login",method="GET"} 2
http_requests_total{code="200",handler="/auth/{connector}/login",method="POST"} 10
http_requests_total{code="200",handler="/token",method="POST"} 5
http_requests_total{code="302",handler="/auth/{connector}",method="GET"} 4
http_requests_total{code="303",handler="/approval",method="POST"} 3
http_requests_total{code="303",handler="/auth/{connector}/login",method="POST"} 1
http_requests_total{code="500",handler="/approval",method="GET"} 1

These are login requests. I make ten, nine with an invalid password.

http_requests_total{code="200",handler="/auth/{connector}/login",method="POST"} 10

Thus, as we can see, Dex doesn't differentiate between successful and invalid login attempts.

It is easy to fix, though. The following patch makes Dex return the 401 code if the login attempt was unsuccessful.

diff --git a/server/templates.go b/server/templates.go
index 9be8019a..21ec3191 100644
--- a/server/templates.go
+++ b/server/templates.go
@@ -292,6 +292,9 @@ func (t *templates) password(r *http.Request, w http.ResponseWriter, postURL, la
 		Invalid        bool
 		ReqPath        string
 	}{postURL, backLink, lastUsername, usernamePrompt, lastWasInvalid, r.URL.Path}
+	if lastWasInvalid {
+		w.WriteHeader(http.StatusUnauthorized)
+	}
 	return renderTemplate(w, t.passwordTmpl, data)
 }
 

http_requests_total{code="303",handler="/auth/{connector}/login",method="POST"} 1
http_requests_total{code="401",handler="/auth/{connector}/login",method="POST"} 9

Now you can see one successful attempt and nine unsuccessful. Thus you can track the rate of unsuccessful attempts.
What do you folks think about it? Does it solve your issue?

[sagikazarmark]

You cannot lock/ban specific users based on metrics. I guess they'd like to use a fail2ban-like solution which is fine and I see why Dex should support that, but we should do that in a way that doesn't open users up to other issues.

I'm thinking about introducing "log channels" where the user could configure which categories of events they would like to see in the logs. That way we could have an access-like log where we log each login attempt and its result.

All logs would still go to stdout, but each log would have an extra channel attribute. This would require #2020 though.

I'm looking for existing examples out there to validate this idea. If you have any in mind, please share.

[lsangelov]

At the moment we only have as a requirement the logging of "Failed Login Attempts". These logs will be used for audit logging in case some sort of attack is committed to our system. We will use these logs as a starting point for investigation. In this manner metrics will help us just to know that something is happening but without any further information like which user is a subject of the attack.

Fail2ban-like solution is nice to have at the moment as I agree that IP based banning is the better solution than account locking. At the moment our goal is just to who, when and what is doing. Banning will come in the next phase.

By the way do you have a way to obtain the IP of the caller to login function?

At the moment the only solution for us will be "Failed Login Attempts" to be part of the standard logging so we can monitor stdout for any suspicious activities. I think that the option with the flag DEX_LOG_FAILED_LOGIN_ATTEMPTS=true should work the opposite way. By default "Failed Login Attempts" should be part of the stdout as they might reveal a possible attack. In the other way round DEX users will be blind for such attacks.

Is there any chance to have logs for "Failed Login Attempts" in the standard logs?
Are there any specific conditions that has to be met?

Thanks for your cooperation.

[sagikazarmark]

At the moment the only solution for us will be "Failed Login Attempts" to be part of the standard logging so we can monitor stdout for any suspicious activities. I think that the option with the flag DEX_LOG_FAILED_LOGIN_ATTEMPTS=true should work the opposite way.

Not sure what you mean by "the opposite way". By turning this flag on, failed login attempts would show up in the logs.

In the other way round DEX users will be blind for such attacks.

They might have other means to detect attacks. I'm not saying this feature shouldn't be available, I'm saying it should be opt-in instead of opt-out.

Is there any chance to have logs for "Failed Login Attempts" in the standard logs?

As I said, for now we'd accept this change hidden behind a flag. In the long term, I think the solution I described as channels would be a better solution.

[nabokihms]

Related issue #1813

[nabokihms]

Actually, I realized that many security standards consider a failed login attempt a security event, so it should be collected and processed to a security events journal (an SIEM system?)

Anyway, metrics are cool for monitoring, but not suitable for this case. I'm going to merge this PR and hope that Dex logs will not be flooded 🙂

[nabokihms]

@i-amelia thank you for the PR.