[Package Signing] Add more Validate Certificate integration tests #338
Conversation
loic-sharma
requested review from
joelverhagen,
agr,
xavierdecoster,
dtivel and
skofman1
|
@dtivel Issuing CAs with weak signatures proved to be difficult as the necessary APIs are not public and I ran into issues when using reflection. If we feel strongly that we should cover this case, I can modify client code to support this testing scenario. |
loic-sharma
force-pushed
the
loshar-integration-tests
branch
from
e89c573
to
9266bc4
Compare
loic-sharma
force-pushed
the
loshar-integration-tests
branch
from
9266bc4
to
0fe2a81
Compare
| @@ -26,10 +26,12 @@ public class CertificateVerificationResult | |||
| DateTime? statusUpdateTime = null, | |||
| DateTime? revocationTime = null) | |||
| { | |||
| if (status != EndCertificateStatus.Revoked && revocationTime.HasValue) | |||
| if (revocationTime.HasValue && | |||
There was a problem hiding this comment.
This was wrong. Certificates can be both invalid and revoked.
| else if (ResultHasOnlyFlags(result, X509ChainStatusFlags.NotTimeValid | X509ChainStatusFlags.HasWeakSignature)) | ||
| // NotTimeValid and HasWeakSignature fail packages only at ingestion. It is assumed that a chain with HasWeakSignature will | ||
| // ALWAYS have NotSignatureValid. | ||
| else if (result.StatusFlags == X509ChainStatusFlags.NotTimeValid || |
There was a problem hiding this comment.
From integration testing, I found that HasWeakSignature due to a key with insufficient length will also have the flag NotSignatureValid. @dtivel suggested that we be strict and warn if NotSignatureValid is not present along with HasWeakSignature. I added tests to enforce this behavior.
| return CreateSigningCertificate(intermediateCa); | ||
| } | ||
|
|
||
| private BCCertificate IssueCaCertificate( |
There was a problem hiding this comment.
IssueCaCertificate and NewCertificateAuthority are using client APIs that are currently private, hence the reflection. @dtivel is doing the necessary changes to the client APIs so that this reflection will not be necessary in the future.
loic-sharma
force-pushed
the
loshar-integration-tests
branch
from
592bed7
to
8461ce3
Compare
| yield return new object[] | ||
| { | ||
| /* use: */ EndCertificateUse.CodeSigning, | ||
| /* flags: */ X509ChainStatusFlags.NotTimeValid | X509ChainStatusFlags.HasWeakSignature, | ||
| /* flags: */ X509ChainStatusFlags.HasWeakSignature, |
There was a problem hiding this comment.
It would be clear to have a private class with real property names which is later mapped to object[].
There was a problem hiding this comment.
I'd like to keep abstractions and cognitive overhead to a minimum on tests. I would argue that the clarity of a private class would be about the same as today with the comments. I think the biggest win of using a private class would be added type safety, but I'm not particularly worried about this on tests.
There was a problem hiding this comment.
The comments can get out of date and could have different formatting from developer to developer. Code styles are much easier to describe and understand. Refactoring tools won't pick up on things like this.
There was a problem hiding this comment.
How is a private class an abstraction? Concerning cognitive overhead, wouldn't object initializer be the same as this?
There was a problem hiding this comment.
I don't feel too strongly about this, I'll go ahead and fix this.
* 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.
* 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.
No description provided.