Skip to content

Source: Introduce Usernames #338

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
julianbrost merged 1 commit into from
Nov 12, 2025
Merged

Source: Introduce Usernames #338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
View file
Open in desktop
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
28 changes: 11 additions & 17 deletions internal/config/runtime.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,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 +204,24 @@ 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 && tmpSrc.ListenerUsername.String == user {
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
View file
Open in desktop
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
5 changes: 3 additions & 2 deletions internal/incident/incidents_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,9 @@ func TestLoadOpenIncidents(t *testing.T) {

// Insert a dummy source for our test cases!
source := &config.Source{
Type: "notifications",
Name: "Icinga Notifications",
Type: "notifications",
Name: "Icinga Notifications",
ListenerUsername: types.MakeString("notifications"),
}
source.ChangedAt = types.UnixMilli(time.Date(2009, time.November, 10, 23, 0, 0, 0, time.UTC))
source.Deleted = types.Bool{Bool: false, Valid: true}
Expand Down
3 changes: 2 additions & 1 deletion internal/object/objects_test.go
View file
Open in desktop
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
7 changes: 5 additions & 2 deletions schema/mysql/schema.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,13 +212,16 @@ 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

@sukhwinder33445 sukhwinder33445 Nov 7, 2025

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

@oxzi oxzi Nov 7, 2025

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.

Copy link
Collaborator

@julianbrost julianbrost Nov 10, 2025

Choose a reason for hiding this comment

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

maybe the primary key can be changed from an ID to this username

I mean technically possible, but that would then also have to be used for foreign keys (obviously) and make it pretty much impossible to change the username for a source. Also keep in mind that it's currently included in the object IDs as objects are handled as being source-specific:

icinga-notifications/internal/object/object.go

Lines 197 to 199 in cea444e

sourceBytes := make([]byte, 8)
binary.BigEndian.PutUint64(sourceBytes, uint64(source))
h.Write(sourceBytes)

Copy link
Member Author

@oxzi oxzi Nov 11, 2025

Choose a reason for hiding this comment

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

Due to the size of the changes, I am also no fan of changing the primary key.

However, back to the original comment:

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.

Without a password, Icinga Notifications would reject the connection. That's intended.

But, as I commented above, this would allow schema migrations and keep elements of old and now removed sources - such as Icinga 2.

Copy link
Collaborator

@julianbrost julianbrost Nov 11, 2025

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.

Why both though? I was a bit too focused on listener_password_hash, as that was the culprit in the the original comment. For that one, I see that the nullable for the migration applies: we can't really set a meaningful password during the migration, so the plan for the migration is:

  1. Apply the schema upgrade.
  2. Manually assign passwords to icinga2 sources in web.
  3. Configure these source credentials in Icinga DB.

Note that web should probably enforce that listener_password_hash is set. Otherwise that'll create a source that can't authenticate itself. A corresponding not null constraint can then be added in a migration for a later version (otherwise, the migration for this version would need a second schema migration file that needs to be applied later).

However, for listener_username, the migration file sets the value to the previously used source-${id} string, so I don't see a reason why that shouldn't get a not null constraint.

Copy link
Member Author

@oxzi oxzi Nov 12, 2025

Choose a reason for hiding this comment

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

Based on @nilmerg's comment and your reply, I have added a CHECK (deleted = 'y' OR listener_username IS NOT NULL) to allow listener_username to be NULL for deleted rows from web. Otherwise, the UNIQUE constraint ensures no username appearing twice.

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),
CONSTRAINT ck_source_listener_username_or_deleted CHECK (deleted = 'y' OR listener_username IS NOT NULL),

-- 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
5 changes: 5 additions & 0 deletions schema/mysql/upgrades/0.2.0-source-username.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
ALTER TABLE source ADD COLUMN listener_username varchar(255) AFTER name;
UPDATE source SET listener_username = CONCAT('source-', source.id) WHERE deleted = 'n';
ALTER TABLE source
ADD CONSTRAINT uk_source_listener_username UNIQUE (listener_username),
ADD CONSTRAINT ck_source_listener_username_or_deleted CHECK (deleted = 'y' OR listener_username IS NOT NULL);
7 changes: 5 additions & 2 deletions schema/pgsql/schema.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,13 +244,16 @@ 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),
CONSTRAINT ck_source_listener_username_or_deleted CHECK (deleted = 'y' OR listener_username IS NOT NULL),

-- 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
5 changes: 5 additions & 0 deletions schema/pgsql/upgrades/0.2.0-source-username.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
ALTER TABLE source ADD COLUMN listener_username varchar(255);
UPDATE source SET listener_username = CONCAT('source-', source.id) WHERE deleted = 'n';
ALTER TABLE source
ADD CONSTRAINT uk_source_listener_username UNIQUE (listener_username),
ADD CONSTRAINT ck_source_listener_username_or_deleted CHECK (deleted = 'y' OR listener_username IS NOT NULL);
Loading