[2FA] Add AAD login support for NuGet Gallery #5257

shishirx34 · 2018-01-10T03:15:40Z

This PR adds AAD support to NuGet gallery. The new login will work just like the personal MSA. You can login with your AAD account and link it to an existing nuget.org account or create a new account. This is configured as a new provider, so that we can transition away from previous way of logging in.
I will add new configuration values in deployment PR. This PR uses Microsoft.Owin.Security.OpenIdConnect middleware which can now login with a common endpoint that facilitates single sign on flow for Personal/Work microsoft accounts.

Also, the code takes care of maintaining backwards compatibility with existing MSA accounts and translates the account information into older format. Also added tests for the new provider and some missing coverage in AuthenticationService.

xavierdecoster · 2018-01-10T10:23:25Z

src/NuGetGallery/Authentication/AuthenticationService.cs

            {
-                foreach (var authenticator in Authenticators.Values)
+                var userInfo = author.GetAuthInformation(externalIdentity);
+                var emailSuffix = userInfo.Email == null ? String.Empty : (" <" + userInfo.Email + ">");


Very much a nit and pretty sure the gallery code base is inconsistent, but should we prefer string over String?

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

done

xavierdecoster · 2018-01-10T10:24:52Z

src/NuGetGallery/Authentication/Providers/AuthInformation.cs

+        public AuthInformation() { }
+
+        public AuthInformation(string identifier, string name, string email, string authenticationType, string tenantId = null)
+        {


Do we need argument null checks here?

Nope, validation can be done in the service where needed.

xavierdecoster · 2018-01-10T10:27:30Z

src/NuGetGallery/Authentication/Providers/AuthInformation.cs

+
+        public string AuthenticationType { get; private set; }
+
+        public AuthInformation() { }


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

removed

xavierdecoster · 2018-01-10T10:32:25Z

src/NuGetGallery/Authentication/Providers/Authenticator.cs

+        /// <returns><see cref="AuthInformation"/></returns>
+        public virtual AuthInformation GetAuthInformation(ClaimsIdentity claimsIdentity)
+        {
+            return new AuthInformation();


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

I agree, its better.

chenriksson · 2018-01-10T17:00:05Z

src/NuGetGallery/Authentication/AuthenticationService.cs

@@ -474,12 +473,21 @@ public virtual ActionResult Challenge(string providerName, string redirectUrl)
        public virtual CredentialViewModel DescribeCredential(Credential credential)
        {
            var kind = GetCredentialKind(credential.Type);
-            Authenticator auther = null;
+            Authenticator author = null;


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

chenriksson · 2018-01-10T17:04:15Z

src/NuGetGallery/Authentication/Providers/Authenticator.cs

+        /// </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)


Author and Auther are confusing me.

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

done

chenriksson · 2018-01-10T17:04:40Z

src/NuGetGallery/Authentication/Providers/Authenticator.cs

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


nit: use var

done

chenriksson · 2018-01-10T17:08:27Z

src/NuGetGallery/Authentication/Providers/Authenticator.cs

            }

-            authenticationType = null;
-            return false;
+            return string.Equals(firstClaim.Issuer, BaseConfig.AuthenticationType, StringComparison.OrdinalIgnoreCase);


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. :-(

You are right, it is not enforced. I would rather have the check to prevent unseen issues later.

chenriksson · 2018-01-10T17:12:36Z

...etGallery/Authentication/Providers/AzureActiveDirectory/AzureActiveDirectoryAuthenticator.cs

-            }
-
-            return base.TryMapIssuerToAuthenticationType(issuer, out authenticationType);
+            return MSAIdentityExtractor.GetAuthInformation(claimsIdentity, DefaultAuthenticationType);


MSAIdentityExtractor is for both MSA and AAD?

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.

chenriksson · 2018-01-10T17:25:57Z

src/NuGetGallery/Authentication/Providers/CommonAuth/CommonAuthAuthenticator.cs

+        }
+
+        public static readonly string DefaultAuthenticationType = "CommonAuth";
+        public static readonly string PersonalMSATenant = "9188040d-6c67-4c5b-b112-36a304b66dad";


Looks like this is personal (non-tenant) token id? Does that imply MSA, or can still be AAD?

Just looking at:
https://docs.microsoft.com/en-us/azure/active-directory/develop/active-directory-v2-protocols-oauth-code

This is the tenant for Personal MSA accounts, under the v2 protocol it is returned as the tenant id for Personal MSA accounts, same as the previous external.MicrosoftAccount credentials validated using the MSA flow.

chenriksson · 2018-01-10T17:27:26Z

src/NuGetGallery/Authentication/Providers/CommonAuth/CommonAuthAuthenticator.cs

+            if (siteRoot.StartsWith("http://", StringComparison.OrdinalIgnoreCase)) 
+            {
+                siteRoot = siteRoot.Replace("http://", "https://");
+            }


Is this necessary - don't we already have redirect configuration for this?

This value is passed to the Azure AD to redirect with the claims after authentication. This is added as a query string and validated on the AD side before redirection, it needs to be https

Maybe clarify in comment why this redirect is necessary for AAD, so nobody mistakenly removes it?

chenriksson · 2018-01-10T17:36:16Z

src/NuGetGallery/Authentication/Providers/CommonAuth/CommonAuthAuthenticator.cs

+                // where as the existing Microsoft account identifiers are 16 character wide.
+                // For e.g old format: 0ae45d63e22e4a60, newer format: 00000000-0000-0000-0AE4-5D63-E22E4A60
+                // We need to format the values into the older format for backwards compatibility
+                identifier = idClaim.Value.Replace("-", "").Substring(16).ToLowerInvariant();


Do you know why they left pad with zeros? Just wondering if we'll ever need those digits... would it be useful to have 2 versions of the Id claim, for 16- and 32- characters?

I checked with Identity folks and it is safe to use the 16 character string for MSA accounts.

chenriksson · 2018-01-10T17:38:13Z

src/NuGetGallery/Authentication/Providers/Utils/MSAIdentityExtractor.cs

+            var emailClaim = claimsIdentity.FindFirst(ClaimTypes.Email);
+            return new AuthInformation(identifierClaim.Value, nameClaim.Value, emailClaim?.Value, authType, tenantId: null);
+        }
+    }


Is there anything MSA specific in here? Could this just be put into a generic ClaimsExtensions class?

Nothing specific for MSA, renamed it to ClaimsExtensions, this will be removed eventually when we remove older providers.

chenriksson · 2018-01-10T17:40:39Z

src/NuGetGallery/Controllers/AuthenticationController.cs

+                    name = userInfo.Name;
+                    email = userInfo.Email;
+                }
+                catch (Exception)


Is there a more specific Exception type we expect to see? What's the scenario?

Should we add trace log?

Before this change, when we found a user trying to register a new account directly with MSA, we would show email we got from the auth and allow the user to edit. Going forward, we will not allow this to happen, however, for backwards compatibility(more so especially for tests) we get one or the other claim. I haven't yet modified those tests as we need to maintain backwards compatibility. Hence, for now we will swallow the exceptions. In future we will surface the error to the view model and show appropriate error to the user. No point in adding trace here yet.

skofman1 · 2018-01-10T18:28:59Z

src/NuGetGallery/Authentication/Providers/AuthInformation.cs

+
+namespace NuGetGallery.Authentication.Providers
+{
+    public class AuthInformation


Please use full words (AuthenticationInformation)

renamed to IdentityInformation which seems more accurate.

skofman1 · 2018-01-10T18:32:46Z

src/NuGetGallery/Authentication/AuthenticationService.cs

+                {
+                    author = authenticator;
+                    break;
+                }
            }


you can retwrite this using FirstOrDefault

skofman1 · 2018-01-10T18:35:59Z

src/NuGetGallery/Authentication/Providers/CommonAuth/CommonAuthAuthenticator.cs

+using NuGetGallery.Configuration;
+using Owin;
+
+namespace NuGetGallery.Authentication.Providers.CommonAuth


Please use Aythentication and not Auth everywhere

skofman1 · 2018-01-10T18:41:26Z

src/NuGetGallery/Authentication/Providers/CommonAuth/CommonAuthAuthenticator.cs

+            var nameClaim = claimsIdentity.FindFirst(V2Claims.Name);
+            if (nameClaim == null)
+            {
+                throw new ArgumentException($"External Authentication is missing required claim: '{V2Claims.Name}'");


why do we require Name claim? Why would it be missing?

removed the name claim null check, since it is not a necessity.

skofman1 · 2018-01-10T18:43:46Z

src/NuGetGallery/Authentication/Providers/Utils/MSAIdentityExtractor.cs

+            var identifierClaim = claimsIdentity.FindFirst(ClaimTypes.NameIdentifier);
+            if (identifierClaim == null)
+            {
+                throw new ArgumentException("External Authentication is missing required claim: " + ClaimTypes.NameIdentifier);


nit: the 2 exception strings are formatted differently

fixed.

cristinamanum · 2018-01-10T18:46:02Z

src/NuGetGallery/Authentication/AuthenticationService.cs

-                Authenticators.TryGetValue(providerName, out auther);
+                if (string.IsNullOrEmpty(credential.TenantId))
+                {
+                    string providerName = credential.Type.Split('.')[1];


Will the providerName always be the second ?

Yes. external.MicrosoftAccount external.AzureActiveDiretory

shishirx34 · 2018-01-10T18:59:48Z

src/NuGetGallery/Authentication/Providers/CommonAuth/CommonAuthAuthenticator.cs

+            return string.Equals(issuer.Value, expectedIssuer, StringComparison.OrdinalIgnoreCase);
+        }
+
+        public override AuthInformation GetAuthInformation(ClaimsIdentity claimsIdentity)


May be combine this and above method as TryGet

I looked into this, and I think its better to throw the exception here, given that we want to disallow missing emails going forward. In subsequent PRs I will clean it up to throw a well defined exception that we can surface to the user as missing email.

chenriksson · 2018-01-11T18:14:43Z

src/NuGetGallery/Authentication/AuthenticationService.cs


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


So, we'll switch over to the new APIs (OpenIdConnect) once the SQL is run to populate tenantId for existing credentials?

Yes, and after we enable the config values for new provider.

chenriksson

Looks great!

chenriksson · 2018-01-11T18:18:43Z

src/NuGetGallery/Authentication/AuthenticationService.cs

+                {
+                    authenticator = Authenticators
+                        .Values
+                        .First(provider => provider.Name.Equals(AzureActiveDirectoryV2Authenticator.DefaultAuthenticationType, StringComparison.OrdinalIgnoreCase));


Why not use TryGetValue here or FirstOrDefault like you do for identity lookup. Just wondering about consistency.

Good idea to use FirstOrDefault

chenriksson · 2018-01-11T18:20:27Z

src/NuGetGallery/Authentication/AuthenticationService.cs

-            var nameClaim = result.Identity.FindFirst(ClaimTypes.Name);
-            if (nameClaim == null)
+            var externalIdentity = result.Identity;
+            Authenticator author = Authenticators


Would rename author => authenticator

chenriksson · 2018-01-11T18:20:57Z

src/NuGetGallery/Authentication/AuthenticationService.cs

@@ -687,13 +682,13 @@ private string FormatCredentialType(string credentialType)

        private string FormatExternalCredentialType(string externalType)
        {
-            Authenticator auther;
-            if (!Authenticators.TryGetValue(externalType, out auther))
+            Authenticator author;


Would rename author => authenticator

chenriksson · 2018-01-11T18:23:04Z

src/NuGetGallery/Authentication/Providers/Authenticator.cs

+        /// </summary>
+        /// <param name="claimsIdentity">The claims identity returned by the identity</param>
+        /// <returns><see cref="IdentityInformation"/></returns>
+        public virtual IdentityInformation GetIdentityInformation(ClaimsIdentity claimsIdentity)


Why not leave this abstract? Do any children actually use this?

Yes, there are other children where we need default value.

chenriksson · 2018-01-11T18:26:38Z

src/NuGetGallery/Authentication/Providers/CommonAuth/CommonAuthAuthenticator.cs

+            if (siteRoot.StartsWith("http://", StringComparison.OrdinalIgnoreCase)) 
+            {
+                siteRoot = siteRoot.Replace("http://", "https://");
+            }


Maybe clarify in comment why this redirect is necessary for AAD, so nobody mistakenly removes it?

chenriksson · 2018-01-11T18:29:27Z

...ication/Providers/AzureActiveDirectoryV2/AzureActiveDirectoryV2AuthenticatorConfiguration.cs

+        {
+            base.ApplyToOwinSecurityOptions(options);
+
+            var opts = options as OpenIdConnectAuthenticationOptions;


nit: would rename to reflect more specific cast, like openIdOptions

chenriksson · 2018-01-11T18:31:29Z

...allery.Facts/Authentication/Providers/CommonAuth/AzureActiveDirectoryV2AuthenticatorFacts.cs

+            [Fact]
+            public void GetUIReturnsCorrectValues()
+            {
+                //Arrange


nit: space after //

shishirx34 added 18 commits November 16, 2017 14:08

Add common endpoint support

00ba375

Merge remote-tracking branch 'origin/dev' into 2fa-phase0-aad

140939f

Merge remote-tracking branch 'origin/dev' into 2fa-phase0-aad

68dcaf2

Merge remote-tracking branch 'origin/dev' into 2fa-phase0-aad

39f6118

merge with dev

35e9022

add MFA support for microsoft accounts

01b59e4

Merge remote-tracking branch 'origin/dev' into 2fa-phase0-aad

879430d

update configs

b0df1fd

Refactor

c5d8efa

refactor the core code

972d16c

refactor code

a35a4be

Fix the tests

908ae1c

refactor claim extractor

426c829

nit fix

77786db

Merge remote-tracking branch 'origin/dev' into 2fa-phase0-aad

e32e8bd

add tests

2a1f33c

refactor and cleanup, add more tests for auth

c690735

Update test names

7402572

shishirx34 requested a review from chenriksson January 10, 2018 03:15

shishirx34 assigned joelverhagen, xavierdecoster, ryuyu, anangaur, cristinamanum, skofman1 and scottbommarito and unassigned anangaur Jan 10, 2018

xavierdecoster approved these changes Jan 10, 2018

View reviewed changes

chenriksson reviewed Jan 10, 2018

View reviewed changes

skofman1 reviewed Jan 10, 2018

View reviewed changes

cristinamanum reviewed Jan 10, 2018

View reviewed changes

shishirx34 commented Jan 10, 2018

View reviewed changes

skofman1 approved these changes Jan 10, 2018

View reviewed changes

Address feedback

ac35d7d

chenriksson reviewed Jan 11, 2018

View reviewed changes

chenriksson approved these changes Jan 11, 2018

View reviewed changes

address feedback

68562ea

shishirx34 merged commit d29d2f1 into dev Jan 11, 2018

shishirx34 deleted the 2fa-phase0-aad branch January 11, 2018 19:41


		public string AuthenticationType { get; private set; }

		public AuthInformation() { }

[2FA] Add AAD login support for NuGet Gallery #5257

[2FA] Add AAD login support for NuGet Gallery #5257

Conversation

shishirx34 commented Jan 10, 2018

Choose a reason for hiding this comment

shishirx34 Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xavierdecoster Jan 10, 2018 • edited by shishirx34 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenriksson Jan 10, 2018 • edited by shishirx34 Loading

Choose a reason for hiding this comment

chenriksson Jan 10, 2018 • edited by shishirx34 Loading

Choose a reason for hiding this comment

chenriksson Jan 10, 2018 • edited by shishirx34 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shishirx34 Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

skofman1 Jan 10, 2018 • edited by shishirx34 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shishirx34 Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenriksson left a comment

Choose a reason for hiding this comment

chenriksson Jan 11, 2018 • edited by shishirx34 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shishirx34 Jan 10, 2018 •

edited

Loading

xavierdecoster Jan 10, 2018 •

edited by shishirx34

Loading

chenriksson Jan 10, 2018 •

edited by shishirx34

Loading

chenriksson Jan 10, 2018 •

edited by shishirx34

Loading

chenriksson Jan 10, 2018 •

edited by shishirx34

Loading

shishirx34 Jan 10, 2018 •

edited

Loading

skofman1 Jan 10, 2018 •

edited by shishirx34

Loading

shishirx34 Jan 10, 2018 •

edited

Loading

chenriksson Jan 11, 2018 •

edited by shishirx34

Loading