Add support for ECC profiles #2398

mrsuciu · 2023-11-27T14:30:24Z

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

CertificateIdentifierCollection applicationCertificates, string pkiRoot = null, string rejectedRoot = null )

…into workeccV1

…plicationCertificateType

…ates

…cryption)

ThomasNehring · 2024-07-14T16:36:02Z

I just had a look at the wireshark trace of the handshake between ConsoleReferenceClient and ConsoleReferenceServer.

The good news is that the GetEndpointsResponse shows the ECC profiles. That's nice.

Otoh, it looks as if the server offers all the old and deprecated security policies (Basic128Rsa15, Basic 256). Is this by intention? I think we should try not not expose them by default. (This is probably just a configuration issue).

mrsuciu · 2024-07-15T06:44:38Z

@ThomasNehring The server's Quickstarts.ReferenceServer.Config.xml configuration contains the deprecated security policies below the  comment. They are there for backward compatibility testing but can be removed anytime.

mregen · 2024-07-23T09:04:18Z

Libraries/Opc.Ua.Configuration/Opc.Ua.Configuration.csproj.bak

+    <RootNamespace>Opc.Ua.Configuration</RootNamespace>
+    <Description>OPC UA Configuration Class Library</Description>
+    <IsPackable>true</IsPackable>
+    <PackageLicenseExpression>MIT</PackageLicenseExpression>


remove .bak, I must have unintentionally pushed it

mregen · 2024-08-29T08:06:32Z

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?

VerifySequenceNumber takes policy into account

romanett

comments for first 17 changed files

romanett · 2024-09-30T16:53:22Z

Applications/ConsoleReferenceServer/Quickstarts.ReferenceServer.Config.xml

@@ -46,6 +79,7 @@
    <RejectSHA1SignedCertificates>true</RejectSHA1SignedCertificates>
    <RejectUnknownRevocationStatus>true</RejectUnknownRevocationStatus>
    <MinimumCertificateKeySize>2048</MinimumCertificateKeySize>
+    <MinimumECCertificateKeySize>256</MinimumECCertificateKeySize>


isn`t this obosolete?

romanett · 2024-09-30T16:55:17Z

Applications/ConsoleReferenceServer/Quickstarts.ReferenceServer.Config.xml

@@ -122,6 +189,14 @@
        <SecurityMode>SignAndEncrypt_3</SecurityMode>
        <SecurityPolicyUri>http://opcfoundation.org/UA/SecurityPolicy#Basic256</SecurityPolicyUri>
      </ServerSecurityPolicy>
+      <ServerSecurityPolicy>


these shall be removed

romanett · 2024-09-30T17:01:44Z

Libraries/Opc.Ua.Client/Session/Session.cs

@@ -5639,7 +5625,14 @@ ITransportChannel transportChannel
            EndpointDescription endpoint = m_endpoint.Description;
            SignatureData clientSignature = SecurityPolicies.Sign(m_instanceCertificate, endpoint.SecurityPolicyUri, dataToSign);

+#if TODO    // new code inside, please check if the update is valid


whats up with this

@romanett I think @mregen suggests a more thorough review of the FindUserTokenPolicy I added to deprecate the one under the TODO ?

romanett · 2024-09-30T17:05:19Z

Libraries/Opc.Ua.Client/Session/Session.cs

+        /// <param name="responseHeader"></param>
+        /// <param name="serverCertificate"></param>
+        /// <exception cref="ServiceResultException"></exception>
+        protected virtual void ProcessResponseAdditionalHeader(ResponseHeader responseHeader, X509Certificate2 serverCertificate)


Does a more precise name exist in the spec for this header?

romanett · 2024-09-30T17:09:46Z

Libraries/Opc.Ua.Configuration/ApplicationInstance.cs

        /// </summary>
-        public async Task DeleteApplicationInstanceCertificate(CancellationToken ct = default)
+        public async Task DeleteApplicationInstanceCertificate(string[] profileIds = null, CancellationToken ct = default)


I oppose extending the interface before supporting only deleting certain profiles

romanett · 2024-09-30T17:12:47Z

Libraries/Opc.Ua.Configuration/ApplicationInstance.cs

+                throw new ServiceResultException(StatusCodes.BadConfigurationError, "The Ecc certificate type is not supported.");
+#else
+                ECCurve curve = default(ECCurve);
+                if (id.CertificateType == ObjectTypeIds.EccApplicationCertificateType ||


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

romanett · 2024-10-01T05:32:41Z

Libraries/Opc.Ua.Security.Certificates/X509Certificate/X509PfxUtils.cs

@@ -63,6 +63,28 @@ private static X509KeyUsageFlags GetKeyUsage(X509Certificate2 cert)
            return allFlags;
        }

+        /// <summary>
+        /// Verify RSA key pair of two certificates.


comment is misleading, supports both ecc & RSA

romanett · 2024-10-01T10:24:05Z

Stack/Opc.Ua.Core/Schema/SecuredApplicationHelpers.cs

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


is this needed before merging?

romanett · 2024-10-01T12:15:31Z

Stack/Opc.Ua.Core/Types/Utils/Utils.cs

+        /// <summary>
+        /// Lazy helper for checking ECC eliptic curve support for running OS
+        /// </summary>
+        private static readonly Dictionary<string, Lazy<bool>> s_eccCurveSupportCache = new Dictionary<string, Lazy<bool>>{


is this used anywhere?

romanett · 2024-10-01T12:20:11Z

Stack/Opc.Ua.Core/Security/Certificates/EccUtils.cs

+        /// </summary>
+        /// <param name="certificate"></param>
+        /// <returns></returns>
+        public static ECDsa GetPublicKey(X509Certificate2 certificate)


Isnt this function also present as non ecc specific variants in another class, if yes i think we can remove it in EccUtils?

romanett · 2024-10-02T08:25:36Z

Stack/Opc.Ua.Core/Security/Certificates/CertificateIdentifier.cs

@@ -738,6 +972,12 @@ public Task<X509Certificate2> LoadPrivateKey(string thumbprint, string subjectNa
            return Task.FromResult<X509Certificate2>(null);
        }

+        /// <inheritdoc/>
+        public Task<X509Certificate2> LoadPrivateKey(string thumbprint, string subjectName, NodeId certificateType, string password)


why return always null?

mregen and others added 30 commits October 6, 2023 10:11

Squash proto ecc (OPCFoundation#1998)

294192f

clean a few warning

fd3fcd1

Instantiate HMAC instance depending on provided algorithm.

dee6088

fix build

b17271c

Added ApplicationCertificates XML tag (bcw compat)

d63925f

Use ListOfCertificateIdentifier for configuration

5b1fe70

Use ApplicationCertificates tag

ab6d8fb

Added new Interface AddSecurityConfiguration(

d23450d

CertificateIdentifierCollection applicationCertificates, string pkiRoot = null, string rejectedRoot = null )

Reenabled special cases in ValidateCertificateType

8fc81d3

Use KeySize property value if set under IsECSecureForProfile method

b1cd86e

Check minimKeySize for ApplicationCertificates

108f7ed

Keep and mark as obsolete ApplicationInstance methods for bacword compat

9a25264

Fix CfgManager UpdateCertificate()

a2e0a4b

Added missing interface implementation

a0e4b0f

Added CertificateTypeString to improve visual appearance

97b4c87

Commented ECCUtils code

9efe4a7

Corrected ConsoleRefClient Configuration

a01d276

net48 has null Oid values for Brainpool curves

f165dee

Merge branch 'workeccV1' of https://github.com/mrsuciu/UA-.NETStandard …

0b6d74f

…into workeccV1

Updated tests to new API

58f583b

Default certificate type for "old" style configuration is RsaSha256Ap…

453ad95

…plicationCertificateType

SemaphoreSlim is not reentrant => removed from ResetValidatedCertific…

11f3085

…ates

Fixed loading of disposed cached certificates

48cce5c

Modified projects to correctly use ECC_SUPPORT flag

1ada020

Handle EphemerousKey from Server side

c7ebfe1

Added minimal test

e62ad97

Added UserIdentityToken Encypt/Decrypt functionality

1a8ab07

Preserve key material in UserIdentityToken encryption/decription

0ed43b9

Removed ECC support from NETSTANDARD2_0 (introduced bu UserIdentityEn…

f41f10a

…cryption)

Added UserIdentity encryption positive roundtrip unit tests

c6a10d7

Modirfied Opc.Ua.Client.cproj to generate the APICompat suppression file

8d28310

mregen added the ecc label Jul 12, 2024

mregen added 6 commits July 19, 2024 09:18

merge main into ecc_support

4280d0c

do not duplicate the code for additional header user ecc secret

8b998ad

fix formatting

3fcb5e4

simplify code

f20f206

Merge remote-tracking branch 'origin/master' into ecc_support

9916621

add test and a few missing bits in sessionconfiguration

32c2f9e

mregen reviewed Jul 23, 2024

View reviewed changes

Sequence Header sequenceNumber calculation depends on Security Policy

15574d4

mrsuciu linked an issue Jul 31, 2024 that may be closed by this pull request

Ensure ECC profiles use the new sequence number algorithm #2674

Open

5 tasks

mrsuciu and others added 8 commits July 31, 2024 19:37

Removed Libraries/Opc.Ua.Configuration/Opc.Ua.Configuration.csproj.bak

37c44f7

improve a comment

9b4ecf6

merge main

f5992a2

add a review comment

1a47848

Merge remote-tracking branch 'origin/master' into ecc_support

3e6329e

fix a comment

88e1ac6

Merge remote-tracking branch 'origin/master' into ecc_support

c6b0cc3

Merge with latest master

7d1496d

mrsuciu added 5 commits August 30, 2024 17:25

Unified SessionConfiguration

f6f6ca3

Unified UserIdentityToken Encrypt/Decrypt

d79719f

Removed MinimumECCertificateKeySize

7d1da41

Merge with latest master

d336990

Initial sequenceNumber value depends on policy;

91f6852

VerifySequenceNumber takes policy into account

romanett requested changes Sep 30, 2024

View reviewed changes

romanett reviewed Oct 2, 2024

View reviewed changes

Simplify GetNewSequenceNumber

69ab50a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for ECC profiles #2398

Add support for ECC profiles #2398

mrsuciu commented Nov 27, 2023 •

edited by mregen

Loading

ThomasNehring commented Jul 14, 2024

mrsuciu commented Jul 15, 2024

mregen Jul 23, 2024

mregen commented Aug 29, 2024

romanett left a comment

romanett Sep 30, 2024

romanett Sep 30, 2024

romanett Sep 30, 2024

mrsuciu Oct 1, 2024

romanett Sep 30, 2024

romanett Sep 30, 2024

romanett Sep 30, 2024

romanett Oct 1, 2024

romanett Oct 1, 2024

romanett Oct 1, 2024

romanett Oct 1, 2024

romanett Oct 2, 2024

Add support for ECC profiles #2398

Are you sure you want to change the base?

Add support for ECC profiles #2398

Conversation

mrsuciu commented Nov 27, 2023 • edited by mregen Loading

Proposed changes

Types of changes

Checklist

Further comments

ThomasNehring commented Jul 14, 2024

mrsuciu commented Jul 15, 2024

Choose a reason for hiding this comment

mregen commented Aug 29, 2024

romanett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrsuciu commented Nov 27, 2023 •

edited by mregen

Loading