Skip to content
This repository was archived by the owner on Jul 30, 2024. It is now read-only.

Commit 0c7b7c8

Browse files
loic-sharmaloic-sharma
authored
[Repository Signing] Allow unsigned packages if owner has malformed username (#503)
Packages that are owned by users with malformed usernames are not repository signed (see https://github.com/NuGet/Engineering/issues/1582). The validator that verifies that a package is repository signed should take this into account. When all malformed usernames have been removed from the Gallery, this change will be reverted. This is tracked by: https://github.com/NuGet/Engineering/issues/1592 Addresses https://github.com/NuGet/Engineering/issues/1591
1 parent 006cdef commit 0c7b7c8

6 files changed

Lines changed: 147 additions & 23 deletions

File tree

‎src/NuGet.Services.Validation.Orchestrator/NuGet.Services.Validation.Orchestrator.csproj‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@
5959
<Compile Include="Symbols\SymbolsMessageEnqueuer.cs" />
6060
<Compile Include="Symbols\SymbolsValidator.cs" />
6161
<Compile Include="Symbols\SymbolsValidationConfiguration.cs" />
62+
<Compile Include="UsernameHelper.cs" />
6263
<Compile Include="ValidatingEntitites\IValidatingEntity.cs" />
6364
<Compile Include="IValidationOutcomeProcessor.cs" />
6465
<Compile Include="IValidationPackageFileService.cs" />

‎src/NuGet.Services.Validation.Orchestrator/PackageSigning/ProcessSignature/BaseSignatureProcessor.cs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
using System;
55
using System.Threading.Tasks;
66
using Microsoft.Extensions.Logging;
7-
using NuGet.Jobs.Validation;
87
using NuGet.Jobs.Validation.Storage;
98
using NuGet.Services.Validation.Orchestrator.Telemetry;
109

‎src/NuGet.Services.Validation.Orchestrator/PackageSigning/ProcessSignature/PackageSignatureValidator.cs‎

Lines changed: 55 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,10 @@
88
using Microsoft.Extensions.Options;
99
using NuGet.Jobs.Validation;
1010
using NuGet.Jobs.Validation.Storage;
11+
using NuGet.Services.Validation.Orchestrator;
1112
using NuGet.Services.Validation.Orchestrator.PackageSigning.ScanAndSign;
1213
using NuGet.Services.Validation.Orchestrator.Telemetry;
14+
using NuGetGallery;
1315

1416
namespace NuGet.Services.Validation.PackageSigning.ProcessSignature
1517
{
@@ -23,6 +25,7 @@ public class PackageSignatureValidator : BaseSignatureProcessor, IValidator
2325
private readonly IValidatorStateService _validatorStateService;
2426
private readonly IProcessSignatureEnqueuer _signatureVerificationEnqueuer;
2527
private readonly ISimpleCloudBlobProvider _blobProvider;
28+
private readonly ICorePackageService _packages;
2629
private readonly ScanAndSignConfiguration _config;
2730
private readonly ITelemetryService _telemetryService;
2831
private readonly ILogger<PackageSignatureValidator> _logger;
@@ -31,6 +34,7 @@ public PackageSignatureValidator(
3134
IValidatorStateService validatorStateService,
3235
IProcessSignatureEnqueuer signatureVerificationEnqueuer,
3336
ISimpleCloudBlobProvider blobProvider,
37+
ICorePackageService packages,
3438
IOptionsSnapshot<ScanAndSignConfiguration> configAccessor,
3539
ITelemetryService telemetryService,
3640
ILogger<PackageSignatureValidator> logger)
@@ -39,6 +43,7 @@ public PackageSignatureValidator(
3943
_validatorStateService = validatorStateService ?? throw new ArgumentNullException(nameof(validatorStateService));
4044
_signatureVerificationEnqueuer = signatureVerificationEnqueuer ?? throw new ArgumentNullException(nameof(signatureVerificationEnqueuer));
4145
_blobProvider = blobProvider ?? throw new ArgumentNullException(nameof(blobProvider));
46+
_packages = packages ?? throw new ArgumentNullException(nameof(packages));
4247
_telemetryService = telemetryService ?? throw new ArgumentNullException(nameof(telemetryService));
4348
_logger = logger ?? throw new ArgumentNullException(nameof(logger));
4449

@@ -60,45 +65,57 @@ public override async Task<IValidationResult> GetResultAsync(IValidationRequest
6065
{
6166
var result = await base.GetResultAsync(request);
6267

63-
return Validate(result);
68+
return Validate(request, result);
6469
}
6570

6671
public override async Task<IValidationResult> StartAsync(IValidationRequest request)
6772
{
6873
var result = await base.StartAsync(request);
6974

70-
return Validate(result);
75+
return Validate(request, result);
7176
}
7277

73-
private IValidationResult Validate(IValidationResult result)
78+
private IValidationResult Validate(IValidationRequest request, IValidationResult result)
7479
{
7580
/// The package signature validator runs after the <see cref="PackageSignatureProcessor" />.
7681
/// All signature validation issues should be caught and handled by the processor.
7782
if (result.Status == ValidationStatus.Failed || result.NupkgUrl != null)
7883
{
79-
if (_config.RepositorySigningEnabled)
84+
if (!_config.RepositorySigningEnabled)
8085
{
81-
_logger.LogCritical(
82-
"Unexpected validation result in package signature validator. This may be caused by an invalid repository " +
83-
"signature. Throwing an exception to force this validation to dead-letter. " +
86+
_logger.LogInformation(
87+
"Ignoring invalid validation result in package signature validator as repository signing is disabled. " +
8488
"Status = {ValidationStatus}, Nupkg URL = {NupkgUrl}, validation issues = {Issues}",
8589
result.Status,
8690
result.NupkgUrl,
8791
result.Issues.Select(i => i.IssueCode));
8892

89-
throw new InvalidOperationException("Package signature validator has an unexpected validation result");
93+
return ValidationResult.Succeeded;
9094
}
91-
else
95+
96+
// TODO: Remove this.
97+
// See: https://github.com/NuGet/Engineering/issues/1592
98+
if (HasOwnerWithInvalidUsername(request))
9299
{
93-
_logger.LogInformation(
94-
"Ignoring invalid validation result in package signature validator as repository signing is disabled. " +
100+
_logger.LogWarning(
101+
"Ignoring invalid validation result in package signature validator as the package has an owner with an invalid username. " +
95102
"Status = {ValidationStatus}, Nupkg URL = {NupkgUrl}, validation issues = {Issues}",
96103
result.Status,
97104
result.NupkgUrl,
98105
result.Issues.Select(i => i.IssueCode));
99106

100107
return ValidationResult.Succeeded;
101108
}
109+
110+
_logger.LogCritical(
111+
"Unexpected validation result in package signature validator. This may be caused by an invalid repository " +
112+
"signature. Throwing an exception to force this validation to dead-letter. " +
113+
"Status = {ValidationStatus}, Nupkg URL = {NupkgUrl}, validation issues = {Issues}",
114+
result.Status,
115+
result.NupkgUrl,
116+
result.Issues.Select(i => i.IssueCode));
117+
118+
throw new InvalidOperationException("Package signature validator has an unexpected validation result");
102119
}
103120

104121
/// Suppress all validation issues. The <see cref="PackageSignatureProcessor"/> should
@@ -116,5 +133,32 @@ private IValidationResult Validate(IValidationResult result)
116133

117134
return result;
118135
}
136+
137+
private bool HasOwnerWithInvalidUsername(IValidationRequest request)
138+
{
139+
var registration = _packages.FindPackageRegistrationById(request.PackageId);
140+
141+
if (registration == null)
142+
{
143+
_logger.LogError("Attempted to validate package that has no package registration");
144+
145+
throw new InvalidOperationException($"Registration for package id {request.PackageId} does not exist");
146+
}
147+
148+
var owners = registration.Owners.Select(o => o.Username).ToList();
149+
150+
if (owners.Any(UsernameHelper.IsInvalid))
151+
{
152+
_logger.LogWarning(
153+
"Package {PackageId} {PackageVersion} has an owner with an invalid username. {Owners}",
154+
request.PackageId,
155+
request.PackageVersion,
156+
owners);
157+
158+
return true;
159+
}
160+
161+
return false;
162+
}
119163
}
120164
}

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

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,6 @@ namespace NuGet.Services.Validation.Orchestrator.PackageSigning.ScanAndSign
2020
[ValidatorName(ValidatorName.ScanAndSign)]
2121
public class ScanAndSignProcessor : IProcessor
2222
{
23-
private const string UsernameRegex = @"^[A-Za-z0-9][A-Za-z0-9_\.-]+[A-Za-z0-9]$";
24-
2523
private readonly IValidationEntitiesContext _validationContext;
2624
private readonly IValidatorStateService _validatorStateService;
2725
private readonly ICorePackageService _packageService;
@@ -218,7 +216,9 @@ private async Task<bool> ShouldRepositorySignAsync(IValidationRequest request, L
218216
return false;
219217
}
220218

221-
if (owners.Any(IsInvalidUsername))
219+
// TODO: Remove this check.
220+
// See: https://github.com/NuGet/Engineering/issues/1582
221+
if (owners.Any(UsernameHelper.IsInvalid))
222222
{
223223
_logger.LogWarning(
224224
"Package {PackageId} {PackageVersion} has an owner with an invalid username. Scanning instead of signing. {Owners}",
@@ -248,10 +248,5 @@ private List<string> FindPackageOwners(IValidationRequest request)
248248
.Select(o => o.Username)
249249
.ToList();
250250
}
251-
252-
private bool IsInvalidUsername(string username)
253-
{
254-
return !Regex.IsMatch(username, UsernameRegex, RegexOptions.None, TimeSpan.FromSeconds(5));
255-
}
256251
}
257252
}

‎src/NuGet.Services.Validation.Orchestrator/UsernameHelper.cs‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
// Copyright (c) .NET Foundation. All rights reserved.
2+
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
3+
4+
using System;
5+
using System.Text.RegularExpressions;
6+
7+
namespace NuGet.Services.Validation.Orchestrator
8+
{
9+
// TODO: Remove this type.
10+
// See: https://github.com/NuGet/Engineering/issues/1582
11+
// See: https://github.com/NuGet/Engineering/issues/1592
12+
public static class UsernameHelper
13+
{
14+
private const string UsernameRegex = @"^[A-Za-z0-9][A-Za-z0-9_\.-]+[A-Za-z0-9]$";
15+
16+
public static bool IsInvalid(string username)
17+
{
18+
return !Regex.IsMatch(username, UsernameRegex, RegexOptions.None, TimeSpan.FromSeconds(5));
19+
}
20+
}
21+
}

‎tests/NuGet.Services.Validation.Orchestrator.Tests/PackageSigning/ProcessSignature/PackageSignatureValidatorFacts.cs‎

Lines changed: 67 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,10 @@ public async Task WhenRepositorySigningEnabled_ThrowsExceptionIfValidationFails(
103103
// Arrange
104104
_config.RepositorySigningEnabled = true;
105105

106+
_packages
107+
.Setup(p => p.FindPackageRegistrationById(_validationRequest.Object.PackageId))
108+
.Returns(_packageRegistration);
109+
106110
_validatorStateService
107111
.Setup(x => x.GetStatusAsync(It.IsAny<IValidationRequest>()))
108112
.ReturnsAsync(new ValidatorStatus
@@ -114,8 +118,10 @@ public async Task WhenRepositorySigningEnabled_ThrowsExceptionIfValidationFails(
114118
ValidatorIssues = new List<ValidatorIssue>(),
115119
});
116120

117-
// Act
118-
await Assert.ThrowsAsync<InvalidOperationException>(() => _target.GetResultAsync(_validationRequest.Object));
121+
// Act & Assert
122+
var e = await Assert.ThrowsAsync<InvalidOperationException>(() => _target.GetResultAsync(_validationRequest.Object));
123+
124+
Assert.Equal("Package signature validator has an unexpected validation result", e.Message);
119125
}
120126

121127
[Fact]
@@ -124,6 +130,10 @@ public async Task WhenRepositorySigningEnabled_ThrowsExceptionIfPackageIsModifie
124130
// Arrange
125131
_config.RepositorySigningEnabled = true;
126132

133+
_packages
134+
.Setup(p => p.FindPackageRegistrationById(_validationRequest.Object.PackageId))
135+
.Returns(_packageRegistration);
136+
127137
_validatorStateService
128138
.Setup(x => x.GetStatusAsync(It.IsAny<IValidationRequest>()))
129139
.ReturnsAsync(new ValidatorStatus
@@ -136,8 +146,41 @@ public async Task WhenRepositorySigningEnabled_ThrowsExceptionIfPackageIsModifie
136146
NupkgUrl = "https://nuget.org/a.nupkg"
137147
});
138148

149+
// Act & Assert
150+
var e = await Assert.ThrowsAsync<InvalidOperationException>(() => _target.GetResultAsync(_validationRequest.Object));
151+
152+
Assert.Equal("Package signature validator has an unexpected validation result", e.Message);
153+
}
154+
155+
[Fact]
156+
public async Task WhenRepositorySigningEnabled_IgnoresFailedValidationIfOwnerHasMalformedUsername()
157+
{
158+
// Arrange
159+
_config.RepositorySigningEnabled = true;
160+
161+
_packages
162+
.Setup(p => p.FindPackageRegistrationById(_validationRequest.Object.PackageId))
163+
.Returns(_packageRegistrationWithBadUsername);
164+
165+
_validatorStateService
166+
.Setup(x => x.GetStatusAsync(It.IsAny<IValidationRequest>()))
167+
.ReturnsAsync(new ValidatorStatus
168+
{
169+
ValidationId = ValidationId,
170+
PackageKey = PackageKey,
171+
ValidatorName = ValidatorName.PackageSignatureProcessor,
172+
State = ValidationStatus.Failed,
173+
ValidatorIssues = new List<ValidatorIssue>(),
174+
});
175+
139176
// Act
140-
await Assert.ThrowsAsync<InvalidOperationException>(() => _target.GetResultAsync(_validationRequest.Object));
177+
var result = await _target.GetResultAsync(_validationRequest.Object);
178+
179+
// Assert
180+
Assert.Null(result.NupkgUrl);
181+
Assert.Empty(result.Issues);
182+
Assert.Equal(ValidationStatus.Succeeded, result.Status);
183+
141184
}
142185

143186
public static IEnumerable<object[]> PossibleValidationStatuses => possibleValidationStatuses.Select(s => new object[] { s });
@@ -436,19 +479,23 @@ public abstract class FactsBase
436479
protected readonly Mock<IValidatorStateService> _validatorStateService;
437480
protected readonly Mock<IProcessSignatureEnqueuer> _packageSignatureVerifier;
438481
protected readonly Mock<ISimpleCloudBlobProvider> _blobProvider;
482+
protected readonly Mock<ICorePackageService> _packages;
439483
protected readonly Mock<IOptionsSnapshot<ScanAndSignConfiguration>> _configAccessor;
440484
protected readonly Mock<ITelemetryService> _telemetryService;
441485
protected readonly ILogger<PackageSignatureValidator> _logger;
442486
protected readonly Mock<IValidationRequest> _validationRequest;
443487
protected readonly PackageSignatureValidator _target;
444488

445489
protected readonly ScanAndSignConfiguration _config;
490+
protected readonly PackageRegistration _packageRegistration;
491+
protected readonly PackageRegistration _packageRegistrationWithBadUsername;
446492

447493
public FactsBase(ITestOutputHelper output)
448494
{
449495
_validatorStateService = new Mock<IValidatorStateService>();
450496
_packageSignatureVerifier = new Mock<IProcessSignatureEnqueuer>();
451497
_blobProvider = new Mock<ISimpleCloudBlobProvider>();
498+
_packages = new Mock<ICorePackageService>();
452499
_config = new ScanAndSignConfiguration();
453500
_configAccessor = new Mock<IOptionsSnapshot<ScanAndSignConfiguration>>();
454501
_telemetryService = new Mock<ITelemetryService>();
@@ -464,10 +511,27 @@ public FactsBase(ITestOutputHelper output)
464511

465512
_configAccessor.Setup(a => a.Value).Returns(_config);
466513

514+
_packageRegistration = new PackageRegistration
515+
{
516+
Owners = new[]
517+
{
518+
new User { Username = "GoodUsername" }
519+
}
520+
};
521+
522+
_packageRegistrationWithBadUsername = new PackageRegistration
523+
{
524+
Owners = new[]
525+
{
526+
new User { Username = "Bad Username" }
527+
}
528+
};
529+
467530
_target = new PackageSignatureValidator(
468531
_validatorStateService.Object,
469532
_packageSignatureVerifier.Object,
470533
_blobProvider.Object,
534+
_packages.Object,
471535
_configAccessor.Object,
472536
_telemetryService.Object,
473537
_logger);

0 commit comments

Comments
 (0)