-
Notifications
You must be signed in to change notification settings - Fork 1
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 However, since the column is As these fields are
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
|
||
| 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); |
| 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); |
There was a problem hiding this comment.
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 amap[string]*Sourcefor Sources instead? That would make this whole loop and comparison unnecessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
That's the reason why. I guess changing the primary key's type would be a bigger change, including on the web part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My problem? Well, as far as I know, you use
subtle.ConstantTimeCompareonly 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.Is it that big? I don't even think web has to change anything other than dropping the
idand adding the new columns to their models.