Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions doc/20-HTTP-API.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,12 @@ After creating a source in Icinga Notifications Web,
the specified credentials can be used via HTTP Basic Authentication to submit a JSON-encoded
[`Event`](https://github.com/Icinga/icinga-go-library/blob/main/notifications/event/event.go).

The authentication is performed via HTTP Basic Authentication, expecting `source-${id}` as the username,
`${id}` being the source's `id` within the database, and the configured password.
The authentication is performed via HTTP Basic Authentication using the source's username and password.

!!! info

Before Icinga Notifications version 0.2.0, the username was a fixed string based on the source ID, such as `source-${id}`.
When upgrading a setup from an earlier version, these usernames are still valid, but can be changed in Icinga Notifications Web.

Events sent to Icinga Notifications are expected to match rules that describe further event escalations.
These rules can be created in the web interface.
Expand Down
32 changes: 15 additions & 17 deletions internal/config/runtime.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package config

import (
"context"
"crypto/subtle"
"database/sql"
"errors"
"github.com/icinga/icinga-go-library/database"
Expand All @@ -13,7 +14,6 @@ import (
"github.com/icinga/icinga-notifications/internal/timeperiod"
"go.uber.org/zap"
"golang.org/x/crypto/bcrypt"
"strconv"
"strings"
"sync"
"time"
Expand Down Expand Up @@ -205,29 +205,27 @@ func (r *RuntimeConfig) GetSourceFromCredentials(user, pass string, logger *logg
r.RLock()
defer r.RUnlock()

sourceIdRaw, sourceIdOk := strings.CutPrefix(user, "source-")
if !sourceIdOk {
logger.Debugw("Cannot extract source ID from HTTP basic auth username", zap.String("user_input", user))
return nil
}
sourceId, err := strconv.ParseInt(sourceIdRaw, 10, 64)
if err != nil {
logger.Debugw("Cannot convert extracted source Id to int", zap.String("user_input", user), zap.Error(err))
return nil
var src *Source
for _, tmpSrc := range r.Sources {
if !tmpSrc.ListenerUsername.Valid {
continue
}
if subtle.ConstantTimeCompare([]byte(tmpSrc.ListenerUsername.String), []byte(user)) == 1 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why using constant time comparison for this? Shouldn't a simple == check be enough for this? Also, previously the sources were mapped by their IDs, and was easy to look up based on that. However, now that you don't have to lookup by ID anymore, and the username has a unique database constraint, why not use a map[string]*Source for Sources instead? That would make this whole loop and comparison unnecessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use a map[string]*Source for Sources instead?

Ah maybe because the exsiting incremental config updates machinery operates on the primary key. But is the source ID relevant for anything now? Can't we just drop the ID and make the username its primary key instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why using constant time comparison for this?

Because an external adversary could - in theory - enumerate available usernames. It's the same thing as for passwords, as just this pair gains access. Take the good aged CVE-2004-1602 as an example.

Furthermore, what's your problem with this approach? It's not wrong.

Ah maybe because the exsiting incremental config updates machinery operates on the primary key. But is the source ID relevant for anything now? Can't we just drop the ID and make the username its primary key instead?

That's the reason why. I guess changing the primary key's type would be a bigger change, including on the web part.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Furthermore, what's your problem with this approach? It's not wrong.

My problem? Well, as far as I know, you use subtle.ConstantTimeCompare only when you compare secrets like passwords or tokens and not usernames because usernames aren't secrets, otherwise we would store a hashed version of it in the database just like for passwords. I don't know enough context to understand the linked CVE but as for Notifications should bring nothing but confusion.

I guess changing the primary key's type would be a bigger change, including on the web part.

Is it that big? I don't even think web has to change anything other than dropping the id and adding the new columns to their models.

src = tmpSrc
break
}
}

src, ok := r.Sources[sourceId]
if !ok {
logger.Debugw("Cannot check credentials for unknown source ID", zap.Int64("id", sourceId))
if src == nil {
logger.Debugw("Cannot find source for username", zap.String("user", user))
return nil
}

err = src.PasswordCompare([]byte(pass))
err := src.PasswordCompare([]byte(pass))
if errors.Is(err, bcrypt.ErrMismatchedHashAndPassword) {
logger.Debugw("Invalid password for this source", zap.Int64("id", sourceId))
logger.Debugw("Invalid password for source", zap.Int64("id", src.ID))
return nil
} else if err != nil {
logger.Errorw("Failed to verify password for this source", zap.Int64("id", sourceId), zap.Error(err))
logger.Errorw("Failed to verify password for source", zap.Int64("id", src.ID), zap.Error(err))
return nil
}

Expand Down
1 change: 1 addition & 0 deletions internal/config/source.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ type Source struct {
Type string `db:"type"`
Name string `db:"name"`

ListenerUsername types.String `db:"listener_username"`
ListenerPasswordHash types.String `db:"listener_password_hash"`
listenerPassword []byte `db:"-"`
listenerPasswordMutex sync.Mutex
Expand Down
3 changes: 2 additions & 1 deletion internal/object/objects_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,14 @@ func TestRestoreMutedObjects(t *testing.T) {
"type": "notifications",
"name": "Icinga Notifications",
"changed_at": int64(1720702049000),
"user": "jane.doe",
"pwd_hash": "$2y$", // Needed to pass the database constraint.
}
// We can't use config.Source here unfortunately due to cyclic import error!
id, err := database.InsertObtainID(
ctx,
tx,
`INSERT INTO source (type, name, changed_at, listener_password_hash) VALUES (:type, :name, :changed_at, :pwd_hash)`,
`INSERT INTO source (type, name, changed_at, listener_username, listener_password_hash) VALUES (:type, :name, :changed_at, :user, :pwd_hash)`,
args)
require.NoError(t, err, "populating source table should not fail")

Expand Down
6 changes: 4 additions & 2 deletions schema/mysql/schema.sql
Original file line number Diff line number Diff line change
Expand Up @@ -212,13 +212,15 @@ CREATE TABLE source (
-- will likely need a distinguishing value for multiple sources of the same type in the future, like for example
-- the Icinga DB environment ID for Icinga 2 sources

-- This column is required to limit API access for incoming connections to the Listener.
-- The username will be "source-${id}", allowing early verification.
-- listener_{username,password_hash} are required to limit API access for incoming connections to the Listener.
listener_username varchar(255),
Comment on lines +215 to +216
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While testing Icinga/icinga-notifications-web#390 with the branch https://github.com/Icinga/icinga-notifications/ pull/324, I noticed that icingadb was unable to connect to the notifications API because I had accidentally omitted to define a “listener_password” when creating a resource.

However, since the column is nullable, no error is displayed if a resource is defined without a password.

As these fields are required, they should not be nullable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both columns are nullable to ease the database schema migration. But, as @yhabteab also suggested above, maybe the primary key can be changed from an ID to this username, being UNIQUE and everything.

listener_password_hash text,

changed_at bigint NOT NULL,
deleted enum('n', 'y') NOT NULL DEFAULT 'n',

CONSTRAINT uk_source_listener_username UNIQUE(listener_username),

-- The hash is a PHP password_hash with PASSWORD_DEFAULT algorithm, defaulting to bcrypt. This check roughly ensures
-- that listener_password_hash can only be populated with bcrypt hashes.
-- https://icinga.com/docs/icinga-web/latest/doc/20-Advanced-Topics/#manual-user-creation-for-database-authentication-backend
Expand Down
3 changes: 3 additions & 0 deletions schema/mysql/upgrades/0.2.0-source-username.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
ALTER TABLE source ADD COLUMN listener_username varchar(255) AFTER name;
UPDATE source SET listener_username = CONCAT('source-', source.id);
ALTER TABLE source ADD CONSTRAINT uk_source_listener_username UNIQUE(listener_username);
6 changes: 4 additions & 2 deletions schema/pgsql/schema.sql
Original file line number Diff line number Diff line change
Expand Up @@ -244,13 +244,15 @@ CREATE TABLE source (
-- will likely need a distinguishing value for multiple sources of the same type in the future, like for example
-- the Icinga DB environment ID for Icinga 2 sources

-- This column is required to limit API access for incoming connections to the Listener.
-- The username will be "source-${id}", allowing early verification.
-- listener_{username,password_hash} are required to limit API access for incoming connections to the Listener.
listener_username varchar(255),
listener_password_hash text,

changed_at bigint NOT NULL,
deleted boolenum NOT NULL DEFAULT 'n',

CONSTRAINT uk_source_listener_username UNIQUE(listener_username),

-- The hash is a PHP password_hash with PASSWORD_DEFAULT algorithm, defaulting to bcrypt. This check roughly ensures
-- that listener_password_hash can only be populated with bcrypt hashes.
-- https://icinga.com/docs/icinga-web/latest/doc/20-Advanced-Topics/#manual-user-creation-for-database-authentication-backend
Expand Down
3 changes: 3 additions & 0 deletions schema/pgsql/upgrades/0.2.0-source-username.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
ALTER TABLE source ADD COLUMN listener_username varchar(255);
UPDATE source SET listener_username = CONCAT('source-', source.id);
ALTER TABLE source ADD CONSTRAINT uk_source_listener_username UNIQUE(listener_username);
Loading