search

OPCFoundation / UA-.NETStandard

OPC Unified Architecture .NET Standard
Other
1.96k stars 946 forks source link

SubscriptionDiagnosticsData is not accessible through the current session. #2686

Closed marcschier closed 2 months ago

marcschier commented 3 months ago

Type of issue

Current Behavior

Trying to read the subscription diagnostics through the diagnosticnodemanager does not work. While the code is implemented to filter to the current accessing session, just like the other diagnostics information, here the role is checked. This is no where documented in the spec (so far as I could find). Removing the role check hock makes it work.

Expected Behavior

I can access Variables.Server_ServerDiagnostics_SubscriptionDiagnosticsArray just like the Server_ServerDiagnostics_SessionsDiagnosticsSummary_SessionDiagnosticsArray, but the array is filtered to the current session.

Steps To Reproduce

No response

Environment

- OS:
- Environment:
- Runtime:
- Nuget Version:
- Component:
- Server:
- Client:

Anything else?

No response

marcschier commented 3 months ago

I will submit a PR.

The overall ability to enable the server diagnostics using write is also disable with the comment

// dynamic change of enabledFlag is disabled to pass CTT

I wonder if someone can shed a light as to why CTT fails if the enable flag is "CurrentWrite".

marcschier commented 3 months ago

@mregen FYI

romanett commented 3 months ago

@marcschier I am not really shure if it the right approach to just delete the access restrictions. We introduced those restrictions with this PR:

1878

And the implementation shure has some issues, see:

2099

1993

Mabye we can find a solution that works for everyone

marcschier commented 3 months ago

If we can find the reason for this restriction in the spec, sure, but I cannot see any any reason why you cannot get diagnostics for just your session, which seems perfectly valid and safe and sanctioned by the standard.. Case in point session diagnostic and even security diagnostic is accessible. The change that added the restriction (thanks for linking) looks it was done to be a sample if I read the description correctly. If we did this as a sample I expect a flag that a user can enable. Although I expect samples to live outside the sdk. Anyway, this needs to be fixed and reverted and I would suggest we look at the original change also. Happy to join the WG meeting next Thursday and happy to learn more also.

romanett commented 3 months ago

@marcschier I took a deeper look at the issue and found some implications in the spec.

First of all I agree with your PR, as the spec does not mention to restrict the SubscriptionDiagnosticsArray anywhere.

However I would like to verify the limiting to the sessions own subscription works as intended before approving. Maybe we can create a test case to prove this (similar tests exists in the GDS to prove the access restrictions of the ServerConfiguration Methods).

In the long term I would like to fix these access restrictions completely as they seem incomplete, doing over and underblocking (see #2099 #1993).

So first of all the spec specifies the following:

However the spec does not specify there what an authorized user is. The OnReadUserRolePermissions callback requires well known role security admin. And is currently enabled for the ServerDiagnosticsSummaryType as well as all of the SessionDiagnostics individual values. (and also to the SubscriptionDiagnostics Array). However not the SessionDiagnosticsArray & SessionSecurityDiagnostics Array (causing #2099). The child nodes however are restricted.

The issue in #1993 will be fixed with your change I suppose. As the SubscriptionDiagnosticsArray will then be readable.

The on ReadUserRolePermissions function tries to allow reads for nodes beloning to the own session, but i am not shure if this really works:

admitUser = (node.NodeId == context.SessionId) || HasApplicationSecureAdminAccess(context);

marcschier commented 3 months ago

The access to the server session diagnostic for only your session works, I can only see my session in the array. A test might be useful, no doubt, we should add that to the backlog.

Admin access to all session should absolutely be required, no way we want others to snoop sessions they don't own. For the security diagnostics, it does not look sensible what is exposed there, more like metrics, but it is not helpful to the client anyway unlike the other information.

We hear from users of the client stack that in reconnect situation subscriptions are leaked on the server. The access to the info is used by our client to validate this in production.

ThomasNehring commented 3 months ago

I think we should try to get Randy involved in this discussion. Since, through impersonation, a session can be assigned to a new user, it is not clear to me whether a user assigned to a session should always be granted access to all session diagnostics.

PaulHunkar commented 3 months ago

The specification are fairly clear - if a session ends, a new session can be created as long as the same credentials are used and the subscription can be reclaimed (via transfer subscription). When it is moved over then the diagnostics related to that subscription should be available to the new session, but not before.

For impersonation - if the logged in user changes - the security may change (different roles - resulting in different permissions) and all action shall be updated to the new users rights - i.e. if I was an Operator and the a "Manager" logins in then the application can see or do things it could not before, but the same is true in the other direction - is an SecurityAdmin was logged in and an Operator logins in instead (on the same session) - subscription might suddenly start returning Access denied for values.

In general a user should be able to see the diagnostic associated with their own session, but nothing else, unless they have some level of elevated rights - OPC does not specify this for general counter or non-security related information. It does mandate that security level information shall be restricted to securityadministrative personnel and this is for information other than my own session. This is a little open ended in that Roles are not required - but if they are supported then it is a shall. The actual role of SecurityAdmin is also open-ended in that it could be a vendor specific role (I have 3 different SecurityAdmin roles, one for each unit in my plant - SecurityAdminUnit1...). The key point is that a vendor can configure roles and the default roles and permissions in an SDK shall follow the spec, but they shall also be configurable - i.e. I can add roles and change the behaviour (what roles are securityAdmin or Admin etc).

Hopefully this helps

Paul

On Mon, Jul 29, 2024 at 10:12 AM ThomasNehring @.***> wrote:

I think we should try to get Randy involved in this discussion. Since, through impersonation, a session can be assigned to a new user, it is not clear to me whether a user assigned to a session should always be granted access to all session diagnostics.

— Reply to this email directly, view it on GitHub https://github.com/OPCFoundation/UA-.NETStandard/issues/2686#issuecomment-2256056406, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEI2CSBAKOMWTZZBSIP3FIDZOZETTAVCNFSM6AAAAABLQIFTXOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJWGA2TMNBQGY . You are receiving this because you are subscribed to this thread.Message ID: @.***>

romanett commented 2 months ago

image Diagnostics are available for the own session through the session diagnostics summary. An Admin uses the ServerDiagnostics to view the diagnostics using the DiagnosticsArray