Temp keys telemetry and auditing #3945

chenriksson · 2017-05-11T15:36:18Z

Auditing and telemetry for user security policies (subscription, unsubscription, failed evaluations)

chenriksson · 2017-05-11T15:42:27Z

src/NuGetGallery/Security/SecurityPolicyService.cs

                    if (!result.Success)
                    {
+                        Auditing.SaveAuditRecordAsync(new FailedUserSecurityPolicyAuditRecord(
+                            user.Username, GetAuditAction(action), foundPolicies)).Wait();


Want to call out this Wait() -- any better suggestions? The issue is that policy evaluation is done from an action filter, but MVC5 filters don't support async.

Alternatives, which I'm not fond of:
-Move policy evaluation inside action methods
-Fire SaveAuditRecordAsync, but don't wait
-Create the audit record here, but write from an async handler or something?

I would recommend making Evaluate async, and moving the Wait() to the attribute. In this case, if Evaluate is called from a different location in the future (say, a controller), it can be called correctly

In reply to: 116026021 [](ancestors = 116026021)

Yes agreed. Make as much of the stack async. If the filter is the thing that can't be async, that's the place that should have the .Result or .Wait().

In reply to: 116067013 [](ancestors = 116067013,116026021)

joelverhagen · 2017-05-11T18:14:37Z

src/NuGetGallery/Security/ISecurityPolicyService.cs

@@ -14,6 +16,16 @@ namespace NuGetGallery.Security
    public interface ISecurityPolicyService
    {
        /// <summary>
+        /// Auditing for the security policy service.
+        /// </summary>
+        IAuditingService Auditing { get; }


Auditing [](start = 25, length = 8)

Commented offline about this, dependencies should not be on the interface

joelverhagen · 2017-05-11T18:15:54Z

public enum AuditedUserAction

Is this enum serialized somewhere? If so, is it serialized as int or string?

Refers to: src/NuGetGallery.Core/Auditing/AuditedUserAction.cs:6 in 9358458. [](commit_id = 9358458, deletion_comment = False)

joelverhagen · 2017-05-11T18:16:14Z

src/NuGetGallery.Core/Auditing/FailedUserSecurityPolicyAuditRecord.cs

+    /// <summary>
+    /// Audit record for failed user security policy evaluations.
+    /// </summary>
+    public class FailedUserSecurityPolicyAuditRecord : AuditRecord<AuditedPackageAction>


FailedUserSecurityPolicyAuditRecord [](start = 17, length = 35)

I'm loving these read-only types. Very nice.

joelverhagen · 2017-05-11T18:17:07Z

src/NuGetGallery.Core/Auditing/UserAuditRecord.cs

@@ -16,6 +17,7 @@ public class UserAuditRecord : AuditRecord<AuditedUserAction>
        public CredentialAuditRecord[] Credentials { get; }
        public CredentialAuditRecord[] AffectedCredential { get; }
        public string AffectedEmailAddress { get; }
+        public AuditedUserSecurityPolicy[] Policies { get; }


AuditedUserSecurityPolicy [](start = 15, length = 25)

Nit, if it's all the same we should be using IEnumerable or IReadOnlyList if you need indexing. Arrays are mutable which is probably not necessary here.

@joelverhagen Left as arrays, which is consistent with other audit records properties. I'd prefer to be consistent for now, and revisit as a separate item since it's general feedback for all auditing. Added this feedback to #3946.

Yup, sounds great.

skofman1 · 2017-05-11T18:28:40Z

src/NuGetGallery.Core/Auditing/AuditedPackageAction.cs

@@ -12,7 +12,6 @@ public enum AuditedPackageAction
        Unlist,
        Edit,
        UndoEdit,
-
-
+        Verify


Shims will need to be updated as well

joelverhagen · 2017-05-11T18:49:02Z

We should wait till @dtivel fills us in the process of updating our internal auditing implementation. As it stands today, it seems like we should have both PRs ready and reviewed before we merge to dev.

joelverhagen · 2017-05-11T18:49:11Z

🕐

chenriksson · 2017-05-11T20:00:27Z

@joelverhagen action enum gets serialized as string in audit record. Also added audit record for Verify per our discussion.

This PR is against tempkeys feature branch, but agree I should have Shims PR ready before merging this to dev.

cristinamanum · 2017-05-11T20:02:17Z

src/NuGetGallery/Controllers/ApiController.cs

@@ -245,7 +245,7 @@ public virtual Task<ActionResult> GetNuGetExe()
            return (ActionResult)result ?? new EmptyResult();
        }

-        private HttpStatusCodeWithBodyResult VerifyPackageKeyInternal(User user, Credential credential, string id, string version)
+        private async Task<HttpStatusCodeWithBodyResult> VerifyPackageKeyInternal(User user, Credential credential, string id, string version)


Consider adding "async" in the method name.

cristinamanum · 2017-05-11T20:03:35Z

src/NuGetGallery/Filters/ApiAuthorizeAttribute.cs

+                var evaluateTask = SecurityPolicyService.EvaluateAsync(SecurityPolicyAction.Value, httpContext);
+                evaluateTask.Wait();
+
+                SecurityPolicyResult = evaluateTask.Result;


.Result will do the wait. There is no need for the line 95.

cristinamanum · 2017-05-11T20:06:16Z

src/NuGetGallery/Security/SecurePushSubscription.cs

                CredentialTypes.IsApiKey(c.Type) &&
                (
                    c.Scopes.Count == 0 ||
                    c.Scopes.Any(s =>
                        s.AllowedAction.Equals(NuGetScopes.PackagePush, StringComparison.OrdinalIgnoreCase) ||
                        s.AllowedAction.Equals(NuGetScopes.PackagePushVersion, StringComparison.OrdinalIgnoreCase)
                        ))
-                );
+                ).ToList();


Why was the ToList needed?

skofman1 · 2017-05-11T21:04:36Z

src/NuGetGallery.Core/Auditing/FailedUserSecurityPolicyAuditRecord.cs

+
+        public override string GetPath()
+        {
+            return Path; // store in <auditpath>/failedusersecuritypolicy/all


does this path mean the blob storage folder where this audit record is stored? In that case why not save it in the user's folder?

skofman1 · 2017-05-11T21:05:36Z

src/NuGetGallery.Core/Auditing/UserAuditAction.cs

-
-namespace NuGetGallery.Auditing
-{
-    public enum UserAuditAction


why delete?

Was dead code; UserAuditRecord uses AuditedUserAction instead.

Though, I should probably use present tense verbs for actions to match the others.

skofman1 · 2017-05-11T21:07:29Z

src/NuGetGallery.Core/Auditing/UserAuditRecord.cs

+        public UserAuditRecord(User user, AuditedUserAction action, IEnumerable<UserSecurityPolicy> policies)
+            : this(user, action, Enumerable.Empty<Credential>())
+        {
+            Policies = policies.Select(p => new AuditedUserSecurityPolicy(p)).ToArray();


policies are available as part of the user object. Why pass them separately?

I should rename AffectedPolicies and add a comment. These only include the policies that are currently being subscribed to or unsubscribed from.

skofman1 · 2017-05-11T21:08:23Z

🕐

skofman1 · 2017-05-11T22:48:04Z

Like was discussed with @joelverhagen and @dtivel , we should have auditing for successful policy validation as well.

chenriksson · 2017-05-12T16:26:50Z

@skofman1 @joelverhagen Added auditing of successful evaluation as well

joelverhagen · 2017-05-12T16:31:08Z

src/NuGetGallery/Security/SecurePushSubscription.cs

@@ -18,6 +21,9 @@ public class SecurePushSubscription : IUserSecurityPolicySubscription
        private const string MinClientVersion = "4.1.0";
        private const int PushKeysExpirationInDays = 30;

+        private IAuditingService Auditing { get; }


IAuditingService [](start = 16, length = 16)

nit: typically these are private readonly fields rather than props

dtivel · 2017-05-12T16:12:21Z

.vs/config/applicationhost.config

@@ -155,7 +155,7 @@
        <sites>
            <site name="NuGet Gallery (nuget.localtest.me)" id="2">
                <application path="/">
-                    <virtualDirectory path="/" physicalPath="..\..\src\NuGetGallery" />
+                    <virtualDirectory path="/" physicalPath="D:\Nuget\NuGetGallery\src\NuGetGallery" />


Should this file be checked in? This path doesn't exist on my machine.

Perhaps add this file to .gitignore.

dtivel · 2017-05-12T16:14:12Z

src/NuGetGallery.Core/Auditing/FailedUserSecurityPolicyAuditRecord.cs

+            }
+            if (policies == null || policies.Count() == 0)
+            {
+                throw new ArgumentException(nameof(policies));


This constructor takes a message. This other constructor takes a message and parameter name.

dtivel · 2017-05-12T16:15:53Z

src/NuGetGallery.Core/Auditing/FailedUserSecurityPolicyAuditRecord.cs

+    /// <summary>
+    /// Audit record for failed user security policy evaluations.
+    /// </summary>
+    public class FailedUserSecurityPolicyAuditRecord : AuditRecord<AuditedSecurityPolicyAction>


FailedSecurityPolicyEvaluationAuditRecord or FailedUserSecurityPolicyEvaluationAuditRecord?

dtivel · 2017-05-12T16:18:08Z

src/NuGetGallery.Core/Auditing/UserAuditRecord.cs

+        /// <summary>
+        /// Subset of user policies affected by the action (subscription / unsubscription).
+        /// </summary>
+        public AuditedUserSecurityPolicy[] AffectedPolicies { get; }


Be sure to update shims.

dtivel · 2017-05-12T16:20:19Z

src/NuGetGallery/Security/ISecurityPolicyService.cs

@@ -39,6 +39,6 @@ public interface ISecurityPolicyService
        /// <param name="action">Security policy action.</param>
        /// <param name="context">Authorization context.</param>
        /// <returns>Policy result indicating success or failure.</returns>
-        SecurityPolicyResult Evaluate(SecurityPolicyAction action, HttpContextBase httpContext);
+        Task<SecurityPolicyResult> EvaluateAsync(SecurityPolicyAction action, HttpContextBase httpContext);


/// <returns>A task that represents the asynchronous operation. /// The task result (<see cref="Task{TResult}.Result" />) returns a <see cref="SecurityPolicyResult" /> /// instance.</returns>

dtivel · 2017-05-12T16:47:20Z

tests/NuGetGallery.Facts/Security/SecurityPolicyServiceFacts.cs

@@ -56,73 +58,92 @@ public void UserHandlers()
        }

        [Fact]
-        public void EvaluateThrowsIfHttpContextNull()
+        public async void EvaluateThrowsIfHttpContextNull()


As a general rule I like to see test names in the format:

MemberName_Expectation

For example, this test would be:

EvaluateAsync_ThrowsForNullHttpContext

Then it is very clear what is being tested and what the expectation is. Without this and with refactorings over time you can be left with tests where the original intent isn't clear.

The previous test (UserHandlers) doesn't follow this format, so the intent of the test isn't stated. Other tests (like this one) don't seem to accurately describe the member under test. It may, but from a cursory look I didn't see OnSubscribe anywhere in the test. Shouldn't it be SubscribeAsync_DoesNotExpireNonPushCredentials?

Can you please review all your test names to ensure they are both accurate and clearly named? Most of them look pretty good with respect to stating expectations, but the name of the member under test isn't always accurate and some expectations are just missing.

dtivel · 2017-05-12T16:50:43Z

tests/NuGetGallery.Core.Facts/Auditing/UserSecurityPolicyAuditRecordFacts.cs

+
+namespace NuGetGallery.Auditing
+{
+    public class UserSecurityPolicyAuditRecordFacts


Need more tests than just null/empty argument validation. What about verifying that the constructor initializes properties correctly? There is non-trivial code in the constructor that is testable.

What about testing the GetPath(...) override?

dtivel · 2017-05-12T16:54:03Z

src/NuGetGallery/Security/SecurityPolicyService.cs

            return SecurityPolicyResult.SuccessResult;
        }

+        private AuditedSecurityPolicyAction GetAuditAction(SecurityPolicyAction policyAction)


Do you have EvaluateAsync(...) tests that cover each case below?

yes - reviewed

dtivel · 2017-05-12T16:55:16Z

src/NuGetGallery.Core/Auditing/UserAuditRecord.cs

@@ -55,7 +61,13 @@ public UserAuditRecord(User user, AuditedUserAction action, string affectedEmail
        {
            AffectedEmailAddress = affectedEmailAddress;
        }
-
+
+        public UserAuditRecord(User user, AuditedUserAction action, IEnumerable<UserSecurityPolicy> affectedPolicies)


Please add tests for this constructor in UserAuditRecordTests.cs.

dtivel · 2017-05-12T16:56:20Z

tests/NuGetGallery.Facts/Security/SecurePushSubscriptionFacts.cs

@@ -17,15 +19,18 @@ public class SecurePushSubscriptionFacts
        [Fact]
        public void SubscriptionName()
        {
+            // Arrange.
+            var subscription = CreateSecurityPolicyService().UserSubscriptions.First();


Would Single() be a stronger test? The same feedback applies to subsequent tests.

loic-sharma · 2017-05-12T18:05:34Z

src/NuGetGallery/Security/SecurePushSubscription.cs

                if (!key.Expires.HasValue || key.Expires > expires)
                {
+                    await _auditing.SaveAuditRecordAsync(


From my understanding, this will save each audit record sequentially. Would it be better to kick off all of the save audit record tasks and then doing an await Task.WhenAll?

dtivel

Looks good. Thanks, Christy!

loic-sharma · 2017-05-15T16:28:39Z

src/NuGetGallery/Security/SecurityPolicyService.cs

@@ -88,7 +88,8 @@ public virtual IEnumerable<IUserSecurityPolicySubscription> UserSubscriptions
                    var result = handler.Evaluate(new UserSecurityPolicyEvaluationContext(httpContext, foundPolicies));

                    await Auditing.SaveAuditRecordAsync(new UserSecurityPolicyAuditRecord(
-                        user.Username, GetAuditAction(action), foundPolicies, result.Success, result.ErrorMessage));
+                        user.Username, GetAuditAction(action), foundPolicies, result.Success, result.ErrorMessage))
+                        .ConfigureAwait(false);


Can you explain this? From my understanding, this means you don't need to save the current context, but aren't you using the context's result directly after this await?

Mistake... removed. Had accidentally left it in after trying something...

skofman1 · 2017-05-16T04:40:32Z

src/NuGetGallery.Core/Auditing/UserSecurityPolicyAuditRecord.cs

+namespace NuGetGallery.Auditing
+{
+    /// <summary>
+    /// Audit record for failed user security policy evaluations.


failed and succeeded policies evaluation

skofman1 · 2017-05-16T04:46:39Z

src/NuGetGallery/App_Start/DefaultDependenciesModule.cs

@@ -207,6 +207,10 @@ protected override void Load(ContainerBuilder builder)
                .As<ISecurityPolicyService>()
                .InstancePerLifetimeScope();

+            builder.RegisterType<SecurePushSubscription>()


any reason not to have it as a SingleInstance() ? Same for SecurityPolicyService. The more we use singleton, the cheaper a request will be.

skofman1 · 2017-05-16T04:50:06Z

src/NuGetGallery/Controllers/ApiController.cs

+
+            // Write an audit record
+            await AuditingService.SaveAuditRecordAsync(
+                new PackageAuditRecord(package, AuditedPackageAction.Verify, "Verified via API."));


is the string "Verified via API." needed? The action implies that already. Consider removing.

skofman1 · 2017-05-16T04:57:01Z

src/NuGetGallery/Security/SecurityPolicyService.cs

@@ -182,7 +233,7 @@ private static IEnumerable<UserSecurityPolicyHandler> CreateUserHandlers()
        /// </summary>
        private static IEnumerable<IUserSecurityPolicySubscription> CreateUserSubscriptions()
        {
-            yield return new SecurePushSubscription();
+            yield return DependencyResolver.Current.GetService<SecurePushSubscription>();


why not use dependency injection in the ctor?

chenriksson added 2 commits May 11, 2017 07:36

Temp keys telemetry and auditing

9358458

cleanup

ecbe381

dnfclas added the cla-already-signed label May 11, 2017

chenriksson requested review from dtivel, joelverhagen, cristinamanum, skofman1, scottbommarito, agr, loic-sharma and ryuyu May 11, 2017 15:36

chenriksson commented May 11, 2017

View reviewed changes

joelverhagen reviewed May 11, 2017

View reviewed changes

skofman1 reviewed May 11, 2017

View reviewed changes

PR feedback

b2eb7b9

cristinamanum reviewed May 11, 2017

View reviewed changes

cristinamanum approved these changes May 11, 2017

View reviewed changes

PR feedback

1898e07

skofman1 reviewed May 11, 2017

View reviewed changes

PR feedback

b333ed8

PR feedback

fc0de20

revert apphost.config

f72f6d7

joelverhagen reviewed May 12, 2017

View reviewed changes

PR feedback

8f86fbc

dtivel requested changes May 12, 2017

View reviewed changes

loic-sharma reviewed May 12, 2017

View reviewed changes

PR feedback

b1bb73c

dtivel approved these changes May 12, 2017

View reviewed changes

Fixing deadlock

a2fc8c8

loic-sharma reviewed May 15, 2017

View reviewed changes

Removing unintended code

236c5f0

skofman1 reviewed May 16, 2017

View reviewed changes

skofman1 approved these changes May 16, 2017

View reviewed changes

PR feedback

b8e9b12

skofman1 approved these changes May 16, 2017

View reviewed changes

chenriksson merged this pull request into tempkeys May 16, 2017

chenriksson deleted the chenriks-tempkeys-auditing branch May 16, 2017 18:34

chenriksson added a commit that referenced this pull request May 16, 2017

Temp keys telemetry and auditing (#3945)

85850ea

chenriksson added a commit that referenced this pull request May 16, 2017

Temp keys telemetry and auditing (#3945)

cdc08cb

Temp keys telemetry and auditing #3945

Temp keys telemetry and auditing #3945

Conversation

chenriksson commented May 11, 2017

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

joelverhagen commented May 11, 2017

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

joelverhagen commented May 11, 2017

joelverhagen commented May 11, 2017

chenriksson commented May 11, 2017

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 May 11, 2017

skofman1 commented May 11, 2017

chenriksson commented May 12, 2017

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

loic-sharma May 12, 2017 • edited Loading

Choose a reason for hiding this comment

dtivel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenriksson May 15, 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

skofman1 May 16, 2017 • edited Loading

Choose a reason for hiding this comment

loic-sharma May 12, 2017 •

edited

Loading

chenriksson May 15, 2017 •

edited

Loading

skofman1 May 16, 2017 •

edited

Loading