vt-middleware / passay

Password policy enforcement for Java.
http://www.passay.org
Other
281 stars 64 forks source link

Password generator does not always generate valid password. #152

Closed smiklosovic closed 2 months ago

smiklosovic commented 3 months ago

@serac @dfish3r

I have a password validator which also includes a rule like IllegalSequenceRule. I have also a generator but PasswordGenerator is taking into account only CharacterRule-s.

So what might happen is that a password is generated with exactly same character rules as my validator is using, but it might happen that it will create a password which also includes illegal sequence. So what I generate with the character rules is just not valid when I test it with my validator which uses same character rules (plus IllegalSequenceRule).

This is quite hard to detect in practice but a simple test which generates e.g. 1 million passwords yields some passwords (like ... couple hundreds, it really depends) which will not pass the validator.

In my software, I can not simply allow this to happen. What I am doing is that when I generate a password, I will run it via a validator and if it passes I return it to a user, but when it fails, I just try to create a new one. I am basically running it in a loop until a password is generated in such a way that it will pass the validator.

Now, in great majority of cases, a valid password will be generated just on the first attempt but this is just annoying stuff to deal with.

MV:@@kjihG2]MZ8w{zs{

but you see that Ghijk (reversed kjihG) is illegal because it is a sequence.

serac commented 3 months ago

What happens if you add your illegal sequence rules to PasswordGenerator?

smiklosovic commented 3 months ago

@serac I can't. Because PasswordGenerator has public String generatePassword(final int length, final List<CharacterRule> rules).

But IllegalSequenceRule does not implement CharacterRule, just Rule.

serac commented 2 months ago

I believe we can make this work with an API change. I'll experiment and follow up.

serac commented 2 months ago

@smiklosovic I believe the PR cited above will resolve your problem. Turned out to be a pretty clean patch IMO.