Have I Been Pwned Integration (Password security enhancements) #832 #974

amosfolz · 2019-05-06T00:14:00Z

This will need more work but I feel is at a place to open up for some feedback.
The proposed feature adds two additional config values under site.password_security.

'password_security' => [
             'enforce_no_compromised'   => '5', // Set to false to turn off this feature. Otherwise, provide a numeric string, which sets the maximum number
                                       // of times that is "acceptable" for a password to have appeared in breaches. The recommended and most secure
                                       // option is '0' - meaning only passwords that are not on the list of compromised passwords will be allowed.

             'enforce_update_passwords' => true // Settings this to true will check user's passwords against list of known compromised passwords at each log on.
]

(I am open to suggestions on the naming convention for these).

codecov · 2019-05-06T00:37:35Z

Codecov Report

Merging #974 (a5432d6) into develop (c92bf0b) will decrease coverage by 0.12%.
The diff coverage is 58.06%.

@@              Coverage Diff              @@
##             develop     #974      +/-   ##
=============================================
- Coverage      67.76%   67.64%   -0.13%     
- Complexity      1997     2021      +24     
=============================================
  Files            169      171       +2     
  Lines           6995     7086      +91     
=============================================
+ Hits            4740     4793      +53     
- Misses          2255     2293      +38

Impacted Files	Coverage Δ	Complexity Δ
.../src/Database/Migrations/v430/UpdateUsersTable.php	`100.00% <ø> (ø)`	`3.00 <0.00> (ø)`
...nkles/account/src/Controller/AccountController.php	`95.59% <25.92%> (-4.41%)`	`92.00 <2.00> (+6.00)`	⬇️
.../account/src/ServicesProvider/ServicesProvider.php	`77.86% <44.44%> (-1.96%)`	`11.00 <0.00> (+1.00)`	⬇️
...kles/account/src/Authenticate/PasswordSecurity.php	`70.21% <70.21%> (ø)`	`14.00 <14.00> (?)`
...grations/v430/AddPasswordResetColumnUsersTable.php	`100.00% <100.00%> (ø)`	`3.00 <3.00> (?)`
...account/src/Repository/PasswordResetRepository.php	`100.00% <100.00%> (ø)`	`1.00 <0.00> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c92bf0b...a5432d6. Read the comment docs.

lcharette

The controller is really long... While having private function in the controller is not wrong, I think it would make more sense to put both checkPassword and checkHash in a separate (static) class? Would make easier to use those methods elsewhere as well as writing proper tests.

amosfolz · 2019-05-07T14:04:07Z

The controller is really long... While having private function in the controller is not wrong, I think it would make more sense to put both checkPassword and checkHash in a separate (static) class? Would make easier to use those methods elsewhere as well as writing proper tests.

I thought they would probably need to be moved. I will need some guidance on where to move them to...

Move checkHash and checkPassword

Silic0nS0ldier · 2019-05-09T02:28:34Z

Looks like there are some changes on master that aren't on develop. Probably want to sync it up. Would make the diff clearer.

Silic0nS0ldier

Few things to be addressed here. There will likely need to be some discussion in most areas first though.

app/sprinkles/account/config/default.php

app/sprinkles/account/locale/en_US/messages.php

app/sprinkles/account/src/Authenticate/PasswordSecurity.php

app/sprinkles/account/src/Controller/AccountController.php

Update configuration options.

Add `passwordSecurity` as to `ServicesProviders`

Refactor

Add passwordSecurity service.

Add `cache` and `breaches` options to `enforce_no_compromised`

Add PasswordSecurity as a service

Reorganize to align with contributing style guide

amosfolz · 2019-06-30T16:07:16Z

The experience around the (root) admin account being on a compromised passwords list could use some work. Namely, the admin is not forced to perform a password reset to login despite the recorded user activity stating that they have to. Probably not great that the account with the most access isn't covered by these additional protections.

Good catch! This is because the if that checks if flag_password_reset_required is set was using an instance of currentUser from prior to that field being set. (so it wasn't enforcing the change until the next login.) This should be fixed now.

…ncements

Edit a few comments

amosfolz · 2019-07-15T00:35:11Z

I believe this is ready for another review.

…ncments

Silic0nS0ldier

A few issues, but the core logic looks good. I haven't tested it this time however.

Silic0nS0ldier · 2019-07-20T04:20:14Z

app/sprinkles/account/src/Authenticate/PasswordSecurity.php

+     *
+     * @return array Array of password hashes in the format c2d18a7d49b0d4260769eb03d027066d29a:181 - or <hash>:<number of breaches.
+     */
+    private function getHashArrayFromAPI($hashPrefix)


How would this handle connectivity issues/outage of the HIBP Passwords API? It looks like there isn't any handling for it, potentially it would prevent login.

You're right.

I'll think about the best way to handle this. We probably want to try to query HIBP at least a few times before allowing user to login and bypass the check...

Got to be careful with retry logic. At scale, it can translate to a massive spike in requests for an API.
Possible scenarios include;

API returns 500 error due to a fault, could be an edge case or everything, retry logic here would easily result in a larger number of requests (CloudFlare wouldn't cache these either, so it would fairly consistently hit the function itself until potentially DDoS protection gets triggered).

Response codes indicate that retries should not be attempted exist, we would be wasting our own time retrying these.

Connection timeouts could be attributed to failures anywhere along the connection (local misconfiguration, API hit its own response timeout and terminated).

Deliberate rate limiting by HIBP (if a UF site were to become heavily used, we could end up hitting this)

Ultimately, it may come down to not retrying at all and logging that the check failed. HIBP passwords should be supplemental to the security such that any UF site doesn't get busted by any issues.

I've tried to fix this in the recent commits. (Also switched over to Guzzle)

app/sprinkles/account/src/Database/Migrations/v430/AddPasswordRestColumnUsersTable.php

Silic0nS0ldier · 2019-07-20T04:25:23Z

app/sprinkles/account/src/Database/Migrations/v430/AddPasswordRestColumnUsersTable.php

+    /**
+     * {@inheritdoc}
+     */
+    public static $dependencies = [


Could we use reflection here? E.g;

use \UserFrosting\Sprinkle\Account\Database\Migrations\v400\GroupsTable`; ... public static $dependencies = [ GroupsTable::class, ... ];

I generally prefer to have source level references where possible as it makes tracking down references later, and performing refactors (with tooling assistance) much quicker.

I think we could. If this is the preference I should probably update the other two "modifying" migrations in 4.3.

@lcharette Should I change this and the other migrations that were already merged?

Yes. Both will work, but reflection is easier to work with in IDE

It seems a leading \ is being added somewhere inside the migrator.

[ERROR] Unfulfillable migrations found :: => \UserFrosting\Sprinkle\Account\Database\Migrations\v430\AddPasswordResetColumnUsersTable (Missing dependency : UserFrosting\Sprinkle\Account\Database\Migrations\v400\GroupsTable)

Some changes will likely need to be made in the migrator if we want to make this work. Unless we want to try to make those changes for 4.3, should probably just hold off on this for now.

app/sprinkles/account/src/Database/Migrations/v430/AddPasswordRestColumnUsersTable.php

Silic0nS0ldier · 2019-07-20T04:31:33Z

app/sprinkles/account/src/Repository/PasswordResetRepository.php

@@ -33,7 +33,9 @@ class PasswordResetRepository extends TokenRepository
    protected function updateUser(UserInterface $user, $args)
    {
        $user->password = Password::hash($args['password']);
+        $user->flag_password_reset_required = false;
        // TODO: generate user activity? or do this in controller?


Technically outside scope of this, but does this have an answer yet? Are we logging updates to the user at all?

It doesn't look like it is happening in controller or here, so I would say no.

It would make sense to log some type of message "User set a new password".

Then the controller would be responsible for logging activity messages related to why a user needs to reset their password. (like HIBP, admin forcing reset, etc..)

app/sprinkles/account/assets/userfrosting/js/uf-password-security.js

Still need to add where to log messages to

Add typecasts

lcharette · 2019-07-27T14:24:42Z

FYI, I tagged this to 4.4.x as I think it will require more testing from both @Silic0nS0ldier and I.

Silic0nS0ldier · 2021-04-24T12:01:50Z

As can be seen, I'm trying to resurrect this PR nearly 2 years on. Its been synced with master and had some issues resolved (mainly PHP crashes relating to recently completed deprecations and type violations).

lcharette · 2023-11-25T02:28:56Z

Closing this, as it need to be updated for UF5. It should be implement as it's own sprinkle anyway.

amosfolz added 5 commits May 5, 2019 23:49

update AccountController.php

2956102

Update asset bundle and routes

11c28a4

add js file and update messages.php

5db2b63

update default.php config

1da5093

Move uf-password-security.js to correct directory

0e66188

lcharette reviewed May 6, 2019

View reviewed changes

amosfolz added 2 commits May 7, 2019 19:20

Cleanup AccountController.php

798707d

Move checkHash and checkPassword

Create PasswordSecurity.php

5bb27bf

Silic0nS0ldier requested changes May 9, 2019

View reviewed changes

amosfolz commented May 9, 2019

View reviewed changes

app/sprinkles/account/src/Controller/AccountController.php Outdated Show resolved Hide resolved

amosfolz added 5 commits May 12, 2019 16:12

Update PasswordSecurity.php

3b4bda7

Update AccountController.php

68d8502

Update default.php

676183f

Update AccountController.php

c86a613

Merge pull request #1 from amosfolz/pw-security-fixes

c58d81b

Update configuration options.

lcharette added this to the 4.3.0 milestone May 14, 2019

amosfolz added 11 commits May 14, 2019 20:57

Update AccountController.php

f04ef44

Add `passwordSecurity` as to `ServicesProviders`

Update PasswordSecurity.php

fc69108

Refactor

Update ServicesProvider.php

67738de

Add passwordSecurity service.

Add cache and breaches options

6947754

Add `cache` and `breaches` options to `enforce_no_compromised`

Refactor PasswordSecurity

e0b8f01

Update AccountController.php

527438d

Merge branch 'pw-security-enhancments' into pw-security-updates

cca5ffb

Merge pull request #2 from amosfolz/pw-security-updates

f3f2d1d

Add PasswordSecurity as a service

Update messages.php

62505aa

Update PasswordSecurity.php

09ca0c2

Reorganize to align with contributing style guide

Create uf-password-security.js

fd28ef2

Fix redirect not working the first time flag is set

5f2cbbd

amosfolz added 7 commits July 12, 2019 08:28

Merge remote-tracking branch 'upstream/develop' into pw-security-enha…

156b812

…ncements

Merge branch 'develop' into pw-security-enhancments

0f2d530

Merge branch 'develop' into pw-security-updates

a6803d8

Merge branch 'pw-security-enhancements' into pw-security-updates

03b0a95

Delete PasswordSecurityTest.php

1deb817

Update PasswordSecurity.php

e697436

Edit a few comments

Merge branch 'pw-security-updates' into pw-security-enhancements

534a099

Merge remote-tracking branch 'upstream/develop' into pw-security-enha…

b3b23e9

…ncments

Silic0nS0ldier requested changes Jul 20, 2019

View reviewed changes

amosfolz added 8 commits July 21, 2019 14:18

Fix styling and other minor adjustments

e985aee

catch errors when querying HIBP

c988e49

Still need to add where to log messages to

Update PasswordSecurity.php

af349af

Merge branch 'pw-security-enhancements' into pw-security-updates

6a0e7fa

Add GuzzleHttp as core dependency

a58175b

Add exception and typecast

3f89221

Update PasswordSecurity.php

a6533c7

Add typecasts

Style fix

d040308

lcharette modified the milestones: 4.3.0, 4.4.0 Jul 27, 2019

lcharette modified the milestones: 4.4.0, 4.5.0 Jan 11, 2020

Silic0nS0ldier added 2 commits April 24, 2021 21:17

Merge branch 'master' into pw-security-enhancments

a10c40e

Misc. fixes to permit exploratory testing

a5432d6

lcharette closed this Nov 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Have I Been Pwned Integration (Password security enhancements) #832 #974

Have I Been Pwned Integration (Password security enhancements) #832 #974

amosfolz commented May 6, 2019

codecov bot commented May 6, 2019 •

edited

lcharette left a comment

amosfolz commented May 7, 2019

Silic0nS0ldier commented May 9, 2019

Silic0nS0ldier left a comment

amosfolz commented Jun 30, 2019

amosfolz commented Jul 15, 2019

Silic0nS0ldier left a comment

Silic0nS0ldier Jul 20, 2019

amosfolz Jul 20, 2019

Silic0nS0ldier Jul 20, 2019

amosfolz Jul 24, 2019

Silic0nS0ldier Jul 20, 2019

amosfolz Jul 20, 2019

amosfolz Jul 20, 2019

lcharette Jul 20, 2019

amosfolz Jul 21, 2019

Silic0nS0ldier Jul 20, 2019

amosfolz Jul 24, 2019

amosfolz Jul 24, 2019 •

edited

lcharette commented Jul 27, 2019

Silic0nS0ldier commented Apr 24, 2021

lcharette commented Nov 25, 2023

Have I Been Pwned Integration (Password security enhancements) #832 #974

Have I Been Pwned Integration (Password security enhancements) #832 #974

Conversation

amosfolz commented May 6, 2019

codecov bot commented May 6, 2019 • edited

Codecov Report

lcharette left a comment

Choose a reason for hiding this comment

amosfolz commented May 7, 2019

Silic0nS0ldier commented May 9, 2019

Silic0nS0ldier left a comment

Choose a reason for hiding this comment

amosfolz commented Jun 30, 2019

amosfolz commented Jul 15, 2019

Silic0nS0ldier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amosfolz Jul 24, 2019 • edited

Choose a reason for hiding this comment

lcharette commented Jul 27, 2019

Silic0nS0ldier commented Apr 24, 2021

lcharette commented Nov 25, 2023

codecov bot commented May 6, 2019 •

edited

amosfolz Jul 24, 2019 •

edited