When timestamp is untrusted, display the primary signature details and modified output for various verbosity levels in nuget verify command#3992
Conversation
|
|
||
| #if IS_SIGNING_SUPPORTED | ||
| using (var zip = new ZipArchive(ZipReadStream, ZipArchiveMode.Read, leaveOpen: true)) | ||
| if (!_isSigned.HasValue) |
There was a problem hiding this comment.
When debugging dotnet nuget verify command, I noticed that IsSignedAsync method is invoked multiple times in the same code path. Hence added a nullable bool _isSigned to avoid executing the logic multiple times (a minor perf. improvement).
There was a problem hiding this comment.
nit: Consider positive checks where possible.
There was a problem hiding this comment.
I am leaning towards keeping the current implementation as is. If you prefer a positive check, I can change the code as shown below. Please let me know your feedback.
if (_isSigned.HasValue)
return Task.FromResult(_isSigned.Value);
_isSigned = false;
using (var zip = new ZipArchive(ZipReadStream, ZipArchiveMode.Read, leaveOpen: true))
{
var signatureEntry = zip.GetEntry(SigningSpecifications.SignaturePath);
if (signatureEntry != null &&
string.Equals(signatureEntry.Name, SigningSpecifications.SignaturePath, StringComparison.Ordinal))
{
_isSigned = true;
}
}
return Task.FromResult(_isSigned.Value);3e228c6 to
594e688
Compare
src/NuGet.Core/NuGet.Commands/VerifyCommand/VerifyCommandRunner.cs
Outdated
Show resolved
Hide resolved
|
|
||
| #if IS_SIGNING_SUPPORTED | ||
| using (var zip = new ZipArchive(ZipReadStream, ZipArchiveMode.Read, leaveOpen: true)) | ||
| if (!_isSigned.HasValue) |
There was a problem hiding this comment.
nit: Consider positive checks where possible.
heng-liu
left a comment
There was a problem hiding this comment.
Good job @kartheekp-ms !
Just have one question in the below comment.
...t.Core/NuGet.Packaging/Signing/Verification/SignatureTrustAndValidityVerificationProvider.cs
Show resolved
Hide resolved
…ous verbsity levels
594e688 to
e49ae89
Compare
erdembayar
left a comment
There was a problem hiding this comment.
Looks good. Just 2 comments.
Bug
Fixes: NuGet/Home#10316
Fixes: NuGet/Home#10535
Regression? Last working version:
Description
Modified output displayed for various verbosity levels of
dotnet nuget verifyandnuget.exe verifycommand as per spec. Improved log messages & error codes in docs to improve the customer experience based upon the learnings from the recent Debian incident.When timestamp is untrusted, display the primary signature details to fix NuGet/Home#10535
Once this PR is merged,
dotnet nuget verifyandnuget.exe verifycommand displays output in the following order.PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Documentation