Allow Rfc6238AuthenticationService time-step size and time-step window to be configurable

Hello,

I have some security concerns regarding the default value of 3 minutes for time-step. It seems very long.

aspnetcore/src/Identity/Extensions.Core/src/Rfc6238AuthenticationService.cs

Line 14 in ce16ff0

private static readonly TimeSpan _timestep = TimeSpan.FromMinutes(3);

In addition allowing variance of two steps ahead and behind make the window even larger.

aspnetcore/src/Identity/Extensions.Core/src/Rfc6238AuthenticationService.cs

Lines 101 to 113 in ce16ff0

	// Allow a variance of no greater than 9 minutes in either direction
	var currentTimeStep = GetCurrentTimeStepNumber();
	using (var hashAlgorithm = new HMACSHA1(securityToken))
	{
	for (var i = -2; i <= 2; i++)
	{
	var computedTotp = ComputeTotp(hashAlgorithm, (ulong)((long)currentTimeStep + i), modifier);
	if (computedTotp == code)
	{
	return true;
	}
	}
	}

RFC recommends a time-step size of 30 seconds and a time-step window of less than 10 minutes.

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

After analysis, it would seem that the values defined in the Rfc6238AuthenticationService are mainly used to generate tokens sent by SMS (PhoneNumberTokenProvider)

The values to generate time-based one-time password used for authenticator code verification (Like Google or Microsoft Authenticator) are redefined in the AuthenticatorTokenProvider class.
These values appear consistent with the recommendations described in the RFC.

aspnetcore/src/Identity/Extensions.Core/src/AuthenticatorTokenProvider.cs

Lines 57 to 68 in 65e933e

	var hash = new HMACSHA1(Base32.FromBase32(key));
	var unixTimestamp = Convert.ToInt64(Math.Round((DateTime.UtcNow - new DateTime(1970, 1, 1, 0, 0, 0)).TotalSeconds));
	var timestep = Convert.ToInt64(unixTimestamp / 30);
	// Allow codes from 90s in each direction (we could make this configurable?)
	for (int i = -2; i <= 2; i++)
	{
	var expectedCode = Rfc6238AuthenticationService.ComputeTotp(hash, (ulong)(timestep + i), modifier: null);
	if (expectedCode == code)
	{
	return true;
	}
	}

It seems to be a good idea to make the values defined in the Rfc6238AuthenticationService class configurable to:

• Avoid duplicating part of the algorithm in the AuthenticatorTokenProvider class.
• Have the possibility to modify the lifetime of the tokens sent by SMS

[idimakosprofilesw]

Hello, what would be the steps to help bring this forward?
We are interested in having this in the next LTS.

Could you provide some guidance in terms of the way this would need to be configurable?

cc @mkArtakMSFT @davidfowl

[davidfowl]

cc @javiercn @blowdart

[blowdart]

We haven't been prepared to make this a more public api due to support concerns. This hasn't changed. Unless @mkArtakMSFT wants to take the dev burden it will remain not public.

[davidfowl]

@idimakosprofilesw Can you make a case for why it should be public? What would it help in your scenario? That hasn't been clearly expressed in this issue as yet.

[idimakosprofilesw]

@davidfowl thank you for your time.

We are using PhoneNumberTokenProvider from AspNetIdentity to generate SMS OTP codes for two factor authentication for the end users of our application.
An example repo of a similar approach can be found here: https://github.com/Jurabek/IdentityServer4.PhoneNumberAuth/blob/master/IdentityServer4.PhoneNumberAuth/Controllers/VerifyPhoneNumberController.cs#L107

However, the API enforces:
9 minutes expiry time
3 minutes of time step

In our business (mobile banking), these time windows are too large.
More sensible for us would be 2 minutes expiry and 1 minute time step.

These values are hardcoded into the source of AspNetIdentity, and there is no clear way on how to extend them other than to fork these services (or to workaround it in the application layer)
So, before forking, we thought to get in touch with you first to see if there is a way to contribute.

[davidfowl]

@blowdart this seems reasonable. Is there a reason not to expose these settings? (other than the usual, due diligence we do when we expose APIs?)

[blowdart]

It's an internal class. We feel the settings are correct for our use.

We already have di for custom implementations, and we don't want to support our implementation outside of our use (and it would probably need to more to CoreFx to be correctly positioned, which is another burden)

[davidfowl]

Is @idimakosprofilesw 's scenario not something that we support or intend to support ?