Temp keys user policies #3820

chenriksson · 2017-04-25T15:27:22Z

Adding support for user security policies, including policies for use of temp keys and min client version

Onboarding and auditing will be separate PRs

skofman1 · 2017-04-25T15:58:01Z

src/NuGetGallery/Controllers/ApiController.cs

@@ -293,6 +294,7 @@ private HttpStatusCodeWithBodyResult VerifyPackageKeyInternal(User user, Credent
        [RequireSsl]
        [ApiAuthorize]
        [ApiScopeRequired(NuGetScopes.PackagePush, NuGetScopes.PackagePushVersion)]
+        [SecurityPolicy(SecurityPolicyAction.PackagePush)]


consider merging SecurityPolicy attribute with ApiAuthorize attribute.

skofman1 · 2017-04-25T16:03:37Z

src/NuGetGallery/App_Start/DefaultDependenciesModule.cs

+            builder.RegisterType<SecurityPolicyService>()
+                .AsSelf()
+                .As<ISecurityPolicyService>()
+                .InstancePerLifetimeScope();


Any reason for ISecurityPolicyService not to be a single instance (SingleInstance())?

I think we should prefer InstancePerLifetimeScope over SuntingleInstance until we have a caching/perf/behavior requirement for it. It's easier for unintended interaction between requests to occur with SingleInstance

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

skofman1 · 2017-04-25T16:06:05Z

src/NuGetGallery/Security/SecurityPolicyService.cs

+    /// </summary>
+    public class SecurityPolicyService : ISecurityPolicyService
+    {
+        public IEnumerable<UserSecurityPolicyHandler> UserPolicyHandlers { get; private set; }


consider making this a static

skofman1 · 2017-04-25T16:10:56Z

// Copyright (c) .NET Foundation. All rights reserved.

null checks

Refers to: src/NuGetGallery/Security/UserSecurityPolicyContext.cs:1 in d018112. [](commit_id = d018112, deletion_comment = False)

cristinamanum · 2017-04-25T16:11:55Z

src/NuGetGallery.Core/Entities/EntitiesContext.cs

+                .WillCascadeOnDelete(true);
+
+            modelBuilder.Entity<UserSecurityPolicy>()
+                .HasKey(p => p.Key);


Is not this duplication of the line 123.

skofman1 · 2017-04-25T16:18:08Z

Looks great! 👍

cristinamanum · 2017-04-25T16:47:27Z

src/NuGetGallery/Security/RequireMinClientVersionForPushPolicy.cs

+        private Version GetMaxOfMinClientVersions(UserSecurityPolicyContext context)
+        {
+            var policyStates = context.Policies.Select(p => JsonConvert.DeserializeObject<State>(p.Value));
+            return policyStates.Max(s => s.MinClientVersion);


Can be nulls among these values? In this case will the Max throw?

Shouldn't be null, but added logic to guard against it

cristinamanum · 2017-04-25T16:52:58Z

src/NuGetGallery/Security/RequireMinClientVersionForPushPolicy.cs

+            var clientVersionString = context.HttpContext.Request?.Headers[Constants.ClientVersionHeaderName];
+
+            Version clientVersion;
+            return Version.TryParse(clientVersionString, out clientVersion) ? clientVersion : null;


Do we want to log in AI the null case? The Evaluate below will return a non success result, the user scenario may be impacted in this case.

Instead of logging in individual handlers, I'm planning to add telemetry and auditing to the SecurityPolicyService in a separate PR. AI can log any policy failures and include username, policy name and value.

If that will be in a different PR is ok.

cristinamanum · 2017-04-25T16:53:29Z

src/NuGetGallery/Security/RequireMinClientVersionForPushPolicy.cs

+            if (clientVersion == null || clientVersion < minClientVersion)
+            {
+                return new SecurityPolicyResult(false, string.Format(CultureInfo.CurrentCulture,
+                    Strings.SecurityPolicy_RequireMinClientVersionForPush, minClientVersion));


Do we want to log in AI the fact that the clientVersion < minClientVersion?

joelverhagen · 2017-04-25T17:30:57Z

src/NuGetGallery/Security/RequireMinClientVersionForPushPolicy.cs

+        {
+            [JsonProperty("v")]
+            [JsonConverter(typeof(VersionConverter))]
+            public Version MinClientVersion { get; set; }


Version [](start = 19, length = 7)

should be a NuGetVersion, really

joelverhagen · 2017-04-25T17:31:32Z

src/NuGetGallery/Security/UserSecurityPolicyHandler.cs

+    /// </summary>
+    public abstract class UserSecurityPolicyHandler
+    {
+        public string Name { get; private set; }


private [](start = 34, length = 7)

can remove private set;
This is readonly right?

joelverhagen · 2017-04-25T17:31:59Z

src/NuGetGallery/Security/SecurityPolicyResult.cs

+        /// <summary>
+        /// Whether security policy criteria was successfully met.
+        /// </summary>
+        public bool Success { get; private set; }


private [](start = 35, length = 7)

can remove private set; as this is readonly

joelverhagen · 2017-04-25T17:37:19Z

src/NuGetGallery.Core/Entities/UserSecurityPolicy.cs

+        /// <summary>
+        /// Type name for the policy handler that provides policy behavior.
+        /// </summary>
+        [Required]


Required [](start = 9, length = 8)

Can' this [Required] attribute be defined in EntitiesContext instead? Seems like you are requiring User.

HasRequired in EntitiesContext is for a required relationship. I added Required here since it's the only non-nullable column that isn't a PK/FK. Not sure how this actually gets used, but we don't seem to be consistent with [Required] usage.

joelverhagen · 2017-04-25T17:38:01Z

src/NuGetGallery/Filters/SecurityPolicyAttribute.cs

+        /// <summary>
+        /// Security policy service.
+        /// </summary>
+        public ISecurityPolicyService SecurityPolicyService { get; private set; }


ISecurityPolicyService [](start = 15, length = 22)

why public?

joelverhagen · 2017-04-25T17:44:55Z

tests/NuGetGallery.Facts/Security/RequireMinClientVersionForPushPolicyFacts.cs

+        [InlineData("4.1.0")]
+        [InlineData("3.0.0")]
+        [InlineData("2.0.0,4.1.0")]
+        public void EvaluateReturnsFailureIfClientVersionLower(string minClientVersions)


EvaluateReturnsFailureIfClientVersionLower [](start = 20, length = 42)

client version is a NuGetVersion not a Version so we should test a couple NuGetVersion variants as well

4.1.0-beta1

joelverhagen · 2017-04-25T17:46:01Z

🕐

joelverhagen · 2017-04-25T17:47:57Z

src/NuGetGallery.Core/Entities/UserSecurityPolicy.cs

+        /// Type name for the policy handler that provides policy behavior.
+        /// </summary>
+        [Required]
+        public string Name { get; set; }


Name [](start = 22, length = 4)

I think type name is an implementation detail. We should have a hard coded constant for this (either an int which is consistent with the rest of the DB or const string).

Policy handler names are now constants. I left as strings which I think will be easier to read when querying the DB. This PR only reads policies, but the next PR (onboarding) will start writing them.

joelverhagen · 2017-04-25T17:48:30Z

src/NuGetGallery/Security/RequirePackageVerifyScopePolicy.cs

+    public class RequirePackageVerifyScopePolicy : UserSecurityPolicyHandler
+    {
+        public RequirePackageVerifyScopePolicy()
+            : base(nameof(RequirePackageVerifyScopePolicy), SecurityPolicyAction.PackageVerify)


RequirePackageVerifyScopePolicy [](start = 26, length = 31)

Please use a const string or const int for this. nameof is easy to change with refactoring tools

I see you have unit tests for this so we aren't blind to the change, but I think we should be a bit more explicit about this string. It could even be a public const string property on this class.

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

joelverhagen · 2017-04-25T17:54:47Z

src/NuGetGallery/Security/SecurityPolicyResult.cs

+    {
+        public static SecurityPolicyResult SuccessResult = new SecurityPolicyResult(true, null);
+
+        public SecurityPolicyResult(bool success, string errorMessage)


SecurityPolicyResult [](start = 15, length = 20)

seems like success = true and errorMessage != null is an invalid state, right? I like making the ctor private in this case and having a CreateErrorResult(string errorMessage) method.

joelverhagen · 2017-04-25T17:55:28Z

src/NuGetGallery/Security/SecurityPolicyService.cs

+            yield return new RequireMinClientVersionForPushPolicy();
+            yield return new RequirePackageVerifyScopePolicy();
+        }
+


nit: remove extra line

joelverhagen · 2017-04-25T17:57:36Z

src/NuGetGallery/Security/RequirePackageVerifyScopePolicy.cs

+    public class RequirePackageVerifyScopePolicy : UserSecurityPolicyHandler
+    {
+        public RequirePackageVerifyScopePolicy()
+            : base(nameof(RequirePackageVerifyScopePolicy), SecurityPolicyAction.PackageVerify)


SecurityPolicyAction [](start = 60, length = 20)

Maybe I am missing it but where are we asserting that RequirePackageVerifyScopePolicy has action PackageVerify and not something else?

joelverhagen · 2017-04-25T17:58:01Z

Functional test(s)?

chenriksson · 2017-04-25T21:28:18Z

@joelverhagen I need to create onboarded test account for the functional test. Am thinking of doing onboarding in next PR, then auditing/telemetry, then functional test.

joelverhagen · 2017-04-25T21:30:12Z

Please make sure the deployment task mentions running EF migrations.

loic-sharma · 2017-04-27T17:23:37Z

src/NuGetGallery/Filters/ApiAuthorizeAttribute.cs

+            // Resolve the policy service if security policy checks are required.
+            if (SecurityPolicyAction.HasValue)
+            {
+                SecurityPolicyService = ((AppController)filterContext.Controller)?.GetService<ISecurityPolicyService>();


This will throw if the ApiAuthorizeAttribute is used on a controller that doesn't extend AppController, right? I would recommend that you add documentation that this attribute must be used on controllers that extend AppController. Also, maybe throw an exception if the controller isn't an AppController to make this easier to debug if this issue ever comes up?
*

loic-sharma · 2017-04-27T17:34:58Z

src/NuGetGallery/Security/RequireMinClientVersionForPushPolicy.cs

+            var policyStates = context.Policies
+                .Where(p => !string.IsNullOrEmpty(p.Value))
+                .Select(p => JsonConvert.DeserializeObject<State>(p.Value));
+            return policyStates.Max(s => s.MinClientVersion);


Any particular reason for the policyStates variable? You could merge the two statements into one and return its result directly.

loic-sharma · 2017-04-27T17:37:36Z

src/NuGetGallery/Security/SecurityPolicyResult.cs

+        /// <returns></returns>
+        public static SecurityPolicyResult CreateErrorResult(string errorMessage)
+        {
+            if (string.IsNullOrEmpty(errorMessage))


string.IsNullOrEmpty seems a little too strong. Would a null check be better here?

chenriksson added 2 commits April 25, 2017 08:13

Temp keys security policies

3bd8f93

Fix test name

d018112

dnfclas added the cla-already-signed label Apr 25, 2017

chenriksson requested review from skofman1, scottbommarito, joelverhagen, ryuyu and cristinamanum April 25, 2017 15:27

skofman1 reviewed Apr 25, 2017

View reviewed changes

cristinamanum reviewed Apr 25, 2017

View reviewed changes

joelverhagen reviewed Apr 25, 2017

View reviewed changes

chenriksson added 3 commits April 25, 2017 13:44

PR feedback

85c8612

cleanup

8308c39

PR: guard against nulls

55ce5ba

joelverhagen approved these changes Apr 25, 2017

View reviewed changes

cristinamanum approved these changes Apr 27, 2017

View reviewed changes

chenriksson merged commit 53e74ae into dev Apr 27, 2017

scottbommarito mentioned this pull request Apr 27, 2017

Deployment 2017.05.03 #3830

Closed

22 tasks

chenriksson deleted the chenriks-tempkeys-policies branch April 27, 2017 14:23

loic-sharma reviewed Apr 27, 2017

View reviewed changes

chenriksson mentioned this pull request Apr 28, 2017

Support Is(Absolute)Latest for SemVer2 + semVerLevel for SearchService #3842

Merged

Temp keys user policies #3820

Temp keys user policies #3820

Conversation

chenriksson commented Apr 25, 2017

Choose a reason for hiding this comment

skofman1 Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skofman1 commented Apr 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skofman1 commented Apr 25, 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

joelverhagen commented Apr 25, 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

joelverhagen commented Apr 25, 2017

chenriksson commented Apr 25, 2017

joelverhagen commented Apr 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skofman1 Apr 25, 2017 •

edited

Loading