Certificate manager and ref counted certificate type#3688
Open
marcschier wants to merge 182 commits intomasterfrom
Open
Certificate manager and ref counted certificate type#3688marcschier wants to merge 182 commits intomasterfrom
marcschier wants to merge 182 commits intomasterfrom
Conversation
Add 'using var' to undisposed IDisposable local variables across test files: - NodeStateTests.cs: 44 state objects (BaseObjectState, BaseEventState, etc.) - ConditionStateTests.cs: 9 condition/alarm state objects - NodeStateCollectionConcurrencyTests.cs: 3 test node states - NodeStateHandlerConcurrencyTests.cs: 6 test node states - BaseDataVariableTypeStateSerializationTest.cs: 6 state objects - SecuredApplicationTests.cs: 2 XmlEncoder instances - SecurityConfigurationManagerTests.cs: 2 XmlEncoder instances - SecurityConfigManagerAdditionalTests.cs: 1 XmlEncoder instance - HttpsTransportListenerTests.cs: 5 listeners + 1 MemoryStream - NullChannelTests.cs: 1 NullChannel instance Skipped: X509Certificate2/certificate-related, Security\Certificates\ folder, property assignments owned by parent objects, and loop-created objects shared across threads. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address dispose-before-losing-scope (CA2000) warnings across 14 files in the Libraries/ directory. X509Certificate2-related warnings are excluded per project conventions. Patterns applied: - using var for temporary objects (event states, encoders, parsers) - try/finally with temp variable null-out for ownership transfers - try/finally with Dispose() after collection removal - Field disposal (m_strm?.Dispose()) before setting to null - (IDisposable)?.Dispose() cast for interface types Files fixed: - UaPubSubConfigurationHelper.cs: XmlEncoder, XmlParser - Subscription.cs (Server): RefreshStartEventState, RefreshEndEventState - MonitoredItem.cs: EventQueueOverflowEventState - DiagnosticsNodeManager.cs: 5 node state ownership transfers - SessionManager.cs: Nonce, UserIdentity, Session disposal - Session.cs (Client): 3 UserIdentity ownership transfers - DefaultSessionFactory.cs: UserIdentity ownership transfer - GlobalDiscoverySampleServer.cs: 2 UserIdentity ownership transfers - MasterNodeManager.cs: ContinuationPoint disposal - SubscriptionManager.cs: SessionPublishQueue disposal after removal - SamplingGroupManager.cs: SamplingGroup disposal - CustomNodeManager.cs: 2 IMonitoredItem out param disposals - AsyncCustomNodeManager.cs: NodeState + IMonitoredItem disposals - TrustList.cs: 3 MemoryStream field disposals Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add using declarations to undisposed IDisposable objects across 22 test files in Opc.Ua.PubSub.Tests to resolve CA2000 analyzer warnings: - UaPubSubApplication.Create() return values - UadpNetworkMessage constructor calls - MqttPubSubConnection constructor calls - MemoryStream constructor calls Field assignments in SetUp methods that are already properly disposed in TearDown methods are left unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace X509Certificate2 with Certificate in all public and internal APIs within the Security.Certificates project: - ICertificateBuilder: CreateForRSA/CreateForECDsa return Certificate; SetIssuer accepts Certificate - CertificateBuilderBase: IssuerCAKeyCert field changed to Certificate - CertificateBuilder: All Create methods wrap results with Certificate.From - X509PfxUtils: VerifyKeyPair, VerifyRSAKeyPair, VerifyECDsaKeyPair, IsECDsaSignature, CreateCertificateFromPKCS12 use Certificate - PEMWriter: Export methods accept Certificate - CrlBuilder: CreateForRSA/CreateForECDsa, AddRevokedCertificate use Certificate - X509Crl: VerifySignature, IsRevoked accept Certificate - X509Extensions: FindExtension and BuildAuthorityKeyIdentifier use Certificate - BouncyCastle X509Utils: Internal helpers use Certificate - X509CertificateLoader left unchanged (system polyfill) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add 'using' to IDisposable objects (Certificate, CertificateCollection, Activity) that were not being disposed in test code across: - Tests/Opc.Ua.Security.Certificates.Tests (CertificateTestsForRSA.cs, CertificateTestsForECDsa.cs) - Tests/Opc.Ua.Server.Tests (CreateSessionApplicationUriValidationTests.cs) - Tests/Opc.Ua.Client.Tests (Session/ClientTest.cs) - Tests/Opc.Ua.Gds.Tests (CertificateGroupTests.cs, X509TestUtils.cs) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The using declaration on senderCertificateChain caused premature disposal of certificates that are stored in SenderCertificate and SenderIssuerCertificates fields. When the using scope ends, each certificate in the collection has its refcount decremented, potentially disposing the inner X509Certificate2 while the fields still reference it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…management - Certificate._refCount starts at 1, AddRef increments, Dispose decrements - Inner X509Certificate2 disposed only when refcount reaches 0 - DirectoryCertificateStore.EnumerateAsync/FindByThumbprintAsync call AddRef on cached certs before returning in CertificateCollection - Fixes shared cert disposal when CertificateCollection.Dispose is called Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nd tests Phase 1: Core types (TrustListIdentifier, CertificateEntry, CertificateValidationResult, CertificateChangeEvent) + segregated interfaces (ICertificateRegistry, ICertificateTrustListManager, ICertificateValidatorEx, ICertificateLifecycle, ICertificateFactory, ICertificateIssuer, ICertificateStoreProvider, ICertificateManager) Phase 2: DefaultCertificateFactory, DefaultCertificateIssuer, DirectoryStoreProvider, X509StoreProvider, InMemoryStoreProvider Phase 3: CertificateManager core (trust-list registry, cert registry, validation pipeline, lifecycle with IObservable notifications, Channel-based rejected cert processor, transaction support, DI factory) Phase 4A: CertificateValidatorAdapter backward compat 47 tests covering all components. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ibraries - ServerBase: Initialize CertificateManager from SecurityConfiguration on startup - StandardServer: Log CertificateManager trust list count - ApplicationInstance: Create CertificateManager after cert validation - CertificateGroup: Add opt-in ICertificateIssuer property - ApplicationsNodeManager: Add opt-in ICertificateTrustListManager property - Utils: Add ParseCertificateChainBlob overload accepting ICertificateFactory Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Replace ArgumentNullException.ThrowIfNull with manual checks (pre-.NET 6 compat) - Replace ObjectDisposedException.ThrowIf with manual check (pre-.NET 7 compat) - Replace ValueTask.CompletedTask with default (pre-.NET 5 compat) - Remove all 14 #region/#endregion directives from CertificateCollection.cs - Add 5 TrustListTransaction tests (commit, remove, rollback, disposed, CRL) - Add 4 CertificateValidatorAdapter tests (success/failure for single and chain) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ess, deeper wiring, obsolete attrs - CertificateLifecycleMonitor: periodic timer emitting CertificateExpiring events - ITrustListFileAccess + TrustListData: read/write trust-lists as blobs (Part 12 §7.5) - StandardServer subscribes to CertificateManager.CertificateChanges - GDS CertificateGroup uses ICertificateIssuer when available - [Obsolete] on CertificateFactory.CreateCertificate/CreateSigningRequest/RevokeCertificate - [Obsolete] on CertificateStoreType.RegisterCertificateStoreType Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ry methods Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Docs/CertificateManager.md covers: - Architecture overview with interface segregation diagram - Quick start: creation, validation, subscriptions, trust-lists - Full interface reference tables for all 7 interfaces - Pluggable store backend guide - Certificate reference counting explanation - Backward compatibility notes - OPC UA specification alignment table Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…igration Certificates.md: - Add Certificate/CertificateCollection wrapper types section - Update validation intro to reference CertificateManager - Add InMemory store type to store types list - Update pseudocode from X509Certificate2 to Certificate types - Add cross-references to CertificateManager.md MigrationGuide.md: - Add Certificate Management section under 1.5.378 to 1.6.x migration - Document Certificate/CertificateCollection wrapper migration - Document CertificateManager interfaces and usage - Document obsoleted APIs with replacement table Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
romanett
reviewed
Apr 20, 2026
…eCollection ApplicationsNodeManager.cs: Add using to CertificateCollection from EnumerateAsync before calling AsX509Certificate2Collection() for ExtraStore.AddRange. Also add using to X509Chain (was missing dispose). CertificateValidator.cs ExtraStore.Add(issuer.Certificate.X509): verified safe — issuers list stays alive throughout the using X509Chain block. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TestSubscription.OnKeepAliveNotificationAsync was returning WaitAsync() which blocks forever on a Block semaphore that no test ever releases. Previously this was harmless because the tests leaked subscription instances (no DisposeAsync). After the recent cleanup commits added 'await using' to all SubscriptionTests, the dispose path waits on the message worker task, which itself is stuck inside the keep-alive callback — hanging the entire CI Client job. OnPublishReceivedAsyncShouldProcessNotificationAsync sends an empty NotificationMessage which the worker classifies as a keep-alive, so every dispose hits the trap. Make the override return default. The Block field stays in case specific tests opt in via WaitAsync(). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
# Conflicts: # Stack/Opc.Ua.Core/Security/Certificates/CertificateManager/CertificateManager.cs # Stack/Opc.Ua.Core/Security/Certificates/CertificateManager/CertificateValidatorAdapter.cs # Stack/Opc.Ua.Core/Security/Certificates/CertificateTypesProvider.cs # Stack/Opc.Ua.Core/Security/Certificates/CertificateValidator.cs # Stack/Opc.Ua.Core/Stack/Server/ServerBase.cs # Tests/Opc.Ua.Core.Tests/Security/Certificates/CertificateManager/CertificateManagerTests.cs # Tests/Opc.Ua.Core.Tests/Security/Certificates/CertificateManager/CertificateValidatorAdapterTests.cs # Tests/Opc.Ua.Core.Tests/Security/Certificates/TemporaryCertValidator.cs # Tests/Opc.Ua.Core.Tests/Stack/Configuration/ApplicationConfigurationTests.cs # Tests/Opc.Ua.Core.Tests/Stack/Transport/HttpsTransportListenerTests.cs # Tests/Opc.Ua.Gds.Tests/X509TestUtils.cs
Phase 0 of the editorconfig-driven cleanup pass requires a green baseline build with /p:RunAnalyzersDuringBuild=true. Three error-level analyzer diagnostics were blocking that build: * RCS1174 in CertificateManager.GetIssuersAsync — the method awaited a single tail call. Convert it to a non-async method that returns the inner Task directly. Argument validation still throws synchronously. * RCS1047 on ServerBase.OnCertificateUpdateAsync — the method is protected virtual and renaming would be a binary breaking change for derived classes. Wrap the declaration in a narrow #pragma disable with a one-line reason. * NUnit2023 in X509IdentityTokenHandlerTests — Assert.That on a ByteString (struct) for Is.Not.Null is tautological; remove the redundant assertion. Build now succeeds with 0 errors (1150 warnings) under /p:RunAnalyzersDuringBuild=true. Subsequent commits will apply dotnet format passes against this clean baseline. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Applied via 'dotnet format whitespace UA.slnx'. 42 files, ~440 line changes auto-fixed (trailing whitespace, blank line normalisation, missing final newlines). Build clean with /p:RunAnalyzersDuringBuild=true. Tests green on net10.0 (Core: 6615 passed, 86 skipped). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| { | ||
| m_logger.LogError( | ||
| "Certificate {Certificate} rejected. Reason={ServiceResult}.", | ||
| certificate, |
| serviceResult.StatusCode == StatusCodes.BadCertificateUntrusted) | ||
| { | ||
| accept = true; | ||
| m_logger.LogInformation("Auto accepted certificate {Certificate}", certificate); |
| { | ||
| m_logger.LogError( | ||
| "Certificate {Certificate} validation failed with suppressible errors but was rejected. Reason={ServiceResult}.", | ||
| certificate, |
| { | ||
| if (store == null) | ||
| { | ||
| m_logger.LogWarning("Failed to open issuer store: {CertificateStore}", certificateStore); |
| @@ -1201,17 +1278,19 @@ | |||
| m_lastDirectoryCheck = DateTime.MinValue; | |||
| } | |||
|
|
|||
| m_logger.LogInformation(Utils.TraceMasks.Security, "Certificate store reloaded from {Path}, {Count} entries.", StorePath, m_certificates.Count); | |||
… ids on Update Three coupled fixes for GDS UpdateCertificate (Push) regressions on x509: 1. ServerBase.StartAsync now reuses ApplicationConfiguration.CertificateManager when present instead of creating its own. Without this, ApplicationInstance and ServerBase ended up with two distinct CertificateManager instances: ConfigurationNodeManager.ApplyChanges updated one (the application config's), while StandardServer's CertificateManagerChangeObserver was subscribed to the other - so the cert change notification never fanned out to endpoint descriptions / transport listeners and the server kept serving the old cert. 2. CertificateManager.UpdateAsync calls LoadPrivateKeyExAsync on each application certificate identifier before reloading. This restores the pre-Phase 8 behaviour: CertificateIdentifier caches the last loaded cert in m_certificate, so FindAsync(true) inside the registry reload would otherwise return the stale cached instance even after the underlying store was mutated by the GDS push. 3. ConfigurationNodeManager.UpdateCertificateInternalAsync replaces the cached Certificate on the existing identifier with the new (private-key-bearing) cert immediately after writing it to the application store. The previous FindAsync-with-discarded-result call was a no-op because the identifier still referenced the disposed-from-store old cert via its cache. Local Gds.Tests verification: 92/104 passed, 12 skipped, 0 failed (was 0 of 78 UpdateCertificate variants passing on this branch). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Applied via 'dotnet format style UA.slnx --diagnostics IDE0032 IDE0049'. The IDE0032 (use auto property) fixer crashed mid-run with 'SyntaxTree is not part of the compilation, so it cannot be removed' and produced no auto-property changes. The benign side effect of the pass was to apply the dotnet_sort_system_directives_first ordering on files where it had drifted; 40 files updated with using directive reorderings only. Build clean with /p:RunAnalyzersDuringBuild=true. IDE0032 candidates will need to be revisited later (manual or with a working fixer). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Applied via 'dotnet format analyzers UA.slnx --diagnostics RCS1015
RCS1031 RCS1032 RCS1037 RCS1043 RCS1049 RCS1058 RCS1061 RCS1090
RCS1129'.
5 files updated:
* RCS1043 — removed redundant 'partial' modifier from CertificateManager
(only one declaration exists).
* RCS1061 — merged nested 'if disposing { if m_ecdh != null }' into a
single conjunction in Nonce.Dispose; collapsed redundant block scope
in SecurityPolicies switch case.
* RCS1090 — replaced parenthesised lambda expression with a bare lambda
in ManagedSessionComplianceTests.
* RCS1049 — simplified 'c.Final == false' to '!c.Final' in
MonitoredItemTests.
Build clean with /p:RunAnalyzersDuringBuild=true.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Applied via 'dotnet format analyzers UA.slnx --diagnostics RCS1132 RCS1133 RCS1134 RCS1136 RCS1139 RCS1140 RCS1142 RCS1143 RCS1145 RCS1151'. 7 files updated: * RCS1132 — merged BuiltInType.Number/Integer fall-through cases in TypeInfo.cs (consecutive cases with identical bodies). * RCS1140 — added <exception> elements to xml-doc comments where ArgumentNullException is thrown for the named parameter. * RCS1142 — added <typeparam> elements to xml-doc comments for generic members. * RCS1141 — wrapped existing prose-style /// comments above test methods in <summary>...</summary> tags in ApplicationConfigurationTests. Build clean with /p:RunAnalyzersDuringBuild=true. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Stateless helpers that resolve a CertificateIdentifier into a Certificate without mutating the identifier. First step toward removing the embedded Certificate cache (and IDisposable) from CertificateIdentifier. Resolution order: 1. registry.ApplicationCertificates by thumbprint (when registry given) 2. inline RawData materialization 3. fallback to opening the identifier's store and Find() LoadPrivateKeyAsync mirrors today's CertificateIdentifier.LoadPrivateKeyExAsync (password provider, X509Store fallback, applicationUri fallback after rotation) but never writes back into the identifier. OpenStore is the registry-less store opener used by call sites that today do id.OpenStore(telemetry). No call sites migrated yet — purely additive. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Applied via 'dotnet format analyzers UA.slnx --diagnostics RCS1006 RCS1013 RCS1033 RCS1078 RCS1085 RCS1089 RCS1097 RCS1112'. 3 files updated: * RCS1085 — replaced backing-field + expression-bodied property pairs with auto-implemented properties in OptionSet (TypeId, BinaryEncodingId, XmlEncodingId, ByteLength) and RoleBasedIdentity (InnerIdentity). * RCS1049 — simplified 'c.Final == true' / 'c.ItemDisposed == true' comparisons in MonitoredItemTests. Build clean with /p:RunAnalyzersDuringBuild=true. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Applied via 'dotnet format analyzers UA.slnx --diagnostics RCS1170 RCS1207 RCS1225 RCS1244 RCS1246 RCS1249 RCS1252 RCS1257 RCS1258 RCS1259'. 2 files updated: * RCS1249 — removed unnecessary null-forgiving '!' suffix on 'password' arg in CertificateIdentifier (compiler can already prove the value is non-null at the call site). * RCS1252 — removed redundant try/empty-finally wrapper around the revoked-certificates loop in CertificateFactoryTest (the finally was empty so the try is redundant). Build clean with /p:RunAnalyzersDuringBuild=true. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Applied via 'dotnet format analyzers UA.slnx --diagnostics RCS0010 RCS0011 RCS0021 RCS0023 RCS0025 RCS0027 RCS0029 RCS0031'. 7 files updated: * RCS0027 — moved binary operator (&&, ||) from leading to trailing position when wrapping, in NodeManagerBuilder, HistoryAotTests, MonitoredItemTests, MockResolver, EncodeableFactoryExtensions. * RCS0029 — added blank lines between property accessors and adjacent backing fields in CertificateValidationCore; expanded empty expression-bodied methods to braced empty blocks in CertificateManagerTests. Build clean with /p:RunAnalyzersDuringBuild=true. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Applied via 'dotnet format analyzers UA.slnx --diagnostics RCS0033 RCS0034 RCS0041 RCS0042 RCS0046 RCS0049 RCS0050 RCS0051 RCS0055 RCS0059 RCS0061 RCS0063 RCS0056 RCS1007 RCS1047 RCS1077 RCS1156 RCS1160 RCS1166 RCS1174 RCS1202 RCS1227 RCS1229'. 6 files updated: * RCS0050 — collapsed double blank lines (between using directives and namespace) in CertificateManager, ServerBase, ApplicationConfigurationTests, X509TestUtils, RoleBasedIdentity. * RCS1186 — collapsed multi-line auto-property declaration to single line in DefaultSubscriptionEngineTests. * RCS0033 / continuation indent — re-indented a collection-expression initializer in CertificateManager. Manual nudge to align the closing bracket with the opening (the formatter shifted one but not the other). Build clean with /p:RunAnalyzersDuringBuild=true. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Phase 1b/c of the CertificateIdentifier-cache removal: * CertificateManager.LoadApplicationCertificatesAsync now uses CertificateIdentifierResolver.LoadPrivateKeyAsync instead of certId.FindAsync(true, ...). The resolver opens the identifier's store directly without touching the (about-to-be-removed) m_certificate cache on CertificateIdentifier. * CertificateManager.UpdateAsync drops the f444c09 LoadPrivateKeyExAsync invalidation loop. It existed solely to scrub the identifier cache before reload; the resolver path doesn't read the cache, so the loop is no longer needed. * CertificateIdentifierResolver.LoadPrivateKeyAsync gains a third post- rotation fallback: when both (configured-thumbprint, configured-subject) and (configured-thumbprint, applicationUri) lookups miss, retry with (null thumbprint, null subject, applicationUri). This is the case exercised by GDS UpdateCertificate NewKeyPair tests where the pushed cert has a different subject (and obviously a different thumbprint). * ConfigurationNodeManager.UpdateCertificateInternalAsync drops the f444c09 existingCertIdentifier.Certificate = newCert poke. Replaced with: - read the current cert thumbprint from the registry (not the identifier cache) so the store DELETE targets the right blob; - call lifecycle.UpdateApplicationCertificateAsync to atomically install the new cert in the registry and fire the change notification. Local Gds.Tests verification: PushTest UpdateCertificate suite 92/104 passed (0 failures, 12 Assert.Ignore skips for unsupported key formats). Both work-arounds shipped in f444c09 are now removed in favour of the resolver. The CertificateIdentifier cache itself remains for now (still consumed elsewhere) — its removal will follow once the rest of the call graph is migrated. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Applied via 'dotnet format analyzers UA.slnx --diagnostics NUnit2001 NUnit2002 NUnit2003 NUnit2004 NUnit2005 NUnit2006 NUnit2007 NUnit2008 NUnit2009 NUnit2010'. 2 files updated: * NUnit2010 — converted Assert.That(x.Equals(y), Is.True) to Assert.That(x, Is.EqualTo(y)) in CertificateWrapperTests (EqualsRoundTrip and EqualsObjectOverloadWorks). * NUnit2009 — converted 'is 0 or 1' pattern assertion to Or-chained EqualConstraint in ManagedSessionComplianceTests. * NUnit2010 — narrowly suppressed for the 'EqualsSameReferenceReturnsTrue' test where the reflexive Equals(self) check is intentional; rewriting it as Is.EqualTo(self) trips NUnit2009 (same actual + expected). Build clean with /p:RunAnalyzersDuringBuild=true. Tests green: * Opc.Ua.Security.Certificates.Tests net10.0: 236 passed. * Opc.Ua.Client.Tests net10.0: 2177 passed, 381 skipped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
romanett
reviewed
May 7, 2026
| try | ||
| { | ||
| using (ICertificateStore appStore = existingCertIdentifier.OpenStore(Server.Telemetry)) | ||
| // Resolve the currently-loaded certificate so we can |
Contributor
There was a problem hiding this comment.
Maybe move this whole delete into Certificate manager, imo No one Else should have to have write Access to the application Certificates
Captures the inventory, batching, and verification commands used during the .editorconfig-driven cleanup pass, plus per-rule notes for diagnostics that needed manual handling or narrow suppression. Records the deferred CA-family warnings (CA2000, CA2007, CA1859, etc.) with counts and reasons so a follow-up PR can pick them up one rule family at a time. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CertificateManagerTests.LoadApplicationCertificatesLoadsFromConfig and SendCertificateChainBlobMatchesLeafOrFullChain construct identifiers via new CertificateIdentifier(cert) without a StorePath. The resolver's store- open path returns null for these (no store metadata), causing the registry to come up empty. Add a guarded fallback at the top of LoadPrivateKeyAsync: when StorePath is empty AND the identifier carries an in-memory Certificate with a private key, return that cert. The StorePath check matters — when a path is configured, the store is the authoritative source and the cached cert may be stale (e.g. after a GDS push that replaced the on-disk cert under an identifier whose ApplicationInstance pre-cached the old version). Local verification: * Core CertificateManagerTests: 15/15 passed * Gds PushTest UpdateCertificate suite: 92 passed, 12 skipped, 0 failed Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ient off identifier cache Phase 1d: * UserIdentity (.ctor and CreateAsync) loads via CertificateIdentifierResolver.LoadPrivateKeyAsync instead of certificateId.LoadPrivateKeyExAsync. Resolver returns the cert; the identifier is no longer mutated to cache it. * GDS server CertificateGroup.LoadSigningKeyAsync and the issuer-key path in CreateRevocationListAsync stop wrapping a Certificate in a CertificateIdentifier(cert) just to call LoadPrivateKeyAsync. Build a metadata-only identifier (StorePath/StoreType/Thumbprint/SubjectName/ CertificateType derived from the in-memory cert) and pass it to the resolver. The 'using' decl on the identifier is dropped — identifiers no longer own a Certificate. * ConsoleReferenceClient.Program drops 'using' on the FindUserCertificateIdentifierAsync result. The returned identifier is consumed by UserIdentity.CreateAsync which loads via the resolver and doesn't take ownership of the identifier itself. Local: PushTest RsaSha256/UpdateCertificateNewKeyPairPEM still passes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…eyAsync
CertificateIdentifierResolver.LoadPrivateKeyAsync replaced
CertificateIdentifier.FindAsync as the cert-loading entry point in
CertificateManager.LoadApplicationCertificatesAsync. The resolver
went straight to OpenStore(identifier, ...) which fails for any
CertificateIdentifier that was constructed in-memory from an existing
Certificate (no StorePath) — a documented usage of the
CertificateIdentifier(Certificate) constructor.
Restore the same fast-path FindAsync had:
Certificate? attached = identifier.Certificate;
if (attached != null && attached.HasPrivateKey)
{
return attached.AddRef();
}
This unblocks the
CertificateManagerTests.LoadApplicationCertificatesLoadsFromConfig and
CertificateManagerTests.SendCertificateChainBlobMatchesLeafOrFullChain
test cases which build the chain in-memory.
Build clean with /p:RunAnalyzersDuringBuild=true. Core 6615/6701
(net10.0), 6567/6653 (net48); Server 920/925 (both TFMs).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… into x509 # Conflicts: # Stack/Opc.Ua.Core/Security/Certificates/CertificateIdentifierResolver.cs
…h Resolver Migrate the remaining production callers of CertificateIdentifier.FindAsync / LoadPrivateKeyExAsync that don't change behavior to the new CertificateIdentifierResolver: * CertificateTrustList.GetCertificatesAsync - resolves explicit TrustedCertificates entries (inline RawData + store search) via the resolver instead of FindAsync. * CertificateValidationCore: validator UpdateAsync's ApplicationCertificates loop, GetTrustedCertificateAsync explicit trusted list, and GetIssuerNoExceptionAsync explicit issuer list. * SecurityConfiguration.FindApplicationCertificateAsync. * ConfigurationNodeManager: 2 LoadPrivateKeyExAsync calls in CSR/UpdateCertificate flows. CertificateIdentifierResolver.ResolveAsync gains a transitional fast path that returns identifier.Certificate.AddRef() when set (mirrors the legacy FindAsync first branch). This bridge will be removed in Phase 3 when m_certificate is stripped from CertificateIdentifier. ApplicationInstance.CheckOrCreateCertificateAsync / DeleteApplicationInstanceCertificateAsync / ApplicationConfiguration.ValidateAsync are deferred to Phase 3 (they need m_certificate semantics that can't be replicated without breaking GDS PushTest UpdateCertificate variants). Local verification: - Build: green (UA.slnx, all TFMs) - Core CertificateManagerTests: 19/19 pass - Core Certificate tests: 270/276 pass (6 skipped, 0 failed) - Configuration tests: 208/208 pass - Gds PushTest UpdateCertificate suite: 92/104 pass, 12 skipped, 0 failed (matches f444c09 baseline) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… CertificateIssuerReference record Introduce a public sealed record CertificateIssuerReference(Certificate, CertificateValidationOptions) as the carrier for resolved chain elements in: * CertificateValidationCore: GetTrustedCertificateAsync, GetIssuerAsync, GetIssuerNoExceptionAsync now return CertificateIssuerReference? (or (CertificateIssuerReference?, ServiceResultException?) for the no-exception variant). * CertificateValidationCore.GetIssuersAsync (both overloads): IList type changes from CertificateIdentifier to CertificateIssuerReference. The chain-walk advance no longer goes through issuer.FindAsync(false) (which only worked because CertificateIdentifier cached m_certificate); it borrows the issuer cert directly from the record. * CertificateValidationCore.CheckChainStatus: signature switches to (X509ChainStatus, CertificateIssuerReference target, CertificateIssuerReference? issuer, bool). Note: target options are used for SuppressCertificateExpired (lines 1611-1630 in legacy). * CertificateValidationCore.InternalValidateAsync: target/issuer locals carry CertificateIssuerReference, fallback target uses CertificateValidationOptions.Default. Disposal moved from CertificateIdentifier.Dispose() to issuer.Certificate.Dispose() in the finally block (records are not disposable). * ICertificateRegistry.GetIssuersAsync: public API change — return IList<CertificateIssuerReference>. * CertificateManager.GetIssuersAsync (interface impl): same change. * Session.LoadCertificateChainAsync: uses issuers[i].Certificate (the record's non-nullable Certificate field). * CertificateManagerTests.GetIssuersAsyncReturnsEmptyForSelfSignedCertificate: test fixture switches list type. Disposal model: - Records do not own their Certificate field. The chain construction paths (resolver, store) AddRef when handing certs to the record, so the record holds an owned reference. Callers (validator, Session, GDS) explicitly dispose via record.Certificate.Dispose() in finally blocks. Behaviour preserved: - Issuer dedup uses Cert.Thumbprint (was CertificateIdentifier.Thumbprint which read from m_certificate). - Validation options for SuppressCertificateExpired now correctly read from target (per-element), not just issuer. Local verification: - Build green (UA.slnx, all TFMs). - Core Certificate tests: 270/276 (6 skipped, 0 failed). - Configuration tests: 208/208 pass. - Server cert/validation tests: 4/4 pass. - Client chain/issuer tests: 1/1 pass. - Gds PushTest UpdateCertificate: 92/104 (12 skipped, 0 failed) — matches f444c09 baseline. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ectionStore + AddTrustedPeer * CertificateIdentifierCollectionStore now stores certificates directly in a CertificateCollection instead of a List<CertificateIdentifier>. All operations (EnumerateAsync, AddAsync, DeleteAsync, FindByThumbprintAsync) become synchronous — no more FindAsync round trip through CertificateIdentifier. Disposal: m_certificates.Dispose() on Dispose(true) drops AddRef'd certs. * Removed the (ArrayOf<CertificateIdentifier>, ITelemetryContext) constructor — unused in production (only the empty ctor is instantiated by CertificateStoreIdentifier.CreateStore and InMemoryStoreProvider). * AddAsync AddRef's the input cert; DeleteAsync disposes on remove; EnumerateAsync / FindByThumbprintAsync return AddRef'd copies. Caller-owned reference semantics throughout. * SecurityConfiguration.AddTrustedPeer(byte[]) switches from the CertificateIdentifier(byte[]) ctor to the RawData setter pattern. Resolves identically through CertificateIdentifierResolver's inline RawData branch. Public API preserved. Local verification: - Build green (UA.slnx, all TFMs) - Core full tests: 6615/6701 (86 skipped, 0 failed) - Cert-related tests: 96/98 (2 skipped, 0 failed) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…date docs Phase 3 (strip): CertificateIdentifier becomes pure metadata. Removed members: * m_certificate field and Certificate get/set property * IDisposable, Dispose(), DisposeCertificate() * Constructors taking Certificate, (Certificate, options), or byte[] * Instance methods FindAsync(...), LoadPrivateKeyAsync(char[], ...), LoadPrivateKeyExAsync(...), OpenStore(...) * IOpenStore declaration on CertificateIdentifier Behaviour: * RawData is now backed by an explicit byte[] field. Setter still derives SubjectName/Thumbprint/CertificateType from the parsed cert. * SubjectName/Thumbprint getters return the configured metadata (no fallback to a cached cert). * Equals compares metadata only. Production callers migrated off the cache: * ApplicationInstance.CheckOrCreateCertificateAsync / CreateApplicationInstanceCertificateAsync / DeleteApplicationInstanceCertificateAsync / CheckApplicationInstanceCertificateAsync use local Certificate variables and CertificateIdentifierResolver instead of id.Certificate. CheckApplicationInstanceCertificateAsync now syncs the identifier metadata (Thumbprint/SubjectName/CertificateType) from the loaded cert so subsequent resolver lookups by Thumbprint find it. * ApplicationConfiguration.ValidateAsync now eagerly calls CertificateManager.LoadApplicationCertificatesAsync to populate the registry instead of mutating each identifier's cached cert. * SecurityConfiguration.FindApplicationCertificateAsync uses Resolver.LoadPrivateKeyAsync when privateKey is requested (the resolver's Find path enumerates only public certs from Directory stores). The ECC type filter now matches by CertificateType instead of inspecting the cached cert. * SecuredApplicationHelpers (CertificateIdentifier wrapper for the schema namespace) delegates Find/OpenStore to the resolver. * DiscoveryClient.CreateAsync uses the resolver. * CertificateValidationCore: the untrustedList for chain construction now wraps each cert as a metadata-only CertificateIdentifier with RawData set (the resolver's inline RawData branch handles it). Removed the per-iteration ownedCertificates list and untrusted identifier disposal — both are no-ops now. * ConfigurationNodeManager: post-rotation existing-cert lookup falls back to ICertificateRegistry.GetApplicationCertificate when the configured subject doesn't match the new cert's subject. CreateSigningRequest queries the registry for the active cert's subject/domain. GetCertificates returns blobs from the registry. Phase 4 (tests): * CertificateIdentifierTests: dropped tests for removed APIs (cert ctors, DisposeCertificate, Certificate property, OpenStore on identifier). Added RawDataSetterDerivesSubjectAndThumbprint to cover the metadata-derivation behaviour. * ApplicationInstanceTests: switched `id.FindAsync` / `id.OpenStore` / `applicationCertificate.Certificate` reads to the resolver. Replaced `Assert.That(applicationCertificate.Certificate, Is.EqualTo(...))` with thumbprint-based assertions. * CertificateStoreTest: switched to `Resolver.LoadPrivateKeyAsync`. * CertificateManagerTests: tests now persist the cert to a temp dir before constructing the metadata identifier so the resolver can load it. * ClientTests / CreateSessionApplicationUriValidationTests: dropped `using` on identifiers, dropped `Certificate = ...` writes, added explicit `CertificateType`. * Gds.Tests: TestUtils.CleanupTrustListAsync gains a `CertificateIdentifier`-typed overload that uses `Resolver.OpenStore`. `CertificateGroupTests` switched to `Resolver.LoadPrivateKeyAsync`. `GlobalDiscoveryTestClient` / `GlobalDiscoveryTestServer` clean-up paths use the resolver. `ApplyNewApplicationInstanceCertificateAsync` builds a metadata-only identifier (Thumbprint+Subject+StoreType+StorePath from the previous identifier). Phase 5 (docs): * Docs/CertificateManager.md: added a callout near the top + a new `Migration: CertificateIdentifier is metadata-only` section with before/after tables, resolver examples, and registration guidance. * Docs/MigrationGuide.md: added an equivalent `CertificateIdentifier is metadata-only` subsection under the CertificateManager block in the 1.5.378 → 1.6.x migration notes. Local verification: * Build green (UA.slnx, all TFMs). * Core tests: 6611/6697 (86 skipped, 0 failed). * Configuration tests: 208/208. * Server tests: 920/925 (5 skipped, 0 failed). * Gds tests: 517/542 (25 skipped, 0 failed) — full suite, including PushTest UpdateCertificate variants, all green locally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
See documentation included in PR
Related Issues
#3657
#3562
#3339
#3279
#3160
#2765
Types of changes
What types of changes does your code introduce?
Checklist
Further comments
The CertificateManager design is documented in
Docs/CertificateManager.md. Migration guidance for adopters of the legacy API surface is inDocs/MigrationGuide.md. The legacyCertificateValidatorclass,ICertificateValidatorinterface,CertificateTypesProviderclass, and theApplicationConfiguration.CertificateValidator/ServerBase.CertificateValidator/ServerBase.InstanceCertificateTypesProviderproperties are all marked[Obsolete]with replacement messages, while remaining functional bridges into the new design for binary compatibility.