On 12/5/20 10:23 AM, Moritz Röhrich wrote: > Dear Mr. Landley, > > I have implemented `pwgen`, a small password generation utility for toybox. > This utility is useful, not only for passwords but also for generating > random strings in general. > > Hopefully the code meets quality expectations, but I'd be more than happy to > take criticism. > > Best regards, Moritz
Hmmm, hadn't heard of this before but: https://linux.die.net/man/1/pwgen Can't argue. Your implementation is small and looks sane. Ok, applied to pending. As for cleanup, toys.optflags & FLAG(A) is unnecessary: the FLAG(A) macro expands to (toys.optflags & FLAG_A). I inlined get_rand_chr() into the one call site so the two calls just become "while (strchr(illegal, c=blah));" and then I can inline THAT at its one call site, ala: for (i = 0; i<l; i++) while (strchr(illegal, pw[i] = 33+(rand()%93))); And then inline THAT at its one and only call site... The rand() function does not spark joy, I have xgetrandom() for a reason. Why does the optstr have ? to pass through unknown arguments? What does that accomplish? (It lets you pass negative values in for the two numeric arguments, neither of which makes sense as a negative number...) get_illegal_chars() is only called in one place but then the resulting variable is only USED in one place, so the tests might as well just be in that place? (This nicely glosses over the way it's allocating 94 chars and then strcat() a user-supplied string to that buffer with no bounds checking: let's not do that.) You generate an array of passwords and then output them with two for() loops, but each is only used exactly once. Why not just generate one at a time in the for loop and reuse the same buffer? In fact I have a 4k toybuf and if I just cap the supported password size at 4k I don't have to allocate or free anything. I installed the other pwgen to see what it does, and yes when run with no arguments it outputs a zillion passwords. And then when I run that output through "wc" to see how many columns and lines of output it's generating, it produces ONE password. (Because why NOT have different behavior depending on whether or not the output is to a tty? Ah, the man page says it's an anti shoulder surfing measure. Makes sense. Also, "as secure as completely completely random" is a duplicated word in the man page. Anyway, I should update our help text to explain WHY it's doing that...) It's hardwiring columns to 80 but we have detection plumbing. (Hmmm, the previous one wasn't going right up to the right edge and the one I just wrote is. Aesthetically tweaking it to produce one less password per line match the other one...) The FLAG(y) string is possibly easier to understand as: if ((c>'9' && c<'A') || (c>'Z' && c<'a') || c>'z') (Although the order the string is stated in doesn't match up with the ascii table and I had to hunt and peck to prove it.) Hmmm, this is producing a LOT more capital letters than the other version, which also falls under "human readable affordance". let's see... Top bit of entropy per byte isn't really used, so I'll squelch capitals when it's set. (That should make 1/4 of letters capital.) The other one doesn't do a page full for "pwgen -1", fixed... Hmmm, $ toybox pwgen -y p:Q1$h=C h6W`ieZ< Q`o!b|+) 1apBp}nT er@7mKgi waAqC[7i v<y\:jzt [#o=Nw7w tx1^1Uo[ o`B]y84{ wjdsl>%n R=<h[*0" #m*+(z!( qbZf,3h) fs&oc1C0 `?#-sstC r`mR{ht{ i%g'FA$> ofy=#t}7 rCRWEmlq 7A;/`|}= rvqv|swe wT\z-(sw ,Cr*y6c. $ pwgen -y Eegae:B9 pee3Boh{ Hie~j3Lu aew)a3Jo zae'Cho5 quah!Ph5 EJa(X5Ee zui7Aez) Too2Ed)o kap.ae4L ahj$i8Se Aile-ch4 nah+w3Ea wa"Zo9ea Shu4dae+ tuNg]u7e giY!oc9o duG5eiz- sahc7eS* ooPi@z0e eX7nei_d iV/ae1se eiQu4om^ Ni>pig1o That's still a very different character distribution. He's squelching more capitals than I am, and at least half the punctuation... ah, that's not what he's doing. When you request punctuation you get ONE instance of punctuation, and one, or less often two, or almost never three capital letters... this is a curve isn't it? $ pwgen 8 1000 | sed 's/[^A-Z]//g' | while read i; do echo $i | wc -c; done | sort | uniq -c 754 2 217 3 25 4 4 5 Yes, that is DEFINITELY intentional. Right. And what I've got so far is just doing: $ ./pwgen 8 1000 | sed 's/[^A-Z]//g' | while read i; do echo $i | wc -c; done | sort | uniq -c 188 1 376 2 293 3 107 4 31 5 5 6 Which is... eh? Close enough? The other one is guaranteeing that each password has at least one number, and with -y adds ONE instance of punctuation to each password. That means every generated passwords can comply with a site policies, which is not how this is designed but I'm not redesigning it just now... Rob _______________________________________________ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net