Return AdditionalInfo only to users with permission. (#2124)

- Add permission validation if client requests DiagnosticInfo.
OPCFoundation · Apr 18, 2023 · 04721cb · 04721cb
1 parent abfbde3
commit 04721cb
Show file tree

Hide file tree

Showing 8 changed files with 52 additions and 6 deletions.
diff --git a/Libraries/Opc.Ua.Server/Session/Session.cs b/Libraries/Opc.Ua.Server/Session/Session.cs
@@ -419,6 +419,23 @@ public virtual void ValidateRequest(RequestHeader requestHeader, RequestType req
             }
         }
 
+        /// <summary>
+        /// Validate the diagnostic info.
+        /// </summary>
+        public virtual void ValidateDiagnosticInfo(RequestHeader requestHeader)
+        {
+            const uint additionalInfoDiagnosticsMask = (uint)(DiagnosticsMasks.ServiceAdditionalInfo | DiagnosticsMasks.OperationAdditionalInfo);
+            if ((requestHeader.ReturnDiagnostics & additionalInfoDiagnosticsMask) != 0)
+            {
+                var currentRoleIds = m_effectiveIdentity?.GrantedRoleIds;
+                if ((currentRoleIds?.Contains(ObjectIds.WellKnownRole_SecurityAdmin)) == true ||
+                    (currentRoleIds?.Contains(ObjectIds.WellKnownRole_ConfigureAdmin)) == true)
+                {
+                    requestHeader.ReturnDiagnostics |= (uint)DiagnosticsMasks.UserPermissionAdditionalInfo;
+                }
+            }
+        }
+
         /// <summary>
         /// Checks if the secure channel is currently valid.
         /// </summary>

diff --git a/Libraries/Opc.Ua.Server/Session/SessionManager.cs b/Libraries/Opc.Ua.Server/Session/SessionManager.cs
@@ -130,7 +130,7 @@ public virtual void Shutdown()
             {
                 // stop the monitoring thread.
                 m_shutdownEvent.Set();
-                
+
                 // dispose of session objects.
                 foreach (Session session in m_sessions.Values)
                 {
@@ -473,6 +473,9 @@ public virtual OperationContext ValidateRequest(RequestHeader requestHeader, Req
                     // validate request header.
                     session.ValidateRequest(requestHeader, requestType);
 
+                    // validate user has permissions for additional info
+                    session.ValidateDiagnosticInfo(requestHeader);
+
                     // return context.
                     return new OperationContext(requestHeader, requestType, session);
                 }

diff --git a/Stack/Opc.Ua.Core/Stack/Server/ServerBase.cs b/Stack/Opc.Ua.Core/Stack/Server/ServerBase.cs
@@ -1186,6 +1186,9 @@ protected virtual void ValidateRequest(RequestHeader requestHeader)
             {
                 throw new ServiceResultException(StatusCodes.BadRequestHeaderInvalid);
             }
+
+            // mask valid diagnostic masks
+            requestHeader.ReturnDiagnostics &= (uint)DiagnosticsMasks.All;
         }
 
         /// <summary>

diff --git a/Stack/Opc.Ua.Core/Types/BuiltIn/DiagnosticInfo.cs b/Stack/Opc.Ua.Core/Types/BuiltIn/DiagnosticInfo.cs
@@ -258,7 +258,8 @@ private void Initialize()
                 }
             }
 
-            if ((DiagnosticsMasks.ServiceAdditionalInfo & diagnosticsMask) != 0)
+            if ((DiagnosticsMasks.ServiceAdditionalInfo & diagnosticsMask) != 0 &&
+                (DiagnosticsMasks.UserPermissionAdditionalInfo & diagnosticsMask) != 0)
             {
                 m_additionalInfo = result.AdditionalInfo;
             }

diff --git a/Stack/Opc.Ua.Core/Types/BuiltIn/DiagnosticMasks.cs b/Stack/Opc.Ua.Core/Types/BuiltIn/DiagnosticMasks.cs
@@ -18,7 +18,7 @@ namespace Opc.Ua
     /// The DiagnosticsMasks enumeration.
     /// </summary>
     [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Naming", "CA1717:OnlyFlagsEnumsShouldHavePluralNames"), Flags]
-    public enum DiagnosticsMasks
+    public enum DiagnosticsMasks : uint
     {
         /// <summary>
         /// ServiceSymbolicId = 0,
@@ -143,6 +143,15 @@ public enum DiagnosticsMasks
         /// <summary>
         /// All = 1023
         /// </summary>
-        All = 1023
+        All = 1023,
+
+        /// <summary>
+        /// UserPermissionAdditionalInfo = 0x80000000,
+        /// </summary>
+        /// <remarks>
+        /// Mask for internal use only.
+        /// </remarks>
+        UserPermissionAdditionalInfo = 0x80000000
+
     }
 }
diff --git a/Tests/Opc.Ua.Client.Tests/ClientTest.cs b/Tests/Opc.Ua.Client.Tests/ClientTest.cs
@@ -267,6 +267,13 @@ public void ReadOnDiscoveryChannel(int readCount)
                 var sre = Assert.Throws<ServiceResultException>(() =>
                     sessionClient.Read(null, 0, TimestampsToReturn.Neither,
                         readValues, out var results, out var diagnosticInfos));
+                StatusCode statusCode = StatusCodes.BadSecurityPolicyRejected;
+                // race condition, if socket closed is detected before the error was returned,
+                // client may report channel clo sed instead of security policy rejected
+                if (StatusCodes.BadSecureChannelClosed == sre.StatusCode)
+                {
+                    Assert.Inconclusive("Unexpected Status: {0}", sre);
+                }
                 Assert.AreEqual(StatusCodes.BadSecurityPolicyRejected, sre.StatusCode, "Unexpected Status: {0}", sre);
             }
         }
@@ -290,6 +297,12 @@ public void GetEndpointsOnDiscoveryChannel()
                     profileUris.Add($"https://opcfoundation.org/ProfileUri={i}");
                 }
                 var sre = Assert.Throws<ServiceResultException>(() => client.GetEndpoints(profileUris));
+                // race condition, if socket closed is detected before the error was returned,
+                // client may report channel closed instead of security policy rejected
+                if (StatusCodes.BadSecureChannelClosed == sre.StatusCode)
+                {
+                    Assert.Inconclusive("Unexpected Status: {0}", sre);
+                }
                 Assert.AreEqual(StatusCodes.BadSecurityPolicyRejected, sre.StatusCode, "Unexpected Status: {0}", sre);
             }
         }

diff --git a/Tests/Opc.Ua.Core.Tests/Types/BuiltIn/BuiltInTests.cs b/Tests/Opc.Ua.Core.Tests/Types/BuiltIn/BuiltInTests.cs
@@ -77,7 +77,7 @@ protected void TearDown()
         /// </summary>
         protected void SetRepeatedRandomSeed()
         {
-            int randomSeed = TestContext.CurrentContext.Random.Next() + kRandomStart;
+            int randomSeed = TestContext.CurrentContext.CurrentRepeatCount + kRandomStart;
             RandomSource = new RandomSource(randomSeed);
             DataGenerator = new DataGenerator(RandomSource);
         }

diff --git a/Tests/Opc.Ua.Core.Tests/Types/Encoders/EncoderCommon.cs b/Tests/Opc.Ua.Core.Tests/Types/Encoders/EncoderCommon.cs
@@ -98,7 +98,7 @@ protected void TearDown()
         /// </summary>
         protected void SetRepeatedRandomSeed()
         {
-            int randomSeed = TestContext.CurrentContext.Random.Next() + kRandomStart;
+            int randomSeed = TestContext.CurrentContext.CurrentRepeatCount + kRandomStart;
             RandomSource = new RandomSource(randomSeed);
             DataGenerator = new DataGenerator(RandomSource);
         }