Skip to content
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

Slimming down the lib further #121

Closed
NicolasCARPi opened this issue Apr 15, 2024 · 6 comments · Fixed by #122
Closed

Slimming down the lib further #121

NicolasCARPi opened this issue Apr 15, 2024 · 6 comments · Fixed by #122

Comments

@NicolasCARPi
Copy link
Collaborator

NicolasCARPi commented Apr 15, 2024

So I was looking at:

public function getRngProvider(): IRNGProvider
{
if ($this->rngprovider !== null) {
return $this->rngprovider;
}
if (function_exists('random_bytes')) {
return $this->rngprovider = new CSRNGProvider();
}
if (function_exists('openssl_random_pseudo_bytes')) {
return $this->rngprovider = new OpenSSLRNGProvider();
}
if (function_exists('hash')) {
return $this->rngprovider = new HashRNGProvider();
}
throw new TwoFactorAuthException('Unable to find a suited RNGProvider');

And I wondered: in which case would anyone use something other than PHP's own random_bytes() function?

Before hacking away at the code, I wanted to open an issue to discuss it. IMHO, the openssl_random_pseudo_bytes can go away, along with the hash one, which should definitely go as it's not cryptographically secure.

Let's rely on the good work done by PHP devs to give us this CSPRNG function, while still leaving the door open for anyone to provide their own, of course, no need to lock it down. But no need to support anything else than the gold standard, don't you think?

TwoFactorAuth being a security related library, making the code smaller leaves less chances for bugs, and alleviates the maintenance, too!

@RobThree
Copy link
Owner

RobThree commented Apr 15, 2024

This is a leftover from the PHP5 era I guess. I don't mind if the openssl & hash ones get trashed. We stopped supporting PHP5 long ago. I would like to keep the interface because some people may be using their own implementations (I can see people using a hardware RNG for example).

@NicolasCARPi
Copy link
Collaborator Author

@RobThree See #122 then. And yes, users can still swap out the rngprovider for their own, if they need to.

@RobThree
Copy link
Owner

RobThree commented Apr 15, 2024

Dangit! I was removing them too, you beat me to it (and did a better job).

Screenshot 2024-04-15 at 23 37 08

👊

(Editted because I drag & dropped the wrong screenshot 🤪 I'm an idiot 🤦 )

@willpower232
Copy link
Collaborator

while you're at it, might be worth cleaning up all the unreachable code in codeEquals as hash_equals is PHP >= 5.6 so definitely always exists

@NicolasCARPi
Copy link
Collaborator Author

@willpower232 done, good spot!

When I'm done with this lib it'll be a 3 lines function :p

@willpower232
Copy link
Collaborator

wait till they announce it as a native library or something haha

willpower232 pushed a commit that referenced this issue Apr 16, 2024
* remove insecure rng providers

and remove the openssl provider. We now rely exclusively on
random_bytes(), as there are no reasons not to. Fix #121

* remove the isSecure property of the test rng class

* remove pointless test rng class

we were testing a test class, which didn't make a lot of sense.

* Revert "remove pointless test rng class"

This reverts commit f6da6be.

* Reapply "remove pointless test rng class"

This reverts commit 06220d4.

* assing rng provider to class attribute

this also aligns with other providers

* remove polyfill for hash_equals
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants