-
Notifications
You must be signed in to change notification settings - Fork 19
[Monitoring] Ensure packages are signed #348
Conversation
...Services.Metadata.Catalog.Monitoring/Validation/Test/Catalog/PackageHasSignatureValidator.cs
Outdated
Show resolved
Hide resolved
...Services.Metadata.Catalog.Monitoring/Validation/Test/Catalog/PackageHasSignatureValidator.cs
Outdated
Show resolved
Hide resolved
...tadata.Catalog.Monitoring/Validation/Test/Exceptions/MissingPackageSignatureFileException.cs
Outdated
Show resolved
Hide resolved
var validators = new List<IAggregateValidator>(); | ||
|
||
validators.AddRange(endpointInputs.Select(e => endpointFactory.Create(e))); | ||
validators.Add(new CatalogAggregateValidator(validatorFactory, expectsSignature)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should have tests for CatalogAggregateValidator
, but wouldn't it be simpler to do away with CatalogAggregateValidator
and conditionally add the validator here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wouldn't be simpler, no. IAggregateValidator
and IValidator
return different values. The thing that runs validators expects them to be wrapped by an IAggregateValidator
so that an AggregateValidationResult
is returned.
src/NuGet.Services.Metadata.Catalog.Monitoring/Validation/Test/ValidationContext.cs
Outdated
Show resolved
Hide resolved
public class RunValidatorAsync : FactsBase | ||
{ | ||
[Fact] | ||
public async Task ReturnsGracefullyIfLeafsHaveSignatureFile() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaves?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I should go back to elementary school 😄
@@ -47,8 +47,8 @@ public void Constructor_InitializesProperties() | |||
cancellationToken); | |||
|
|||
Assert.Same(package, context.Package); | |||
Assert.Same(catalogIndexEntries, context.Entries); | |||
Assert.Same(deletionAuditEntries, context.DeletionAuditEntries); | |||
Assert.Equal(catalogIndexEntries.Count(), context.Entries.Count); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a weaker assertion than Same(...)
; the elements could be different. If order is deterministic, can you simply assert the collections are equal?
Assert.Equal(catalogIndexEntries, context.Entries);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assertion is equivalent as both collections are empty. Also, I can't do Same(...);
as ValidationContext
accepts an IEnumerable
and converts it to a IReadOnlyList
.
...Services.Metadata.Catalog.Monitoring/Validation/Test/Catalog/PackageHasSignatureValidator.cs
Outdated
Show resolved
Hide resolved
...Services.Metadata.Catalog.Monitoring/Validation/Test/Catalog/PackageHasSignatureValidator.cs
Outdated
Show resolved
Hide resolved
...Services.Metadata.Catalog.Monitoring/Validation/Test/Catalog/PackageHasSignatureValidator.cs
Outdated
Show resolved
Hide resolved
...tadata.Catalog.Monitoring/Validation/Test/Exceptions/MissingPackageSignatureFileException.cs
Outdated
Show resolved
Hide resolved
...tadata.Catalog.Monitoring/Validation/Test/Exceptions/MissingPackageSignatureFileException.cs
Outdated
Show resolved
Hide resolved
var context = CreateValidationContext( | ||
catalogEntries: new[] | ||
{ | ||
new CatalogIndexEntry( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can probably reduce this code with a CreateDeleteCatalogEntry
helper method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather limit the numbers of helper methods.
* Feed2Catalog: replace inner loops with one outer loop (#329) Resolve NuGet/Engineering#1526. * V3: rename asynchronous IStorage methods (#336) Progress on NuGet/NuGetGallery#6267. * Test: fix flaky basic search tests (#337) Progress on NuGet/NuGetGallery#6292. * Add signing to search service (#338) Progress on https://github.com/NuGet/Engineering/issues/1644 * Test: add registration tests (#341) Progress on NuGet/NuGetGallery#6317. * remove unused scripts (#340) * MonitoringProcessor: improve throughput (#342) Progress on NuGet/NuGetGallery#6327. * Catalog2Dnx: improve throughput (#335) Progress on NuGet/NuGetGallery#6267. * Functional tests should not be dependent on a specific hash and file size of the test package (#343) * Add MicroBuild dependency and signing of output DLLs (#345) Progress on https://github.com/NuGet/Engineering/issues/1644 * Rewrite SafeRoleEnvironment to not use Assembly.LoadWithPartialName (#339) * Test: improve and add registration tests (#346) More progress on NuGet/NuGetGallery#6317. * Use ServerCommon commit that is consistent with other repositories (#347) Progress on https://github.com/NuGet/Engineering/issues/1644 * [Monitoring] Ensure packages are signed (#348) This adds a validation to ensure indexed packages are signed by verifying that the package contains a [package signature file](https://github.com/NuGet/Home/wiki/Package-Signatures-Technical-Details#-the-package-signature-file). This validation is disabled for now. To enable this validation, the flag `-expectsSignature true` must be added to ng.exe's command line arguments. This validation will be enabled once all packages are repository signed. Part of NuGet/Engineering#1627 * V3: make stylistic consistency and cleanup changes (#349) Progress on NuGet/NuGetGallery#6411.
This adds a validation to ensure indexed packages are signed by verifying that the package contains a package signature file.
This validation is disabled for now. To enable this validation, the flag
-expectsSignature true
must be added to ng.exe's command line arguments. This validation will be enabled once all packages are repository signed.Part of https://github.com/NuGet/Engineering/issues/1627