search

pyauth / pyotp

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

Allow passing secrets as bytes #138

Closed peterthomassen closed 2 years ago

peterthomassen commented 2 years ago

When the storage backend has the secrets as bytes, it is unnecessary to convert to string for passing into pyotp, which converts it back to bytes to compute the digest.

This PR allows passing the secret directly as bytes, and makes some related code adjustments. It does so by coercing the type as needed. (I also considered storing only bytes in self.secret, but rejected the idea as it would break uses that write it directly.)

kislyuk commented 2 years ago

Thanks for your contribution. This PR will not be accepted in its current form, because of the potential bugs (which can escalate into security issues) associated with accepting secrets in different formats through the same interface.

I am open to renaming s to secret: Optional[str] and introducing a new keyword argument, secret_bytes: bytes.

peterthomassen commented 2 years ago

The solution proposed in the PR is fully (!) backwards-compatible. How could that cause any security or non-security issue?

kislyuk commented 2 years ago

I am not concerned about backwards compatibility but about ergonomics/usability.

Accepting the secret as either string or bytes, and treating it differently as a result, can confuse the implementer into passing the secret as bytes in its base32 encoded form, confusing them further and reducing the entropy of the resulting HMAC input space.

This did highlight to me that we don't check for minimum secret length (128 bytes per the RFC) nor minimum digits (6 per the RFC) so I'm going to implement that.

kislyuk commented 2 years ago

Thanks again for your contribution, please let me know if you're interested in updating it based on the changes requested and we can reopen