From 420353f93eadf42b26f6ec3993e5a91d1af95fb7 Mon Sep 17 00:00:00 2001 From: Roman Ettlinger Date: Fri, 2 Feb 2024 14:54:07 +0100 Subject: [PATCH 1/4] add RevokeCertificateMethod to Client and Server --- .../GlobalDiscoveryServerClient.cs | 19 ++++++++ .../ApplicationsNodeManager.cs | 48 ++++++++++++++++++- Tests/Opc.Ua.Gds.Tests/ClientTest.cs | 16 +++++++ 3 files changed, 81 insertions(+), 2 deletions(-) diff --git a/Libraries/Opc.Ua.Gds.Client.Common/GlobalDiscoveryServerClient.cs b/Libraries/Opc.Ua.Gds.Client.Common/GlobalDiscoveryServerClient.cs index 9e5084272f..163f8439da 100644 --- a/Libraries/Opc.Ua.Gds.Client.Common/GlobalDiscoveryServerClient.cs +++ b/Libraries/Opc.Ua.Gds.Client.Common/GlobalDiscoveryServerClient.cs @@ -618,6 +618,25 @@ public void UnregisterApplication(NodeId applicationId) applicationId); } + /// + /// Revokes a Certificate issued to the Application by the CertificateManager + /// + /// The application id. + /// The certificate to revoke + public void RevokeCertificate(NodeId applicationId, byte[] certificate) + { + if (!IsConnected) + { + Connect(); + } + + Session.Call( + ExpandedNodeId.ToNodeId(Opc.Ua.Gds.ObjectIds.Directory, Session.NamespaceUris), + ExpandedNodeId.ToNodeId(Opc.Ua.Gds.MethodIds.CertificateDirectoryType_RevokeCertificate, Session.NamespaceUris), + applicationId, + certificate); + } + /// /// Requests a new certificate. /// diff --git a/Libraries/Opc.Ua.Gds.Server.Common/ApplicationsNodeManager.cs b/Libraries/Opc.Ua.Gds.Server.Common/ApplicationsNodeManager.cs index 2b61e45c94..f0bd478bd9 100644 --- a/Libraries/Opc.Ua.Gds.Server.Common/ApplicationsNodeManager.cs +++ b/Libraries/Opc.Ua.Gds.Server.Common/ApplicationsNodeManager.cs @@ -417,8 +417,7 @@ protected override NodeState AddBehaviourToPredefinedNode(ISystemContext context activeNode.GetTrustList.OnCall = new GetTrustListMethodStateMethodCallHandler(OnGetTrustList); activeNode.GetCertificateStatus.OnCall = new GetCertificateStatusMethodStateMethodCallHandler(OnGetCertificateStatus); activeNode.StartSigningRequest.OnCall = new StartSigningRequestMethodStateMethodCallHandler(OnStartSigningRequest); - // TODO - //activeNode.RevokeCertificate.OnCall = new RevokeCertificateMethodStateMethodCallHandler(OnRevokeCertificate); + activeNode.RevokeCertificate.OnCall = new RevokeCertificateMethodStateMethodCallHandler(OnRevokeCertificate); activeNode.CertificateGroups.DefaultApplicationGroup.CertificateTypes.Value = new NodeId[] { Opc.Ua.ObjectTypeIds.RsaSha256ApplicationCertificateType }; activeNode.CertificateGroups.DefaultApplicationGroup.TrustList.LastUpdateTime.Value = DateTime.UtcNow; @@ -580,6 +579,51 @@ private ServiceResult OnUnregisterApplication( return ServiceResult.Good; } + private ServiceResult OnRevokeCertificate( + ISystemContext context, + MethodState method, + NodeId objectId, + NodeId applicationId, + byte[] certificate) + { + HasApplicationAdminAccess(context); + + if (m_database.GetApplication(applicationId) == null) + { + return new ServiceResult(StatusCodes.BadNotFound, "The ApplicationId does not refer to a registered application."); + } + + bool revoked = false; + var certifcateToRevoke = new X509Certificate2(certificate); + foreach (var certType in m_certTypeMap) + { + try + { + byte[] applicationCertificate; + if (m_database.GetApplicationCertificate(applicationId, certType.Value, out applicationCertificate)) + { + if (applicationCertificate != null) + { + if (new X509Certificate2(applicationCertificate).Equals(certifcateToRevoke)) + { + RevokeCertificateAsync(certificate).Wait(); + revoked = true; + } + } + } + } + catch + { + Utils.LogError("Failed to revoke: {0}", certType.Value); + } + } + if (!revoked) + { + throw new ServiceResultException(StatusCodes.BadInvalidArgument, "The certificate is not a Certificate for the specified Application that was issued by the CertificateManager."); + } + return ServiceResult.Good; + } + private ServiceResult OnFindApplications( ISystemContext context, MethodState method, diff --git a/Tests/Opc.Ua.Gds.Tests/ClientTest.cs b/Tests/Opc.Ua.Gds.Tests/ClientTest.cs index 7898f5c5c2..45585826d4 100644 --- a/Tests/Opc.Ua.Gds.Tests/ClientTest.cs +++ b/Tests/Opc.Ua.Gds.Tests/ClientTest.cs @@ -968,6 +968,22 @@ public void GetInvalidCertificateStatus() } } + [Test, Order(895)] + public void RevokeGoodCertificates() + { + AssertIgnoreTestWithoutInvalidRegistration(); + AssertIgnoreTestWithoutGoodNewKeyPairRequest(); + ConnectGDS(true); + foreach (var application in m_goodApplicationTestSet) + { + m_gdsClient.GDSClient.RevokeCertificate(application.ApplicationRecord.ApplicationId, application.Certificate); + + Assert.That(() => { + m_gdsClient.GDSClient.RevokeCertificate(application.ApplicationRecord.ApplicationId, application.Certificate); + }, Throws.Exception); + } + } + [Test, Order(900)] public void UnregisterGoodApplications() { From e61bc6dbe4d6a33cb331c3262cf5923cad723222 Mon Sep 17 00:00:00 2001 From: Roman Ettlinger Date: Sun, 18 Feb 2024 13:27:48 +0100 Subject: [PATCH 2/4] register Method revoke certificate improve execution time handle all cases where revocation fails improve test --- .../ApplicationsNodeManager.cs | 49 ++++++++++++------- Tests/Opc.Ua.Gds.Tests/ClientTest.cs | 4 +- 2 files changed, 33 insertions(+), 20 deletions(-) diff --git a/Libraries/Opc.Ua.Gds.Server.Common/ApplicationsNodeManager.cs b/Libraries/Opc.Ua.Gds.Server.Common/ApplicationsNodeManager.cs index f0bd478bd9..9253400b3c 100644 --- a/Libraries/Opc.Ua.Gds.Server.Common/ApplicationsNodeManager.cs +++ b/Libraries/Opc.Ua.Gds.Server.Common/ApplicationsNodeManager.cs @@ -270,25 +270,36 @@ private ICertificateGroup GetGroupForCertificate(byte[] certificate) return null; } - private async Task RevokeCertificateAsync(byte[] certificate) + private async Task RevokeCertificateAsync(byte[] certificate) { + bool revoked = false; if (certificate != null && certificate.Length > 0) { ICertificateGroup certificateGroup = GetGroupForCertificate(certificate); if (certificateGroup != null) { + X509Certificate2 x509 = null; try { - var x509 = new X509Certificate2(certificate); - await certificateGroup.RevokeCertificateAsync(x509).ConfigureAwait(false); + x509 = new X509Certificate2(certificate); + Security.Certificates.X509CRL crl = await certificateGroup.RevokeCertificateAsync(x509).ConfigureAwait(false); + if (crl != null) + { + revoked = true; + } } catch (Exception e) { Utils.LogError(e, "Unexpected error revoking certificate. {0} for Authority={1}", new X509Certificate2(certificate).Subject, certificateGroup.Id); } + finally + { + x509?.Dispose(); + } } } + return revoked; } protected async Task InitializeCertificateGroup(CertificateGroupConfiguration certificateGroupConfiguration) @@ -403,6 +414,8 @@ protected override NodeState AddBehaviourToPredefinedNode(ISystemContext context Opc.Ua.Gds.CertificateDirectoryState activeNode = new Opc.Ua.Gds.CertificateDirectoryState(passiveNode.Parent); + activeNode.RevokeCertificate = new RevokeCertificateMethodState(passiveNode); + activeNode.Create(context, passiveNode); activeNode.QueryServers.OnCall = new QueryServersMethodStateMethodCallHandler(OnQueryServers); activeNode.QueryApplications.OnCall = new QueryApplicationsMethodStateMethodCallHandler(OnQueryApplications); @@ -592,29 +605,27 @@ private ServiceResult OnRevokeCertificate( { return new ServiceResult(StatusCodes.BadNotFound, "The ApplicationId does not refer to a registered application."); } + if (certificate == null || certificate.Length == 0 ) + { + throw new ServiceResultException(StatusCodes.BadInvalidArgument, "The certificate is not a Certificate for the specified Application that was issued by the CertificateManager."); + } bool revoked = false; - var certifcateToRevoke = new X509Certificate2(certificate); foreach (var certType in m_certTypeMap) { - try + byte[] applicationCertificate; + + if (!m_database.GetApplicationCertificate(applicationId, certType.Value, out applicationCertificate) + || applicationCertificate == null + || !Utils.IsEqual(applicationCertificate, certificate)) { - byte[] applicationCertificate; - if (m_database.GetApplicationCertificate(applicationId, certType.Value, out applicationCertificate)) - { - if (applicationCertificate != null) - { - if (new X509Certificate2(applicationCertificate).Equals(certifcateToRevoke)) - { - RevokeCertificateAsync(certificate).Wait(); - revoked = true; - } - } - } + continue; } - catch + + revoked = RevokeCertificateAsync(certificate).Result; + if (revoked) { - Utils.LogError("Failed to revoke: {0}", certType.Value); + break; } } if (!revoked) diff --git a/Tests/Opc.Ua.Gds.Tests/ClientTest.cs b/Tests/Opc.Ua.Gds.Tests/ClientTest.cs index 45585826d4..9c1959f9e8 100644 --- a/Tests/Opc.Ua.Gds.Tests/ClientTest.cs +++ b/Tests/Opc.Ua.Gds.Tests/ClientTest.cs @@ -977,7 +977,9 @@ public void RevokeGoodCertificates() foreach (var application in m_goodApplicationTestSet) { m_gdsClient.GDSClient.RevokeCertificate(application.ApplicationRecord.ApplicationId, application.Certificate); - + } + foreach (var application in m_invalidApplicationTestSet) + { Assert.That(() => { m_gdsClient.GDSClient.RevokeCertificate(application.ApplicationRecord.ApplicationId, application.Certificate); }, Throws.Exception); From 338ceb7d3bc2945a3e69e74890236071b73a0427 Mon Sep 17 00:00:00 2001 From: Roman Ettlinger Date: Tue, 27 Feb 2024 10:10:10 +0100 Subject: [PATCH 3/4] use AuthorizationHelper --- .../ApplicationsNodeManager.cs | 29 +++++++++---------- .../AuthorizationHelper.cs | 1 + 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/Libraries/Opc.Ua.Gds.Server.Common/ApplicationsNodeManager.cs b/Libraries/Opc.Ua.Gds.Server.Common/ApplicationsNodeManager.cs index 49ac258054..eddecc6660 100644 --- a/Libraries/Opc.Ua.Gds.Server.Common/ApplicationsNodeManager.cs +++ b/Libraries/Opc.Ua.Gds.Server.Common/ApplicationsNodeManager.cs @@ -255,23 +255,20 @@ private async Task RevokeCertificateAsync(byte[] certificate) if (certificateGroup != null) { - X509Certificate2 x509 = null; - try + using (X509Certificate2 x509 = new X509Certificate2(certificate)) { - x509 = new X509Certificate2(certificate); - Security.Certificates.X509CRL crl = await certificateGroup.RevokeCertificateAsync(x509).ConfigureAwait(false); - if (crl != null) + try { - revoked = true; + Security.Certificates.X509CRL crl = await certificateGroup.RevokeCertificateAsync(x509).ConfigureAwait(false); + if (crl != null) + { + revoked = true; + } + } + catch (Exception e) + { + Utils.LogError(e, "Unexpected error revoking certificate. {0} for Authority={1}", new X509Certificate2(certificate).Subject, certificateGroup.Id); } - } - catch (Exception e) - { - Utils.LogError(e, "Unexpected error revoking certificate. {0} for Authority={1}", new X509Certificate2(certificate).Subject, certificateGroup.Id); - } - finally - { - x509?.Dispose(); } } } @@ -575,13 +572,13 @@ private ServiceResult OnRevokeCertificate( NodeId applicationId, byte[] certificate) { - HasApplicationAdminAccess(context); + AuthorizationHelper.HasAuthorization(context, AuthorizationHelper.CertificateAuthorityAdmin); if (m_database.GetApplication(applicationId) == null) { return new ServiceResult(StatusCodes.BadNotFound, "The ApplicationId does not refer to a registered application."); } - if (certificate == null || certificate.Length == 0 ) + if (certificate == null || certificate.Length == 0) { throw new ServiceResultException(StatusCodes.BadInvalidArgument, "The certificate is not a Certificate for the specified Application that was issued by the CertificateManager."); } diff --git a/Libraries/Opc.Ua.Gds.Server.Common/RoleBasedUserManagement/AuthorizationHelper.cs b/Libraries/Opc.Ua.Gds.Server.Common/RoleBasedUserManagement/AuthorizationHelper.cs index 4bf0e86e28..c26b1d517a 100644 --- a/Libraries/Opc.Ua.Gds.Server.Common/RoleBasedUserManagement/AuthorizationHelper.cs +++ b/Libraries/Opc.Ua.Gds.Server.Common/RoleBasedUserManagement/AuthorizationHelper.cs @@ -43,6 +43,7 @@ internal static class AuthorizationHelper internal static List DiscoveryAdmin { get; } = new List { GdsRole.DiscoveryAdmin }; internal static List AuthenticatedUserOrSelfAdmin { get; } = new List { Role.AuthenticatedUser, GdsRole.ApplicationSelfAdmin }; internal static List CertificateAuthorityAdminOrSelfAdmin { get; } = new List { GdsRole.CertificateAuthorityAdmin, GdsRole.ApplicationSelfAdmin }; + internal static List CertificateAuthorityAdmin { get; } = new List { GdsRole.CertificateAuthorityAdmin }; /// /// Checks if the current session (context) has one of the requested roles. If is allowed the applicationId needs to be specified From 0102c0278c678494c078df164a7c4ee6632ca2e6 Mon Sep 17 00:00:00 2001 From: Roman Ettlinger Date: Wed, 28 Feb 2024 12:28:34 +0100 Subject: [PATCH 4/4] avoid unnecessary x509 creation --- Libraries/Opc.Ua.Gds.Server.Common/ApplicationsNodeManager.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Libraries/Opc.Ua.Gds.Server.Common/ApplicationsNodeManager.cs b/Libraries/Opc.Ua.Gds.Server.Common/ApplicationsNodeManager.cs index eddecc6660..4f4c73f97a 100644 --- a/Libraries/Opc.Ua.Gds.Server.Common/ApplicationsNodeManager.cs +++ b/Libraries/Opc.Ua.Gds.Server.Common/ApplicationsNodeManager.cs @@ -267,7 +267,7 @@ private async Task RevokeCertificateAsync(byte[] certificate) } catch (Exception e) { - Utils.LogError(e, "Unexpected error revoking certificate. {0} for Authority={1}", new X509Certificate2(certificate).Subject, certificateGroup.Id); + Utils.LogError(e, "Unexpected error revoking certificate. {0} for Authority={1}", x509.Subject, certificateGroup.Id); } } }