Source: Introduce Usernames #338
Conversation
oxzi
force-pushed
the
source-listener-username
branch
from
9e07bb4 to
85e43f8
Compare
oxzi
force-pushed
the
icingadb-source
branch
5 times, most recently
from
e2a1836 to
f3870a3
Compare
oxzi
force-pushed
the
source-listener-username
branch
from
85e43f8 to
6c9901a
Compare
| if !tmpSrc.ListenerUsername.Valid { | ||
| continue | ||
| } | ||
| if subtle.ConstantTimeCompare([]byte(tmpSrc.ListenerUsername.String), []byte(user)) == 1 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
why not use a
map[string]*Sourcefor 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Unique usernames were added to Sources for HTTP Authentication. Before, the HTTP Authentication expected a username based on the Source ID, such as "source-23". This was not very practical. Thus, an unique username column was introduced and the Listener's authentication code was adequately altered. Fixes #227.
oxzi
force-pushed
the
source-listener-username
branch
from
6c9901a to
3ac0733
Compare
| -- listener_{username,password_hash} are required to limit API access for incoming connections to the Listener. | ||
| listener_username varchar(255), |
There was a problem hiding this 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.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Unique usernames were added to Sources for HTTP Authentication.
Before, the HTTP Authentication expected a username based on the Source ID, such as "source-23". This was not very practical. Thus, an unique username column was introduced and the Listener's authentication code was adequately altered.
Fixes #227.