Commit 283441b1 authored by ale's avatar ale

Apply blacklists even on unknown users

IP-based blacklists should run anyway in that case. The change
simplifies the doAuthenticate() flow, and the ratelimit API.

Add a test for this case too.
parent 40fbc768
Pipeline #5909 passed with stages
in 1 minute and 33 seconds
......@@ -51,23 +51,23 @@ type service struct {
aspService string
}
func (s *service) checkRateLimits(user *backend.User, req *auth.Request) (bool, string) {
func (s *service) checkRateLimits(username string, req *auth.Request) (bool, string) {
for _, rl := range s.rl {
if !rl.AllowIncr(user, req) {
if !rl.AllowIncr(username, req) {
return false, rl.rl.name
}
}
for _, bl := range s.bl {
if !bl.Allow(user, req) {
if !bl.Allow(username, req) {
return false, bl.bl.name
}
}
return true, ""
}
func (s *service) notifyBlacklists(user *backend.User, req *auth.Request, resp *auth.Response) {
func (s *service) notifyBlacklists(username string, req *auth.Request, resp *auth.Response) {
for _, bl := range s.bl {
bl.Incr(user, req, resp)
bl.Incr(username, req, resp)
}
}
......@@ -303,6 +303,8 @@ func (s *Server) Authenticate(ctx context.Context, req *auth.Request) *auth.Resp
resp, err := s.doAuthenticate(ctx, req)
if err != nil {
errmsg = fmt.Sprintf(" error=%v", err)
}
if resp == nil {
resp = newError()
}
......@@ -326,10 +328,11 @@ var (
errUserUnknown = errors.New("unknown user")
)
// Function with the actual authentication API logic - we return either a
// successful Response, or an error. The Authenticate() wrapper turns the
// latter into a Response with Status: Error, but this function stays
// readable.
// Function with the actual authentication API logic - we return both
// an auth.Response and an error. The outer Authenticate() wrapper
// turns the latter into a Response with Status: Error anyway, but
// adds explicit logging of the error message (useful for internal
// server errors etc).
func (s *Server) doAuthenticate(ctx context.Context, req *auth.Request) (*auth.Response, error) {
start := time.Now()
......@@ -338,30 +341,39 @@ func (s *Server) doAuthenticate(ctx context.Context, req *auth.Request) (*auth.R
return nil, errServiceUnknown
}
user, ok := s.getUser(ctx, svc, req.Username)
if !ok {
// User is unknown to all backends.
return nil, errUserUnknown
}
// Apply rate limiting and blacklisting _before_ invoking the
// authentication handlers, as they may be CPU intensive.
if allowed, blockedBy := svc.checkRateLimits(user, req); !allowed {
if allowed, blockedBy := svc.checkRateLimits(req.Username, req); !allowed {
ratelimitCounter.With(prometheus.Labels{
"service": req.Service,
}).Inc()
return nil, fmt.Errorf("ratelimited (%s)", blockedBy)
}
resp, err := s.authenticateUser(req, svc, user)
// When a user is unknown, we still want to invoke the
// blacklists, but without a username: this will still apply
// IP-based blacklists, but will prevent memory explosion for
// dictionary scans of the username space.
var blUsername string
var resp *auth.Response
err := errUserUnknown
user, ok := s.getUser(ctx, svc, req.Username)
if !ok {
// User is unknown to all backends.
goto fail
}
blUsername = user.Name
resp, err = s.authenticateUser(req, svc, user)
// Increment stats counters.
authRequestLatency.With(prometheus.Labels{
"service": req.Service,
}).Observe(time.Since(start).Seconds() * 1000)
fail:
// Notify blacklists of the result.
svc.notifyBlacklists(user, req, resp)
svc.notifyBlacklists(blUsername, req, resp)
return resp, err
}
......
......@@ -2,6 +2,7 @@ package server
import (
"context"
"fmt"
"io/ioutil"
"os"
"path/filepath"
......@@ -104,8 +105,8 @@ services:
params:
src: users.yml
rate_limits:
- failed_login_bl
- failed_ip_bl
- failed_login_bl
rate_limits:
failed_login_bl:
limit: 10
......@@ -113,10 +114,9 @@ rate_limits:
blacklist_for: 3600
on_failure: true
keys: [user]
# Meant not to trigger:
failed_ip_bl:
limit: 10000
period: 3600
limit: 10
period: 300
blacklist_for: 3600
on_failure: true
keys: [ip]
......@@ -221,6 +221,32 @@ func TestAuthServer_Blacklist(t *testing.T) {
}
}
func TestAuthServer_Blacklist_UnknownUser(t *testing.T) {
s := createTestServer(t, map[string]string{
"users.yml": testUsersFileStr,
"config.yml": testConfigStrWithRatelimit,
})
defer s.Close()
c := &clientAdapter{s.srv}
// Trigger the failed IP blacklist.
for i := 0; i < 100; i++ {
c.Authenticate(context.Background(), &auth.Request{
Service: "test",
Username: fmt.Sprintf("nonexistinguser%d", i),
Password: []byte("bad_password"),
DeviceInfo: &auth.DeviceInfo{
RemoteAddr: "1.2.3.4",
},
})
}
// Reach through the internals to verify that the IP is in
// the blacklist.
if _, ok := s.srv.services["test"].bl[0].bl.bl["1.2.3.4"]; !ok {
t.Fatalf("IP was not blacklisted")
}
}
func TestAuthServer_Blacklist_BelowLimit(t *testing.T) {
s := createTestServer(t, map[string]string{
"users.yml": testUsersFileStr,
......
......@@ -8,7 +8,6 @@ import (
"time"
"git.autistici.org/id/auth"
"git.autistici.org/id/auth/backend"
"github.com/prometheus/client_golang/prometheus"
)
......@@ -155,15 +154,15 @@ func (b *Blacklist) Incr(key string) {
}
// Function that extracts a key from a request.
type ratelimitKeyFunc func(*backend.User, *auth.Request) string
type ratelimitKeyFunc func(string, *auth.Request) string
// Extract the username from the request.
func usernameKey(user *backend.User, _ *auth.Request) string {
return user.Name
func usernameKey(username string, _ *auth.Request) string {
return username
}
// Extract the client IP address (if present) from the request.
func ipAddrKey(_ *backend.User, req *auth.Request) string {
func ipAddrKey(_ string, req *auth.Request) string {
if req.DeviceInfo != nil {
return req.DeviceInfo.RemoteAddr
}
......@@ -249,23 +248,23 @@ func newAuthRatelimiterBase(config *authRatelimiterConfig) (*authRatelimiterBase
}, nil
}
func (r *authRatelimiterBase) shouldBypass(user *backend.User, req *auth.Request) bool {
func (r *authRatelimiterBase) shouldBypass(username string, req *auth.Request) bool {
for _, b := range r.bypass {
if b.value == b.keyFn(user, req) {
if b.value == b.keyFn(username, req) {
return true
}
}
return false
}
func (r *authRatelimiterBase) key(user *backend.User, req *auth.Request) string {
func (r *authRatelimiterBase) key(username string, req *auth.Request) string {
if len(r.keyFuncs) == 1 {
return r.keyFuncs[0](user, req)
return r.keyFuncs[0](username, req)
}
var parts []string
for _, f := range r.keyFuncs {
parts = append(parts, f(user, req))
parts = append(parts, f(username, req))
}
return strings.Join(parts, rlKeySep)
}
......@@ -287,11 +286,11 @@ func newAuthRatelimiter(name string, config *authRatelimiterConfig) (*authRateli
}, nil
}
func (r *authRatelimiter) AllowIncr(user *backend.User, req *auth.Request) bool {
if r.shouldBypass(user, req) {
func (r *authRatelimiter) AllowIncr(username string, req *auth.Request) bool {
if r.shouldBypass(username, req) {
return true
}
key := r.key(user, req)
key := r.key(username, req)
if key == "" { // An empty key bypasses the rate limit.
return true
}
......@@ -317,22 +316,22 @@ func newAuthBlacklist(name string, config *authRatelimiterConfig) (*authBlacklis
}, nil
}
func (b *authBlacklist) Allow(user *backend.User, req *auth.Request) bool {
if b.shouldBypass(user, req) {
func (b *authBlacklist) Allow(username string, req *auth.Request) bool {
if b.shouldBypass(username, req) {
return true
}
key := b.key(user, req)
key := b.key(username, req)
if key == "" { // An empty key bypasses the blacklist.
return true
}
return b.bl.Allow(key)
}
func (b *authBlacklist) Incr(user *backend.User, req *auth.Request, resp *auth.Response) {
func (b *authBlacklist) Incr(username string, req *auth.Request, resp *auth.Response) {
if b.onFailure && resp != nil && resp.Status == auth.StatusOK {
return
}
b.bl.Incr(b.key(user, req))
b.bl.Incr(b.key(username, req))
}
var blacklistCounter = prometheus.NewCounterVec(prometheus.CounterOpts{
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment