Fix GDS Push UpdateCertificate ignoring configured SecurityConfiguration policy (master378)#3936
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
This PR backports the fix from #3931 to master378 by changing how server-side GDS Push UpdateCertificate validates an incoming CA-signed application certificate, so it no longer (incorrectly) applies hard-coded CertificateValidator policy defaults instead of honoring operator configuration.
Changes:
- Replaces the throwaway
CertificateValidatorvalidation with a lenientX509Chainbuild to verify issuer-chain integrity from the caller-supplied issuer set. - Disables revocation checks/downloads in the chain policy to avoid network dependence during push validation.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master378 #3936 +/- ##
=============================================
+ Coverage 60.10% 60.13% +0.02%
=============================================
Files 378 378
Lines 79063 79069 +6
Branches 13838 13838
=============================================
+ Hits 47521 47548 +27
+ Misses 27120 27107 -13
+ Partials 4422 4414 -8
🚀 New features to boost your workflow:
|
romanett
left a comment
romanett
left a comment
There was a problem hiding this comment.
Imo this fix is too verbose why not seed a new certificate validation from the Security configuration?
@copilot please address |
Done in d2d44d7. Replaced the verbose |
Proposed changes
Backport of the #3931 fix to master378. On this branch, server-side GDS Push
UpdateCertificatevalidated the new application certificate with a throwawayCertificateValidatorthat was never seeded with the server'sSecurityConfiguration. It therefore enforced hard-coded defaults (MinimumCertificateKeySize = 2048,RejectSHA1SignedCertificates = true) regardless of operator configuration — e.g. a server configured for a 1024-bit minimum still rejected a re-issued 1024-bit certificate withBadCertificatePolicyCheckFailed. Regression originated in #3795 (integrity check switched from a lenientX509Chaincheck to the full validator).The master branch reworked this area entirely, so the fix is re-implemented from scratch here, adapted to master378's
X509Certificate2types.ConfigurationNodeManager.UpdateCertificateAsync: replaced the throwawayCertificateValidatorwith a lenientX509Chainintegrity check that verifies only chain integrity — valid signatures and a complete issuer chain — using the caller-supplied issuers as the trust anchor (AllowUnknownCertificateAuthority, revocationNoCheck, downloads disabled). Policy (min key size, SHA-1, revocation) is intentionally not applied here; it remains governed by the server'sSecurityConfigurationon its normal communication channels, so configured values are honored in both stricter and more-lenient directions.Chain status handling:
NoError/UntrustedRootpass;NotSignatureValid/PartialChain/NotValidForUsage/InvalidBasicConstraintsfail withBadSecurityChecksFailed; and the chain element count must match the supplied issuer count to reject incomplete chains.Related Issues
Types of changes
What types of changes does your code introduce?
Put an
xin the boxes that apply. You can also fill these out after creating the PR.Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
The lenient X509Chain approach mirrors the master-branch fix and restores pre-#3795 behavior:
UpdateCertificateverifies only the integrity of the supplied cert + issuer chain, leaving acceptance policy to the server's configuration. ExistingUpdateCertificateCASignedpush tests (RSA + ECC NIST/Brainpool) pass; both review gates (net10.0, net48) build with 0 warnings. Explicit coverage for the non-default-key-size scenario is not added here — worth considering if maintainers want a regression guard.