From 3f59f2068345775da106bf8a824d61b3fd728bdd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Sharma?= Date: Thu, 29 Nov 2018 13:28:24 -0800 Subject: [PATCH] Repository sign packages owned by users with "malformed" usernames (#686) Repository sign packages that are owned by usernames with "malformed" usernames. Addresses https://github.com/NuGet/Engineering/issues/1582 and https://github.com/NuGet/Engineering/issues/1592. --- ...et.Services.Validation.Orchestrator.csproj | 1 - .../PackageSignatureValidator.cs | 41 ------------------- .../ScanAndSign/ScanAndSignProcessor.cs | 17 +------- .../UsernameHelper.cs | 21 ---------- .../PackageSignatureValidatorFacts.cs | 40 ------------------ .../ScanAndSignProcessorFacts.cs | 33 --------------- 6 files changed, 2 insertions(+), 151 deletions(-) delete mode 100644 src/NuGet.Services.Validation.Orchestrator/UsernameHelper.cs diff --git a/src/NuGet.Services.Validation.Orchestrator/NuGet.Services.Validation.Orchestrator.csproj b/src/NuGet.Services.Validation.Orchestrator/NuGet.Services.Validation.Orchestrator.csproj index 6579fb5b2..c41b271a9 100644 --- a/src/NuGet.Services.Validation.Orchestrator/NuGet.Services.Validation.Orchestrator.csproj +++ b/src/NuGet.Services.Validation.Orchestrator/NuGet.Services.Validation.Orchestrator.csproj @@ -69,7 +69,6 @@ - diff --git a/src/NuGet.Services.Validation.Orchestrator/PackageSigning/ProcessSignature/PackageSignatureValidator.cs b/src/NuGet.Services.Validation.Orchestrator/PackageSigning/ProcessSignature/PackageSignatureValidator.cs index 656bf21bf..150eb72d1 100644 --- a/src/NuGet.Services.Validation.Orchestrator/PackageSigning/ProcessSignature/PackageSignatureValidator.cs +++ b/src/NuGet.Services.Validation.Orchestrator/PackageSigning/ProcessSignature/PackageSignatureValidator.cs @@ -93,20 +93,6 @@ private IValidationResult Validate(IValidationRequest request, IValidationResult return ValidationResult.Succeeded; } - // TODO: Remove this. - // See: https://github.com/NuGet/Engineering/issues/1592 - if (HasOwnerWithInvalidUsername(request)) - { - _logger.LogWarning( - "Ignoring invalid validation result in package signature validator as the package has an owner with an invalid username. " + - "Status = {ValidationStatus}, Nupkg URL = {NupkgUrl}, validation issues = {Issues}", - result.Status, - result.NupkgUrl, - result.Issues.Select(i => i.IssueCode)); - - return ValidationResult.Succeeded; - } - _logger.LogCritical( "Unexpected validation result in package signature validator. This may be caused by an invalid repository " + "signature. Throwing an exception to force this validation to dead-letter. " + @@ -133,32 +119,5 @@ private IValidationResult Validate(IValidationRequest request, IValidationResult return result; } - - private bool HasOwnerWithInvalidUsername(IValidationRequest request) - { - var registration = _packages.FindPackageRegistrationById(request.PackageId); - - if (registration == null) - { - _logger.LogError("Attempted to validate package that has no package registration"); - - throw new InvalidOperationException($"Registration for package id {request.PackageId} does not exist"); - } - - var owners = registration.Owners.Select(o => o.Username).ToList(); - - if (owners.Any(UsernameHelper.IsInvalid)) - { - _logger.LogWarning( - "Package {PackageId} {PackageVersion} has an owner with an invalid username. {Owners}", - request.PackageId, - request.PackageVersion, - owners); - - return true; - } - - return false; - } } } \ No newline at end of file diff --git a/src/NuGet.Services.Validation.Orchestrator/PackageSigning/ScanAndSign/ScanAndSignProcessor.cs b/src/NuGet.Services.Validation.Orchestrator/PackageSigning/ScanAndSign/ScanAndSignProcessor.cs index 69c14824d..cbaac7c03 100644 --- a/src/NuGet.Services.Validation.Orchestrator/PackageSigning/ScanAndSign/ScanAndSignProcessor.cs +++ b/src/NuGet.Services.Validation.Orchestrator/PackageSigning/ScanAndSign/ScanAndSignProcessor.cs @@ -134,7 +134,7 @@ public async Task StartAsync(IValidationRequest request) var owners = FindPackageOwners(request); - if (await ShouldRepositorySignAsync(request, owners)) + if (await ShouldRepositorySignAsync(request)) { _logger.LogInformation( "Repository signing {PackageId} {PackageVersion} with {ServiceIndex} and {Owners}", @@ -198,7 +198,7 @@ private bool ShouldSkipScan(IValidationRequest request) return false; } - private async Task ShouldRepositorySignAsync(IValidationRequest request, List owners) + private async Task ShouldRepositorySignAsync(IValidationRequest request) { var hasRepositorySignature = await _validationContext .PackageSignatures @@ -216,19 +216,6 @@ private async Task ShouldRepositorySignAsync(IValidationRequest request, L return false; } - // TODO: Remove this check. - // See: https://github.com/NuGet/Engineering/issues/1582 - if (owners.Any(UsernameHelper.IsInvalid)) - { - _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; } diff --git a/src/NuGet.Services.Validation.Orchestrator/UsernameHelper.cs b/src/NuGet.Services.Validation.Orchestrator/UsernameHelper.cs deleted file mode 100644 index 9c22e6235..000000000 --- a/src/NuGet.Services.Validation.Orchestrator/UsernameHelper.cs +++ /dev/null @@ -1,21 +0,0 @@ -// Copyright (c) .NET Foundation. All rights reserved. -// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. - -using System; -using System.Text.RegularExpressions; - -namespace NuGet.Services.Validation.Orchestrator -{ - // TODO: Remove this type. - // See: https://github.com/NuGet/Engineering/issues/1582 - // See: https://github.com/NuGet/Engineering/issues/1592 - public static class UsernameHelper - { - private const string UsernameRegex = @"^[A-Za-z0-9][A-Za-z0-9_\.-]+[A-Za-z0-9]$"; - - public static bool IsInvalid(string username) - { - return !Regex.IsMatch(username, UsernameRegex, RegexOptions.None, TimeSpan.FromSeconds(5)); - } - } -} diff --git a/tests/NuGet.Services.Validation.Orchestrator.Tests/PackageSigning/ProcessSignature/PackageSignatureValidatorFacts.cs b/tests/NuGet.Services.Validation.Orchestrator.Tests/PackageSigning/ProcessSignature/PackageSignatureValidatorFacts.cs index 2aaa391f1..ad7b25853 100644 --- a/tests/NuGet.Services.Validation.Orchestrator.Tests/PackageSigning/ProcessSignature/PackageSignatureValidatorFacts.cs +++ b/tests/NuGet.Services.Validation.Orchestrator.Tests/PackageSigning/ProcessSignature/PackageSignatureValidatorFacts.cs @@ -153,37 +153,6 @@ public async Task WhenRepositorySigningEnabled_ThrowsExceptionIfPackageIsModifie Assert.Equal("Package signature validator has an unexpected validation result", e.Message); } - [Fact] - public async Task WhenRepositorySigningEnabled_IgnoresFailedValidationIfOwnerHasMalformedUsername() - { - // Arrange - _config.RepositorySigningEnabled = true; - - _packages - .Setup(p => p.FindPackageRegistrationById(_validationRequest.Object.PackageId)) - .Returns(_packageRegistrationWithBadUsername); - - _validatorStateService - .Setup(x => x.GetStatusAsync(It.IsAny())) - .ReturnsAsync(new ValidatorStatus - { - ValidationId = ValidationId, - PackageKey = PackageKey, - ValidatorName = ValidatorName.PackageSignatureProcessor, - State = ValidationStatus.Failed, - ValidatorIssues = new List(), - }); - - // Act - var result = await _target.GetResultAsync(_validationRequest.Object); - - // Assert - Assert.Null(result.NupkgUrl); - Assert.Empty(result.Issues); - Assert.Equal(ValidationStatus.Succeeded, result.Status); - - } - public static IEnumerable PossibleValidationStatuses => possibleValidationStatuses.Select(s => new object[] { s }); } @@ -489,7 +458,6 @@ public abstract class FactsBase protected readonly ScanAndSignConfiguration _config; protected readonly PackageRegistration _packageRegistration; - protected readonly PackageRegistration _packageRegistrationWithBadUsername; public FactsBase(ITestOutputHelper output) { @@ -520,14 +488,6 @@ public FactsBase(ITestOutputHelper output) } }; - _packageRegistrationWithBadUsername = new PackageRegistration - { - Owners = new[] - { - new User { Username = "Bad Username" } - } - }; - _target = new PackageSignatureValidator( _validatorStateService.Object, _packageSignatureVerifier.Object, diff --git a/tests/Validation.PackageSigning.ScanAndSign.Tests/ScanAndSignProcessorFacts.cs b/tests/Validation.PackageSigning.ScanAndSign.Tests/ScanAndSignProcessorFacts.cs index 62ffb1223..4438ff9bc 100644 --- a/tests/Validation.PackageSigning.ScanAndSign.Tests/ScanAndSignProcessorFacts.cs +++ b/tests/Validation.PackageSigning.ScanAndSign.Tests/ScanAndSignProcessorFacts.cs @@ -266,30 +266,6 @@ public async Task EnqueuesScanAndSignIfPackageHasNoRepositorySignature() .Verify(v => v.TryAddValidatorStatusAsync(It.IsAny(), It.IsAny(), It.IsAny()), 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(), It.IsAny(), It.IsAny()), Times.Once); - } - [Fact] public async Task EnqueuesScanAndSignEvenIfRepositorySigningIsDisabled() { @@ -455,15 +431,6 @@ public async Task WhenPackageFitsCriteriaAndIsNotRepositorySigned_DoesNotSkipSca } }; - private PackageRegistration _packageRegistrationWithInvalidUser = new PackageRegistration - { - Owners = new List - { - new User("Billy"), - new User("Satan Claus"), - } - }; - public TheStartAsyncMethod() { _request = new ValidationRequest(Guid.NewGuid(), 42, "somepackage", "somversion", "https://example.com/package.nupkg");