[Repository Signing] Don't repository sign package if owner(s) have malformed username (#498)

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

Author: loic-sharma

src/NuGet.Services.Validation.Orchestrator/PackageSigning/ScanAndSign/ScanAndSignProcessor.cs

@@ -5,12 +5,13 @@
 using System.Collections.Generic;
 using System.Data.Entity;
 using System.Linq;
+using System.Text.RegularExpressions;
 using System.Threading.Tasks;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Options;
 using NuGet.Jobs.Validation;
-using NuGet.Jobs.Validation.Storage;
 using NuGet.Jobs.Validation.ScanAndSign;
+using NuGet.Jobs.Validation.Storage;
 using NuGet.Services.Validation.Vcs;
 using NuGetGallery;
 
@@ -19,6 +20,8 @@ namespace NuGet.Services.Validation.Orchestrator.PackageSigning.ScanAndSign
     [ValidatorName(ValidatorName.ScanAndSign)]
     public class ScanAndSignProcessor : IProcessor
     {
+        private const string UsernameRegex = @"^[A-Za-z0-9][A-Za-z0-9_\.-]+[A-Za-z0-9]$";
+
         private readonly IValidationEntitiesContext _validationContext;
         private readonly IValidatorStateService _validatorStateService;
         private readonly ICorePackageService _packageService;
@@ -131,10 +134,10 @@ public async Task<IValidationResult> StartAsync(IValidationRequest request)
                 return processorStatus.ToValidationResult();
             }
 
-            if (await ShouldRepositorySignAsync(request))
-            {
-                var owners = FindPackageOwners(request);
+            var owners = FindPackageOwners(request);
 
+            if (await ShouldRepositorySignAsync(request, owners))
+            {
                 _logger.LogInformation(
                     "Repository signing {PackageId} {PackageVersion} with {ServiceIndex} and {Owners}",
                     request.PackageId,
@@ -197,7 +200,7 @@ private bool ShouldSkipScan(IValidationRequest request)
             return false;
         }
 
-        private async Task<bool> ShouldRepositorySignAsync(IValidationRequest request)
+        private async Task<bool> ShouldRepositorySignAsync(IValidationRequest request, List<string> owners)
         {
             var hasRepositorySignature = await _validationContext
                 .PackageSignatures
@@ -215,7 +218,18 @@ private async Task<bool> ShouldRepositorySignAsync(IValidationRequest request)
                 return false;
             }
 
-           return true;
+            if (owners.Any(IsInvalidUsername))
+            {
+                _logger.LogWarning(
+                    "Package {PackageId} {PackageVersion} has an owner with an invalid username. Scanning instead of signing. {Owners}",
+                    request.PackageId,
+                    request.PackageVersion,
+                    owners);
+
+                return false;
+            }
+
+            return true;
         }
 
         private List<string> FindPackageOwners(IValidationRequest request)
@@ -234,5 +248,10 @@ private List<string> FindPackageOwners(IValidationRequest request)
                 .Select(o => o.Username)
                 .ToList();
         }
+
+        private bool IsInvalidUsername(string username)
+        {
+            return !Regex.IsMatch(username, UsernameRegex, RegexOptions.None, TimeSpan.FromSeconds(5));
+        }
     }
 }

tests/Validation.PackageSigning.ScanAndSign.Tests/ScanAndSignProcessorFacts.cs

@@ -262,6 +262,30 @@ public async Task EnqueuesScanAndSignIfPackageHasNoRepositorySignature()
                 .Verify(v => v.TryAddValidatorStatusAsync(It.IsAny<IValidationRequest>(), It.IsAny<ValidatorStatus>(), It.IsAny<ValidationStatus>()), Times.Once);
         }
 
+        [Fact]
+        public async Task WhenUsernameInvalid_SkipsScanAndSign()
+        {
+            _config.RepositorySigningEnabled = true;
+
+            _validationContext.Mock();
+            _packageServiceMock
+                .Setup(p => p.FindPackageRegistrationById(_request.PackageId))
+                .Returns(_packageRegistrationWithInvalidUser);
+
+            var result = await _target.StartAsync(_request);
+
+            _packageServiceMock
+                .Verify(p => p.FindPackageRegistrationById(_request.PackageId), Times.Once);
+
+            _enqueuerMock
+                .Verify(e => e.EnqueueScanAsync(_request.ValidationId, _request.NupkgUrl), Times.Once);
+
+            _validatorStateServiceMock
+                .Verify(v => v.TryAddValidatorStatusAsync(_request, _status, ValidationStatus.Incomplete), Times.Once);
+            _validatorStateServiceMock
+                .Verify(v => v.TryAddValidatorStatusAsync(It.IsAny<IValidationRequest>(), It.IsAny<ValidatorStatus>(), It.IsAny<ValidationStatus>()), Times.Once);
+        }
+
         [Fact]
         public async Task EnqueuesScanAndSignEvenIfRepositorySigningIsDisabled()
         {
@@ -336,10 +360,14 @@ public async Task EnqueuesScanIfPackageHasARepositorySignature()
                 }
             });
 
+            _packageServiceMock
+                .Setup(p => p.FindPackageRegistrationById(_request.PackageId))
+                .Returns(_packageRegistration);
+
             var result = await _target.StartAsync(_request);
 
             _packageServiceMock
-                .Verify(p => p.FindPackageRegistrationById(It.IsAny<string>()), Times.Never);
+                .Verify(p => p.FindPackageRegistrationById(It.IsAny<string>()), Times.Once);
 
             _enqueuerMock
                 .Verify(e => e.EnqueueScanAsync(_request.ValidationId, _request.NupkgUrl), Times.Once);
@@ -361,6 +389,10 @@ public async Task WhenPackageIsRepositorySigned_SkipsCheckWhenPackageFitsCriteri
                 }
             });
 
+            _packageServiceMock
+                .Setup(p => p.FindPackageRegistrationById(_request.PackageId))
+                .Returns(_packageRegistration);
+
             _criteriaEvaluatorMock
                 .Setup(ce => ce.IsMatch(It.IsAny<ICriteria>(), It.IsAny<Package>()))
                 .Returns(false);
@@ -414,6 +446,15 @@ public async Task WhenPackageFitsCriteriaAndIsNotRepositorySigned_DoesNotSkipSca
             }
         };
 
+        private PackageRegistration _packageRegistrationWithInvalidUser = new PackageRegistration
+        {
+            Owners = new List<User>
+            {
+                new User("Billy"),
+                new User("Satan Claus"),
+            }
+        };
+
         public TheStartAsyncMethod()
         {
             _request = new ValidationRequest(Guid.NewGuid(), 42, "somepackage", "somversion", "https://example.com/package.nupkg");