-
Notifications
You must be signed in to change notification settings - Fork 686
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
raise warning if insecure sha-1 certificate fingerprint is passed to mssign and reposign commands #5932
Conversation
add tests added tests
This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch. |
private readonly string _noTimestamperWarningCode = NuGetLogCode.NU3002.ToString(); | ||
private readonly string _invalidCertificateFingerprintCode = NuGetLogCode.NU3043.ToString(); | ||
private const string _sha1Hash = "89967D1DD995010B6C66AE24FF8E66885E6E03A8"; |
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 think these break an IDE style rule that static
and const
fields shouldn't be _
-prefixed. Only instance fields should be _
-prefixed.
_noTimestamperWarningCode
-> NoTimestamperWarningCode
_invalidCertificateFingerprintCode
-> InvalidCertificateFingerprintCode
_sha1Hash
-> Sha1Hash
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 renamed _sha1Hash
-> Sha1Hash
.
_noTimestamperWarningCode
and _invalidCertificateFingerprintCode
are read-only strings. IDE analyzers warned of the lack of the prefix _ if I remove the prefix. Hence, I didn't rename the variables.
@@ -137,7 +139,7 @@ public void GetAuthorSignRequest_InvalidFingerprint() | |||
CertificateFile = test.CertificatePath, | |||
CSPName = test.CertificateCSPName, | |||
KeyContainer = test.CertificateKeyContainer, | |||
CertificateFingerprint = "invalid-fingerprint", |
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.
An invalid fingerprint is still a good negative test case. Instead of replacing this value, I suggest converting this test from a Fact
into a Theory
and test both cases, like you did 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.
In this test case we are validating that if an invalid hexadecimal hash has been passed to the certificate fingerprint, then then the command will fail with Can't find specified certificate.
exception.
Once this PR has been merged, the command will throw an exception with NU3034 error code if non-hexadecimal string is passed as fingerprint.
As per your suggestion, I changed the test to pass invalid SHA1 and SHA256 hexadecimal hashes to ensure the certificate lookup fails with the expected exception.
@@ -255,7 +289,7 @@ public void MSSignCommandArgParsing_ValidTimestampHashAlgorithm(string timestamp | |||
var packagePath = Path.Combine(dir, "package.nupkg"); | |||
var timestamper = "https://timestamper.test"; | |||
var certFile = Path.Combine(dir, "cert.p7b"); | |||
var certificateFingerprint = new Guid().ToString(); | |||
var certificateFingerprint = _sha1Hash; |
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.
When testing one validation --- in this case, timestamp algorithm --- amongst many, it's important to pass reasonably valid values for all other (non-tested) validations to isolate the validation under test.
Previously, we passed a GUID string for certificate fingerprint, but now we pass a SHA-1 fingerprint. This PR changes fingerprint validation, specifically for SHA-1 fingerprints, so it's doubly weird that we're passing a deprecated value for a property we're not even testing.
I recommend creating a fake SHA-256 fingerprint and using it wherever we're not testing certificate fingerprint validation.
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.
Thanks for the feedback. Updated tests
Bug
Fixes: https://github.com/NuGet/Client.Engineering/issues/2939
Description
Deprecate the usage of SHA-1 fingerprints in
mssign
andreposign
commands especially forCertificateFingerprint
option. Instead, allowmssign
andreposign
commands to accept SHA-2 (SHA-256, SHA-384 and SHA-512) family fingerprints for searching a local certificate store for the certificate.Here is how the
mssign
andreposign
commands after this PR has been merged:certCollection.Find(X509FindType.FindByThumbprint, CertificateFingerprint, validOnly: false);
I made similar changes to the dotnet sign command in #5895 and nuget.exe sign command in #5924.
PR Checklist