Skip to content

remove insecure rng providers#122

Merged
willpower232 merged 9 commits into
RobThree:masterfrom
NicolasCARPi:nico-slim
Apr 16, 2024
Merged

remove insecure rng providers#122
willpower232 merged 9 commits into
RobThree:masterfrom
NicolasCARPi:nico-slim

Conversation

@NicolasCARPi

@NicolasCARPi NicolasCARPi commented Apr 15, 2024

Copy link
Copy Markdown
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

@RobThree RobThree left a comment

Copy link
Copy Markdown
Owner

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

NicolasCARPi commented Apr 15, 2024

Copy link
Copy Markdown
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

NicolasCARPi commented Apr 15, 2024

Copy link
Copy Markdown
Collaborator Author

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

@RobThree

RobThree commented Apr 15, 2024 •
edited
Loading

Copy link
Copy Markdown
Owner

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

NicolasCARPi commented Apr 15, 2024

Copy link
Copy Markdown
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

RobThree commented Apr 15, 2024

Copy link
Copy Markdown
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

NicolasCARPi commented Apr 15, 2024

Copy link
Copy Markdown
Collaborator Author

Done.

willpower232
willpower232 requested changes Apr 16, 2024
View reviewed changes
Comment thread lib/TwoFactorAuth.php Outdated
Comment thread lib/TwoFactorAuth.php
* Create a new secret
*/
public function createSecret(int $bits = 80, bool $requirecryptosecure = true): string
public function createSecret(int $bits = 80): string

@willpower232 willpower232 Apr 16, 2024

Copy link
Copy Markdown
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

@RobThree RobThree left a comment

Copy link
Copy Markdown
Owner

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
Comment thread lib/TwoFactorAuth.php
@willpower232 willpower232 merged commit 194ecc2 into RobThree:master Apr 16, 2024
@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