Add support for ECC profiles #2398
Conversation
CertificateIdentifierCollection applicationCertificates,
string pkiRoot = null,
string rejectedRoot = null
)
…plicationCertificateType
| @@ -894,7 +915,36 @@ private void ValidateDataTypeDefinition(INode node) | |||
| StructureDefinition structureDefinition = dataTypeDefinition.Body as StructureDefinition; | |||
| Assert.AreEqual(ObjectIds.ProgramDiagnosticDataType_Encoding_DefaultBinary, structureDefinition.DefaultEncodingId); | |||
| } | |||
|
|
|||
| #if mist | |||
Libraries/Opc.Ua.Client/Session.cs
Outdated
| @@ -98,7 +99,7 @@ public partial class Session : SessionClientBatched, ISession | |||
| : | |||
| base(channel) | |||
| { | |||
| Initialize(channel, configuration, endpoint, clientCertificate); | |||
| InitializeAsync(channel, configuration, endpoint, clientCertificate); | |||
There was a problem hiding this comment.
can not be async in a constructor
Libraries/Opc.Ua.Client/Session.cs
Outdated
| @@ -174,30 +175,20 @@ public Session(ITransportChannel channel, Session template, bool copyEventHandle | |||
|
|
|||
| if (m_endpoint.Description.SecurityPolicyUri != SecurityPolicies.None) | |||
There was a problem hiding this comment.
try to move the async portions out of the constructor, then additional calls have to be made to load the certs
| case SecurityPolicies.None: | ||
| { | ||
| // Minimum nonce length by default | ||
| return m_minNonceLength; |
There was a problem hiding this comment.
lets check if this parameter is really needed, or good enough to define the value as a const.
| /// <summary> | ||
| /// The tags of the supported certificate types. | ||
| /// </summary> | ||
| private static Dictionary<uint, string> m_supportedCertificateTypes = new Dictionary<uint, string>() { |
There was a problem hiding this comment.
isn`t this mssing here:
{ ObjectTypes.EccApplicationCertificateType, "??"},
| /// <param name="url">The discovery Url of the server.</param> | ||
| /// <param name="endpoints"></param> | ||
| /// <param name="useSecurity"></param> | ||
| [Obsolete("Use the SelectEndpoint with ApplicationConfiguration instead.")] |
There was a problem hiding this comment.
I'm not familiar with the codebase - I'm just wondering about this ... why introduce and provide a new obsolete Method?
There was a problem hiding this comment.
It is present in the master branch
| /// <summary> | ||
| /// Verify ECDsa key pair of two certificates. | ||
| /// </summary> | ||
| public static bool VerifyECDsaKeyPair( |
There was a problem hiding this comment.
imho we should try to deprecate RSA Specific functions where possible and also try to avoid ecc specific functions where possible and insteady provide Functions supporting both certificate types:
- GetRSAPublicKeySize -> not needed use GetPublicKeySize
- VerifyKeyPair should be depcrecated or handle both certificate types with the latter being my favorite
| /// Get the OPC UA CertificateType. | ||
| /// </summary> | ||
| /// <param name="certificate">The certificate with a signature.</param> | ||
| public static NodeId GetCertificateType(X509Certificate2 certificate) |
There was a problem hiding this comment.
Not shure about this comment: Shouldnt this function be part of X509Utils as well as the ValidateCertificateType function or should all OPC UA Specific logic be in CertificateIdentifier
| @@ -821,19 +888,72 @@ public void OnCertificateValidation(object sender, CertificateValidationEventArg | |||
| Utils.GetAbsoluteDirectoryPath(id.StorePath, true, true, true); | |||
| } | |||
|
|
|||
| var builder = CertificateFactory.CreateCertificate( | |||
There was a problem hiding this comment.
I would ike the CertificateFactory.CreateCertificate to have a method
I provide the certificateType and it creates the RSA or ECC Certificate with the right curve for me depending on the provided certificate type, or else everywhere I create a certificate I need to check the type manually and create the RSA/ECC cert depending on the type. I would also like a global List Class/Enum of Certificate Types (maybe like the well known roles) because in the code ((especially gds) i see a lot of passing around certificate type as string / Node Id and then matching it to a Dictionary of Object Type Ids which i not really see as good practice.
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