Temp keys implementation (#3563) #3646

chenriksson · 2017-03-14T00:00:48Z

Telemetry will come in separate PR

joelverhagen · 2017-03-14T01:28:18Z

src/NuGetGallery/App_Start/Routes.cs

                    version = UrlParameter.Optional
                });

+            routes.MapRoute(


Why add the endpoint here? Do we even document v1 push? I think the data should determine whether we add the endpoint here -- what does AppInsights suggest is the number of usages in the past 90 days?

AI shows only v2 in use. My reasoning was to have same support as 'verifykey'. If we don't add it to v1 because of AI, why not just remove verifykey as well? Thoughts?

joelverhagen · 2017-03-14T01:29:07Z

src/NuGetGallery/App_Start/Routes.cs

+
+            routes.MapRoute(
+                "v2" + RouteName.CreatePackageVerificationKey,
+                "api/v2/package/create-verification-key/{id}/{version}",


No {version} parameter, right?

Correct - missed this one!

Update: per discussion, keeping version for telemetry

joelverhagen · 2017-03-14T01:30:08Z

src/NuGetGallery/Controllers/ApiController.cs

@@ -43,6 +46,8 @@ public partial class ApiController
        public IMessageService MessageService { get; set; }
        public IAuditingService AuditingService { get; set; }
        public IGalleryConfigurationService ConfigurationService { get; set; }
+        public AuthenticationService AuthenticationService { get; set; }
+        public ICredentialBuilder CredentialBuilder { get; set; }


nit: why do these properties have set;? Typically in a DI world these are just set as ctor/instantiation time.

Kept the existing pattern... will see if I can refactor here.

joelverhagen · 2017-03-14T01:35:14Z

src/NuGetGallery/Controllers/ApiController.cs

+        [ApiAuthorize]
+        [ApiScopeRequired(NuGetScopes.PackagePush, NuGetScopes.PackagePushVersion)]
+        [ActionName("CreatePackageVerificationKey")]
+        public virtual Task<ActionResult> CreatePackageVerificationKeyPutAsync(string id)


Why support both PUT and POST? This is a new endpoint so we can be strict.

I actually thing POST is more appropriate here since subsequent calls will create different API keys. (sorry if I have gone back and forth about this)

Was being consistent with package push, but agree we don't need both here. POST does seem the better option. @zhili1208 will need to update the client PR.

joelverhagen · 2017-03-14T01:37:53Z

src/NuGetGallery/Controllers/ApiController.cs

+                var routeDefaults = (RouteData.Route as Route)?.Defaults?.Values;
+                var apiName = routeDefaults == null ? "Unknown" : string.Join("/", routeDefaults);
+
+                HttpContext.Response.Headers.Add(Constants.WarningHeaderName,


This header is now very visible on the client. Every symbol push will result in this warning appearing. Do we really want this level of visibility? I am not sure.

Removing this header

joelverhagen · 2017-03-14T01:39:41Z

src/NuGetGallery/Controllers/ApiController.cs

+            if (HttpContext.Response.Headers != null)
+            {
+                var routeDefaults = (RouteData.Route as Route)?.Defaults?.Values;
+                var apiName = routeDefaults == null ? "Unknown" : string.Join("/", routeDefaults);


When would this be "Unknown"? I think we should not show the warning of we can't tell them what's actually deprecated.

Could you give an example of what apiName would be for the new create verify key endpoint?

joelverhagen · 2017-03-14T01:41:21Z

src/NuGetGallery/Controllers/ApiController.cs

+            return Json(new
+            {
+                Key = credential.Value,
+                Expires = credential.Expires.Value.ToString("O", CultureInfo.CurrentCulture),


We really want invariant culture here -- although am not sure if culture info even affects this.
https://msdn.microsoft.com/en-us/library/az4se3k1(v=vs.110).aspx

Looks like "O" doesn't need culture. Thanks!

joelverhagen · 2017-03-14T01:42:09Z

src/NuGetGallery/Controllers/ApiController.cs

+            {
+                Key = credential.Value,
+                Expires = credential.Expires.Value.ToString("O", CultureInfo.CurrentCulture),
+                PackageScope = credential.Scopes.First().Subject


Why return the scope here? If we are doing First() it seems kind of arbitrary

Scope is an implementation detail. We shouldn't expose it to the client.

joelverhagen · 2017-03-14T01:54:04Z

src/NuGetGallery/Controllers/ApiController.cs

+                // Expire and delete verification key after first use to avoid growing the database tables.
+                if (isVerificationKey)
+                {
+                    await AuthenticationService.RemoveCredential(user, credential);


This may change the error flow for the client. Imagine we return HTTP 500 here due to an exception ApiKeyScopeAllows. The key will be deleted in this finally. If the 500 bubbles all the way back to the client (nuget.exe), when they will retry. To have the same user experience, the client MUST generate a new API key, right?

@zhili1208 -- is this the case? HTTP 500 in the client today causes the exact same HTTP request to be retried. In this particular scenario, we need to generate a new verification key before retrying (I don't recall this in your client PR).

Yes, 500 causes http request retry in client

skofman1 · 2017-03-14T02:30:47Z

src/NuGetGallery/Controllers/ApiController.cs

+            {
+                Key = credential.Value,
+                Expires = credential.Expires.Value.ToString("O", CultureInfo.CurrentCulture),
+                PackageScope = credential.Scopes.First().Subject


Scope is an implementation detail. We shouldn't expose it to the client.

skofman1 · 2017-03-14T02:38:09Z

src/NuGetGallery/Controllers/ApiController.cs


        [HttpGet]
        [RequireSsl]
        [ApiAuthorize]
+        [ApiScopeRequired(NuGetScopes.PackageVerify)]


you should add NuGetScopes.PackagePush, NuGetScopes.PackagePushVersion to the allowed scopes

skofman1 · 2017-03-14T02:46:22Z

src/NuGetGallery.Core/CredentialTypes.cs

@@ -22,6 +22,7 @@ public static class ApiKey
            public const string Prefix = "apikey.";
            public const string V1 = Prefix + "v1";
            public const string V2 = Prefix + "v2";
+            public const string V2Verify = Prefix + "verify.v2";


IMO versioning of "verify api key" should be separate from the regular api keys. I recommend "verify.v1"

skofman1 · 2017-03-14T02:53:22Z

Some minor issues, otherwise looks great!

joelverhagen · 2017-03-14T19:33:09Z

I don't think we should merge this to master until our clean-up job is running. This suggests we should have a feature flag to easily enable and disable creation of keys. What do you think?

scottbommarito

Looks pretty good overall, just a couple things I would like to see changed

scottbommarito · 2017-03-14T17:30:39Z

src/NuGetGallery/Controllers/ApiController.cs

@@ -43,6 +46,8 @@ public partial class ApiController
        public IMessageService MessageService { get; set; }
        public IAuditingService AuditingService { get; set; }
        public IGalleryConfigurationService ConfigurationService { get; set; }
+        public AuthenticationService AuthenticationService { get; set; }


Do we not have an interface for AuthenticationService? Seems weird when every single other property in this class uses an interface.

No, we don't. :-)

scottbommarito · 2017-03-14T17:53:15Z

tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs

@@ -50,6 +53,8 @@ public TestableApiController(MockBehavior behavior = MockBehavior.Default)
            StatisticsService = (MockStatisticsService = new Mock<IStatisticsService>()).Object;
            IndexingService = (MockIndexingService = new Mock<IIndexingService>()).Object;
            AutoCuratePackage = (MockAutoCuratePackage = new Mock<IAutomaticallyCuratePackageCommand>()).Object;
+            CredentialBuilder = new CredentialBuilder();


All of the other members of ApiController are replaced with Mocks here. The idea is that if you would like to use a functionality of an interface, you need to control it yourself by setting up the various methods to act how you want them to.

I would suggest removing AuthenticationService from the constructor and using Mocks like the other fields. You can then setup any methods you need.

scottbommarito · 2017-03-14T17:55:16Z

tests/NuGetGallery.Facts/Authentication/TestCredentialBuilder.cs

@@ -36,12 +37,17 @@ public static Credential CreateV2ApiKey(Guid apiKey, TimeSpan? expiration)
            return CreateApiKey(CredentialTypes.ApiKey.V2, apiKey.ToString(), expiration);
        }

+        public static Credential CreateV2VerificationApiKey(Guid apiKey)


Side note about this file:

It's very strange that TestCredentialBuilder is not an ICredentialBuilder. I would suggest renaming it so that it's no longer confusing, for example TestCredentialHelper.

scottbommarito · 2017-03-14T18:13:21Z

tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs

            {
                // Arrange
-                var controller = new TestableApiController();
-                controller.SetCurrentUser(new User());
+                var user = await CreateUserAsync(CredentialTypes.ApiKey.V2, "anysubject", allowedAction);


As an example of what I mean by the comment above, you can do the following to this test case to make it consistent with the testing pattern of the rest of the file.

Change CreateUserAsync to not call AuthenticationService.AddCredential and call user.Credentials.Add(credential) directly

Setup the Mock<AuthenticationService> to do user.Credentials.Add(credential) when AuthenticationService.AddCredential is called

Verify that AuthenticationService.AddCredential was called (and check that user.Credential is as expected)

An alternative to this is to change the TestableApiController constructor to create the Mock<AuthenticationService> with a setup of AuthenticationService.AddCredential as mentioned in number 2 and then do number 3.

scottbommarito · 2017-03-14T19:20:07Z

tests/NuGetGallery.Facts/Framework/TestExtensionMethods.cs

-                    new Claim(NuGetClaims.ApiKey, string.Empty),
-                    new Claim(NuGetClaims.Scope, scopes));
+                apiKey = credential.Value;
+                scopes = JsonConvert.SerializeObject(credential.Scopes, Formatting.None);


This doesn't seem right to me. We ask the user for scopes, but if they have at least one API key already, then we ignore the requested scopes and use the first API key instead.

I think I'm unnecessarily overloading this method. Will look into separate overload so there's separate APIs for whether scopes are passed as strings or discovered from the user.

scottbommarito · 2017-03-14T19:21:03Z

tests/NuGetGallery.Facts/Framework/TestExtensionMethods.cs


-            if (scopes != null)
+            var credential = user.Credentials?.FirstOrDefault(c => c.Type.StartsWith(CredentialTypes.ApiKey.Prefix));


Why did we need to change this method?

scottbommarito · 2017-03-14T19:24:21Z

src/NuGetGallery/Infrastructure/Authentication/CredentialBuilder.cs

+        {
+            var credential = new Credential(
+               CredentialTypes.ApiKey.VerifyV1,
+               Guid.NewGuid().ToString().ToLowerInvariant(),


Make a helper function for this. This logic is now in both CreateApiKey and CreatePackageVerificationApiKey. We should have a shared method for creating our keys.

private static string CreateKeyString() { return Guid.NewGuid().ToString().ToLowerInvariant(); }

scottbommarito · 2017-03-14T19:28:32Z

tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs

+            [InlineData(CredentialTypes.ApiKey.VerifyV1, "notfoo", NuGetScopes.PackageVerify)]
+            [InlineData(CredentialTypes.ApiKey.VerifyV1, "foo", NuGetScopes.PackagePush)]
+            [InlineData(CredentialTypes.ApiKey.VerifyV1, "foo", NuGetScopes.PackagePushVersion)]
+            public async void VerifyPackageKeyReturns403IfOwnerScopeDoesNotMatch(string credentialType, string subject, string allowedAction)


Since the behavior is not explicit here, I would add a comment explaining that VerifyPackageKey will succeed when subject is "foo" and scope is NuGetScopes.PackageVerify.

Also I think you should probably have a CredentialTypes.ApiKey.V2 and a CredentialTypes.ApiKey.VerifyV1 version for each test case if they act in the same way. In other words, there should be a CredentialTypes.ApiKey.VerifyV1 test for the "bar" case and a CredentialTypes.ApiKey.V2 case for the "foo" and "notfoo" cases, unless the behavior is different.

Most of these test cases are covered - see below. Will try to make it more clear.

VerifyPackageKeyReturns403IfOwnerScopeDoesNotMatch - scopes don't match

ApiKey.V2 where subject and action (Verify is only for VerifyV1) don't match
=> I should separate non-matching subject and action into separate test cases

ApiKey.VerifyV1 where subject doesn't match

ApiKey.VerifyV1 where action doesn't match (push)

ApiKey.VerifyV1 where action doesn't match (pushVersion)

VerifyPackageKeyReturns200IfUserIsAnOwner - scopes match

ApiKey.V2 w/o scope (legacy)

ApiKey.V2 with matching subject and action (Push)

ApiKey.V2 with matching subject and action (PushVersion)

ApiKey.VerifyV1 with matching subject and action (Verify)

Yeah, ultimately I think the main reason I found this confusing was that this test case tested both the subject and the action being incorrect. I think if you separate the test cases into two then it would make much more sense.

chenriksson · 2017-03-14T23:29:21Z

@scottbommarito Refactored the tests to better match existing patterns... both with mocking the auth service and string scopes declared in [InlineData] attributes.

chenriksson · 2017-03-14T23:35:22Z

@joelverhagen Sounds reasonable. To avoid overloading this PR, how about I merge this and send PR for telemetry and feature flag tomorrow?

joelverhagen · 2017-03-14T23:36:38Z

To avoid overloading this PR, how about I merge this and send PR for telemetry and feature flag tomorrow?

As long as it makes it in before a merge to master: 👍

scottbommarito

looks great!

…7.03.27 * tag 'v2017.03.27': (205 commits) Revert UpdateIsLatest optimistic concurrency changes (NuGet#3707) UpdateIsLatest concurrent unlist fix (NuGet#3695) Change telemetry time to use correct format (NuGet#3690) Fix typo of "publically" (NuGet#3636) Fix regression (NuGet#3667) Add credential to Register and RequestPasswordReset audits (NuGet#3666) Functional test for temp keys (NuGet#3664) Telemetry for temp keys (NuGet#3662) Temp keys implementation (NuGet#3563) (NuGet#3646) Extracting code: single type per file (NuGet#3644) Telemetry for package push (NuGet#3649) Upgrade to NuGet.* v4.0.0 dependencies (NuGet#3643) Fix concurrent push test by disabling search hijacking on feed (NuGet#3641) Fixing Package Description truncation (NuGet#3638) Fix Microsoft Account removal (NuGet#3639) Send e-mail when a new API key is created (NuGet#3634) IsLatest Fix: wrong connection string passed to retry context (NuGet#3632) Update WindowsAzure.Storage to 7.0.0 (NuGet#3633) Depend on signed version of Elmah (NuGet#3609) Move AzureEntityList and TableErrorLog to NuGetGallery.Core (NuGet#3607) ...

dnfclas added the cla-already-signed label Mar 14, 2017

chenriksson requested review from joelverhagen, emgarten, zhili1208, rrelyea, dtivel, skofman1 and anangaur March 14, 2017 00:01

joelverhagen reviewed Mar 14, 2017

View reviewed changes

skofman1 requested changes Mar 14, 2017

View reviewed changes

chenriksson mentioned this pull request Mar 14, 2017

push tweaks NuGet/NuGet.Client#1235

Merged

chenriksson force-pushed the chenriks-tempkeys branch from 5f4da31 to d5d0c07 Compare March 14, 2017 18:15

skofman1 approved these changes Mar 14, 2017

View reviewed changes

zhili1208 approved these changes Mar 14, 2017

View reviewed changes

joelverhagen approved these changes Mar 14, 2017

View reviewed changes

scottbommarito suggested changes Mar 14, 2017

View reviewed changes

chenriksson requested a review from scottbommarito March 14, 2017 23:36

scottbommarito approved these changes Mar 14, 2017

View reviewed changes

chenriksson added 5 commits March 15, 2017 08:40

Initial work for temp key endpoints

6988d79

Remaining temp key implementation

808c2ef

Protocol fixes and auditing

2dd8c6b

PR feedback

4d3f8db

PR feedback #2

51b8cc9

chenriksson force-pushed the chenriks-tempkeys branch from 98fc140 to 51b8cc9 Compare March 15, 2017 16:04

Test fix missed in earlier commit

6eeda0e

chenriksson merged commit 2a95e4c into dev Mar 15, 2017

chenriksson deleted the chenriks-tempkeys branch March 15, 2017 16:14


		if (scopes != null)
		var credential = user.Credentials?.FirstOrDefault(c => c.Type.StartsWith(CredentialTypes.ApiKey.Prefix));

Temp keys implementation (#3563) #3646

Temp keys implementation (#3563) #3646

Conversation

chenriksson commented Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenriksson Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelverhagen Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenriksson Mar 14, 2017 • 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

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

skofman1 commented Mar 14, 2017

joelverhagen commented Mar 14, 2017

scottbommarito left a 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

chenriksson commented Mar 14, 2017

chenriksson commented Mar 14, 2017

joelverhagen commented Mar 14, 2017

scottbommarito left a comment

Choose a reason for hiding this comment

chenriksson commented Mar 14, 2017 •

edited

Loading

chenriksson Mar 14, 2017 •

edited

Loading

joelverhagen Mar 14, 2017 •

edited

Loading

chenriksson Mar 14, 2017 •

edited

Loading