Refactor AllowListVerificationProvider #2454
Conversation
PatoBeltran
force-pushed
the
dev-pb-allowUntrusted
branch
2 times, most recently
from
aa2e608 to
43fdc99
Compare
PatoBeltran
changed the title
PatoBeltran
force-pushed
the
dev-pb-allowUntrusted
branch
2 times, most recently
from
1217003 to
f47311b
Compare
PatoBeltran
changed the title
PatoBeltran
force-pushed
the
dev-pb-allowUntrusted
branch
2 times, most recently
from
8021b23 to
16c4598
Compare
PatoBeltran
force-pushed
the
dev-pb-allowUntrusted
branch
from
16c4598 to
60b8736
Compare
src/NuGet.Core/NuGet.Packaging/PackageExtraction/PackageExtractionContext.cs
Show resolved
Hide resolved
src/NuGet.Core/NuGet.Packaging/Signing/Utility/RepositorySignatureInfoUtility.cs
Outdated
Show resolved
Hide resolved
| /// <summary> | ||
| /// Require AllowList to not be null or empty | ||
| /// </summary> | ||
| public bool RequireNonEmptyAllowList { get; } |
There was a problem hiding this comment.
Do you really need this?
Doesn't SignatureValidationMode already tell you whether it can non-empty?
There was a problem hiding this comment.
It's not strictly necessary since we calculate it from the SignatureValidationMode (here). I thought it was good for the sake of readability. Do you think we should remove this and just use the Policy attribute for this?
| <value>A list of trusted signers is required but none was found.</value> | ||
| </data> | ||
| <data name="DefaultError_NoMatchInAllowList" xml:space="preserve"> | ||
| <value>The package signature certificate fingerprint does not match any certificate fingerprint in allow list.</value> |
| verificationProviders.Add(new AllowListVerificationProvider( | ||
| repositoryAllowList, | ||
| repositorySignatureInfo.AllRepositorySigned, | ||
| Strings.Error_NoRepoAllowList, |
There was a problem hiding this comment.
do you really need to pass these string literals? it's confusing to understand the message or meaning of these strings...
There was a problem hiding this comment.
In our context, this verification provided is used in two different ways. One is to verify that a package is signed by the certificate that the repository announces, and the other is to verify that the package is allowed by the client. These strings are used to give a more detailed error message in the case of failure for the different usages of this provider.
Do you think there is a better way to accomplish this? Will adding named parameters to this call improve the readability?
|
|
||
| var allowListResults = await Task.WhenAll(certificateListVertificationRequests.Select(r => VerifyAllowList(r, settings.AllowUntrusted))); | ||
| _emptyListErrorMessage = string.IsNullOrEmpty(emptyListErrorMessage) ? Strings.DefaultError_EmptyAllowList : emptyListErrorMessage; | ||
| _noMatchErrorMessage = string.IsNullOrEmpty(noMatchErrorMessage) ? Strings.DefaultError_NoMatchInAllowList : noMatchErrorMessage; |
There was a problem hiding this comment.
I dont understand the purpose of these strings literals... but seems very vague
|
|
||
| return new SignedPackageVerificationResult(GetValidity(allowListResults), signature, allowListResults.SelectMany(r => r.Issues)); | ||
| public Task<PackageVerificationResult> GetTrustResultAsync(ISignedPackageReader package, PrimarySignature signature, SignedPackageVerifierSettings settings, CancellationToken token) |
There was a problem hiding this comment.
not sure if you really need async here?
There was a problem hiding this comment.
This is to implement the ISignatureVerificationProvider interface. All verification providers work this way
test/NuGet.Core.Tests/NuGet.Packaging.Test/SigningTests/ClientPolicyContextTests.cs
Show resolved
Hide resolved
* SignedPackageVerifierSettings should not have allowList information * allowListVerificationProvider should only receive one allowList
Bug
Fixes: Fourth part of NuGet/Home#6961
Fix
Refactored
AllowListVerificationProviderto only receive one list and be used multiple times if needed.Removed allow lists from
SignedPackageVerifierSettingssince they only apply for theAllowListVerificationProviderand should not be part of the settings.Created a
ClientPolicyContextto store the data related to the signature verification with the appropriate client policies.This PR does not include any functional changes to the verification behavior.