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

generate: Sometimes generates longer passwords than asked. #52

Closed
dmitshur opened this Issue Dec 8, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@dmitshur
Copy link
Contributor

dmitshur commented Dec 8, 2018

passgo generate help says:

passgo generate length=24
	Prints a randomly generated password. The length of this password defaults
	to 24. If a very short length is specified, the generated password will be
	longer than desired and will contain a upper-case, lower-case, symbol, and
	digit.

I've noticed that running passgo generate sometimes generates passwords with greater than 24 length. I've gotten: 25, 32, 37, 39.

It happens with the default passgo generate invocation, but also for other values of length. E.g., when I ran passgo generate 20 a bunch of times, I got lengths like: 24, 25, 32.

To reproduce, run go generate at least 20-30 times.

Is this a bug or intentional behavior which isn't documented?

@dmitshur

This comment has been minimized.

Copy link
Contributor

dmitshur commented Dec 8, 2018

After reading the pc.GeneratePassword code, I suspect I understand what's happening.

It keeps appending to the password until it reaches the requested length, and also has at least one of all the required character types:

// It requires generation of a string password that has a upper case
// letter, a lower case letter, a symbol, and a number.

So in some rare cases, a password of length 24 doesn't yet satisfy all those requirements, so it becomes longer.

Ironically, this is documented here:

If a very short length is specified, the generated password will be
longer than desired and will contain a upper-case, lower-case, symbol, and
digit.

I just didn't think that applied because I didn't consider 24 to be a "very short length". Maybe it'd be good to rephrase it as something like:

The generated password will contain a upper-case, lower-case, symbol,
and digit, and may end up longer than requested in order to accommodate that.
@ejcx

This comment has been minimized.

Copy link
Owner

ejcx commented Jan 3, 2019

Yeah @dmitshur, it totally is documented but at the time I didn't really know what to do, and didn't want to focus on the issue =]. Maybe I could try generating more than once? Perhaps after 100 generations if it doesn't find a password that meets the requirements, it could return a longer one.

@ejcx

This comment has been minimized.

Copy link
Owner

ejcx commented Jan 3, 2019

I also really hate that GeneratePassword function I wrote.

@dmitshur

This comment has been minimized.

Copy link
Contributor

dmitshur commented Jan 3, 2019

Understandable.

I think rephrasing the documentation as I suggested at the bottom of #52 (comment) would be helpful.

Trying more than once isn't a bad idea, I think. Generating passwords is usually done relatively rarely, and isn't the bottleneck. I think you could consider having an upper bound on the time taken, not just iterations. For example, loop as long as 500 milliseconds have not yet passed. If passgo generate takes half a second occasionally, I don't think it'd be very noticeable. As long as people use it occasionally and not in bulk.

In general, this isn't a big problem. I mostly filed the issue because I was afraid this might be indicative of a serious bug, but it turned out to be the result of an intentional design decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment