search

DivineOmega / password_exposed

🔒 Password Exposed Helper Function - Check if a password has been exposed in a data breach.
GNU Lesser General Public License v3.0
213 stars 34 forks source link

Next Major Release - A Suggestion #22

Closed nicklog closed 5 years ago

nicklog commented 5 years ago

Hello,

It's me again ;)

Actually this PR shouldn't be so big, but somehow it happened.

I would implement this library into my own projects, but wanted to use my own HTTP client and cache provider. This was not possible in the original version. So I made some changes to make it possible. Some new dependencies need PHP7 to run. That's why I increased the minimum version to 7.1 in this PR. Additionally I added some help functions and an abstract class and interface. This allows developers to make their own implementations as needed.

Think of it as a kind of beta for version 3. I haven't adapted the Readme yet. Just check it out and give me feedback.

If you would accept the PR i would write a Symfony bridge to ;)

Best regards.

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 126

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/PasswordExposedChecker.php 36 40 90.0%
src/AbstractPasswordExposedChecker.php 32 37 86.49%
<!-- Total: 70 79 88.61% -->
Totals Coverage Status
Change from base Build 124: -0.3%
Covered Lines: 73
Relevant Lines: 82
💛 - Coveralls
coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 143

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/PasswordExposedChecker.php 41 46 89.13%
src/AbstractPasswordExposedChecker.php 36 42 85.71%
<!-- Total: 79 90 87.78% -->
Totals Coverage Status
Change from base Build 124: -1.3%
Covered Lines: 81
Relevant Lines: 92
💛 - Coveralls
DivineOmega commented 5 years ago

Hello. Thanks for the PR.

However, I'm a little concerned that this seems very complex for just allowing developers to use a custom client and cache.

I have implemented the ability to use a custom PSR-6 cache library in #23. I think this is somewhat simpler.

In regards to allowing a customer client, I was thinking of just allowing a PSR-18 compliant client to be passed into the constructor, rather than a guzzle client. We could that create a small PSR-18 adapter for guzzle, that the package would use if a custom client was not provided.

What are you thoughts?

nicklog commented 5 years ago

Hey, thanks for your reply

However, I'm a little concerned that this seems very complex for just allowing developers to use a custom client and cache.

It's not as complicated as it looks. I just added PHP7 typing, implemented an abstract class, added an interface, and implemented getter methods.

I have implemented the ability to use a custom PSR-6 cache library in #23. I think this is somewhat simpler.

Thats good but...

In regards to allowing a customer client, I was thinking of just allowing a PSR-18 compliant client to be passed into the constructor, rather than a guzzle client. We could that create a small PSR-18 adapter for guzzle, that the package would use if a custom client was not provided.

.. I've already done all that. I used PSR-6 and PSR-18 for the constructor as well as the getter methods and PSR-7/PSR-17 for request generation. The Guzzle-Client-Adapter based on PSR-18 and your own cache implementation based on PSR-6 will only be created if you don't pass your own in the constructor. The abstract class is for other developers to make their own customizations and implementations.

What are you thoughts?

Since the changes seem to be multi-layered, the PHP version will be raised and various PSR standards will be introduced, I would suggest to make version 3.0 out of it. The PHP Unit tests are all successful.

The public methods are the same. Only the signature of the constructor has changed slightly. Otherwise, all changes only affect the inner workings of the classes.

What do you say?

DivineOmega commented 5 years ago

Hi. Thanks for your detailed reply. Much appreciated.

I'll re-review your code, hopefully this weekend, and get back to you. :+1:

Thanks again.

nicklog commented 5 years ago

I have to thank you ;)

DivineOmega commented 5 years ago

I've noticed in the passwordExposedByHash method, you have modified the caching system to cache the password exposed status rather than the API response body. It was deliberate decision to cache the response body here, so that passwords with the same 5 characters at the beginning of their SHA1 hash could use the same cache file.

Even without this benefit, the current approach is using a cacheKey that is based on the first 5 characters, meaning the cached status would not be accurate for any other passwords that shared those same first 5 characters (of the hash).

Would you be okay to tweak your code to adopt the existing caching system (the response body, rather than the password exposed status)?

Hope this makes sense.

nicklog commented 5 years ago

Hey,

I get what you mean. Changed it so that the status is stored to the hash in the cache and additionally hashed the cache key again.

I just want to avoid unnecessary overhead. And so each status has its own cache lifetime.

Do you agree?

DivineOmega commented 5 years ago

Hi there,

I'm afraid I'd still prefer the API body response to be cached rather than the status. Although I understand that the way you've implemented this reduces the overhead of parsing the response into a status, it also means that you are creating a directory containing the SHA1 hashes of every password attempted.

I'd consider this to be something of a security risk, as it exposes the passwords in a far less secure manner than the hash formats most web applications will store their passwords (usually with bcrypt or similar). SHA1 password hashes, especially those without salts, are very easy to crack or lookup in rainbow table.

Although somewhat less optimised, this was a deliberate implementation decision for security.

Does this make sense?

nicklog commented 5 years ago

Hey,

okay :) At the moment I can't think of any other option. Then let's do it the proven way.

Do you have any other comments?

DivineOmega commented 5 years ago

This looks great, thank you.

My only two additional suggestion, which are not critical, but would be nice to have are the following.

However, if you could resolve the Travis CI and StyleCI issues, I will look at getting this merged. Thanks again.

nicklog commented 5 years ago

Take a look. Additional i added some information in the composer.json. Please check it if you agree.

If you allow it, please open a new repository called "DivineOmega/password-exposed-bundle" and give me write permission. Would like to write a Symfony bundle.

DivineOmega commented 5 years ago

I'll get this merged in shortly. I might have to tweak a few things related to the tests before release, but other than that looks great.

Thank you for your work on this.

I've created a repo for the Symfony bundle and provided you with write access.

https://github.com/DivineOmega/symfony-password-exposed-bundle

DivineOmega commented 5 years ago

v3.0.0 now released. :tada:

https://github.com/DivineOmega/password_exposed/releases/tag/v3.0.0

nicklog commented 5 years ago

Yeah, cool :) I like it. Thank you for your open mind.