Support ECC profiles #1999

mrsuciu · 2022-11-18T12:27:54Z

Proposed changes

Port support for ECC NIST/Brainpool profiles from prototyping_ecc branch
Implement CertProvider to load certs per connection / profile
Goal to be backward compatible for configuration of existing apps

Whats yet working:

Simple configuration to enable ECC profiles
ECC supported on net472 / 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
CertProvider to load cert/cert chains as per profile
Client/Server can specify supported profiles
Client server connections

ToDo:

SecurityConfiguration to specify app cert types for each profile
Cache the certs in CertProvider
Autodetect the ECC support for brainpool/nist based on platform (mac OS 10 doesn't support brainpool)
Tests for certificate validator for NIST/Brainpool PKI
GDS Push support for ECC (yet no cert groups created on server)
IOP testing with other servers / clients
make Winforms samples work

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

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

lgtm-com · 2022-11-18T12:39:10Z

This pull request introduces 11 alerts when merging f407a5a into 67a794e - view on LGTM.com

new alerts:

4 for Constant condition
3 for Useless assignment to local variable
2 for Container contents are never accessed
1 for Dereferenced variable may be null
1 for Empty branch of conditional, or empty loop body

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

mregen · 2022-11-18T12:45:22Z

Applications/ConsoleReferenceClient/Quickstarts.ReferenceClient.Config.xml

@@ -17,8 +17,10 @@
      <StorePath>%LocalApplicationData%/OPC Foundation/pki/own</StorePath>
      <SubjectName>CN=Console Reference Client, C=US, S=Arizona, O=OPC Foundation, DC=localhost</SubjectName>
    </ApplicationCertificate>
+
+    <ApplicationCertificateTypes>Rsa,NistP256,NistP384,BrainpoolP256r1,BrainpoolP384r1</ApplicationCertificateTypes>



Discuss how the new config layout should be, @mregen provide sample from previous discussion in .NET user group.

Feature request was to have a ApplicationCertificate per ECC security profile, to be able to set different subjects

.gitignore

mregen · 2022-11-18T12:49:31Z

Applications/ConsoleReferenceServer/Quickstarts.ReferenceServer.Config.xml

@@ -112,7 +114,43 @@
        <SecurityMode>SignAndEncrypt_3</SecurityMode>
        <SecurityPolicyUri></SecurityPolicyUri>


expectation were that here are add the ECC profiles if the ECC cert is configured as app cert.

codecov · 2022-11-18T12:52:58Z

Codecov Report

Merging #1999 (50774b6) into master (29a3be5) will decrease coverage by 0.01%.
The diff coverage is 64.98%.

@@            Coverage Diff             @@
##           master    #1999      +/-   ##
==========================================
- Coverage   57.92%   57.91%   -0.01%     
==========================================
  Files         324      326       +2     
  Lines       61677    62523     +846     
==========================================
+ Hits        35725    36210     +485     
- Misses      25952    26313     +361

Impacted Files	Coverage Δ
...ries/Opc.Ua.Security.Certificates/PEM/PEMReader.cs	`93.75% <ø> (ø)`
...Certificates/X509Certificate/CertificateBuilder.cs	`85.63% <ø> (ø)`
...braries/Opc.Ua.Server/Server/ServerInternalData.cs	`88.75% <0.00%> (-0.35%)`	⬇️
...ck/Opc.Ua.Core/Schema/SecuredApplicationHelpers.cs	`0.00% <ø> (ø)`
...Core/Security/Certificates/X509CertificateStore.cs	`78.31% <0.00%> (-0.96%)`	⬇️
...pc.Ua.Core/Security/Constants/SecurityConstants.cs	`100.00% <ø> (ø)`
...ack/Opc.Ua.Core/Stack/Client/ReverseConnectHost.cs	`74.28% <ø> (-1.39%)`	⬇️
...tack/Configuration/SecurityConfigurationManager.cs	`0.00% <ø> (ø)`
...tack/Opc.Ua.Core/Security/Certificates/EccUtils.cs	`24.01% <24.01%> (ø)`
...ndings.Https/Stack/Https/HttpsTransportListener.cs	`72.00% <41.66%> (+0.72%)`	⬆️
... and 30 more

... and 10 files with indirect coverage changes

mregen

good luck!

mregen · 2022-11-18T12:51:27Z

Libraries/Opc.Ua.Client/CoreClientUtils.cs

@@ -125,6 +126,7 @@ public static EndpointDescription SelectEndpoint(string discoveryUrl, bool useSe
        /// <param name="useSecurity">if set to <c>true</c> select an endpoint that uses security.</param>
        /// <param name="discoverTimeout">Operation timeout in milliseconds.</param>
        /// <returns>The best available endpoint.</returns>
+        [Obsolete("Use the SelectEndpoint with ApplicationConfiguration instead.")]


check why is it obsolete, was the function moved to ApplicationConfiguration?

mregen · 2022-11-18T12:52:41Z

Libraries/Opc.Ua.Client/CoreClientUtils.cs

                        {
-                            continue;
+                            // skip unsupported security policies
+                            if (!configuration.SecurityConfiguration.SupportedSecurityPolicies.Contains(endpoint.SecurityPolicyUri))


check if this is a good idea

mregen · 2022-11-18T12:55:34Z

Libraries/Opc.Ua.Client/Session.cs

-                    // load the application instance certificate.
-                    if (m_configuration.SecurityConfiguration.ApplicationCertificate == null)
+                    m_instanceCertificate = LoadCertificate(configuration, m_endpoint.Description.SecurityPolicyUri).GetAwaiter().GetResult();
+                    if (m_instanceCertificate == null)


what is if the policy none is used, but user token should be encrypted with ECC profile ..

mregen · 2022-11-18T12:56:26Z

Libraries/Opc.Ua.Client/Session.cs

                    var cert = m_configuration.SecurityConfiguration.ApplicationCertificate;
+#pragma warning restore CS0618 // Type or member is obsolete


marked as obsolete, to catch cases that need to be handled.

display config info in the exception.

mregen · 2022-11-18T12:59:49Z

Libraries/Opc.Ua.Client/Session.cs

@@ -5635,7 +5628,7 @@ private static async Task<X509Certificate2Collection> LoadCertificateChain(Appli
            {
                clientCertificateChain = new X509Certificate2Collection(clientCertificate);
                List<CertificateIdentifier> issuers = new List<CertificateIdentifier>();
-                await configuration.CertificateValidator.GetIssuers(clientCertificate, issuers).ConfigureAwait(false);
+                await configuration.CertificateValidator.GetIssuers(clientCertificate, issuers, false).ConfigureAwait(false);



mregen · 2022-11-18T14:45:15Z

Stack/Opc.Ua.Core/Stack/Tcp/UaSCBinaryChannel.Symmetric.cs

+                    var serverSalt = Utils.Append(length, s_HkdfServerLabel, serverSecret, clientSecret);
+                    var clientSalt = Utils.Append(length, s_HkdfClientLabel, clientSecret, serverSecret);
+
+                    Utils.LogTrace("Length={0}", Utils.ToHexString(length));


remove if all is working :-)

mregen · 2022-11-18T14:46:41Z

Stack/Opc.Ua.Core/Stack/Tcp/UaSCBinaryChannel.Symmetric.cs

                {
                    return SymmetricSign(token, dataToSign, useClientKeys);
                }
+
+#if GCMMODE


GCMMODE can be removed

mregen · 2022-11-18T14:50:13Z

Tests/Opc.Ua.Client.Tests/ClientTest.cs

@@ -512,7 +512,36 @@ private void ValidateDataTypeDefinition(INode node)
            StructureDefinition structureDefinition = dataTypeDefinition.Body as StructureDefinition;
            Assert.AreEqual(ObjectIds.ProgramDiagnosticDataType_Encoding_DefaultBinary, structureDefinition.DefaultEncodingId);
        }
-
+#if mist


MIST can be removed

mregen · 2022-11-18T14:51:26Z

Tests/Opc.Ua.Gds.Tests/PushTest.cs

@@ -381,13 +381,6 @@ public void UpdateCertificateSelfSignedNoPrivateKey()
        [Test, Order(510)]
        public void UpdateCertificateCASigned()
        {
-#if NETCOREAPP3_1_OR_GREATER


remove if it works now, but unlikely...

targets.props

lgtm-com · 2022-11-18T22:04:57Z

This pull request introduces 11 alerts when merging 5c96b3e into 67a794e - view on LGTM.com

new alerts:

4 for Constant condition
3 for Useless assignment to local variable
2 for Container contents are never accessed
1 for Dereferenced variable may be null
1 for Empty branch of conditional, or empty loop body

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

…andard into work_ecc

EthanChangAED · 2023-12-06T10:51:32Z

@mrsuciu, is this PR still needed? If it is replaced with the newer one, could you close this PR?

Squash proto ecc (#1998)

f407a5a

mregen reviewed Nov 18, 2022

View reviewed changes

mregen marked this pull request as draft November 18, 2022 15:03

clean a few warning

5c96b3e

mregen added this to the 1.4.372 milestone Nov 19, 2022

merge master

62ee797

mrsuciu self-assigned this Feb 9, 2023

mrsuciu and others added 6 commits February 13, 2023 10:52

Instantiate HMAC instance depending on provided algorithm.

983cfce

Merge branch 'work_ecc' of https://github.com/OPCFoundation/UA-.NETSt…

198a494

…andard into work_ecc

merge master

bd50cf8

fix build

86e08fa

merge main

9f39161

print endpoints

50774b6

mregen removed this from the 1.4.373 ECC Support preview milestone Dec 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ECC profiles #1999

Support ECC profiles #1999

mrsuciu commented Nov 18, 2022 •

edited by mregen

lgtm-com bot commented Nov 18, 2022

mregen Nov 18, 2022

mregen Nov 18, 2022

mregen Nov 18, 2022

codecov bot commented Nov 18, 2022 •

edited

mregen left a comment

mregen Nov 18, 2022

mregen Nov 18, 2022

mregen Nov 18, 2022

mregen Nov 18, 2022

mregen Nov 18, 2022

mregen Nov 18, 2022

mregen Nov 18, 2022

mregen Nov 18, 2022

mregen Nov 18, 2022

mregen Nov 18, 2022

lgtm-com bot commented Nov 18, 2022

EthanChangAED commented Dec 6, 2023

		@@ -112,7 +114,43 @@
		<SecurityMode>SignAndEncrypt_3</SecurityMode>
		<SecurityPolicyUri></SecurityPolicyUri>

		var cert = m_configuration.SecurityConfiguration.ApplicationCertificate;
		#pragma warning restore CS0618 // Type or member is obsolete

Support ECC profiles #1999

Are you sure you want to change the base?

Support ECC profiles #1999

Conversation

mrsuciu commented Nov 18, 2022 • edited by mregen

Proposed changes

Types of changes

Checklist

Further comments

lgtm-com bot commented Nov 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 18, 2022 • edited

Codecov Report

mregen 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

lgtm-com bot commented Nov 18, 2022

EthanChangAED commented Dec 6, 2023

mrsuciu commented Nov 18, 2022 •

edited by mregen

codecov bot commented Nov 18, 2022 •

edited