Skip to content

Commit de9b824

Browse files
committed
Source: Allow cached password comparison
Move the bcrypt password comparison up to the Source and introduce a cache there. This reduces the expensive bcrypt calls by a lot, reducing the authorization delay from round about two seconds to single-digit milliseconds on my machine. On a similar note, we should consider changing the API to allow creating some token after an initial password authorization instead of passing the authentication data all the time.
1 parent 408690d commit de9b824

File tree

4 files changed

+51
-14
lines changed

4 files changed

+51
-14
lines changed

internal/config/runtime.go

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -221,14 +221,7 @@ func (r *RuntimeConfig) GetSourceFromCredentials(user, pass string, logger *logg
221221
return nil
222222
}
223223

224-
if !src.ListenerPasswordHash.Valid {
225-
logger.Debugw("Cannot check credentials for source without a listener_password_hash", zap.Int64("id", sourceId))
226-
return nil
227-
}
228-
229-
// If either PHP's PASSWORD_DEFAULT changes or Icinga Web 2 starts using something else, e.g., Argon2id, this will
230-
// return a descriptive error as the identifier does no longer match the bcrypt "$2y$".
231-
err = bcrypt.CompareHashAndPassword([]byte(src.ListenerPasswordHash.String), []byte(pass))
224+
err = src.PasswordCompare([]byte(pass))
232225
if errors.Is(err, bcrypt.ErrMismatchedHashAndPassword) {
233226
logger.Debugw("Invalid password for this source", zap.Int64("id", sourceId))
234227
return nil

internal/config/source.go

Lines changed: 46 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,12 @@
11
package config
22

33
import (
4-
"github.com/icinga/icinga-go-library/types"
4+
"crypto/subtle"
5+
"fmt"
56
"github.com/icinga/icinga-notifications/internal/config/baseconf"
67
"go.uber.org/zap/zapcore"
8+
"golang.org/x/crypto/bcrypt"
9+
"sync"
710
)
811

912
// Source entry within the ConfigSet to describe a source.
@@ -13,7 +16,9 @@ type Source struct {
1316
Type string `db:"type"`
1417
Name string `db:"name"`
1518

16-
ListenerPasswordHash types.String `db:"listener_password_hash"`
19+
ListenerPasswordHash string `db:"listener_password_hash"`
20+
listenerPassword []byte `db:"-"`
21+
listenerPasswordMutex sync.Mutex
1722
}
1823

1924
// MarshalLogObject implements the zapcore.ObjectMarshaler interface.
@@ -24,6 +29,45 @@ func (source *Source) MarshalLogObject(encoder zapcore.ObjectEncoder) error {
2429
return nil
2530
}
2631

32+
// PasswordCompare checks if a password matches this Source's password with a cache.
33+
//
34+
// This method returns nil if the password is correct, [bcrypt.ErrMismatchedHashAndPassword] in case of an invalid
35+
// password and another error if something else went wrong.
36+
//
37+
// This cache might be necessary as, for the moment, bcrypt is used to hash the passwords. By design, bcrypt is
38+
// expensive, resulting in unnecessary delays when excessively using the API.
39+
//
40+
// If either PHP's PASSWORD_DEFAULT changes or Icinga Web 2 starts using something else, e.g., Argon2id, this will
41+
// return a descriptive error as the identifier does no longer match the bcrypt "$2y$".
42+
//
43+
// If a Source changes, it will be recreated - RuntimeConfig.applyPendingSources has a nil updateFn - and the cache is
44+
// automatically purged.
45+
func (source *Source) PasswordCompare(password []byte) error {
46+
if source.ListenerPasswordHash == "" {
47+
return fmt.Errorf("source has no password hash to compare")
48+
}
49+
50+
source.listenerPasswordMutex.Lock()
51+
defer source.listenerPasswordMutex.Unlock()
52+
53+
if source.listenerPassword != nil {
54+
if subtle.ConstantTimeCompare(source.listenerPassword, password) != 1 {
55+
return bcrypt.ErrMismatchedHashAndPassword
56+
}
57+
58+
return nil
59+
}
60+
61+
err := bcrypt.CompareHashAndPassword([]byte(source.ListenerPasswordHash), password)
62+
if err != nil {
63+
return err
64+
}
65+
66+
source.listenerPassword = password
67+
68+
return nil
69+
}
70+
2771
// applyPendingSources synchronizes changed sources.
2872
func (r *RuntimeConfig) applyPendingSources() {
2973
incrementalApplyPending(

internal/incident/incidents_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,10 @@ func TestLoadOpenIncidents(t *testing.T) {
2323
db := testutils.GetTestDB(ctx, t)
2424

2525
// Insert a dummy source for our test cases!
26-
source := config.Source{
26+
source := &config.Source{
2727
Type: "notifications",
2828
Name: "Icinga Notifications",
29-
ListenerPasswordHash: types.MakeString("$2y$"), // Needed to pass the database constraint.
29+
ListenerPasswordHash: "$2y$", // Needed to pass the database constraint.
3030
}
3131
source.ChangedAt = types.UnixMilli(time.Date(2009, time.November, 10, 23, 0, 0, 0, time.UTC))
3232
source.Deleted = types.Bool{Bool: false, Valid: true}

internal/listener/listener.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -96,9 +96,9 @@ func (l *Listener) sourceFromAuthOrAbort(w http.ResponseWriter, r *http.Request)
9696
}
9797
}
9898

99-
w.Header().Set("WWW-Authenticate", `Basic realm="icinga-notifications"`)
99+
w.Header().Set("WWW-Authenticate", `Basic realm="icinga-notifications source"`)
100100
w.WriteHeader(http.StatusUnauthorized)
101-
_, _ = fmt.Fprintln(w, "please provide the debug-password as basic auth credentials (user is ignored)")
101+
_, _ = fmt.Fprintln(w, "expected valid icinga-notifications source basic auth credentials")
102102
return nil, false
103103
}
104104

0 commit comments

Comments
 (0)