-
Notifications
You must be signed in to change notification settings - Fork 31
Allow disabling local password through broker.conf #1234
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: Shiv Tyagi <[email protected]>
…rd` in broker.conf Signed-off-by: Shiv Tyagi <[email protected]>
5495017 to
ba71c03
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1234 +/- ##
==========================================
- Coverage 87.64% 80.54% -7.10%
==========================================
Files 91 20 -71
Lines 6231 987 -5244
Branches 111 0 -111
==========================================
- Hits 5461 795 -4666
+ Misses 714 192 -522
+ Partials 56 0 -56 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
adombeck
left a comment
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.
Thank you for working on this, @shiv-tyagi! There's still a bit of work for us to do here, most notably we will have to figure out how to unlock the keyring when there's no local password.
| session.nextAuthModes = []string{authmodes.NewPassword} | ||
|
|
||
| return AuthNext, nil | ||
| // Only require password creation if local password authentication is not disabled |
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.
| // Only require password creation if local password authentication is not disabled | |
| // Require password creation unless local password authentication is disabled |
| ## | ||
| ## When enabled: | ||
| ## - Users will not be able to create or use local passwords | ||
| ## - Device authentication will be required for every login |
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.
| ## - Device authentication will be required for every login | |
| ## - Device authentication will be required for every authentication attempt, | |
| ## including logins and privileged operations (e.g. sudo, polkit actions) |
| ## - Device authentication will be required for every login | ||
| ## - Local password authentication mode will not be offered | ||
| ## | ||
| ## Important: Enabling this option prevents offline login entirely. |
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.
| ## Important: Enabling this option prevents offline login entirely. | |
| ## Important: Enabling this option prevents offline authentication entirely. |
| if !b.cfg.disableLocalPassword { | ||
| session.nextAuthModes = []string{authmodes.NewPassword} | ||
| return AuthNext, nil | ||
| } |
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.
Another consequence of not creating a local password is that the keyring can't be unlocked. #1190 also won't fix it in that case.
| ## When enabled: | ||
| ## - Users will not be able to create or use local passwords | ||
| ## - Device authentication will be required for every login | ||
| ## - Local password authentication mode will not be offered |
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.
Note to self: We should probably point users to force_provider_authentication here, and maybe briefly discuss the use cases of force_provider_authentication and disable_local_password.
| if !b.cfg.disableLocalPassword { | ||
| session.nextAuthModes = []string{authmodes.NewPassword} | ||
| return AuthNext, nil | ||
| } |
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.
Note to self: Manually test:
- ssh
- sudo
- pkexec
- unlocking keyring
Ideally we should add e2e tests for those.
Closes #726
This adds the
disable_local_passwordoption to the broker.conf. When set to true, the local password authentication mode is disabled completely and the user is forced to authenticate with device authentication.In device authentication mode, the set local password step after successful authentication is skipped as well because it is not needed anymore.
I have tested this locally and it works as described.