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

Commit f50ef65

Browse files
loic-sharmaloic-sharma
authored
[Package Signing] Fix idempotency (#267)
Previously, the validate package signature job was designed such that it would fail validation requests for packages that had already been validated unless the validation request was from a revalidation gesture. The idea was that this would make the job "resilient" to unintended state changes caused by message duplication. Essentially, this could happen: 1. Orchestrator sends a `Validate package X's signature` message. Validation passes. 2. Orchestrator resends the `Validate package X's signature` message. Validation fails (even though the signature is still valid). 3. Orchestrator sends a `Revalidate package X's signature` message. Validation passes. This turned out to be a bad idea as the orchestrator may validate the same package multiple times if it was uploaded multiple times concurrently. With this PR, the new results would be: 1. Orchestrator sends a `Validate package X's signature` message. Validation passes. 2. Orchestrator resends the `Validate package X's signature` message. Validation passes. 3. Orchestrator sends a `Revalidate package X's signature` message. Validation passes.
1 parent 384546b commit f50ef65

8 files changed

Lines changed: 35 additions & 109 deletions

File tree

‎src/Validation.PackageSigning.ExtractAndValidateSignature/Job.cs‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
using System.IO;
88
using System.Net;
99
using System.Net.Http;
10+
using System.Reflection;
1011
using System.Threading.Tasks;
1112
using Autofac;
1213
using Autofac.Extensions.DependencyInjection;
@@ -157,7 +158,9 @@ private void ConfigureJobServices(IServiceCollection services, IConfigurationRoo
157158

158159
services.AddSingleton(p =>
159160
{
160-
var assemblyName = typeof(SignatureValidationMessage).Assembly.GetName();
161+
var assembly = Assembly.GetEntryAssembly();
162+
var assemblyName = assembly.GetName().Name;
163+
var assemblyVersion = assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()?.InformationalVersion ?? "0.0.0";
161164

162165
var client = new HttpClient(new WebRequestHandler
163166
{
@@ -166,7 +169,7 @@ private void ConfigureJobServices(IServiceCollection services, IConfigurationRoo
166169
});
167170

168171
client.Timeout = configurationRoot.GetValue<TimeSpan>(PackageDownloadTimeoutName);
169-
client.DefaultRequestHeaders.Add("user-agent", $"{assemblyName.Name}/{assemblyName.Version}");
172+
client.DefaultRequestHeaders.Add("user-agent", $"{assemblyName}/{assemblyVersion}");
170173

171174
return client;
172175
});

‎src/Validation.PackageSigning.ExtractAndValidateSignature/SignatureValidationMessageHandler.cs‎

Lines changed: 20 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -121,26 +121,11 @@ private async Task<bool> HandleUnsignedPackageAsync(ValidatorStatus validation,
121121
message.ValidationId);
122122

123123
// Update the package's state.
124-
// TODO: Determine whether this is a revalidation request.
125-
var result = await _packageSigningStateService.TrySetPackageSigningState(
126-
validation.PackageKey,
127-
message.PackageId,
128-
message.PackageVersion,
129-
isRevalidationRequest: false,
130-
status: PackageSigningStatus.Unsigned);
131-
132-
if (result == SavePackageSigningStateResult.StatusAlreadyExists)
133-
{
134-
_logger.LogWarning(
135-
"Updates to package signature's state are only allowed on explicit revalidations for package {PackageId} {PackageVersion} for {ValidationId}",
136-
message.PackageId,
137-
message.PackageVersion,
138-
message.ValidationId);
139-
140-
// The message's request is invalid and no amount of retrying will fix it.
141-
// Consume the message.
142-
return true;
143-
}
124+
await _packageSigningStateService.SetPackageSigningState(
125+
validation.PackageKey,
126+
message.PackageId,
127+
message.PackageVersion,
128+
status: PackageSigningStatus.Unsigned);
144129

145130
validation.State = ValidationStatus.Succeeded;
146131

@@ -153,17 +138,26 @@ private async Task<bool> HandleUnsignedPackageAsync(ValidatorStatus validation,
153138
// Consume the message.
154139
return true;
155140
}
141+
else
142+
{
143+
_logger.LogWarning(
144+
"Unable to save to save due to stale context, requeueing package {PackageId} {PackageVersion} for validation id: {ValidationId}.",
145+
message.PackageId,
146+
message.PackageVersion,
147+
message.ValidationId);
148+
}
156149
}
157150
catch (DbUpdateException e) when (e.IsUniqueConstraintViolationException())
158151
{
152+
_logger.LogWarning(
153+
0,
154+
e,
155+
"Unable to save to save due to unique contrainst violation, requeueing package {PackageId} {PackageVersion} for validation id: {ValidationId}.",
156+
message.PackageId,
157+
message.PackageVersion,
158+
message.ValidationId);
159159
}
160160

161-
_logger.LogWarning(
162-
"Unable to save to save, requeueing package {PackageId} {PackageVersion} for validation id: {ValidationId}.",
163-
message.PackageId,
164-
message.PackageVersion,
165-
message.ValidationId);
166-
167161
// Message may be retried.
168162
return false;
169163
}

‎src/Validation.PackageSigning.ExtractAndValidateSignature/Storage/IPackageSigningStateService.cs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,10 @@ namespace NuGet.Jobs.Validation.PackageSigning.Storage
88
{
99
public interface IPackageSigningStateService
1010
{
11-
Task<SavePackageSigningStateResult> TrySetPackageSigningState(
11+
Task SetPackageSigningState(
1212
int packageKey,
1313
string packageId,
1414
string packageVersion,
15-
bool isRevalidationRequest,
1615
PackageSigningStatus status);
1716
}
1817
}

‎src/Validation.PackageSigning.ExtractAndValidateSignature/Storage/PackageSigningStateService.cs‎

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,7 @@
1010

1111
namespace NuGet.Jobs.Validation.PackageSigning.Storage
1212
{
13-
public class PackageSigningStateService
14-
: IPackageSigningStateService
13+
public class PackageSigningStateService : IPackageSigningStateService
1514
{
1615
private readonly IValidationEntitiesContext _validationContext;
1716
private readonly ILogger<PackageSigningStateService> _logger;
@@ -24,11 +23,10 @@ public PackageSigningStateService(
2423
_logger = logger ?? throw new ArgumentNullException(nameof(logger));
2524
}
2625

27-
public async Task<SavePackageSigningStateResult> TrySetPackageSigningState(
26+
public async Task SetPackageSigningState(
2827
int packageKey,
2928
string packageId,
3029
string packageVersion,
31-
bool isRevalidationRequest,
3230
PackageSigningStatus status)
3331
{
3432
if (string.IsNullOrEmpty(packageId))
@@ -45,11 +43,6 @@ public async Task<SavePackageSigningStateResult> TrySetPackageSigningState(
4543
// this invariant may be broken due to message duplication.
4644
var signatureState = await _validationContext.PackageSigningStates.FirstOrDefaultAsync(s => s.PackageKey == packageKey);
4745

48-
if (signatureState != null && !isRevalidationRequest)
49-
{
50-
return SavePackageSigningStateResult.StatusAlreadyExists;
51-
}
52-
5346
if (signatureState != null)
5447
{
5548
signatureState.SigningStatus = status;
@@ -65,8 +58,6 @@ public async Task<SavePackageSigningStateResult> TrySetPackageSigningState(
6558
SigningStatus = status
6659
});
6760
}
68-
69-
return SavePackageSigningStateResult.Success;
7061
}
7162
}
7263
}

‎src/Validation.PackageSigning.ExtractAndValidateSignature/Storage/SavePackageSigningStateResult.cs‎

Lines changed: 0 additions & 19 deletions
This file was deleted.

‎src/Validation.PackageSigning.ExtractAndValidateSignature/Validation.PackageSigning.ExtractAndValidateSignature.csproj‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@
4141
</ItemGroup>
4242
<ItemGroup>
4343
<Compile Include="Storage\PackageSigningStateService.cs" />
44-
<Compile Include="Storage\SavePackageSigningStateResult.cs" />
4544
<Compile Include="Storage\IPackageSigningStateService.cs" />
4645
<Compile Include="Job.cs" />
4746
<Compile Include="Program.cs" />

‎tests/Validation.PackageSigning.ExtractAndValidateSignature.Tests/PackageSigningStateServiceFacts.cs‎

Lines changed: 4 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -25,46 +25,7 @@ public TheTrySetPackageSigningStateMethod(ITestOutputHelper testOutput)
2525
}
2626

2727
[Fact]
28-
public async Task ReturnsStatusAlreadyExistsWhenSignatureStateNotNullAndNotRevalidating()
29-
{
30-
// Arrange
31-
const int packageKey = 1;
32-
const string packageId = "packageId";
33-
const string packageVersion = "1.0.0";
34-
var packageSigningState = new PackageSigningState
35-
{
36-
PackageId = packageId,
37-
PackageKey = packageKey,
38-
SigningStatus = PackageSigningStatus.Valid,
39-
PackageNormalizedVersion = packageVersion
40-
};
41-
42-
var logger = _loggerFactory.CreateLogger<PackageSigningStateService>();
43-
var packageSigningStatesDbSetMock = DbSetMockFactory.Create(packageSigningState);
44-
var validationContextMock = new Mock<IValidationEntitiesContext>(MockBehavior.Strict);
45-
validationContextMock.Setup(m => m.PackageSigningStates).Returns(packageSigningStatesDbSetMock);
46-
47-
// Act
48-
var packageSigningStateService = new PackageSigningStateService(validationContextMock.Object, logger);
49-
50-
// Assert
51-
var result = await packageSigningStateService.TrySetPackageSigningState(
52-
packageKey,
53-
packageId,
54-
packageVersion,
55-
isRevalidationRequest: false,
56-
status: PackageSigningStatus.Valid);
57-
58-
// Assert
59-
Assert.Equal(SavePackageSigningStateResult.StatusAlreadyExists, result);
60-
validationContextMock.Verify(
61-
m => m.SaveChangesAsync(),
62-
Times.Never,
63-
"Saving the context here is unnecessary.");
64-
}
65-
66-
[Fact]
67-
public async Task ReturnsStatusSuccessAndUpdatedExistingStateWhenSignatureStateNotNullAndRevalidating()
28+
public async Task UpdatesExistingStateWhenSignatureStateNotNullAndRevalidating()
6829
{
6930
// Arrange
7031
const int packageKey = 1;
@@ -88,15 +49,13 @@ public async Task ReturnsStatusSuccessAndUpdatedExistingStateWhenSignatureStateN
8849
var packageSigningStateService = new PackageSigningStateService(validationContextMock.Object, logger);
8950

9051
// Assert
91-
var result = await packageSigningStateService.TrySetPackageSigningState(
52+
await packageSigningStateService.SetPackageSigningState(
9253
packageKey,
9354
packageId,
9455
packageVersion,
95-
isRevalidationRequest: true,
9656
status: newStatus);
9757

9858
// Assert
99-
Assert.Equal(SavePackageSigningStateResult.Success, result);
10059
Assert.Equal(newStatus, packageSigningState.SigningStatus);
10160
validationContextMock.Verify(
10261
m => m.SaveChangesAsync(),
@@ -105,7 +64,7 @@ public async Task ReturnsStatusSuccessAndUpdatedExistingStateWhenSignatureStateN
10564
}
10665

10766
[Fact]
108-
public async Task ReturnsStatusSuccessAndAddedNewStateWhenSignatureStateIsNull()
67+
public async Task AddsNewStateWhenSignatureStateIsNull()
10968
{
11069
// Arrange
11170
const int packageKey = 1;
@@ -122,16 +81,13 @@ public async Task ReturnsStatusSuccessAndAddedNewStateWhenSignatureStateIsNull()
12281
var packageSigningStateService = new PackageSigningStateService(validationContextMock.Object, logger);
12382

12483
// Assert
125-
var result = await packageSigningStateService.TrySetPackageSigningState(
84+
await packageSigningStateService.SetPackageSigningState(
12685
packageKey,
12786
packageId,
12887
packageVersion,
129-
isRevalidationRequest: true,
13088
status: newStatus);
13189

13290
// Assert
133-
Assert.Equal(SavePackageSigningStateResult.Success, result);
134-
13591
var newState = validationContextMock.Object.PackageSigningStates.FirstOrDefault();
13692
Assert.NotNull(newState);
13793
Assert.Equal(packageKey, newState.PackageKey);

‎tests/Validation.PackageSigning.ExtractAndValidateSignature.Tests/Validation.PackageSigning.ExtractAndValidateSignature.Tests.csproj‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,5 +61,8 @@
6161
<Name>Validation.PackageSigning.ExtractAndValidateSignature</Name>
6262
</ProjectReference>
6363
</ItemGroup>
64+
<ItemGroup>
65+
<Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
66+
</ItemGroup>
6467
<Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
6568
</Project>

0 commit comments

Comments
 (0)