Skip to content

Add support for ECC profiles#2398

Merged
mregen merged 101 commits into
OPCFoundation:masterfrom
mrsuciu:ecc_support
Oct 17, 2024
Merged

Add support for ECC profiles#2398
mregen merged 101 commits into
OPCFoundation:masterfrom
mrsuciu:ecc_support

Conversation

@mrsuciu

@mrsuciu mrsuciu commented Nov 27, 2023

Copy link
Copy Markdown
Contributor

Proposed changes

  • Port support for ECC NIST/Brainpool profiles from prototyping_ecc branch
  • Implement CertProvider to load certs per connection / profile
  • Backward compatibility of configuration for existing apps
  • SecurityConfiguration specifies app cert types for each profile
  • Autodetect the ECC support for brainpool/nist based on platform (mac OS 10 doesn't support brainpool)
  • ECC supported on net48 / net5.0 / net 6.0 / netstandard2.1
  • ECC supported on windows / linux / macOS (brainpool not < macOS11)
  • Self signed certs for each profile are created on start similar to RSA
  • RSA only profiles are supported on .NET standard 2.0

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply. You can also fill these out after creating the PR.

  • Bugfix (non-breaking change which fixes an issue)
  • Enhancement (non-breaking change which adds functionality)
  • Test enhancement (non-breaking change to increase test coverage)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected, requires version increase of Nuget packages)
  • Documentation Update (if none of the other choices apply)

Checklist

Put an x in 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.

  • I have read the CONTRIBUTING doc.
  • I have signed the CLA.
  • I ran tests locally with my changes, all passed.
  • I fixed all failing tests in the CI pipelines.
  • I fixed all introduced issues with CodeQL and LGTM.
  • I have added tests that prove my fix is effective or that my feature works and increased code coverage.
  • I have added necessary documentation (if appropriate).
  • Any dependent changes have been merged and published in downstream modules.

Further comments

mregen and others added 30 commits October 6, 2023 10:11
            CertificateIdentifierCollection applicationCertificates,
            string pkiRoot = null,
            string rejectedRoot = null
            )
@mregen

mregen commented Aug 29, 2024

Copy link
Copy Markdown
Contributor

code review discussions for follow up

  • check if the serverNonce can be stored as Nonce and unified for ECC and RSA
  • how about key sizes, do they still make sense as they are specified by profile. How do we specify that a nist 384 cert should be used for nist256. But it is not possible to use curve 448 with curve 25519?

@romanett romanett left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments for first 17 changed files

<RejectSHA1SignedCertificates>true</RejectSHA1SignedCertificates>
<RejectUnknownRevocationStatus>true</RejectUnknownRevocationStatus>
<MinimumCertificateKeySize>2048</MinimumCertificateKeySize>
<MinimumECCertificateKeySize>256</MinimumECCertificateKeySize>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn`t this obosolete?

Comment thread Applications/ConsoleReferenceServer/Quickstarts.ReferenceServer.Config.xml Outdated
Comment thread Libraries/Opc.Ua.Client/Session/Session.cs Outdated
Comment thread Libraries/Opc.Ua.Client/Session/Session.cs
/// Deletes all application certificates.
/// </summary>
public async Task DeleteApplicationInstanceCertificate(CancellationToken ct = default)
public async Task DeleteApplicationInstanceCertificate(string[] profileIds = null, CancellationToken ct = default)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ||

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code should live in a static function where it is globally accessible

Comment thread Libraries/Opc.Ua.Security.Certificates/X509Certificate/X509PfxUtils.cs Outdated

/// <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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this needed before merging?

Comment thread Stack/Opc.Ua.Core/Types/Utils/Utils.cs
/// </summary>
/// <param name="certificate"></param>
/// <returns></returns>
public static ECDsa GetPublicKey(X509Certificate2 certificate)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why return always null?

mrsuciu and others added 3 commits October 4, 2024 18:25
…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.
@mregen
mregen requested review from KircMax, mregen and romanett October 17, 2024 11:40
Comment thread Docs/EccProfiles.md
<ServerSecurityPolicy>
<SecurityMode>Sign_2</SecurityMode>
<SecurityPolicyUri></SecurityPolicyUri>
</ServerSecurityPolicy>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are the default policies populated as per available ECC certificates?

Comment thread Docs/EccProfiles.md
<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>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this now a n invalid configuration if the ECC cert is used?

@mregen
mregen merged commit f001eab into OPCFoundation:master Oct 17, 2024
mrsuciu pushed a commit that referenced this pull request Jun 21, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ensure ECC profiles use the new sequence number algorithm IOP: Create CSR from UA Expert fails for ECC certificates Support ECC user token

7 participants