-
Notifications
You must be signed in to change notification settings - Fork 1k
Add Security Notifications #2312
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?
Conversation
Signed-off-by: Matthew H. Irby <[email protected]>
Signed-off-by: Matthew H. Irby <[email protected]>
Signed-off-by: Matthew H. Irby <[email protected]>
Signed-off-by: Matthew H. Irby <[email protected]>
Signed-off-by: Matthew Irby <[email protected]>
Signed-off-by: Matthew Irby <[email protected]>
Signed-off-by: Matthew Irby <[email protected]>
Signed-off-by: Matthew Irby <[email protected]>
Signed-off-by: Matthew Irby <[email protected]>
Signed-off-by: Matthew Irby <[email protected]>
Signed-off-by: Matthew Irby <[email protected]>
Signed-off-by: Matthew H. Irby <[email protected]>
Signed-off-by: Matthew H. Irby <[email protected]>
Signed-off-by: Matthew H. Irby <[email protected]>
Signed-off-by: Matthew H. Irby <[email protected]>
Signed-off-by: Matthew H. Irby <[email protected]>
Signed-off-by: Matthew H. Irby <[email protected]>
…ions Signed-off-by: Matthew H. Irby <[email protected]>
Signed-off-by: Matthew H. Irby <[email protected]>
Signed-off-by: Matthew H. Irby <[email protected]>
|
Thank you @irby 🙏🏼 |
Signed-off-by: Matthew H. Irby <[email protected]>
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.
Currently, I only receive an "MFA enabled" notification when either an OTP secret or a security key is registered for the very first time or an "MFA disabled" notification when all second factors have been removed, but I do not receive notifications for
- removing a second factor when it is not the last one
- adding additional security keys
- adding an OTP secret when there already is a security key registered
But I think there should be notifications for these cases. What do you think @FlxMgdnz ?
| } | ||
|
|
||
| type SecurityNotificationConfiguration struct { | ||
| Enabled bool `yaml:"enabled" json:"enabled,omitempty" koanf:"enabled" jsonschema:"default=false"` |
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.
JSON schema default should probably be true since all of these are set to true in the default config, no?
| Enabled bool `yaml:"enabled" json:"enabled,omitempty" koanf:"enabled" jsonschema:"default=false"` | |
| Enabled bool `yaml:"enabled" json:"enabled,omitempty" koanf:"enabled" jsonschema:"default=true"` |
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.
Yes, that should be made. Initially I made the config default to false but talking to Felix we decided to make this default to true, didn't make this change here after the fact. Will update.
|
|
||
| passkey_create_text: | ||
| description: "" | ||
| other: "Your {{ .ServiceName }} account has been registered with a new passkey." |
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.
I get this message when I already have an account registered and I add a passkey through the profile, so it is not the account that has been registered but the passkey itself, but I do not get this message when creating an account and registering a passkey during registration. I think the other value for the subject is a better fit.
The same holds true for the german text. Not sure about the other languages though.
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.
We should rephrase the message slightly to avoid any misunderstandings. A security notification should never be sent during the registration flow.
Proposed wording:
“A new passkey has been added to your {{ .ServiceName }} account.”
|
|
||
| passkey_create_text: | ||
| description: "" | ||
| other: "Ihr {{ .ServiceName }}-Konto wurde mit einem neuen Passkey registriert." |
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.
See the comment for the english texts.
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.
"Ein neuer Passkey wurde zu Ihrem {{ .ServiceName }}-Konto hinzugefügt."
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.
Will update the translation for all locales.
We discussed this internally again and think it would indeed be better to always send a security notification whenever a second factor is added or removed, instead of using the “2FA enabled” or “2FA disabled” notifications. Overall, the more granular notifications add an extra layer of security. For example, a compromised account where an attacker adds an additional second factor would otherwise go unnoticed. Sorry @irby that I initially said this differently. Could you still make this change? |
|
Hey @FlxMgdnz, yes, I can make the changes. I will be on vacation for the holidays starting tomorrow so it may be after the New Year before I get the updates in. |
Description
Addresses issue #1030 to introduce security notifications to the Hanko backend.
Implementation
By default, this enhancement will enable all security notifications until the notifications are disabled via the config. Each notification type has its own toggle to enable / disable the notification.
These are the security notification types added:
Whenever a notification is sent to the end user, an audit log is recorded noting the type of notification sent and the email address the notification was sent to.
Tests
Besides from some unit tests around default configuration and disabled configurations, there are not any automated tests that address these changes, as I had quite a bit of difficulty with getting unit tests to work with the flow api.
I did manually test all the various security notifications and confirmed they all work as expected.