Skip to content
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

Merged
merged 20 commits into from
Jan 11, 2018
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 49 additions & 45 deletions src/NuGetGallery/Authentication/AuthenticationService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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);
}
Expand All @@ -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;
Copy link
Member

@chenriksson chenriksson Jan 10, 2018

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?

  • done


if (kind == CredentialKind.External)
{
string providerName = credential.Type.Split('.')[1];
Authenticators.TryGetValue(providerName, out auther);
if (string.IsNullOrEmpty(credential.TenantId))
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will the providerName always be the second ?

Copy link
Contributor Author

@shishirx34 shishirx34 Jan 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. external.MicrosoftAccount external.AzureActiveDiretory

Authenticators.TryGetValue(providerName, out author);
}
else
{
author = Authenticators
.Values
.First(provider => provider.Name.Equals(CommonAuthAuthenticator.DefaultAuthenticationType, StringComparison.OrdinalIgnoreCase));
}
}

var credentialViewModel = new CredentialViewModel
Expand All @@ -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,
Expand Down Expand Up @@ -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;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The 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 + ">");
Copy link
Member

Choose a reason for hiding this comment

The 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 string over String?

Copy link
Contributor Author

@shishirx34 shishirx34 Jan 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I just copied this over from previous place. I usually use string. Will fix it.

  • done

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)
Expand Down Expand Up @@ -687,13 +691,13 @@ private string FormatCredentialType(string credentialType)

private string FormatExternalCredentialType(string externalType)
{
Authenticator auther;
if (!Authenticators.TryGetValue(externalType, out auther))
Authenticator author;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would rename author => authenticator

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)
Expand Down Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,5 @@
// 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.
using System;
using System.Collections.Generic;
using System.Linq;
using System.Web;
using Microsoft.Owin.Security;

namespace NuGetGallery.Authentication.Providers.ApiKey
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
// 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.
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Linq;
using System.Web;

namespace NuGetGallery.Authentication.Providers.ApiKey
{
Expand Down
29 changes: 29 additions & 0 deletions src/NuGetGallery/Authentication/Providers/AuthInformation.cs
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use full words (AuthenticationInformation)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed to IdentityInformation which seems more accurate.

{
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() { }
Copy link
Member

@xavierdecoster xavierdecoster Jan 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty constructor and only read-only property access? Smells weird..

  • removed


public AuthInformation(string identifier, string name, string email, string authenticationType, string tenantId = null)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need argument null checks here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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;
}
}
}
29 changes: 23 additions & 6 deletions src/NuGetGallery/Authentication/Providers/Authenticator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Copy link
Member

@chenriksson chenriksson Jan 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Author and Auther are confusing me.

Can we rename auther to authenticator, and maybe change this signature to bool IsProviderForIdentity(ClaimsIdentity)?

  • done

{
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();
Copy link
Member

@chenriksson chenriksson Jan 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: use var

  • done

if (firstClaim == null)
{
authenticationType = BaseConfig.AuthenticationType;
return true;
return false;
}

authenticationType = null;
return false;
return string.Equals(firstClaim.Issuer, BaseConfig.AuthenticationType, StringComparison.OrdinalIgnoreCase);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If BaseConfig.AuthenticationType were guaranteed to be non-null, we could remove the block above and just do:

string.Equals(firstClaim?.Issuer, BaseConfig.AuthenticationType, StringComparison.OrdinalIgnoreCase);

I don't think AuthenticationConfiguration enforces this though. :-(

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any use in returning this new AuthInformation()? Should this become an abstract method instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MSAIdentityExtractor is for both MSA and AAD?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

}
}
}
Loading