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.

Sign up for GitHub

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

Jump to bottom

remove insecure rng providers #122

Merged
merged 9 commits into from
Apr 16, 2024
Merged

remove insecure rng providers #122

merged 9 commits into from
Apr 16, 2024

Conversation

NicolasCARPi
Copy link
Collaborator

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

@NicolasCARPi
remove insecure rng providers
d1792f0 
and remove the openssl provider. We now rely exclusively on
random_bytes(), as there are no reasons not to. Fix #121
@NicolasCARPi NicolasCARPi mentioned this pull request Apr 15, 2024
Closed
RobThree
RobThree approved these changes Apr 15, 2024
View reviewed changes
Copy link
Owner

@RobThree RobThree left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@NicolasCARPi
remove pointless test rng class
f6da6be 
we were testing a test class, which didn't make a lot of sense.
@NicolasCARPi
Copy link
Collaborator Author

I cleaned up the tests, too. The TestRngProvider class inside test was tested, but there is no point to test a class that we have in tests. Tests should test the prod code, here it was doing nothing of the sort.

@NicolasCARPi
Copy link
Collaborator Author

Maybe the last commit was a bit too hasteful. Reverted for now.

@RobThree
Copy link
Owner

RobThree commented Apr 15, 2024 •
edited
Loading

Isn't (or wasn't) it used to test if the 2FA class checked correctly for the isCryptographicallySecure property (or something along those lines).

@RobThree RobThree self-requested a review April 15, 2024 22:09
@NicolasCARPi
Copy link
Collaborator Author

Yeah, looking at it again, and given that getRandomBytes() is simply an alias to random_bytes(), it's a bit pointless to test if f(x) == f(x). We call it once in the test suite, that's enough. Should I reverse the reverse commit then?

@NicolasCARPi NicolasCARPi marked this pull request as ready for review April 15, 2024 22:17
@RobThree
Copy link
Owner

I think it's safe to throw it out, since the isCryptographicallySecure check is no longer performed by the TwoFactorAuth class and it has no use anymore.

@NicolasCARPi
Copy link
Collaborator Author

Done.

willpower232
willpower232 requested changes Apr 16, 2024
View reviewed changes
lib/TwoFactorAuth.php Outdated Show resolved Hide resolved
lib/TwoFactorAuth.php
@@ -51,14 +49,11 @@ public function __construct(
/**
* Create a new secret
*/
public function createSecret(int $bits = 80, bool $requirecryptosecure = true): string
public function createSecret(int $bits = 80): string
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mental note to document in the changelog

@NicolasCARPi
Merge branch 'master' into nico-slim
442eca0 
* master:
  Exclude useless files from dist archive #103
RobThree
RobThree approved these changes Apr 16, 2024
View reviewed changes
Copy link
Owner

@RobThree RobThree left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore my previous comment. LGTM.

@NicolasCARPi
Merge branch 'master' into nico-slim
841fd4e 
* master:
  delete files specific to code editors (#120)
@NicolasCARPi
assing rng provider to class attribute
17f713b 
this also aligns with other providers
willpower232
willpower232 requested changes Apr 16, 2024
View reviewed changes
lib/TwoFactorAuth.php Show resolved Hide resolved
@willpower232 willpower232 merged commit 194ecc2 into RobThree:master Apr 16, 2024
10 checks passed
@NicolasCARPi NicolasCARPi deleted the nico-slim branch April 16, 2024 15:53
@NicolasCARPi NicolasCARPi mentioned this pull request May 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@willpower232 willpower232 willpower232 requested changes

@RobThree RobThree RobThree approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Slimming down the lib further
3 participants
@NicolasCARPi @RobThree @willpower232