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

PasswordGenerator: add new class for generating human-friendly passwords via CryptographicRandom. #2890

Merged
merged 1 commit into from Mar 2, 2017

Conversation

@mkrautz
Copy link
Member

commented Feb 28, 2017

This depends on #2882 and includes the commits of that PR as well. Will need to be removed before landing.

@mkrautz mkrautz force-pushed the mkrautz:password-generator branch 2 times, most recently from 1cf9b63 to 697c4ee Feb 28, 2017

@mkrautz mkrautz changed the title WIP: PasswordGenerator: add new class for generating human-friendly passwords via CryptographicRandom. PasswordGenerator: add new class for generating human-friendly passwords via CryptographicRandom. Feb 28, 2017

@mkrautz mkrautz requested review from Kissaki, hacst and davidebeatrici Feb 28, 2017

@Kissaki
Copy link
Member

left a comment

Isn’t the indinguishability a font issue? Should we just display the password in an adequate font?

Where do we want to use this?

Not sure about the test case. If it tests against \0 it should also check against other unprintable characters, and the ones removed from the password alphabet. Still not testing how strong it is then ofc, which would be hard to test anyway.

Otherwise, looks ok.

#include "PasswordGenerator.h"

// This is a password alphabet that tries to be human-friendly.
// Most cases of ambiguitiy has been removed.

This comment has been minimized.

Copy link
@Kissaki

Kissaki Feb 28, 2017

Member

have been removed

@Kissaki

This comment has been minimized.

Copy link
Member

commented Feb 28, 2017

Where do we want to use this?

See #2891

@mkrautz mkrautz force-pushed the mkrautz:password-generator branch 3 times, most recently from 99fc0e5 to 0315cfc Mar 1, 2017

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2017

Isn’t the indinguishability a font issue? Should we just display the password in an adequate font?

The alphabet is just a nice side-effect. It's as complete as I could come up with at the moment.
I think it's actually more important that it might seem, to ensure users don't shoot themselves in the foot with passwords, i.e., if they write it down to remember it later.

Not sure about the test case. If it tests against \0 it should also check against other unprintable characters, and the ones removed from the password alphabet. Still not testing how strong it is then ofc, which would be hard to test anyway.

The test case now tests against the actual alphabet.

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2017

@Kissaki PTAL.

@Kissaki
Kissaki approved these changes Mar 1, 2017
#include "PasswordGenerator.h"

// Get the password alphabet from PasswordGenerator.
extern QVector<QChar> mumble_password_generator_alphabet();

This comment has been minimized.

Copy link
@Kissaki

Kissaki Mar 1, 2017

Member

Nice trick to work around public visibility in program code but be usable in this test. :)


class PasswordGenerator {
public:
static QString alphabet();

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 1, 2017

Author Member

Shit. I accidently put this here.

This comment has been minimized.

Copy link
@Kissaki

Kissaki Mar 1, 2017

Member

Too bad there's no revision diff where I could spot this as well, without looking through everything again :)

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 1, 2017

Author Member

Yeah... :-(

@mkrautz mkrautz force-pushed the mkrautz:password-generator branch 2 times, most recently from 293eb32 to 8a85d33 Mar 1, 2017

@mkrautz mkrautz force-pushed the mkrautz:password-generator branch from 8a85d33 to a9e7ccf Mar 2, 2017

@mkrautz mkrautz merged commit b046d6f into mumble-voip:master Mar 2, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.