Add support for ECC profiles#2398
Conversation
CertificateIdentifierCollection applicationCertificates,
string pkiRoot = null,
string rejectedRoot = null
)
…plicationCertificateType
|
code review discussions for follow up
|
VerifySequenceNumber takes policy into account
romanett
left a comment
There was a problem hiding this comment.
comments for first 17 changed files
| <RejectSHA1SignedCertificates>true</RejectSHA1SignedCertificates> | ||
| <RejectUnknownRevocationStatus>true</RejectUnknownRevocationStatus> | ||
| <MinimumCertificateKeySize>2048</MinimumCertificateKeySize> | ||
| <MinimumECCertificateKeySize>256</MinimumECCertificateKeySize> |
| /// Deletes all application certificates. | ||
| /// </summary> | ||
| public async Task DeleteApplicationInstanceCertificate(CancellationToken ct = default) | ||
| public async Task DeleteApplicationInstanceCertificate(string[] profileIds = null, CancellationToken ct = default) |
There was a problem hiding this comment.
I oppose extending the interface before supporting only deleting certain profiles
| throw new ServiceResultException(StatusCodes.BadConfigurationError, "The Ecc certificate type is not supported."); | ||
| #else | ||
| ECCurve curve = default(ECCurve); | ||
| if (id.CertificateType == ObjectTypeIds.EccApplicationCertificateType || |
There was a problem hiding this comment.
this code should live in a static function where it is globally accessible
|
|
||
| /// <summary> | ||
| /// TODO: Holds the application certificates but should be generated and the Opc.Ua.Security namespace automatically | ||
| /// TODO: Should replace ApplicationCertificateField in the generated Opc.Ua.Security.SecuredApplication class |
There was a problem hiding this comment.
is this needed before merging?
| /// </summary> | ||
| /// <param name="certificate"></param> | ||
| /// <returns></returns> | ||
| public static ECDsa GetPublicKey(X509Certificate2 certificate) |
There was a problem hiding this comment.
Isnt this function also present as non ecc specific variants in another class, if yes i think we can remove it in EccUtils?
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public Task<X509Certificate2> LoadPrivateKey(string thumbprint, string subjectName, NodeId certificateType, string password) |
…OPCFoundation#2798) The server under test has a extension object with a complex type. Type id = {nsu=http://opcfoundation.org/UA/Machinery/Result/;i=5008}. This contains something with a variant array that is null though (-1). SetProperty should support setting a Array that is null. Therefore, test for null collection (case when length was encoded as -1) before dereferencing ahead of the ToArrray() conversion to Array.
| <ServerSecurityPolicy> | ||
| <SecurityMode>Sign_2</SecurityMode> | ||
| <SecurityPolicyUri></SecurityPolicyUri> | ||
| </ServerSecurityPolicy> |
There was a problem hiding this comment.
are the default policies populated as per available ECC certificates?
| <ua:TokenType>UserName_1</ua:TokenType> | ||
| <!-- passwords must be encrypted - this specifies what algorithm to use --> | ||
| <!-- if no algorithm is specified, the active security policy is used --> | ||
| <ua:SecurityPolicyUri>http://opcfoundation.org/UA/SecurityPolicy#Basic256Sha256</ua:SecurityPolicyUri> |
There was a problem hiding this comment.
isn't this now a n invalid configuration if the ECC cert is used?
…uer store (1.5.x) (#3898) # Description Fixes #3896 on the `1.5.x` maintenance line. A server configured with `SecurityConfiguration.SendCertificateChain = true`, whose application instance certificate is issued by a CA placed in the **issuer store** (`TrustedIssuerCertificates`), sent only the **leaf** certificate instead of the full chain. Clients that rely on receiving the chain then failed to connect. ## Root cause Introduced in `1.5.375` by commit `f001eabcc` ("Add support for ECC profiles", #2398), which moved chain loading into `CertificateTypesProvider.LoadCertificateChainAsync`: ```csharp if (await m_certificateValidator.GetIssuersAsync(certificate, issuers).ConfigureAwait(false)) { for (int i = 0; i < issuers.Count; i++) { certificateChain.Add(issuers[i].Certificate); } } ``` `GetIssuersAsync` returns `isTrusted` — `true` only when the issuer is found in the **trusted** store — but populates the `issuers` out-list regardless (trusted → issuer → untrusted). When the CA lives in the **issuer store** (the spec-recommended setup), the call returns `false`, so the already-resolved issuers were discarded and the chain stayed leaf-only. This is a regression from `1.5.374`, where the issuers were appended **unconditionally**. ## Fix Append the resolved issuers unconditionally; the boolean trust result is intentionally ignored — a server must send its chain even when the issuing CA is in the issuer store rather than the trusted store. ```csharp _ = await m_certificateValidator.GetIssuersAsync(certificate, issuers).ConfigureAwait(false); for (int i = 0; i < issuers.Count; i++) { certificateChain.Add(issuers[i].Certificate); } ``` The companion fix for the 2.0 line (`master`) is in a separate PR. ## Tests New `CertificateTypesProviderTests`: - `LoadCertificateChainResolvesIssuerFromIssuerStoreAsync`: the CA is placed in the issuer store only — **not** the trusted store — and the resolved chain is asserted to be `leaf + CA` (and the raw blob `leaf || CA` byte-for-byte). Confirmed this test **fails** with the original gate and **passes** with the fix. - `LoadCertificateChainFallsBackToLeafWhenIssuerMissingAsync`: when the CA is in neither store, the chain is leaf-only without throwing. Verified locally on **net48** and **net10.0** (new tests + existing `VerifyIssuerChain*` validator tests pass). Build is clean. ## Related Issues Fixes #3896. ## Checklist - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation (no public API change; behavioural fix only). - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. (Ran the certificate suites on net48 + net10.0; full-solution run pending CI.) - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. Co-authored-by: agent <agent@local>
Proposed changes
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