Auto-add Microsoft a co-owner for packages pushed by blessed Organizations #6150

xavierdecoster · 2018-07-09T08:47:02Z

Fixes https://github.com/NuGet/Engineering/issues/1419

Opening this PR already for early review and feedback.
Note: this focuses on the package push API side of things. Not covering the web upload scenario, yet.

chenriksson · 2018-07-09T17:24:38Z

Correct issue link (Engineering):
https://github.com/NuGet/Engineering/issues/1419

chenriksson · 2018-07-09T19:29:38Z

src/NuGetGallery.Core/Services/CoreMessageService.cs

+            }
+        }
+
+        public void SendPackageAddedWithWarningsNotice(Package package, string packageUrl, string packageSupportUrl, string emailSettingsUrl, IEnumerable<string> warningMessages)


Why copy, instead of adding optional warningMessages parameter to existing SendPackageAddedNotice API?

Ah, good one! Fixing :)

chenriksson · 2018-07-09T19:32:35Z

src/NuGetGallery/Controllers/ApiController.cs

-                                    Url.AccountSettings(relativeUrl: false));
+                                if (!packagePolicyResult.HasWarnings)
+                                {
+                                    // Notify user of push with warnings unless async validation in blocking mode is used


nit: comment says this is push with warnings, but that's the else block

chenriksson · 2018-07-09T19:35:35Z

src/NuGetGallery/Security/RequireMicrosoftPackageCompliancePolicy.cs

+            }
+
+            // This particular package policy assumes the existence of a 'Microsoft' user.
+            // Succeed silentely (effectively ignoring this policy when enabled) when that user does not exist.


nit: silently

Thx! Fixed.

chenriksson · 2018-07-09T19:37:40Z

src/NuGetGallery/Security/RequireMicrosoftPackageCompliancePolicy.cs

+            if (context.ExistingPackageRegistration == null)
+            {
+                // We are evaluating a newly pushed package with a new ID.
+                var packageId = context.Package.PackageRegistration.Id;


nit: packageRegistrationId?

chenriksson · 2018-07-09T19:40:28Z

src/NuGetGallery/Security/RequireMicrosoftPackageCompliancePolicy.cs

+                if (!context.Package.PackageRegistration.IsVerified)
+                {
+                    // The owner has not reserved the prefix. Check whether the Microsoft user has.
+                    var prefixIsReservedByMicrosoft = IsPrefixReservedByAccount(context.EntitiesContext, microsoftUser, packageId);


If current owner doesn't own prefix, but MS does -- would the push have already failed before we got here?

Yes... good catch!

The following error is shown when this happens today:

Response status code does not indicate success: 409 (This package ID has been reserved. Please request access to upload to this reserved namespace from the owner of the reserved prefix, or re-upload the package with a different ID.).

This happens in ApiController on line 424, which happens before evaluating the package policies (as we only read the Stream and construct the package later in that call...).

Does that mean that, for users that have this RequireMicrosoftPackageCompliancePolicy set, we should somehow skip that check?

According to the requirements set forth in NuGet/Engineering#1419, we generate a warning (to be sent in the publish successful email), and continue validation, which results in successful package push if the package is deemed compliant.

@skofman1 @chenriksson any thoughts on this question?

Does that mean that, for users that have this RequireMicrosoftPackageCompliancePolicy set, we should somehow skip that check?

I wouldn't skip the check--the more this Microsoft upload flow differs from the normal upload flow, the more likely we are to have bugs.

@skofman1 mentioned that she thinks this policy should be split into 2, one handling the reserved namespace ownership and the other verifying the metadata of the package. I think the reserved namespace policy could be evaluated after getting the ID and version (before the ownership checks are) and the metadata policy could trigger where the combined policy does.

This would enable you to check the reserved namespaces that the package applies to and add Microsoft as an owner of the package before the existing ownership checks are so you won't have to skip them.

@scottbommarito in the event of a newly uploaded package id for which the prefix is not reserved by either the account pushing or Microsoft, we should still continue to validate metadata (see flowchart on NuGet/Engineering#1419), so there's an ordered dependency between such two policies (which is why I considered them one).

Furthermore, the current api scope validation would prevent the code to reach the metadata validation part.

In fact, the only thing that the prefix policy does according to the flowchart, is deciding whether an alternate email is to be sent upon successful validation of the metadata policy...

I think the flowchart is not important. The goal of the feature is that Microsoft is automatically added as a co-owner of packages owned by certain users, those packages must abide by certain metadata rules, and that an email must be sent when those packages are not in a reserved namespace. As long as those goals are accomplished, I don't think the ordering is important, especially given that the current ordering isn't working with our existing flow.

I think to resolve your issue, you should add Microsoft as a co-owner of the package (before the ownership check), make sure the ownership check includes Microsoft, and then do all the validations that you currently do where the evaluation currently is.

Agree with Scott on this. I would keep the code as simple as possible to achieve the goals:

Have a separate policy for metadata - if policy exists for account fail package push. In this case we don't need to worry about reserved namespaces at all.

Have a separate policy for "auto add c-owner" - make sure to run this verification after the metadata one in the code (this might be as simple as putting it in the next block) if you see this on the account add co-owner and send mail if needed.

Regarding:

Does that mean that, for users that have this RequireMicrosoftPackageCompliancePolicy set, we should somehow skip that check?

Lets keep it simple and continue with the check. When a new MSFT team onboards we will manually assign to them all prefixes and security policies they need.

So we can close up on this, and conclude that we do want an upload to fail if Microsoft owns the prefix but the pushing user does not.

This simplifies things a lot, and all validations can be part of a single package policy evaluated where it currently is (after api scope evaluation and before committing the package).

chenriksson · 2018-07-09T19:53:45Z

src/NuGetGallery/Security/RequireMicrosoftPackageCompliancePolicy.cs

+                    isWarning = !prefixIsReservedByMicrosoft;
+
+                    // Mark the package as verified if the prefix has been reserved by Microsoft.
+                    context.Package.PackageRegistration.IsVerified = prefixIsReservedByMicrosoft;


Do we need to monitor for these warnings, and possibly register these prefixes for these unverified packages to the team accounts we notice? I assume PMs would need to notify us, since they manage the MS account?

As per the requirements set forth in NuGet/Engineering#1419, the owners will receive an email containing warning messages, and instructions to reserve the prefix.

can we make such emails also go to [email protected]? That's the alias we monitor for prefix reservation.

We can, but it's not the policy who is in charge of sending the emails. It's the ApiController that sends them.

Do we want to include [email protected] in all package added notices that generated warnings during package policy validation? (currently, this is the only package policy)

only in case of prefix not reserved warning, we should send to [email protected]. If this is easily doable, great! If not, that's ok too, Daniel and I can just create a new filter based on the subject line :)

It's non-trivial to make that distinction currently, so I guess the filter is the way to go for now :)

chenriksson · 2018-07-09T19:55:47Z

src/NuGetGallery/Security/RequireMicrosoftPackageCompliancePolicy.cs

+            var reservedNamespacesForId = (from request in entitiesContext.Set<ReservedNamespace>()
+                                           where (request.IsPrefix && id.StartsWith(request.Value))
+                                               || (!request.IsPrefix && id.Equals(request.Value))
+                                           select request).ToList();


Should this logic be in the ReservedNamespaceService, like the other checks?

The tricky part is that I don't have access to the ReservedNamespaceService and I can't inject it without causing a circular dependency loop (which is why the IEntitiesContext is passed along instead, which is the most generic way of providing access to the db).

Can you explain the circular dependency loop? I don't see it here, and I think it would be easy for ApiController to pass a ReservedNamespaceService to the context.

I did exactly that before changing it again to the current approach, and had a circular dependency error when injecting both ReservedNamespaceService and UserService instead of IEntitiesContext, as ReservedNamespaceService also depends on UserService.

So, to clarify

UserService depends on SecurityPolicyService

ReservedNamespaceService depends on UserService

SecurityPolicyService creates the PackageSecurityPolicyEvaluationContext to pass to the RequireMicrosoftPackageCompliancePolicy using the services it's been injected with

Regardless of this loop, I think it's really important for you to be able to get these services for your DI. After discussing this a bit with @loic-sharma, he suggested making SecurityPolicyService dependent on the DI container (IContainer?) and then using it to get the services. There might be a better way, but I think it's important you can get access to whatever services you need in the policy handler.

I agree it is important to gain access to those services...

Do we have an example where we inject the Autofac.IContainer anywhere? Or should we use DependencyResolver.Current?

To clarify @scottbommarito's comment: I suggested that you use the service locator pattern. From this StackOverflow post, it seems that the proper pattern in Autofac is to accept an IComponentContext.

I don't know off-hand if the Gallery has used this pattern before. However, I do know that the Service Bus jobs use the service locator pattern using ASP.NET's dependency injection libraries here.

Circular dependencies are typically resolved by requesting Lazy<X> (or Func<X>) instead of X in one of the constructors to break the loop. Is it possible to do in this case?

chenriksson · 2018-07-09T19:57:53Z

src/NuGetGallery/Security/RequireMicrosoftPackageCompliancePolicy.cs

+        private bool IsPackageMetadataCompliant(Package package)
+        {
+            // Author validation
+            if (!package.FlattenedAuthors.Split(new[] { ',', ' ' }, StringSplitOptions.RemoveEmptyEntries).Contains(MicrosoftUsername))


Contains should probably use IEqualityComparer w/o ignore case?

Are usernames case-sensitive? I thought they weren't, so we should ignore casing?

They're not case sensitive, but I believe code analysis usually suggests that equality comparer be used in these APIs. It may be good to be explicit anyways - using InvariantCulture, instead of InvariantCultureIgnoreCase.

I believe you meant InvariantCultureIgnoreCase to be used, as the comparison is not case-sensitive. Fixed.

skofman1 · 2018-07-10T00:38:39Z

src/NuGetGallery/Security/RequireMicrosoftPackageCompliancePolicy.cs

+            // If the package being evaluated has not been registered yet (new package ID),
+            // this policy requires the prefix to be reserved.
+            var isWarning = false;
+            if (context.ExistingPackageRegistration == null)


Can we split this policy into two policies:

Policy that ensure reserved prefix

Policy that validates metadata. The metadata policy will be general, i.e. the required values will be loaded from the DB config. This will also be useful to configure it differently for teams that use different copyrights, like Xamarin.

The two policies can be part of a "Subscription", like what we did for Secure Push.

skofman1 · 2018-07-10T00:42:02Z

src/NuGetGallery/Security/RequireMicrosoftPackageCompliancePolicy.cs

+            if (!string.Equals(package.Copyright, "(c) Microsoft Corporation. All rights reserved.")
+                && !string.Equals(package.Copyright, "© Microsoft Corporation. All rights reserved."))
+            {
+                return false;


We should give the user a good error message about why the package is not compliant.

skofman1 · 2018-07-10T00:49:12Z

src/NuGetGallery/Security/SecurityPolicyService.cs

+        /// <param name="package">The package to evaluate.</param>
+        /// <param name="packageRegistration">The package registration. Will be <code>null</code> if the <paramref name="package"/> has a new package ID.</param>
+        /// <returns></returns>
+        public Task<SecurityPolicyResult> EvaluatePackagePoliciesAsync(


The name implies that the security policy is on a package, and not on a user. But it's actually on a user... perhaps we should use the existing method name (EvaluateUserPoliciesAsync)? Same thought about other parts of the code. What do you think?

They evaluate a package, not a user. The user is the subscriber to the policy, but the object being evaluated by the policy is the package.

The API is also quite different, as a Package, PackageRegistration and IEntitiesContext are provided to the handlers to do their thing. They are also evaluated at a different time in the flow. To me, they are similar, yet different.

skofman1 · 2018-07-10T00:50:54Z

src/NuGetGallery/Security/RequireMicrosoftPackageCompliancePolicy.cs

+    {
+        public const string PolicyName = nameof(RequireMicrosoftPackageCompliancePolicy);
+
+        internal const string MicrosoftUsername = "Microsoft";


can Microsoft be a parameter to the policy (from DB)?

I'm assuming you refer to the JSON State stored with the policy? Sure thing.

There's likely more to it than just changing this username to make this a reusable policy (e.g. copyright notice, maybe additional or different validations).
I thought we discussed that for v1, this was not critical.

To be able to use this in a flexible way, we'll need to work on admin UI, right?
In fact, as this policy is not part of the DefaultSubscription, we need an explicit action to subscribe to this policy given a set of configurable parameters (similar to the "Transform user to organization" action).

I think we have two options here:

Keep the policies general and configurable, and add the "Microsoft" specific settings in the Subscription level. i.e, there will be a MicrosoftTeam subscription that will create policies with the specific configs for user name, copyright, etc. The admin UI can remain simple. In edge cases like Xamarin packages needing different config, we can go to DB.

Keep the subscription general as well, and have the user provide the config in the admin UI. It's more UI work but helps avoid harding "Microsoft" in our code.

scottbommarito · 2018-07-10T18:04:02Z

src/NuGetGallery/Security/RequireMicrosoftPackageCompliancePolicy.cs

+                    isWarning = !prefixIsReservedByMicrosoft;
+
+                    // Mark the package as verified if the prefix has been reserved by Microsoft.
+                    context.Package.PackageRegistration.IsVerified = prefixIsReservedByMicrosoft;


This feels hacky. By setting the flag here directly we are bypassing the normal code path that's used to set this flag. Furthermore, this could lead to many issues because most of the code does not expect a package to be marked as verified when it doesn't actually have any owners that own its reserved namespace.

I agree, I haven't yet reviewed the full code, but, we shouldn't mark the package as verified during upload, rather we should allow the upload(the restriction on the non-owner of namespace) and when Microsoft is added, it should auto add the IsVerified since it owns the namespace it belongs to.

We're no longer setting IsVerified manually. This is now done by calling the IPackageOwnershipManagementService.AddPackageOwnerAsync API.

scottbommarito · 2018-07-10T18:07:23Z

src/NuGetGallery/Security/RequireMicrosoftPackageCompliancePolicy.cs

+            // Automatically add 'Microsoft' as co-owner when metadata is compliant.
+            if (!context.Package.PackageRegistration.Owners.Select(o => o.Username).Contains(MicrosoftUsername, StringComparer.OrdinalIgnoreCase))
+            {
+                context.Package.PackageRegistration.Owners.Add(microsoftUser);


PackageService.AddPackageOwnerAsync is the intended way to add owners to a PackageRegistration. We are bypassing the normal code path here which WILL result in the required signer of the package not being set as expected. I know there is also a method that will not only add the owner but also mark the package as verified if the owner owns the reserved namespace it's in. I would suggest doing that instead of making these changes to the package manually.

Thanks for the pointers! Those calls all commit to the db, right?
This "manual" stuff may happen before the registration even exists in the db... (e.g. when pushing a new ID). Committing to the db should remain a single atomic operation handled by the IUploadService. I may need to tweak those existing APIs with a bool commitChanges = true argument.

Adding such overload for SetRequiredSignerAsync is maybe not what we're after...

Yep, we regularly add bool commitChanges = true to our existing APIs if we need to call them but not commit. I would do that and then call the API here.

After looking through the code, it seems like PackageOwnershipManagementService.AddPackageOwnerAsync is the API you should be calling. I would add the commitChanges parameter there and to any APIs that it calls.

Adding the commitChanges parameter to IPackageOwnershipManagementService.AddPackageOwnerAsync (which creates a db transaction) affects many other APIs called by this method.

Including the PackageService.SetRequiredSignerAsync method. How would you add this parameter in there, which also saves audit records?

This all makes me wonder if it would make sense for these new package policies to have a Pre-Upload and Post-Upload evaluation moment...

Check prefix ownership and metadata validation pre-upload, and if the policy generates no errors, commit the package (and registration in case of new id), then continue with post-upload actions as defined by the policy and based on pre-upload evaluation result...

However, the co-owner must be added before signing validation.

Thoughts? Could this potentially mess up async validation? (what triggers the async validation?)

Any additional commits in the upload flow WILL break async validation. We had a live site that was caused by a similar bug. You may be able to get away with additional commits if they happen AFTER the existing commit but it sounds risky, especially if the first commit succeeds and the second fails.

Regarding adding commitChanges, I would check around, but I think in most places we just change

await _entitiesContext.SaveChangesAsync();

to

if (commitChanges) { await _entitiesContext.SaveChangesAsync(); }

while ignoring anything else going on in the method, including audit records. I think we already accept that it's possible that an audit record may be emitted for an operation that failed afterwards.

scottbommarito · 2018-07-10T18:08:53Z

src/NuGetGallery/Security/RequireMicrosoftPackageCompliancePolicy.cs

+            var reservedNamespacesForId = (from request in entitiesContext.Set<ReservedNamespace>()
+                                           where (request.IsPrefix && id.StartsWith(request.Value))
+                                               || (!request.IsPrefix && id.Equals(request.Value))
+                                           select request).ToList();


Can you explain the circular dependency loop? I don't see it here, and I think it would be easy for ApiController to pass a ReservedNamespaceService to the context.

scottbommarito · 2018-07-10T18:10:05Z

src/NuGetGallery/Strings.resx

@@ -911,4 +911,10 @@ If you would like to update the linked Microsoft account you can do so from the
  <data name="FailedValidationHardDeleteReason" xml:space="preserve">
    <value>Automatic hard-delete for reupload of package that failed validation</value>
  </data>
+  <data name="SecurityPolicy_RequireMicrosoftPackageMetadataComplianceForPush" xml:space="preserve">
+    <value>Package is not compliant with metadata requirements for Microsoft packages on NuGet.org. Go to &lt;insert URL here&gt; for more information.</value>


reminder that insert URL needs to be updated when the URL is available

The package is not compliant with metadata requirements for Microsoft packages on NuGet.org. Go to http://aka.ms/Microsoft-NuGet-Compliance for more information.

Thanks! :) Fixed.

scottbommarito · 2018-07-10T18:11:22Z

src/NuGetGallery/Security/RequireMicrosoftPackageCompliancePolicy.cs

+        private bool IsPackageMetadataCompliant(Package package)
+        {
+            // Author validation
+            if (!package.FlattenedAuthors.Split(new[] { ',', ' ' }, StringSplitOptions.RemoveEmptyEntries).Contains(MicrosoftUsername))


Is there anything else that parses the list of FlattenedAuthors? I feel like there should be a shared API for parsing the FlattenedAuthors into a string list.

This seems to be the only place in the gallery code where we need to do this.

scottbommarito · 2018-07-10T18:13:58Z

src/NuGetGallery/Security/RequireMicrosoftPackageCompliancePolicy.cs

+
+        public Task OnSubscribeAsync(UserSecurityPolicySubscriptionContext context)
+        {
+            return Task.CompletedTask;


Maybe we should enumerate through the user's packages and add Microsoft as a package owner if the package passes the metadata requirements when a user is onboarded to this policy.

That sounds like a good idea. We should also unlock the package if it is locked as part of adding Microsoft a co-owner.

Created tracking issue for this suggestion.

scottbommarito · 2018-07-10T18:15:27Z

src/NuGetGallery/Security/SecurityPolicyResult.cs

@@ -42,5 +62,9 @@ public static SecurityPolicyResult CreateErrorResult(string errorMessage)
        /// Error message, if the security policy criteria was not met.
        /// </summary>
        public string ErrorMessage { get; }
+
+        public bool HasWarnings { get { return _warningMessages.Any(); } }


nit: public bool HasWarnings => _warningMessages.Any();

scottbommarito · 2018-07-10T18:15:46Z

src/NuGetGallery/Security/SecurityPolicyResult.cs

+
+        public bool HasWarnings { get { return _warningMessages.Any(); } }
+
+        public IReadOnlyCollection<string> WarningMessages { get { return _warningMessages; } }


nit: public IReadOnlyCollection<string> WarningMessages => _warningMessages;

loic-sharma · 2018-07-12T19:51:33Z

src/NuGetGallery/Strings.resx

+    <value>The package is not compliant with metadata requirements for Microsoft packages on NuGet.org. Go to http://aka.ms/Microsoft-NuGet-Compliance for more information.</value>
+  </data>
+  <data name="SecurityPolicy_RequirePackagePrefixReserved" xml:space="preserve">
+    <value>You have not published a package with this prefix in the past. This means other users may be able to push packages starting with the same prefix. Contact [email protected] to reserve the prefix. Learn more about Package ID prefix reservation.</value>


You have not published a package with this prefix in the past. This means other users may be able to push packages starting with the same prefix. Contact [email protected] to reserve the prefix. Learn more about Package ID prefix reservation. [](start = 11, length = 242)

The "learn more" bit is missing a link. What do you think of this wording?

This is the first time you've published a package with this prefix. Other users may be able to push packages with similar IDs. You can contact [email protected] to reserve the prefix. Go to for more information.

@karann-msft what is the link to be used here? (to learn more about prefix reservation)

This one? https://docs.microsoft.com/en-us/nuget/reference/id-prefix-reservation

@xavierdecoster yes that looks right. sorry for the late reply.

loic-sharma · 2018-07-12T19:59:15Z

src/NuGetGallery.Core/Services/CoreMessageService.cs

+            var warningMessagesPlaceholder = string.Empty;
+            if (hasWarnings)
+            {
+                subject = $"[{CoreConfiguration.GalleryOwner.DisplayName}] Package published with Warnings - {package.PackageRegistration.Id} {package.Version}";


Warnings [](start = 98, length = 8)

Should this be lower cased?

skofman1 · 2018-07-25T16:19:50Z

                        if (!(ConfigurationService.Current.AsynchronousPackageValidationEnabled && ConfigurationService.Current.BlockingAsynchronousPackageValidationEnabled))

on nuget.org async validation is enabled, so this code block will never be called (the notification mail is sent by the orchestrator after validation is done)

Refers to: src/NuGetGallery/Controllers/ApiController.cs:533 in 6228911. [](commit_id = 6228911d9196b59402fffc2c12f6350378d1eb02, deletion_comment = False)

skofman1 · 2018-07-25T16:28:53Z

src/NuGetGallery/Security/RequireMicrosoftPackageCompliancePolicy.cs

+            }
+
+            // Copyright validation
+            if (!string.Equals(package.Copyright, "(c) Microsoft Corporation. All rights reserved.")


hardcoding the copyright allowed values will surely get us in trouble in the future when we deal with Xamarin packages for example. We should at least have the option to go to the DB and manually modify the requirement for those packages.

skofman1 · 2018-07-25T16:33:37Z

src/NuGetGallery/Security/RequireMicrosoftPackageCompliancePolicy.cs

+            {
+                // This will also mark the package as verified if the prefix has been reserved by Microsoft.
+                // The entities context is committed later as a single atomic transaction (see PackageUploadService).
+                await context.PackageOwnershipManagementService.AddPackageOwnerAsync(context.Package.PackageRegistration, microsoftUser, commitChanges: false);


How about this order of validations:

Check metadata - exit with failure if not compliant.

Add MSFT as co-owner - this will set the "isVerified" if needed.

Check if "IsVerified" is true - return warning if not.

Sounds good. In that case, I don't even need the ReservedNamespaceService in this policy, as I can simply check on context.Package.PackageRegistration.IsVerified at the end of the policy evaluation.

Updated order of validations.

skofman1 · 2018-07-25T16:36:31Z

src/NuGetGallery.Core/Services/CoreMessageService.cs

@@ -27,10 +28,24 @@ public CoreMessageService(IMailSender mailSender, ICoreMessageServiceConfigurati
        public IMailSender MailSender { get; protected set; }
        public ICoreMessageServiceConfiguration CoreConfiguration { get; protected set; }

-        public void SendPackageAddedNotice(Package package, string packageUrl, string packageSupportUrl, string emailSettingsUrl)
+        public void SendPackageAddedNotice(Package package, string packageUrl, string packageSupportUrl, string emailSettingsUrl, IEnumerable<string> warningMessages = null)


This mail will never be sent by the Gallery on nuget.org (but by the orchestrator). You should create a separate mail for the warning.

Great point! Plumbing the warnings down to orchestrator would be a mess.

Couldn't we do the warning emails asynchronously for "blessed" owners? This would also mean we could check for existing IDs that don't fall under a namespace.

Where do you suggest I add this warning email in code?

I could add the following in ApiController after the if block that checks for blocking validation mode:

// Emit warning messages if any else if (packagePolicyResult.HasWarnings) { // Notify user of push unless async validation in blocking mode is used MessageService.SendPackageAddedNotice(package, Url.Package(package.PackageRegistration.Id, package.NormalizedVersion, relativeUrl: false), Url.ReportPackage(package.PackageRegistration.Id, package.NormalizedVersion, relativeUrl: false), Url.AccountSettings(relativeUrl: false), packagePolicyResult.WarningMessages); }

I like @joelverhagen 's idea to send warning e-mails about packages that don't fall under a namespace from a separate job. However, it will be much more work, so I vote to keep it in the gallery.
One thing I would do, is have a separate e-mail with this warning and not the "package added notice" because people will get the same e-mail twice - one from Gallery and one from Orchestrator and will find it confusing.

joelverhagen · 2018-07-25T19:05:46Z

src/NuGetGallery/Security/RequireMicrosoftPackageCompliancePolicy.cs

+                .Split(new[] { ',', ' ' }, StringSplitOptions.RemoveEmptyEntries)
+                .Contains(MicrosoftUsername, StringComparer.InvariantCultureIgnoreCase))
+            {
+                return false;


false [](start = 23, length = 5)

How does the caller know what the specific problem is?

Shouldn't we be returning an error message, e.g. "The package copyright does not match the policy blah blah".

joelverhagen · 2018-07-25T19:12:48Z

src/NuGetGallery/Controllers/ApiController.cs

@@ -489,6 +490,17 @@ public virtual Task<ActionResult> CreatePackagePost()
                                owner,
                                currentUser);

+                            var packagePolicyResult = await SecurityPolicyService.EvaluatePackagePoliciesAsync(


EvaluatePackagePoliciesAsync [](start = 82, length = 28)

The implication here is that the "Package" entity has enough details for package policies to be performed. You'll notice that pretty much all of the other package validation above acts on PackageMetadata, PackageArchiveReader, or PackageRegistration (if it exists).

Given PackageMetadata and PackageArchiveReader is a superset of the data available on Package, why not use these in the validation instead?

Because when generating the Package, the IsVerified property is properly set already, which gives the policy an indication of whether the account pushing the package has registered the prefix yet.

I can move it up, so this is called before creating the Package and BEFORE adding the entity to the entity context, but that would likely mean we make the call to check for IsVerified for the pushing user twice (once when evaluating the policy, once when creating the package).

I see, there is a sneaky dependency on IsVerified. Makes sense.

An additional dependency: when the package being pushed concerns a new package registration, the PackageRegistration will be null if we didn't create the Package entity yet, so there's no way we can add an owner to the registration in that case.

I think there's no other way but to run this policy after generating the Package entity and adding it to the db context.

I updated PackageSecurityPolicyEvaluationContext to no longer depend on IEntitiesContext directly, but instead leverage IUserService (and IPackageOwnershipManagementService).

I added the following test that asserts we call their APIs with commitChanges: false.
https://github.com/NuGet/NuGetGallery/pull/6150/files#diff-3e9af76f191871062da1ac6b81a382ecR48

joelverhagen · 2018-07-25T19:13:26Z

src/NuGetGallery/Controllers/ApiController.cs

+
+                            if (!packagePolicyResult.Success)
+                            {
+                                return new HttpStatusCodeWithBodyResult(HttpStatusCode.BadRequest, packagePolicyResult.ErrorMessage);


HttpStatusCodeWithBodyResult [](start = 43, length = 28)

We have to now performed validations AFTER adding the entity to the entity context.

This is only okay if we are very rigorous about not committing (SaveChangesAsync on entity context) until we are very sure all validations are done.

There was a time in the past when uploading a readme Markdown during UI upload actually did a commit early. This was a bug that went undetected for months.

Would it be possible to run this validation before adding entities to the entity context?

I am okay with leaving this after mutating the DB context, but we need a safe guard to be sure that commit does not be called early. Any ideas on that? It seems like we need concrete implementations of all ApiController dependencies in a unit test EXCEPT DB context and then we say "Moq, make sure SaveChangesAsync was only called once".

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

joelverhagen · 2018-07-25T19:32:38Z

src/NuGetGallery/Security/RequireMicrosoftPackageCompliancePolicy.cs

+            // If the package being evaluated has not been registered yet (new package ID),
+            // this policy requires the prefix to be reserved.
+            var isWarning = false;
+            if (context.ExistingPackageRegistration == null)


ExistingPackageRegistration [](start = 24, length = 27)

Why isn't this a boolean, PackageRegistrationAlreadyExists? We don't actually need the existing instance since it is on Package.PackageRegistration, right?

True! Fixing.

xavierdecoster · 2018-07-27T15:45:18Z

In today's update:

new MicrosoftTeamSubscription
the new RequirePackageMetadataCompliancePolicy is now generic, and validation values are passed in via policy State (which is configurable in the db)
no more using IEntitiesContext directly in RequirePackageMetadataCompliancePolicy (instead using IUserService and IPackageOwnershipManagementService
a new unit test asserts that the new package security policy calls service APIs using commitChanges:false

Open questions:

if package policy results in warnings, a sync message is currently sent when gallery is configured to use async validation
there's still a todo in the OnSubscribeAsync of MicrosoftTeamSubscription: should that be part of v1?

skofman1 · 2018-07-27T22:46:06Z

src/NuGetGallery/Security/RequirePackageMetadataCompliancePolicy.cs

+                return SecurityPolicyResult.CreateErrorResult(
+                    string.Format(
+                        CultureInfo.CurrentCulture,
+                        Strings.SecurityPolicy_RequireMicrosoftPackageMetadataComplianceForPush,


you removed "Microsoft" from this code, but it's still here. I guess there's no easy way to make the string generic given the URL is hardcoded.

Oversight :) The error message could be part of the policy state.

xavierdecoster · 2018-07-30T07:58:53Z

@skofman1 oops, good catch :) I've pushed an update that addresses that scenario. Notice the subtle difference in email subject (published vs pushed) and email body (no link to email settings when push warning message is sent).

Merging is blocked by @scottbommarito's request for changes, but if all good, feel free to merge and deploy to DEV later today.

…tions #1419

…ComplianceForPush

…s used in SecurityPolicyService. Calling IPackageOwnershipManagementService in RequireMicrosoftPackageCompliancePolicy (instead of using IEntitiesContext directly)

…cePolicy

…aluationContext

…mit changes in the policy

…ation is disabled

xavierdecoster · 2018-08-02T06:42:20Z

@scottbommarito your review is still requesting changes. Can you please check if changes are good, or let me know what needs changed still?

@karann-msft @skofman1 had an offline discussion with @joelverhagen regarding the email with warnings message, and he had an interesting suggestion:

Is it necessary the email is sent immediately?

Why don’t we just create a dedicated job to scan Microsoft owner IDs every day and send an email once on IDs that don’t have reserved namespaces. This way we get the benefit for existing IDs too. Or we can make it a report that PMs see and reach out to teams manually. Since this is sent from the same sender as normal package push and to the same recipient, don’t you think it’s likely this message will go unnoticed? I imagine the package push notification is noise for a lot of users.

I guess it all depends on the answer to the above question, and the amount of effort we want to put into it.

skofman1 · 2018-08-02T06:59:53Z

Xavier, as I commented above regrading Joel's suggestion: it's good, but let's not invest the work now, and leave the code as is.
Feel free to merge if you think Scott's comments were addressed.

All CR feedback addressed. Created tracking issue for OnSubscribeAsync suggestion.

… Organizations (#6150)"

* Revert "Added package compliance policy checks when uploading through NuGet.org UI (#6272)" This reverts commit af20787. * Revert "Support MicrosoftTeamSubscription on organization accounts (#6264)" This reverts commit 945e2fa. * Revert "Auto-add Microsoft as co-owner for packages pushed by blessed Organizations (#6150)"

xavierdecoster self-assigned this Jul 9, 2018

xavierdecoster requested review from loic-sharma, shishirx34, chenriksson, skofman1 and scottbommarito July 9, 2018 08:47

chenriksson reviewed Jul 9, 2018

View reviewed changes

skofman1 reviewed Jul 10, 2018

View reviewed changes

scottbommarito previously requested changes Jul 10, 2018

View reviewed changes

xavierdecoster force-pushed the dev-issue1419 branch from 17be044 to f5da0de Compare July 11, 2018 13:22

loic-sharma reviewed Jul 12, 2018

View reviewed changes

xavierdecoster force-pushed the dev-issue1419 branch from d480107 to 6228911 Compare July 25, 2018 13:44

skofman1 reviewed Jul 25, 2018

View reviewed changes

joelverhagen reviewed Jul 25, 2018

View reviewed changes

joelverhagen mentioned this pull request Jul 25, 2018

Fail fast when package signing status does not match owner/ID requirements #6210

Merged

joelverhagen reviewed Jul 25, 2018

View reviewed changes

xavierdecoster force-pushed the dev-issue1419 branch 2 times, most recently from d5fb57c to 6bfb7e9 Compare July 27, 2018 10:50

skofman1 reviewed Jul 27, 2018

View reviewed changes

xavierdecoster added 19 commits July 31, 2018 13:50

Auto-add Microsoft a co-owner for packages pushed by blessed Organiza…

e6582d9

…tions #1419

CR feedback

39c66eb

Explicitly specify comparer for string.Contains

dda6b7b

Fix wording and url in SecurityPolicy_RequireMicrosoftPackageMetadata…

77e909b

…ComplianceForPush

Fix some nits

888a4db

CR feedback

416422e

Policy handlers are now evaluated async and service locator pattern i…

a752833

…s used in SecurityPolicyService. Calling IPackageOwnershipManagementService in RequireMicrosoftPackageCompliancePolicy (instead of using IEntitiesContext directly)

Leverage IReservedNamespaceService in RequireMicrosoftPackageComplian…

3215d25

…cePolicy

Ensure warnings are propagated when evaluating package policies.

0ca3d41

Introduce PackageRegistrationAlreadyExists on PackageSecurityPolicyEv…

99c983e

…aluationContext

Provide more detailed errors when package not compliant

e31f55b

Change order of evaluations in RequireMicrosoftPackageCompliancePolicy

afd5dc6

Removed unnecessary parameter

c10d2cb

Use IUserService instead of IEntitiesContext, and assert we don't com…

68331d9

…mit changes in the policy

Introduce MicrosoftTeamSubscription

68aca0c

Add missing file to source control

2b57652

CR feedback

1a9a19a

Separate email for package pushed with warnings

d617b21

ensure warnings propagate in package published email when async valid…

53df528

…ation is disabled

xavierdecoster force-pushed the dev-issue1419 branch from 361f3dd to 53df528 Compare July 31, 2018 12:41

xavierdecoster merged commit 073b40b into dev Aug 2, 2018

xavierdecoster deleted the dev-issue1419 branch August 2, 2018 07:29

scottbommarito mentioned this pull request Aug 7, 2018

[Reg cert] Admin should have permissions to manage certificates for an org #6071

Closed

joelverhagen mentioned this pull request Aug 8, 2018

Collapse multi-line reason phrases in HttpStatusCodeWithBodyResult #6280

Merged

xavierdecoster added a commit that referenced this pull request Aug 8, 2018

Revert "Auto-add Microsoft as co-owner for packages pushed by blessed…

7df5010

… Organizations (#6150)"


		public bool HasWarnings { get { return _warningMessages.Any(); } }

		public IReadOnlyCollection<string> WarningMessages { get { return _warningMessages; } }

Auto-add Microsoft a co-owner for packages pushed by blessed Organizations #6150

Auto-add Microsoft a co-owner for packages pushed by blessed Organizations #6150

Conversation

xavierdecoster commented Jul 9, 2018 • edited Loading

chenriksson commented Jul 9, 2018

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

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

agr Jul 17, 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

skofman1 Jul 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

Choose a reason for hiding this comment

xavierdecoster Jul 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

shishirx34 Jul 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

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

xavierdecoster Jul 11, 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

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

xavierdecoster Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xavierdecoster commented Jul 9, 2018 •

edited

Loading

agr Jul 17, 2018 •

edited

Loading

skofman1 Jul 10, 2018 •

edited

Loading

xavierdecoster Jul 10, 2018 •

edited

Loading

shishirx34 Jul 10, 2018 •

edited

Loading

xavierdecoster Jul 11, 2018 •

edited

Loading

xavierdecoster Jul 13, 2018 •

edited

Loading