search

xlzd / gotp

Golang OTP(One-Time Password) Library.
MIT License
468 stars 67 forks source link

Permit a more full range of chars to be used in 'RandomSecrets'. #9

Closed morrowc closed 4 years ago

morrowc commented 4 years ago

Pull request #8 - adds lowercase, numbers and special characters to the set permitted for use in RandomSecret().

mergenchik commented 4 years ago

Initial alphabet is Base32 alphabet, which has some advantages, especially if secret is being used in URL which is a case. Maybe you can implement an additional method and with some parameter in code random number will be generated by using your implementation.

morrowc commented 4 years ago

A secret stuffed into a URL should almost certainly not be in the clear... so I think you mean: "Please provide a wrapper function to RandomSecret which Base32's (or url-safe-encodes) the resulting secret"

correct?

morrowc commented 4 years ago

This is done, btw.

mergenchik commented 4 years ago

Sorry, could not correctly explain what I mean. IMHO it would be better not changing existing RandomSecret method but implement a new one, giving it a different name. Since it can be used in other projects and that secret generated by that method is used in ProvisioningUri (sorry for my mistake, not URL but URI in my initial comment).

morrowc commented 4 years ago

It looks like, according to sourcegraph's view: https://sourcegraph.com/github.com/xlzd/gotp@master/-/blob/utils.go#L79:6&tab=references

the only user of this function is ... this code. I think it's safe to just make this better. Leaving it as is makes the possible passwds created pretty horrificly bad, so why don't we just make the result better, eh? :)

mergenchik commented 4 years ago

this usage only includes projects which source is publicly available.

So, actually there is a key specification which says it should be Base32 encoded, to work with Google Authenticator.

I think it is same to generate random byte array and then encode it with Base32 and generate random string of Base32 alphabet.

morrowc commented 4 years ago

So, it looks, to me, like the function built: RandomSecret()

before I looked/changed things makes a passwd with at most 32 possible options per char returned. That's not really great :(

if you're argument is that: 1) existing people may be using this - Great, let's get them some better security. 2) the use-cases may be to form into QRCodes/etc so we need base32 - ok, url-escape or just added Base32 should be fine 3) I think providing bad passwds as part of infrastructure software is a bad plan, let's make this better.

mergenchik commented 4 years ago

If we will solve problem with 2. then 1. and 3. are OK.

about 2., in current implementation, since at the end, whatever we do will be random string of alphabet used in base32, which if decoded back maps to byte array without restrictions (any byte from 0x00 to 0xFF) we have more randomness per character in final string than generating random string of alphabet of 82 chars (10 + 26 + 26 + 20) and then getting base32 encoding of it.

For example, I think it is the same:

could I explain my point?

mergenchik commented 4 years ago

Thank you for your interest I am closing this issue Chris (@morrowc ). Please feel free to leave more comments if you have something more.