search

smallstep / cli

🧰 A zero trust swiss army knife for working with X509, OAuth, JWT, OATH OTP, etc.
https://smallstep.com/cli
Apache License 2.0
3.56k stars 248 forks source link

Add support for HOTP #492

Open jgiannuzzi opened 3 years ago

jgiannuzzi commented 3 years ago

What would you like to be added

The crypto otp command advertises support for both TOTP and HOTP, but it turns out only TOTP is implemented. The underlying library (github.com/pquerna/otp) support HOTP with a very similar interface.

Why this is needed

Because support for it is advertised, and because actually supporting it would allow to generate and validate many software and hardware HOTP tokens.

jgiannuzzi commented 3 years ago

I would be very happy to implement this feature myself 😄 I just need to know how we should structure the generate and verify commands to differentiate between TOTP and HOTP.

Should there be a new argument --hotp? Should there instead be totp and hotp subcommands? 🤷

For the generate command, --period only makes sense for TOTP. Maybe specifying --period 0 would mean HOTP? For the verify command, --period, --skew, and --time only make sense for TOTP, and a new argument --counter would be required for HOTP. Maybe specifying --counter would mean HOTP?

I am not sure what the best UX would be, please advise!

maraino commented 3 years ago

I'm not very familiar with that part of the code it was initially implemented by @mmalone and then I've reorganized a bit. I think to use --hopt or --type [hopt|topt] makes the more sense.

What do you think @mmalone?

jgiannuzzi commented 3 years ago

I like the idea of --type [hotp|totp], especially if we make it default to totp for backwards compatibility.

Thanks, @maraino!

mmalone commented 3 years ago

Oh, man. Honestly, this is a corner of the codebase that I haven't looked at in a long time and I'm glad to see that someone is getting value out of it. I'm generally biased towards being explicit and maintaining backwards compatibility, so I think that would mean an explicit --type [hotp|totp] with totp as the default. Either way this is gonna make a bit of a mess in documentation and input validation since there are a bunch of flags that are TOTP or HOTP specific. But I think I can live with that and it's better than the alternatives.

I'd love to see this implemented. Thanks in advance!