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

[NEW] E2E password generator #24114

Merged
merged 17 commits into from
Feb 21, 2022
Merged

[NEW] E2E password generator #24114

merged 17 commits into from
Feb 21, 2022

Conversation

ostjen
Copy link
Contributor

@ostjen ostjen commented Jan 6, 2022

Created a new password generator for e2e private keys using readable words

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Further comments

@ostjen ostjen requested a review from sampaiodiego January 6, 2022 14:45
@tassoevan
Copy link
Contributor

generateMnemonicPhrase may be a good addition to our @rocket.chat/string-helpers package (currently on Fuselage monorepo), but I'm wonder if rely on Math.random() is fine. The previous solution was based on meteor/random, which differs in the pseudorandom generator used.

@ostjen
Copy link
Contributor Author

ostjen commented Jan 7, 2022

I can switch to meteor/random, but I don't think it'd be good too still rely on meteor ... and @sampaiodiego do you think the generateMnemonicPhrase should be on @rocket.chat/string-helpers?

app/e2e/client/helper.js Outdated Show resolved Hide resolved
app/e2e/client/wordList.ts Outdated Show resolved Hide resolved
@sampaiodiego
Copy link
Member

I'm not particularly worried about Math.random() .. if the issue is that it is using repeated words too often we can maybe change it to make sure it always use different words? or maybe adding a few more Math.random() calls would be enough

@ostjen
Copy link
Contributor Author

ostjen commented Jan 11, 2022

I'm not particularly worried about Math.random() .. if the issue is that it is using repeated words too often we can maybe change it to make sure it always use different words? or maybe adding a few more Math.random() calls would be enough

I don't see Math.random() as a problem too, I think what @tassoevan was worried was just that we used meteor pseudorandom algo in the previous solution

@tassoevan
Copy link
Contributor

@sampaiodiego @ostjen The problem with Math.random is not being cryptographically secure, so it's not an option for generating passwords. Unsurprisingly, sometimes Snyk marks Math.random() as a vulnerability. This is better: https://developer.mozilla.org/en-US/docs/Web/API/Crypto/getRandomValues

@sampaiodiego
Copy link
Member

sampaiodiego commented Jan 11, 2022

@sampaiodiego @ostjen The problem with Math.random is not being cryptographically secure, so it's not an option for generating passwords. Unsurprisingly, sometimes Snyk marks Math.random() as a vulnerability. This is better: https://developer.mozilla.org/en-US/docs/Web/API/Crypto/getRandomValues

the point is that Math.random is not being used to generate the password itself.. it's being used to get an index position of words array list..

@tassoevan
Copy link
Contributor

it's being used to get an index position of words array list..

Which are combined to generate a password.

Copy link
Member

@sampaiodiego sampaiodiego left a comment

Choose a reason for hiding this comment

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

I don't know the root cause but while testing this I found out the keys were not successfully being generated anymore. not sure if the new password is not compatible (?)

app/e2e/client/helper.js Outdated Show resolved Hide resolved
app/e2e/client/helper.js Outdated Show resolved Hide resolved
@ostjen ostjen dismissed stale reviews from pierre-lehnen-rc and sampaiodiego February 21, 2022 15:03

changes already made

@ostjen ostjen merged commit 0706212 into develop Feb 21, 2022
@ostjen ostjen deleted the e2e_new_pwd branch February 21, 2022 15:04
@pierre-lehnen-rc pierre-lehnen-rc mentioned this pull request Mar 1, 2022
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 this pull request may close these issues.

5 participants