[DB-2037] Add EKUs phase 2: dual server certificates - #5586
[DB-2037] Add EKUs phase 2: dual server certificates#5586timothycoleman wants to merge 15 commits into
Conversation
… the server was originally duplicated when this plugin lived in a separate repo
- classifies inbound ceriticates into node/user/unclassified based on EKUs - used by both certificate authentication providers to guarantee consistent classification (can't match both providers)
…icate will be used for outgoing connections to other nodes
The node certificate authentication provider required client certificates
to have an IP or DNS Subject Alternative Name. This was introduced before
EKU-based classification existed, as a heuristic for "this cert belongs
to a machine rather than a user."
With the inbound certificate classifier now distinguishing node certs
from user certs by their EKU profile, the SAN requirement is redundant.
Neither the .NET SslStream framework nor KurrentDB checks SANs against
the actual connection origin for client certificates, so the SAN check was
purely existence-based and provided no security benefit. (TimC: this isn't just an AI guess - we really checked)
- Remove the SAN check from NodeCertificateAuthenticationProvider
- Remove tests that were testing SAN-specific behaviour; rename and
tighten the remaining tests to reflect what they actually cover
(key usages, CN matching)
- Move the test classes into a sub-namespace to avoid name collisions
We've kept the HasIpOrDnsSan extension method because the outgoing node cert does still require it
and it may be helpful later during startup validation
…nodes use the outgoing cert as the required base for user certs (so all client certs come from the same CA)
… a cluster if we aren't part of a cluster we won't ever use it as a client cert
Deploying eventstore with
|
| Latest commit: |
7118b5a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1e02ab3e.eventstore.pages.dev |
| Branch Preview URL: | https://timothycoleman-eku-phase2.eventstore.pages.dev |
timothycoleman
changed the title
Review Summary by QodoAdd dual server certificates for EKU phase 2 with inbound classification
WalkthroughsDescription• Implement dual certificate support for cluster communication - Add separate node client certificate configuration options - Support both file and Windows certificate store sources - Validate node client certificate at startup in multi-node clusters • Refactor certificate classification to unified inbound certificate classifier - Replace separate IsServerCertificate/IsClientCertificate methods - Classify certificates as Node, User, or Unclassified based on EKU profile - Remove redundant SAN requirement from node certificate authentication • Update certificate expiry monitoring to track both certificates • Consolidate certificate loading logic with helper methods Diagramflowchart LR
A["Configuration Options"] -->|Load| B["Node Certificate"]
A -->|Load| C["Node Client Certificate"]
B -->|Validate| D["Certificate Chain Verification"]
C -->|Validate| D
C -->|Classify| E["CertificateClassification"]
E -->|Monitor| F["Certificate Expiry Monitor"]
C -->|Use for| G["Outbound Cluster Connections"]
B -->|Use for| H["Inbound Server Connections"]
File Changes1. src/KurrentDB.Common/Utils/CertificateClassification.cs
|
Code Review by Qodo
1.
|
There was a problem hiding this comment.
Pull request overview
Adds “dual server certificates” support by introducing a dedicated node client certificate (for outbound intra-cluster connections) alongside the existing node/server certificate, and refactors certificate classification/validation to be shared and consistent across node + user auth paths.
Changes:
- Introduces node-client certificate configuration (file/store + trusted roots) and loads/validates node + node-client certificates at startup.
- Centralizes inbound certificate classification into
CertificateExtensions.ClassifyInboundCertificate(...)and updates node/user auth providers to use it. - Updates cluster networking to send the node-client certificate on outbound connections and updates certificate expiry monitoring to handle multiple certs.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/KurrentDB.Core/Services/Transport/Http/Authentication/NodeCertificateAuthenticationProvider.cs | Switches node-auth logic to EKU-based inbound certificate classification. |
| src/KurrentDB.Core/Configuration/ClusterVNodeOptionsExtensions.cs | Refactors certificate loading helpers; adds node-client cert loader + node-client trusted roots loader. |
| src/KurrentDB.Core/Configuration/ClusterVNodeOptions.cs | Adds node-client certificate option groups (file/store + node-client trusted root store options). |
| src/KurrentDB.Core/ClusterVNode.cs | Routes outbound node traffic to use node-client certificate; updates injected certificate tuple; updates expiry monitor wiring. |
| src/KurrentDB.Core/Certificates/OptionsCertificateProvider.cs | Loads node + node-client certs; validates chains; startup classification check for node-client cert. |
| src/KurrentDB.Core/Certificates/CertificateProvider.cs | Stores node-client certificate + intermediates on the provider. |
| src/KurrentDB.Core/Certificates/CertificateExpiryMonitor.cs | Monitors multiple certificates and de-duplicates by thumbprint. |
| src/KurrentDB.Core.XUnit.Tests/Certificates/inbound_certificate_classification.cs | Adds xUnit coverage for new inbound classification rules. |
| src/KurrentDB.Core.XUnit.Tests/Certificates/OptionsCertificateProviderTests.cs | Adds xUnit coverage for single/dual cert loading + reserved CN behavior. |
| src/KurrentDB.Core.Tests/Services/Transport/Http/Authentication/unix_socket_authentication_provider.cs | Renames namespace for unix socket auth provider tests. |
| src/KurrentDB.Core.Tests/Services/Transport/Http/Authentication/node_certificate_authentication_provider.cs | Updates tests to reflect classification changes; removes SAN-based cases. |
| src/KurrentDB.Core.Tests/Certificates/subject_alternative_names.cs | Adds tests for new HasIpOrDnsSan() helper. |
| src/KurrentDB.Core.Tests/Certificates/key_usages.cs | Removes old NUnit key-usage tests (replaced by new xUnit tests). |
| src/KurrentDB.Core.Tests/Certificates/CertificateExpiryMonitorTests.cs | Updates constructor usage + assertion for new expiry log format. |
| src/KurrentDB.Common/Utils/CertificateExtensions.cs | Adds HasIpOrDnsSan() and new ClassifyInboundCertificate(...); adjusts SAN parsing to return empty on crypto error. |
| src/KurrentDB.Common/Utils/CertificateClassification.cs | Adds new enum to represent inbound certificate classification. |
| src/KurrentDB.Auth.UserCertificates/UserCertificateAuthenticationProvider.cs | Reuses common inbound classification logic (removes plugin-local EKU checks). |
| src/KurrentDB.Auth.UserCertificates/KurrentDB.Auth.UserCertificates.csproj | References KurrentDB.Common to access shared certificate utilities. |
| src/KurrentDB.Auth.UserCertificates/CertificateExtensions.cs | Removes duplicated certificate extension logic from the plugin. |
| src/KurrentDB.Auth.UserCertificates.Tests/CertificateExtensionsTests.cs | Removes tests for removed plugin-local certificate extension logic. |
| docs/server/security/user-authentication.md | Updates user-cert requirements to reference node client certificate. |
| docs/server/security/protocol-security.md | Documents node-client certificate configuration options. |
| docs/server/release-schedule/release-notes.md | Updates release notes header/version/date for 26.1 series. |
| docs/server/quick-start/whatsnew.md | Adds “Dual certificate support for cluster communication” entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _internalServerCertificateValidator = (cert, chain, errors, otherNames) => ValidateServerCertificate(cert, chain, errors, _intermediateCertsSelector, _trustedRootCertsSelector, otherNames); | ||
| _internalClientCertificateValidator = (cert, chain, errors) => ValidateClientCertificate(cert, chain, errors, _intermediateCertsSelector, _trustedRootCertsSelector); | ||
| _internalClientCertificateValidator = (cert, chain, errors) => ValidateClientCertificate(cert, chain, errors, _nodeClientIntermediateCertsSelector, _trustedRootCertsSelector); | ||
| _externalServerCertificateValidator = (cert, chain, errors, otherNames) => ValidateServerCertificate(cert, chain, errors, _intermediateCertsSelector, _trustedRootCertsSelector, otherNames); |
There was a problem hiding this comment.
_internalClientCertificateValidator still uses _trustedRootCertsSelector (roots for the main/node server certificate). In dual-cert setups where the node-client certificate chains to a different CA than the node server cert, inbound intra-cluster connections will fail client-certificate validation. Consider storing/selecting a separate node-client trusted-root collection (e.g., from LoadNodeClientTrustedRootCertificates) and using it for internal client-certificate validation.
| // Validate the node client certificate | ||
| if (hasNodeClientCert) { | ||
| var nodeClientTrustedRootCerts = options.LoadNodeClientTrustedRootCertificates(); | ||
|
|
||
| foreach (var trustedRootCert in nodeClientTrustedRootCerts) { | ||
| Log.Information("Loading trusted root for node client certificate. Subject: {subject}, Thumbprint: {thumbprint}", trustedRootCert.SubjectName.Name, trustedRootCert.Thumbprint); | ||
| } | ||
|
|
||
| if (!VerifyCertificates("node client", nodeClientCertificate, nodeClientIntermediates, nodeClientTrustedRootCerts)) { | ||
| return LoadCertificateResult.VerificationFailed; | ||
| } | ||
| } |
There was a problem hiding this comment.
TryLoadNodeClientCertificate() supports node-client-specific trusted roots, but LoadCertificates() doesn't persist nodeClientTrustedRootCerts anywhere (only TrustedRootCerts is stored). This makes it difficult for the rest of the node to validate inbound node-client certs or user certs against the correct trust roots when dual-cert mode uses a different CA. Consider adding a NodeClientTrustedRootCerts field to CertificateProvider and assigning it here.
| public X509Certificate2 Certificate; | ||
| public X509Certificate2Collection IntermediateCerts; | ||
| public X509Certificate2 NodeClientCertificate; | ||
| public X509Certificate2Collection NodeClientIntermediateCerts; |
There was a problem hiding this comment.
CertificateProvider exposes NodeClientCertificate/NodeClientIntermediateCerts but does not expose a node-client trusted-root collection. If node-client-specific trusted roots are configurable, the provider likely needs to surface them so validators can build chains for inbound intra-cluster client certificates and for user-cert auth consistently.
| public X509Certificate2Collection NodeClientIntermediateCerts; | |
| public X509Certificate2Collection NodeClientIntermediateCerts; | |
| public X509Certificate2Collection NodeClientTrustedRootCerts; |
| .AddSingleton<Func<(X509Certificate2 Node, X509Certificate2Collection Intermediates, | ||
| X509Certificate2Collection Roots)>> | ||
| (() => (_certificateSelector(), _intermediateCertsSelector(), _trustedRootCertsSelector())) | ||
| (() => ( | ||
| _nodeClientCertificateSelector(), | ||
| _nodeClientIntermediateCertsSelector(), | ||
| _trustedRootCertsSelector())) |
There was a problem hiding this comment.
The certificate tuple injected for UserCertificateAuthenticationProvider returns the node-client cert and intermediates, but still returns _trustedRootCertsSelector() (main/node server trusted roots). If user certs are expected to chain to the node-client CA in dual-cert setups, this should likely use the node-client trusted roots (or a combined set) to avoid rejecting otherwise-valid user certificates.
| string reservedNodeCN; | ||
| var reservedNodeCNOption = nameof(options.Certificate.CertificateReservedNodeCommonName); | ||
|
|
||
| // Determine the CN pattern expected from incomming node certificates. |
There was a problem hiding this comment.
Typo in comment: "incomming" should be "incoming".
| // Determine the CN pattern expected from incomming node certificates. | |
| // Determine the CN pattern expected from incoming node certificates. |
| // Unclassified Cert: | ||
| error = "Certificate is not a user certificate. "; | ||
| error += hasServerAuthEku | ||
| ? "Certificate has the serverAuth EKU but not the clientAuth EKU. " + | ||
| "If you are using a certificate from a public CA that does not include the clientAuth EKU, " + | ||
| "please see the documentation for the DisableClientAuthEkuValidation configuration option." | ||
| : "Certificate has the EKU extension but does not have the serverAuth EKU."; |
There was a problem hiding this comment.
The error text for unclassified certificates starts with "Certificate is not a user certificate", but this message is also surfaced when a certificate fails node classification (e.g., serverAuth-only with DisableClientAuthEkuValidation=false). Consider making the message neutral (e.g., "Certificate was not recognized as a node or user certificate") or explicitly reference the expected classification to reduce confusion in logs.
|
|
||
| This is useful when you want to use a certificate from a public CA for inbound client connections (which may only have the `serverAuth` EKU), while using a certificate from a private CA with both the `serverAuth` and `clientAuth` EKUs for inter-node communication. Using dual certificates this way is an alternative to enabling [`DisableClientAuthEkuValidation`](#disable-client-authentication-eku-validation) and keeps the node in compliance with RFC 5280. | ||
|
|
||
| If no node client certificate is configured, the node will use its main certificate as it's client certificate. |
There was a problem hiding this comment.
Grammar: "as it's client certificate" should be "as its client certificate".
| If no node client certificate is configured, the node will use its main certificate as it's client certificate. | |
| If no node client certificate is configured, the node will use its main certificate as its client certificate. |
| # Release notes | ||
|
|
||
| This page contains the release notes for KurrentDB v26.0. | ||
| This page contains the release notes for KurrentDB v26.1. |
There was a problem hiding this comment.
The page says it's for KurrentDB v26.1, but the first release entry is labeled/linking to 26.0.1. Please align the heading/link (26.1.x) with the stated version, or adjust the page intro if it should still be 26.0.x.
| This page contains the release notes for KurrentDB v26.1. | |
| This page contains the release notes for KurrentDB v26.0. |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
… store configuration they can be different
Per the EKU KDD
Refactors:
Main feature:
Aside: