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

BypassTowFactor in SignInManager:ExternalLoginSignInAsync should default to False #2067

Closed
wijnsema opened this Issue Nov 14, 2018 · 25 comments

Comments

Projects
None yet
5 participants
@wijnsema

wijnsema commented Nov 14, 2018

As requested by @brockallen in #850 it is now possible to bypass 2FA in case of an external login.

I'm sure there are good reasons to bypass 2FA, however the current implementation is far to general:

  • All providers all treated equal (some providers might not even support 2FA at all)
  • All users all treated equal (users can have 2FA enabled or not)

This bypass of 2FA really needs more refinement.

What makes this really a problem is the fact that it is enabled in the UI templates!
As a developer using the template and enabling 2FA you expect 2FA to work for both local and external login!

Furthermore, the redirect to the 2FA page is missing, so changing to call to bypassTwoFactor = false results in a confusing error that the user already exists.

In my opinion the following fragment from ExternalLogin.cshtml.cs

var result = await _signInManager.ExternalLoginSignInAsync(info.LoginProvider, 
      info.ProviderKey, isPersistent: false, bypassTwoFactor: true);
if (result.Succeeded)
{
   _logger.LogInformation("{Name} logged in with {LoginProvider} provider.", 
      info.Principal.Identity.Name, info.LoginProvider);
   return LocalRedirect(returnUrl);
}

should be replaced with

var result = await _signInManager.ExternalLoginSignInAsync(info.LoginProvider, 
   info.ProviderKey, isPersistent: false, bypassTwoFactor: false);
if (result.Succeeded)
{
   _logger.LogInformation("{Name} logged in with {LoginProvider} provider.", 
      info.Principal.Identity.Name, info.LoginProvider);
   return LocalRedirect(returnUrl);
}
if (result.RequiresTwoFactor)
{
   return RedirectToPage("./LoginWith2fa", new { ReturnUrl = returnUrl });
}

If you enabled bypassTwoFactor it will still work.

@vankampenp

This comment has been minimized.

vankampenp commented Nov 15, 2018

Exactly, that is how I implemented it in my UI customization

@blowdart

This comment has been minimized.

Member

blowdart commented Nov 19, 2018

If you are using external providers we expect the 2fa enforcement to be their concern, not that of your app.

@vankampenp

This comment has been minimized.

vankampenp commented Nov 19, 2018

@blowdart
Maybe I don't understand, but the user is able to set userId and password and login. Than the user can set 2fa.
When login-in, the user is able to choose the use of their Microsoft account, Facebook, or twitter to login.
Are you saying that in that case, they should skip 2fa?

@blowdart

This comment has been minimized.

Member

blowdart commented Nov 19, 2018

2fa in that regard is for when they use the app specific username and password, not for when they authenticate via social logins.

@vankampenp

This comment has been minimized.

vankampenp commented Nov 19, 2018

There are two issues with this.
First, my app contains sensitive data, and it makes sense to use 2fa with that. Someone might have a different view on the sensitivity of their Facebook account, and not use 2fa with that. Now when you would leave the concern for 2fa enforcement to the external provider, the very fact that I have a Facebook login possibility on the site, a hacker can use the Facebook login to bypass the 2fa. In other words, if any site has social media logins, then the user needs to put 2fa on all those social media accounts to get the same protection level.

The second issue is that the code currently does not work that way.

var result = await _signInManager.ExternalLoginSignInAsync(info.LoginProvider, 
      info.ProviderKey, isPersistent: false, bypassTwoFactor: true);
if (result.Succeeded)
{
   _logger.LogInformation("{Name} logged in with {LoginProvider} provider.", 
      info.Principal.Identity.Name, info.LoginProvider);
   return LocalRedirect(returnUrl);
}

In the above code, result.Succeeded returns false if my app wants 2fa to be checked (cookie expired). So I think @wijnsema is correct to suggest that the code needs to check on result.RequiresTwoFactor.

@wijnsema

This comment has been minimized.

wijnsema commented Nov 19, 2018

If you are using external providers we expect the 2fa enforcement to be their concern, not that of your app.

From a theoretical point of view, I agree with you. In practice, with social providers like Facebook and Twitter the 2fa from the app can be a welcome extra security layer.

Like mentioned before: As a developer using Identity I was surprised to see 2fa not working with external login. But I understand your explanation.

@blowdart

This comment has been minimized.

Member

blowdart commented Nov 19, 2018

Well what if someone added 2fa to facebook already? Do people really expect two 2fa prompts? I think 4fa might be an unwelcome shock :)

@vankampenp

This comment has been minimized.

vankampenp commented Nov 19, 2018

@wijnsema

This comment has been minimized.

wijnsema commented Nov 19, 2018

Maybe not a big problem.

  1. They were probably logged in to Facebook earlier, and trusted their computer for a week or so.
  2. 2fa might not be the same for all platforms
@blowdart

This comment has been minimized.

Member

blowdart commented Nov 19, 2018

The code bug is a bug, @HaoK can you look at that?

I'd respectfully disagree on the idea that "you expect 2FA to work for both local and external login", as that's not how it's designed, and to be honest it's not something I've seen in the wild, so this would be something the templates wouldn't support without you changing the flow as you have,

@blowdart blowdart added the bug label Nov 19, 2018

@vankampenp

This comment has been minimized.

vankampenp commented Nov 19, 2018

@blowdart

This comment has been minimized.

Member

blowdart commented Nov 19, 2018

Yes, I accept its a scenario that would work for you, but templates are starter points which cover the common cases, hence saying it's not suitable for the templates themselves.

@vankampenp

This comment has been minimized.

vankampenp commented Nov 19, 2018

@vankampenp

This comment has been minimized.

vankampenp commented Nov 19, 2018

I suggest you change the template to:

var result = await _signInManager.ExternalLoginSignInAsync(info.LoginProvider, 
      info.ProviderKey, isPersistent: false, bypassTwoFactor: true);
if (result.Succeeded || result.RequiresTwoFactor)
{
   _logger.LogInformation("{Name} logged in with {LoginProvider} provider.", 
      info.Principal.Identity.Name, info.LoginProvider);
   return LocalRedirect(returnUrl);
}

Than nothing breaks

@wijnsema

This comment has been minimized.

wijnsema commented Nov 19, 2018

Indeed it's a matter of opinion, I was merely stating my expectations.

Maybe this discussion is popping up in the future and then we can look at it with a little more statistics at hand.

For now I'm OK with this, good to see the bug is being fixed!

@wijnsema

This comment has been minimized.

wijnsema commented Nov 19, 2018

@vankampenp be careful not overrate 2fa as 'prove' of a users identity. 2fa is just an extra layer. 2fa is as secure as the generated key used to set it up. If this key is leaked somehow (it is not stored encrypted in the default implementations) anybody with this key can bypass 2fa easily.

@blowdart

This comment has been minimized.

Member

blowdart commented Nov 19, 2018

True, if you really wanted proof you'd go for certificates, but even those aren't foolproof.

@vankampenp

This comment has been minimized.

vankampenp commented Nov 19, 2018

@wijnsema thanks for the tip!. If someone gets the contents of my database, I have a whole other problem. In the end nothing is secure.

But it helps me to think that this extra layer prevents someone to guess a password, or reuse a password written on paper or something like that. At least the hacker will need access to the mobile phone as well (or my database, but than they are no longer interested in bypassing the 2fa).

@HaoK

This comment has been minimized.

Member

HaoK commented Nov 19, 2018

@blowdart what code bug are you specifically referring to in here, there's a lot of comments and its not clear to me what behavior is the bug

@blowdart

This comment has been minimized.

Member

blowdart commented Nov 20, 2018

The bit where people say there's a bug :D

var result = await _signInManager.ExternalLoginSignInAsync(info.LoginProvider, 
      info.ProviderKey, isPersistent: false, bypassTwoFactor: true);
if (result.Succeeded)
{
   _logger.LogInformation("{Name} logged in with {LoginProvider} provider.", 
      info.Principal.Identity.Name, info.LoginProvider);
   return LocalRedirect(returnUrl);
}

In the above code, result.Succeeded returns false if my app wants 2fa to be checked (cookie expired). So I think @wijnsema is correct to suggest that the code needs to check on result.RequiresTwoFactor.

@wijnsema

This comment has been minimized.

wijnsema commented Nov 20, 2018

@HaoK I'm not sure this is actually a bug. Maybe @vankampenp can clarify.

From my perspective:
I wanted to change the behavior of the app to require 2fa for external login providers. I changed the ExternalLoginSignInAsync call to bypassTwoFactor: false. But then result.Succeeded is false while result.RequiresTwoFactor is true.

The code then tries to register a new user, which fails because the user already exists!

If you add

if (result.RequiresTwoFactor)
{
   return RedirectToPage("./LoginWith2fa", new { ReturnUrl = returnUrl });
}

just like in Login.cshtml.cs you are redirected to the 2fa page as expected.

If you have bypassTwoFactor = true these lines have no influence.

So yes, you could call this dead code, but it is very convenient for those who change the bypassTwoFactor while not harming those who leave it as is.

@HaoK

This comment has been minimized.

Member

HaoK commented Nov 20, 2018

Yeah that's what I thought, its not really a bug, if this code was still in the template, it makes more sense to change, but since users will need to scaffold and modify the code anyways, they can add that additional 2fa redirect logic after scaffolding, I'm not sure it really make sense for us to add dead code in the library.

@ajcvickers @blowdart thoughts?

@wijnsema

This comment has been minimized.

wijnsema commented Nov 20, 2018

Taking one step back, it wouldn't be dead code if the default would be to have 2fa for external logins...

@ajcvickers

This comment has been minimized.

Member

ajcvickers commented Nov 26, 2018

@HaoK Even though the code is not in the template, given that we tell people to scaffold the code into their applications, it effectively becomes the same as code in the template. Therefore I think we should fix this. 3.0 is fine.

@blowdart blowdart added Features and removed bug labels Nov 29, 2018

@blowdart

This comment has been minimized.

Member

blowdart commented Nov 29, 2018

Leaning towards this not being a bug, we don't want two two factor challenges. Scaffolding out and changing the code is the way to go.

@blowdart blowdart closed this Nov 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment