Improve password generator, add option to minimize recurrences #1337

fonic · 2017-12-29T10:13:31Z

Description

This improves the password generator and adds a new option/feature:

Add new option/feature 'Minimize recurrences': when enabled, picked characters are not considered for further picks by eliminating them from the corresponding groups; groups are reset once exhausted
Modify implementation of 'Pick characters from every group' option to actually do what it says: continuously pick characters from each selected group until password has reached the desired length
Switch location of character group buttons 'a-z' and 'A-Z' in GUI to match group indices of algorithm

Motivation

I often wondered why passwords such as 'Rb<73<U<@>gPW97f'' are generated. Instead of having recurring characters, I'd like the algorithm to choose as many distinct characters as possible, such as 'Rb!73<U]@>gPW91f'.

Also, while reviewing the existing implementation, I noticed the 'Pick characters from every group' only adds one character of each selected group, all other characters are picked entirely random, thus one could theoretically end up with a password like 'aA0?abcdefg', which is not what the option suggests, in my opinion.

DONE

Add new option/feature 'Minimize recurrences' (GUI option, config load/save, algorithm)
Modify implementation of 'Pick characters from every group' option to actually do what it says
Switch location of character group buttons 'a-z' and 'A-Z'

TODO

Determine cause of memory leaks / heap overflow

How has this been tested?

locally

Screenshots:

Types of changes

✅ Bug fix (non-breaking change which fixes an issue)
✅ New feature (non-breaking change which adds functionality)

Checklist:

✅ I have read the CONTRIBUTING document. [REQUIRED]
✅ My code follows the code style of this project. [REQUIRED]
✅ All new and existing tests passed. [REQUIRED]
✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
❌ My change requires a change to the documentation and I have updated it accordingly.
❌ I have added tests to cover my changes.

Improve and extend password generator: - Switch character group buttons 'a-z' and 'A-Z' to match group indices of algorithm - Modify algorithm for 'Pick characters from every group' to actually do what the option says - Add feature 'Minimize recurrences' (GUI option, config load/save, algorithm)

fonic · 2017-12-29T10:30:07Z

To facilitate review of the new implementation/algorithm, I added debug output using qDebug(), but while working locally, this seems be an issue for the build system. Thus, I commented out those calls for now.

I also recommend disabling the shuffle code block in PasswordGenerator.cpp to really see what the algorithm does.

droidmonkey · 2017-12-29T11:56:46Z

While you're at it, could you add an "excluded characters" text entry?

fonic · 2017-12-29T12:04:23Z

While you're at it, could you add an "excluded characters" text entry?

Gladly.

fonic · 2017-12-29T12:08:15Z

There seems to be something terribly wrong with the generator, but not related to my changes, at least not directly.

With a build of branch 'develop', after repeatedly using the generator, I end up with very high memory consumption:

That can't be right, or am I missing something?

phoerious · 2017-12-29T12:47:27Z

Looks like a memory leak.

hifi

Code needs some cleanup.

Tested the feature and it works well, could not reproduce the memory leak, though.

hifi · 2017-12-29T16:25:14Z

src/core/PasswordGenerator.cpp

@@ -20,6 +20,7 @@

 #include "crypto/Random.h"
 #include <zxcvbn.h>
+//#include <QDebug>


Please remove commented code lines from the PR. It makes it easier to review the changes when you only see what was actually removed and added and is also a requirement for merging.

hifi · 2017-12-29T16:25:49Z

src/core/PasswordGenerator.cpp


    QString password;
+    //qDebug().noquote().nospace() << "*** Begin generating password ***";


hifi · 2017-12-29T16:26:18Z

src/core/PasswordGenerator.cpp

+         * NOTE:
+         * Same as below, but more readable, just for reference
+         */
+        /*while (password.length() < m_length) {


Also don't use doc comments inside functions.

This is just a multiline comment, doc comments start with '/**'.

It's a hybrid. Don't start every line with an asterisk is what I wanted to say.

hifi · 2017-12-29T16:26:33Z

src/core/PasswordGenerator.cpp

+            int pos = randomGen()->randomUInt(groups[group].size());
+            password.append(groups[group][pos]);
+
+            //qDebug().noquote().nospace() << "i: " << i << ", group: " << group << ", pos: " << pos << ", groups[group][pos]: '" << groups[group][pos] << "', groups[group]: " << groups[group];


hifi · 2017-12-29T16:26:54Z

src/core/PasswordGenerator.cpp

+            if (m_flags & MinimizeRecurrence) {
+                groups[group].remove(pos);
+                if (groups[group].isEmpty()) {
+                    //qDebug().noquote().nospace() << "exhausted group " << group << ", resetting";


hifi · 2017-12-29T16:27:06Z

src/core/PasswordGenerator.cpp

            password.append(passwordChars[pos]);
+
+            //qDebug().noquote().nospace() << "i: " << i << ", pos: " << pos << ", passwordChars[pos]: '" << passwordChars[pos] << "', passwordChars: " << passwordChars;


hifi · 2017-12-29T16:27:15Z

src/core/PasswordGenerator.cpp

+            if (m_flags & MinimizeRecurrence) {
+                passwordChars.remove(pos);
+                if (passwordChars.isEmpty()) {
+                    //qDebug().noquote().nospace() << "exhausted passwordChars, resetting";


hifi · 2017-12-29T16:27:23Z

src/core/PasswordGenerator.cpp

        }
    }

+    //qDebug().noquote().nospace() << "*** End generating password ***";


hifi · 2017-12-29T16:30:32Z

src/core/PasswordGenerator.cpp

@@ -52,47 +53,99 @@ QString PasswordGenerator::generatePassword() const
 {
    Q_ASSERT(isValid());

-    const QVector<PasswordGroup> groups = passwordGroups();
+    const QVector<PasswordGroup> groups_original = passwordGroups();


Use camelCases for local variables.

hifi · 2017-12-29T16:32:06Z

src/core/PasswordGenerator.cpp


-    QVector<QChar> passwordChars;
+    QVector<QChar> passwordChars_original;


TheZ3ro · 2017-12-29T20:05:59Z

Add new option/feature 'Minimize recurrences': when enabled, picked characters are not considered for further picks by eliminating them from their respective group; groups are reset once exhausted

Doing this will lower the password entropy. Getting char totally randomly vs limiting some char frequency

The high memory usage maybe it's a due to zxcvbn-c, it's using trees and dijkstra's algorithm to check the password entropy
If your password is very very long I think it's fine.
Maybe we are not freeing memory after calling it

phoerious · 2017-12-29T20:18:51Z

I'm not quite comfortable with limiting recurring characters, either. Chance has no memory. Obviously, "aaaaa" is not a strong password, but on the other hand, the chance of generating it randomly is diminishingly small and in the best case as probable as generating any other 5-character sequence.

fonic · 2017-12-30T10:26:32Z

You're right, I guess I didn't think this all the way through :(

Regarding the memory leak, it's not related to the password generator, it's caused by ASAN. Without ASAN, memory consumption stays low as usual.

phoerious · 2018-01-09T10:26:52Z

src/core/PasswordGenerator.cpp

+         */
+        for (int i = 0; i < m_length; i++) {
+            int group = i % groups.size();
+            int pos = randomGen()->randomUInt(groups[group].size());


I really don't like either approach, because they both reduce entropy. In the original approach, the alphabet of the first groups.size() characters is reduced to their group, in your approach the alphabet of each character in the password is reduced to the alphabet of group i % groups.size() (which is worse).

The correct algorithm would be this (pseudo code):

if pickCharFromEveryGroup then: passwordLength = max(passwordLength, groups.size()) forceGroupAt = Map() for i in 0 -> groups.size() do: forceGroupAt.put(randInt(0, passwordLength) not in forceGroupAt, i) for i in 0 -> passwordLength do: if pickCharFromEveryGroup and i in forceGroupAt then: password.append(randCharFromGroup(forceGroupAt.get(i)) else: password.append(randCharFromSelectedGroups())

That way you guarantee at least one random character from every group at a random position.

phoerious

See code comments.

fonic · 2018-01-11T06:44:55Z

Actually, I meant to close this PR, since we've established that this is not a good idea. Just forgot to do it.

phoerious · 2018-01-11T08:31:57Z

I think it would still be a good idea to improve the "use characters from ever group" feature in the way I described above. If you feel like doing it, you're more than welcome.

fonic · 2018-01-12T09:10:14Z

@phoerious: currently no, I had some time to spare during the holidays, but now I'd rather spend the little time I have on finishing PR #1305 once it got reviewed.

fonic added 2 commits December 29, 2017 10:30

Disable debug messages (qDebug calls)

0a149bd

hifi requested changes Dec 29, 2017

View reviewed changes

phoerious reviewed Jan 9, 2018

View reviewed changes

phoerious requested changes Jan 9, 2018

View reviewed changes

phoerious closed this Jan 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve password generator, add option to minimize recurrences #1337

Improve password generator, add option to minimize recurrences #1337

fonic commented Dec 29, 2017 •

edited

Loading

fonic commented Dec 29, 2017 •

edited

Loading

droidmonkey commented Dec 29, 2017

fonic commented Dec 29, 2017

fonic commented Dec 29, 2017 •

edited

Loading

phoerious commented Dec 29, 2017

hifi left a comment

hifi Dec 29, 2017

hifi Dec 29, 2017

hifi Dec 29, 2017

phoerious Jan 9, 2018

fonic Jan 11, 2018

phoerious Jan 11, 2018

hifi Dec 29, 2017

hifi Dec 29, 2017

hifi Dec 29, 2017

hifi Dec 29, 2017

hifi Dec 29, 2017

hifi Dec 29, 2017

hifi Dec 29, 2017

TheZ3ro commented Dec 29, 2017 •

edited

Loading

phoerious commented Dec 29, 2017

fonic commented Dec 30, 2017

phoerious Jan 9, 2018 •

edited

Loading

phoerious left a comment

fonic commented Jan 11, 2018

phoerious commented Jan 11, 2018

fonic commented Jan 12, 2018 •

edited

Loading


		QString password;
		//qDebug().noquote().nospace() << "* Begin generating password *";

		password.append(passwordChars[pos]);

		//qDebug().noquote().nospace() << "i: " << i << ", pos: " << pos << ", passwordChars[pos]: '" << passwordChars[pos] << "', passwordChars: " << passwordChars;


		QVector<QChar> passwordChars;
		QVector<QChar> passwordChars_original;

Improve password generator, add option to minimize recurrences #1337

Improve password generator, add option to minimize recurrences #1337

Conversation

fonic commented Dec 29, 2017 • edited Loading

Description

Motivation

DONE

TODO

How has this been tested?

Screenshots:

Types of changes

Checklist:

fonic commented Dec 29, 2017 • edited Loading

droidmonkey commented Dec 29, 2017

fonic commented Dec 29, 2017

fonic commented Dec 29, 2017 • edited Loading

phoerious commented Dec 29, 2017

hifi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheZ3ro commented Dec 29, 2017 • edited Loading

phoerious commented Dec 29, 2017

fonic commented Dec 30, 2017

phoerious Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

phoerious left a comment

Choose a reason for hiding this comment

fonic commented Jan 11, 2018

phoerious commented Jan 11, 2018

fonic commented Jan 12, 2018 • edited Loading

fonic commented Dec 29, 2017 •

edited

Loading

fonic commented Dec 29, 2017 •

edited

Loading

fonic commented Dec 29, 2017 •

edited

Loading

TheZ3ro commented Dec 29, 2017 •

edited

Loading

phoerious Jan 9, 2018 •

edited

Loading

fonic commented Jan 12, 2018 •

edited

Loading