-
Notifications
You must be signed in to change notification settings - Fork 128
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added support for HMACSHA 256 and 512 #198
Conversation
Also important to note: in my tests of apps using SHA256 Authy and Microsoft Authenticator did not work with non-SHA1 codes but LastPass Authenticator and Google Authenticator both worked. SHA256 I haven't tried others as yet. |
I think we should make it so, if you do not specify the algo, then nothing changes (incl the output). That way it is not a breaking change: anyone upgrading won't see any change. And, as you say, we then just need to add some test with the new behaviour (but existing tests should be left unchanged to confirm we are not changing behaviour). |
@flytzen I think it's all set. Have a look and feel free to make any changes. I do think we'll want to add a note to the README before pushing a new release to nuget, but figured that could be part of the PR to update the version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small suggestion about the constructor. Otherwise love it ❤️
@@ -21,7 +21,13 @@ public class TwoFactorAuthenticator | |||
|
|||
private TimeSpan DefaultClockDriftTolerance { get; set; } | |||
|
|||
public TwoFactorAuthenticator() => DefaultClockDriftTolerance = TimeSpan.FromMinutes(5); | |||
private HashType HashType { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a small point, but I think there is something about binary compatibility with optional params. I.e. if someone replaces the binary for GoogleAuthenticator
but does not recompile, then I think it will break for them. I seem to recall an issue we had with this years ago.
Ideally, we should keep the original constructor with no params and add an additional constructor that takes a HashType
, if we want to be 100% sure we don't break anyone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call.
Co-authored-by: Frans Lytzen <[email protected]>
Adds support for specifying the algorithm (SHA1, SHA256, or SHA512) per the RFC
TODO: update tests to check all three algorithms
Is this technically a breaking change because if the server is configured for SHA256 or SHA512 but the existing clients are using SHA1 they won't be able to authenticate? I kinda think that's the responsibility of the server.
Resolves #197
/cc @flytzen @BrandonPotter