Repository navigation
Conversation
| :locked_account_timeout: 2.minutes | ||
| :amazon_key: | ||
| :amazon_secret: | ||
| :password_rules: |
There was a problem hiding this comment.
i structured it this way because I plan to add expiry_days in a follow up PR, which isn't complexity based, but is a password rule.
Would like your thoughts on naming here.
There was a problem hiding this comment.
Linux calls this password quality (https://linux.die.net/man/5/pwquality.conf), password rules is good too though
There was a problem hiding this comment.
My only thought here is do we expect to use these rules on things other than passwords? Maybe more generic secret_rules if we anticipate wanting to handle secrets, keys, passwords, etc.
There was a problem hiding this comment.
Good question - I don't think so. This is very specifically for database auth passwords. All other passwords and secrets we have come from outside sources so we can't control them.
|
@miq-bot cross-repo-test /all |
| :amazon_key: | ||
| :amazon_secret: | ||
| :password_rules: | ||
| :complexity_enforce: true |
There was a problem hiding this comment.
I'm considering adding a config/settings/development.yml entry here setting this to false. Thoughts?
There was a problem hiding this comment.
Should this just be password_rules.enforce? I'm assuming if we added other non-complexity related rules this would disable those as well
There was a problem hiding this comment.
So, as mentioned in the other comment, I'm going to add expiry_days here in a future PR, which really is independent of complexity (i.e. you can disable complexity checks and keep expiry checks or vice-versa. So this enforce is specific to complexity checks.
Actually now that we're mentioning it though, i don't need a separate enforce flag. For expiry, my plan is to just set to nil or false (or 0) to disable it, and I can do the same thing here with complexity_min_score. Setting the min_score to 0 would just disable complexity checks.
There was a problem hiding this comment.
Ok i dropped this particular setting in favor of the caller can just set complexity_min_score to 0. Then there's also no ambiguity about whether the settings affects all rules or just a subset of rules.
There was a problem hiding this comment.
Right setting min_score to 0 would disable the complexity check but not the words like Ah actually no it looks like that would skip this as well.vmdb or miq
I guess do you intend to have a way to disable all password rules, just password complexity rules.
There was a problem hiding this comment.
Specifically for complexity though, should we set min_score to 0 in dev?
There was a problem hiding this comment.
Ok updated to set to 0 in dev mode.
ccc1b88 to
a67f551
Compare
Use the zxcvbn-ruby gem to enforce password strength when a user sets or changes their password (database auth mode only). Complexity checks are skipped when the password virtual attribute is not set, so existing passwords on upgrade are unaffected. Configuration lives under Settings.authentication.password_rules: - complexity_min_score: 0-4 zxcvbn score threshold (default 3 in production and test; default 0 in development) - complexity_extra_words: operator-defined terms to treat as weak ManageIQ-specific terms (manageiq, miq, vmdb) are hardcoded as a built-in dictionary and are not configurable. The seeded admin user (smartvm password) bypasses the check via a skip_complexity_check flag set during User.seed, allowing first-boot to succeed while keeping enforcement active for all subsequent changes. Passwords over 72 bytes are already rejected by has_secure_password before the complexity check runs, so no separate length guard is needed. Includes a test for the operator-generated password format (RawURLEncoding of 12 random bytes), which scores 4 on zxcvbn.
a67f551 to
1f2f999
Compare
|
Checked commit Fryguy@1f2f999 with ruby 3.3.12, rubocop 1.91.0, haml-lint 0.78.0, and yamllint 1.37.1 |
|
@Fryguy almost certainly a future task, but would we want these rules to be different depending on role / group / tenant? Maybe administrators have a higher complexity requirement, or one tenant has a stricter requirement than another. The current Settings are global, but maybe we merge |
| :allow_nil => true, :message => "must be a valid email address"}, | ||
| :length => {:maximum => 255} | ||
| validates :current_group, :inclusion => {:in => proc { |u| u.miq_groups }, :allow_nil => true, :if => :current_group_id_changed?} | ||
| validate :password_complexity, :if => :password |
There was a problem hiding this comment.
Do we check the validity of the password in the UI if the password isn't strong enough? It might be easier to check it earlier than at save time.
There was a problem hiding this comment.
We check the validity on change password (so that's create or update). This line was interesting, but the way has_secure_password works, :password is not a real attribute stored in the database. It only has a lifetime when someone sets the value in the object, which will only happen if someone intends to change it. Any other time, such as when you query the User object, the :password is nil. So this line really says, "if someone sets the password attribute, then run the validation on save". This allows other non-password changes, such as an email change to not trigger the complexity heck, thereby allowing existing passwords to be considered "ok". I intentionally did not want to prevent upgrades or settings changes from suddenly breaking all existing passwords (even, potentially, the super-admin password).
I will have a follow up PR allowing super-admins to force password changes on demand, and my intention is to create a tool that an admin can run to audit all passwords against the rules and force a password change. This allows the admin to opt-in to that for existing passwords / users.
There was a problem hiding this comment.
If you're asking whether we could update the UI to do strength checks right in the UI - I think so, but that will be a follow up "nice to have". I am concerned we would need the same strength checks on both sides, so I'm not sure how that would work.
EDIT: Actually yeah, I'm pretty sure we can do that. zxcvbn is actually dropbox's algorithm and it's been ported to all the languages. That link is the original javascript version we could use on the UI side, and have a strength checker ahead of time. See also https://upmostly.com/tutorials/build-a-password-strength-meter-react
That's a cool idea, and I don't think the current PR prevents that. Definitely for a future PR. |
|
I added a PR to UI and API (listed as dependent PRs) to remove weak passwords in tests and use the default factory password as much as possible, only using a strong password where the test actually exercises a code path that needs it (e.g. where we tests password changes over the API). |
In ManageIQ/manageiq#23996 password complexity rules are being added, which will fail validation when simple passwords like 'x' or 'smartvm' are passed into user factories. Using the default password ('dummy') avoids failing the password complexity check while still satisfying the test requirements. Part of ManageIQ/manageiq#23996
Factory user calls drop explicit weak passwords so the factory default digest path is used; api_basic_authorize calls use the literal "dummy" string. API payload tests use "Tr0ub4dor&3" since those go through the controller which enforces complexity validation. Part of ManageIQ/manageiq#23996
|
@Fryguy I think all dependent PRs have been merged, it looks like cross repo was kicked off before them, maybe kick that and we can decide to merge this when it's green. |
|
@miq-bot cross-repo-test /all |
|
ManageIQ/manageiq-api#1351 should fix the api test failures |
Use the zxcvbn-ruby gem to enforce password strength when a user sets or changes their password (database auth mode only). Complexity checks are skipped when the password virtual attribute is not set, so existing passwords on upgrade are unaffected.
Configuration lives under Settings.authentication.password_rules:
ManageIQ-specific terms (manageiq, miq, vmdb) are hardcoded as a built-in dictionary and are not configurable.
The seeded admin user (smartvm password) bypasses the check via a skip_complexity_check flag set during User.seed, allowing first-boot to succeed while keeping enforcement active for all subsequent changes.
Passwords over 72 bytes are already rejected by has_secure_password before the complexity check runs, so no separate length guard is needed.
Includes a test for the operator-generated password format (RawURLEncoding of 12 random bytes), which scores 4 on zxcvbn.
@jrafanie Please review.
Depends on