codebude / QRCoder

A pure C# Open Source QR Code implementation
MIT License
4.56k stars 1.09k forks source link

OTP uri may not be formatted correctly #99

Closed tilkinsc closed 6 years ago

tilkinsc commented 6 years ago

https://github.com/OTPLibraries/CSOTP/blob/master/CSOTP.cs

See this for a full URI on line 303

otpauth://totp/na%23me1:account%40whatever1.com?secret=JBSWY3DPEHPK3PXP&issuer=na%23me1&algorithm=SHA1&digits=6&period=30

Where inputs are "na#me1" for cissuer.

Make sure it comes out to this, as these are all the required fields of google authentication, however integration of CSOTP could go into QRCoder since all the data is needed anyways. This goes back to issue #72 of my reign of supporting TOTP and HOTP in too many languages ( see http://github.com/OTPLibraries )

I also noted that you put HMACToString or something instead of HOTPToString and you might want to change TimeToString to TOTPToString

of course the state of my libraries is in great need of optimization, but I chose to go with supporting a lot of common languages first.

codebude commented 6 years ago

Hi @tilkinsc

could you please explain what you mean with your post or what you want me to do? It's not clear.

I already encode cissuer, label, etc. with Uri.EscapeUriString(). I'm quite sure that the Uri.EscapeDataString()-function in the linked code file isn't correct, because technically the otpauth://... is an Uri and should be encoded with the EscapeUri-function. (More info: https://stackoverflow.com/questions/4396598/whats-the-difference-between-escapeuristring-and-escapedatastring)

The only other difference between the current implementation and you sample url/outcome is the part around the parameters &algorithm=SHA1&digits=6.

During implementation I stayed near the Google's documentation (https://github.com/google/google-authenticator/wiki/Key-Uri-Format) which says that algorithm is optional and digits only has to be set, when the value differs from 6/8.

With this points in mind, I'm unsure what exactly this issue is about. So please help to clear things up.

tilkinsc commented 6 years ago

Google authentication is deprecated, see the RFC's

codebude commented 6 years ago

Can you give me links where Google Authenticator is marked as outdated and one with the specification of the followup standard?

tilkinsc commented 6 years ago

example 1
example 2
example 3
example 4

(TOTP) RFC 6230
(HOTP) RFC 4226

Everyone has moved onto using Authy who is actually active in the development and support more things than Google Authentication ever will. They actually outline it here. Authenticator, also, was too pedantic. Please base all future work off the RFCs.

I brought this up because the 'payload' integration with a pull request I am to make, where verbosity is a must. So it must be checked and ensured the payload generator, given the information, behaves properly and is error checked on things like accepting non-base32 secret keys and the like.

codebude commented 6 years ago

Hi Cody,

I still don't get what you want from me/what you want to changed... The four "example" links haven't to do anything with the PayloadGenerator class of this project. Besides of that I can't see that "everyone" has moved to authy. (Especially because they store your data in the cloud - which may be security risk, too.) But all of that has nothing to do with the QRCoder.

The linked RFCs are about the implementation of TOTP/HOTP, but this isn't a part of QRCoder. The only thing QRCoder does, is building an URI-scheme which is passed to the QR code generator. The payload generator is a "helper" - it just formats the string correctly. It's not a TOTP/HOTP tool.

So at the current state I just can't see, what's wrong with the URI format, generated by QRCoder's payload generator.

tilkinsc commented 6 years ago

Oh this was me asking you to check through it.