search

pyauth / pyotp

Python One-Time Password Library
https://pyauth.github.io/pyotp/
Other
3.03k stars 326 forks source link

feat: allow lowercase algorithms #169

Closed chkpwd closed 2 weeks ago

kislyuk commented 3 months ago

Specifying algorithm in lowercase is not documented in existing docs about the otpauth URI scheme. Could you give us an idea of which implementations do this?

chkpwd commented 3 months ago

Ente, for example, exports otpauth URI with a lowercase algorithm.

Sample Secret:

otpauth://totp/test1:chkpwd?algorithm=sha1&digits=6&issuer=test1&period=30&secret=FFFFFFF&codeDisplay=00000
chkpwd commented 2 weeks ago

Any chance you can have a look at this?

kislyuk commented 2 weeks ago

Sorry, I don't think I can accept this PR.

Lowercase algorithm names are not supported by Google Authenticator, which is the original TOTP authenticator app and remains the de facto standard. Google Authenticator rejects otpauth URIs with lowercase algorithm values and so we will reject them, too. Applications that produce such URIs should be fixed.

kislyuk commented 2 weeks ago

(And apologies for the delay in reviewing this)

tigattack commented 2 weeks ago

I understand the desire to maintain compatibility with Google Authenticator, but is strict case sensitivity on the algorithm name essential? Many other TOTP applications are flexible on this point, and enforcing uppercase-only algorithm names will most likely reduce interoperability for minimal benefit.

Since lowercase algorithm names are a common, harmless deviation, my take is that supporting them would improve the usability of the library without any impact on security or functionality.

If strict adherence to Google Authenticator’s interpretation of the otpauth URI is important, a note about this restriction in the documentation could help set expectations for users in clarifying that the library strictly follows Google’s version of the scheme.

I would also note that RFC 3986 section 6.2.2.1 states that "generic syntax components are assumed to be case-sensitive", with some noted exceptions. However I'll be the first to admit that this is rather weak given that the RFC isn't specific to the otpauth scheme and serves little more than a suggestion on this point.

Finally, it felt noteworthy that in Google's own documentation (albeit now archived...) they state that "Currently, the algorithm parameter is ignored by the Google Authenticator implementations", which, IMO, further reduces the importance of adhering to their spec in this specific case.

rippleFCL commented 2 weeks ago

GA came first, true. but since there is not a standard, except a lose one roughly adhered too, and this project generalizes and never once mentions only supporting GA's schema. wouldn't it make sense to add more support for slightly differing schemas? if not maby add to the readme to clarify this library is designed to only to handle GA

kislyuk commented 2 weeks ago

I understand your arguments, but I remain unconvinced that this is a good idea.

The algorithm parameters are not ignored by the latest version of Google Authenticator: uppercase values are accepted and lowercase ones are not.

This project definitely supports authenticator apps other than Google Authenticator, and we will continue doing so. The decision to not support lowercase algorithms is about not introducing new incompatibilities into the ecosystem. At this time, the apps that generate otpauth URIs with lowercase algorithm parameters are not compatible with Google Authenticator. If we accommodate those apps, we encourage this fragmentation. The apps should fix their code instead.

chkpwd commented 2 weeks ago

Thank you for at least hearing us out. I'll go ahead and close this PR.