-
Notifications
You must be signed in to change notification settings - Fork 645
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
[2FA] Add AAD login support for NuGet Gallery #5257
Changes from 18 commits
00ba375
140939f
68dcaf2
39f6118
35e9022
01b59e4
879430d
b0df1fd
c5d8efa
972d16c
a35a4be
908ae1c
426c829
77786db
e32e8bd
2a1f33c
c690735
7402572
ac35d7d
68562ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ | |
using Microsoft.Owin; | ||
using NuGetGallery.Auditing; | ||
using NuGetGallery.Authentication.Providers; | ||
using NuGetGallery.Authentication.Providers.CommonAuth; | ||
using NuGetGallery.Configuration; | ||
using NuGetGallery.Diagnostics; | ||
using NuGetGallery.Infrastructure.Authentication; | ||
|
@@ -22,8 +23,6 @@ namespace NuGetGallery.Authentication | |
{ | ||
public class AuthenticationService | ||
{ | ||
private const string tenantIdClaimType = "http://schemas.microsoft.com/identity/claims/tenantid"; | ||
|
||
private Dictionary<string, Func<string, string>> _credentialFormatters; | ||
private readonly IDiagnosticsSource _trace; | ||
private readonly IAppConfiguration _config; | ||
|
@@ -83,7 +82,7 @@ public virtual async Task<PasswordAuthenticationResult> Authenticate(string user | |
if (user == null) | ||
{ | ||
_trace.Information("No such user: " + userNameOrEmail); | ||
|
||
await Auditing.SaveAuditRecordAsync( | ||
new FailedAuthenticatedOperationAuditRecord( | ||
userNameOrEmail, AuditedAuthenticatedOperationAction.FailedLoginNoSuchUser)); | ||
|
@@ -243,7 +242,7 @@ public virtual async Task CreateSessionAsync(IOwinContext owinContext, Authentic | |
// Issue the session token and clean up the external token if present | ||
owinContext.Authentication.SignIn(identity); | ||
owinContext.Authentication.SignOut(AuthenticationTypes.External); | ||
|
||
// Write an audit record | ||
await Auditing.SaveAuditRecordAsync( | ||
new UserAuditRecord(user.User, AuditedUserAction.Login, user.CredentialUsed)); | ||
|
@@ -459,7 +458,7 @@ public virtual ActionResult Challenge(string providerName, string redirectUrl) | |
|
||
public virtual async Task AddCredential(User user, Credential credential) | ||
{ | ||
if (user is Organization) | ||
if (user is Organization) | ||
{ | ||
throw new InvalidOperationException(Strings.OrganizationsCannotCreateCredentials); | ||
} | ||
|
@@ -474,12 +473,21 @@ public virtual async Task AddCredential(User user, Credential credential) | |
public virtual CredentialViewModel DescribeCredential(Credential credential) | ||
{ | ||
var kind = GetCredentialKind(credential.Type); | ||
Authenticator auther = null; | ||
Authenticator author = null; | ||
|
||
if (kind == CredentialKind.External) | ||
{ | ||
string providerName = credential.Type.Split('.')[1]; | ||
Authenticators.TryGetValue(providerName, out auther); | ||
if (string.IsNullOrEmpty(credential.TenantId)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, we'll switch over to the new APIs (OpenIdConnect) once the SQL is run to populate tenantId for existing credentials? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, and after we enable the config values for new provider. |
||
{ | ||
string providerName = credential.Type.Split('.')[1]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will the providerName always be the second ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. |
||
Authenticators.TryGetValue(providerName, out author); | ||
} | ||
else | ||
{ | ||
author = Authenticators | ||
.Values | ||
.First(provider => provider.Name.Equals(CommonAuthAuthenticator.DefaultAuthenticationType, StringComparison.OrdinalIgnoreCase)); | ||
} | ||
} | ||
|
||
var credentialViewModel = new CredentialViewModel | ||
|
@@ -491,9 +499,9 @@ public virtual CredentialViewModel DescribeCredential(Credential credential) | |
Created = credential.Created, | ||
Expires = credential.Expires, | ||
Kind = kind, | ||
AuthUI = auther?.GetUI(), | ||
AuthUI = author?.GetUI(), | ||
// Set the description as the value for legacy API keys | ||
Description = credential.Description, | ||
Description = credential.Description, | ||
Value = kind == CredentialKind.Token && credential.Description == null ? credential.Value : null, | ||
Scopes = credential.Scopes.Select(s => new ScopeViewModel( | ||
s.Owner?.Username ?? credential.User.Username, | ||
|
@@ -538,51 +546,47 @@ public virtual async Task EditCredentialScopes(User user, Credential cred, IColl | |
public virtual async Task<AuthenticateExternalLoginResult> ReadExternalLoginCredential(IOwinContext context) | ||
{ | ||
var result = await context.Authentication.AuthenticateAsync(AuthenticationTypes.External); | ||
if (result == null) | ||
if (result?.Identity?.Claims == null) | ||
{ | ||
_trace.Information("No external login found."); | ||
return new AuthenticateExternalLoginResult(); | ||
} | ||
var idClaim = result.Identity.FindFirst(ClaimTypes.NameIdentifier); | ||
if (idClaim == null) | ||
|
||
var externalIdentity = result.Identity; | ||
Authenticator author = null; | ||
foreach (var authenticator in Authenticators.Values) | ||
{ | ||
_trace.Error("External Authentication is missing required claim: " + ClaimTypes.NameIdentifier); | ||
return new AuthenticateExternalLoginResult(); | ||
if (authenticator.IsAuthorForIdentity(externalIdentity)) | ||
{ | ||
author = authenticator; | ||
break; | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can retwrite this using FirstOrDefault |
||
|
||
var nameClaim = result.Identity.FindFirst(ClaimTypes.Name); | ||
if (nameClaim == null) | ||
if (author == null) | ||
{ | ||
_trace.Error("External Authentication is missing required claim: " + ClaimTypes.Name); | ||
_trace.Error($"No authenticator found for identity: {externalIdentity.AuthenticationType}"); | ||
return new AuthenticateExternalLoginResult(); | ||
} | ||
|
||
var emailClaim = result.Identity.FindFirst(ClaimTypes.Email); | ||
string emailSuffix = emailClaim == null ? String.Empty : (" <" + emailClaim.Value + ">"); | ||
|
||
var tenantIdClaim = result.Identity.FindFirst(tenantIdClaimType); | ||
|
||
Authenticator auther; | ||
string authenticationType = idClaim.Issuer; | ||
if (!Authenticators.TryGetValue(idClaim.Issuer, out auther)) | ||
try | ||
{ | ||
foreach (var authenticator in Authenticators.Values) | ||
var userInfo = author.GetAuthInformation(externalIdentity); | ||
var emailSuffix = userInfo.Email == null ? String.Empty : (" <" + userInfo.Email + ">"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very much a nit and pretty sure the gallery code base is inconsistent, but should we prefer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I just copied this over from previous place. I usually use
|
||
var identity = userInfo.Name + emailSuffix; | ||
return new AuthenticateExternalLoginResult() | ||
{ | ||
if (authenticator.TryMapIssuerToAuthenticationType(idClaim.Issuer, out authenticationType)) | ||
{ | ||
auther = authenticator; | ||
break; | ||
} | ||
} | ||
Authentication = null, | ||
ExternalIdentity = externalIdentity, | ||
Authenticator = author, | ||
Credential = _credentialBuilder.CreateExternalCredential(userInfo.AuthenticationType, userInfo.Identifier, identity, userInfo.TenantId) | ||
}; | ||
} | ||
|
||
return new AuthenticateExternalLoginResult() | ||
catch (Exception ex) | ||
{ | ||
Authentication = null, | ||
ExternalIdentity = result.Identity, | ||
Authenticator = auther, | ||
Credential = _credentialBuilder.CreateExternalCredential(authenticationType, idClaim.Value, nameClaim.Value + emailSuffix, tenantIdClaim?.Value) | ||
}; | ||
_trace.Error(ex.Message); | ||
return new AuthenticateExternalLoginResult(); | ||
} | ||
} | ||
|
||
public virtual async Task<AuthenticateExternalLoginResult> AuthenticateExternalLogin(IOwinContext context) | ||
|
@@ -687,13 +691,13 @@ private string FormatCredentialType(string credentialType) | |
|
||
private string FormatExternalCredentialType(string externalType) | ||
{ | ||
Authenticator auther; | ||
if (!Authenticators.TryGetValue(externalType, out auther)) | ||
Authenticator author; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would rename |
||
if (!Authenticators.TryGetValue(externalType, out author)) | ||
{ | ||
return externalType; | ||
} | ||
var ui = auther.GetUI(); | ||
return ui == null ? auther.Name : ui.AccountNoun; | ||
var ui = author.GetUI(); | ||
return ui == null ? author.Name : ui.AccountNoun; | ||
} | ||
|
||
private Credential FindMatchingCredential(Credential credential) | ||
|
@@ -803,7 +807,7 @@ private bool IsAccountLocked(User user, out int remainingMinutes) | |
|
||
private DateTime CalculateAccountUnlockTime(int failedLoginCount, DateTime lastFailedLogin) | ||
{ | ||
int lockoutPeriodInMinutes = (int)Math.Pow(AccountLockoutMultiplierInMinutes, (int) ((double)failedLoginCount/AllowedLoginAttempts) - 1); | ||
int lockoutPeriodInMinutes = (int)Math.Pow(AccountLockoutMultiplierInMinutes, (int)((double)failedLoginCount / AllowedLoginAttempts) - 1); | ||
|
||
return lastFailedLogin + TimeSpan.FromMinutes(lockoutPeriodInMinutes); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
namespace NuGetGallery.Authentication.Providers | ||
{ | ||
public class AuthInformation | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use full words (AuthenticationInformation) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. renamed to |
||
{ | ||
public string Identifier { get; private set; } | ||
|
||
public string Name { get; private set; } | ||
|
||
public string Email { get; private set; } | ||
|
||
public string TenantId { get; private set; } | ||
|
||
public string AuthenticationType { get; private set; } | ||
|
||
public AuthInformation() { } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Empty constructor and only read-only property access? Smells weird..
|
||
|
||
public AuthInformation(string identifier, string name, string email, string authenticationType, string tenantId = null) | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need argument null checks here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope, validation can be done in the service where needed. |
||
Identifier = identifier; | ||
Name = name; | ||
Email = email; | ||
AuthenticationType = authenticationType; | ||
TenantId = tenantId; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Security.Claims; | ||
using System.Text.RegularExpressions; | ||
using System.Threading.Tasks; | ||
using System.Web.Mvc; | ||
|
@@ -94,16 +95,32 @@ public virtual ActionResult Challenge(string redirectUrl) | |
return new HttpUnauthorizedResult(); | ||
} | ||
|
||
public virtual bool TryMapIssuerToAuthenticationType(string issuer, out string authenticationType) | ||
/// <summary> | ||
/// Override this method to provide confirmation on identity author. | ||
/// </summary> | ||
/// <param name="claimsIdentity">The claims identity returned by the identity</param> | ||
/// <returns>Returns true if this provider is the author for the identity, false otherwise</returns> | ||
public virtual bool IsAuthorForIdentity(ClaimsIdentity claimsIdentity) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Can we rename
|
||
{ | ||
if (string.Equals(issuer, BaseConfig.AuthenticationType, StringComparison.OrdinalIgnoreCase)) | ||
// If the issuer of the claims identity is same as that of the authentication type then this is the author. | ||
Claim firstClaim = claimsIdentity?.Claims?.FirstOrDefault(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: use
|
||
if (firstClaim == null) | ||
{ | ||
authenticationType = BaseConfig.AuthenticationType; | ||
return true; | ||
return false; | ||
} | ||
|
||
authenticationType = null; | ||
return false; | ||
return string.Equals(firstClaim.Issuer, BaseConfig.AuthenticationType, StringComparison.OrdinalIgnoreCase); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If
I don't think AuthenticationConfiguration enforces this though. :-( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, it is not enforced. I would rather have the check to prevent unseen issues later. |
||
} | ||
|
||
/// <summary> | ||
/// The providers will override this method to extract the user information | ||
/// from the returned claims by the identity authentication. | ||
/// </summary> | ||
/// <param name="claimsIdentity">The claims identity returned by the identity</param> | ||
/// <returns><see cref="AuthInformation"/></returns> | ||
public virtual AuthInformation GetAuthInformation(ClaimsIdentity claimsIdentity) | ||
{ | ||
return new AuthInformation(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any use in returning this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, its better. |
||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,10 +2,12 @@ | |
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using System.Security.Claims; | ||
using System.Threading.Tasks; | ||
using System.Web.Mvc; | ||
using Microsoft.IdentityModel.Protocols; | ||
using Microsoft.Owin.Security.OpenIdConnect; | ||
using NuGetGallery.Authentication.Providers.Utils; | ||
using NuGetGallery.Configuration; | ||
using Owin; | ||
|
||
|
@@ -66,15 +68,9 @@ public override ActionResult Challenge(string redirectUrl) | |
return new ChallengeResult(BaseConfig.AuthenticationType, redirectUrl); | ||
} | ||
|
||
public override bool TryMapIssuerToAuthenticationType(string issuer, out string authenticationType) | ||
public override AuthInformation GetAuthInformation(ClaimsIdentity claimsIdentity) | ||
{ | ||
if (string.Equals(issuer, Config.Issuer, StringComparison.OrdinalIgnoreCase)) | ||
{ | ||
authenticationType = Config.AuthenticationType; | ||
return true; | ||
} | ||
|
||
return base.TryMapIssuerToAuthenticationType(issuer, out authenticationType); | ||
return MSAIdentityExtractor.GetAuthInformation(claimsIdentity, DefaultAuthenticationType); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MSAIdentityExtractor is for both MSA and AAD? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, for the previous schema MSA and AAD used the same claims to get values. This is an interim thing until we get rid of old providers. |
||
} | ||
} | ||
} |
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.
I think
auther
was intentional... as in, the one doing the "auth".Agree it's a bad name, but maybe
authenticator
is a better one?