Skip to content

Enforce KeyCredential authorization and ownership (OPC 10000-12 §§8.5.5-8.5.7)#4054

Open
marcschier wants to merge 3 commits into
masterfrom
marcschier/review-fix/gds-keycredential-authorization
Open

Enforce KeyCredential authorization and ownership (OPC 10000-12 §§8.5.5-8.5.7)#4054
marcschier wants to merge 3 commits into
masterfrom
marcschier/review-fix/gds-keycredential-authorization

Conversation

@marcschier

@marcschier marcschier commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator

Description

This PR splits the GDS KeyCredential changes from #4021.

Specification

  • OPC 10000-12 v1.05.07 §8.5.5 requires StartRequest to use an encrypted SecureChannel and a Client with the KeyCredentialAdmin Role, ApplicationAdmin Privilege, or ApplicationSelfAdmin Privilege. The ApplicationUri must uniquely identify an application known to the GDS.
  • OPC 10000-12 v1.05.07 §8.5.6 applies the same authorization requirements, requires FinishRequest to use the same SecureChannel Certificate as StartRequest, and defines Bad_RequestNotComplete for a pending request.
  • OPC 10000-12 v1.05.07 §8.5.7 applies the same authorization requirements to Revoke and defines Bad_InvalidArgument for an unknown credential.

Failure

The KeyCredential methods previously verified only an authenticated encrypted SecureChannel. They did not enforce the specification's administrative Role or application-scoped Privileges, persist immutable application ownership, or bind FinishRequest to the initiating Client certificate. A pending request returned BadNothingToDo, invalid ownership lookups used non-conformant status codes, and failure audit text could expose exception details.

Fix

  • Publish and enforce KeyCredentialAdmin, ApplicationAdmin, and ApplicationSelfAdmin permissions, including application scoping.
  • Resolve ApplicationUri to exactly one registered application and persist its immutable ApplicationId.
  • Bind requests to the initiating SecureChannel certificate with a SHA-256 fingerprint and compare it in fixed time when finishing.
  • Atomically re-check request and credential ownership, and fail closed for stores that cannot enforce the new ownership contract.
  • Return BadRequestNotComplete for pending requests and conformant invalid-argument results for unknown request or credential identifiers.
  • Emit success and failure audit events while redacting KeyCredential exception details.

Validation

  • Full Opc.Ua.Gds.Tests: net10.0 (1053 passed, 42 skipped), net48 (858 passed, 237 skipped).
  • Review follow-up KeyCredentialRequestStoreTests: net10.0 (16 passed), net48 (16 passed).
  • git diff --check passed.

Related Issues

Checklist

Only verified claims are checked.

  • I have signed the CLA and read the CONTRIBUTING doc.
  • I have added tests that prove my fix is effective or that my feature works and increased code coverage.
  • I have added all necessary documentation.
  • I have verified that my changes do not introduce (new) build or analyzer warnings.
  • I ran all tests locally using the UA.slnx solution against at least .net framework and .net 10, and all passed.
  • I fixed all failing and flaky tests in the CI pipelines and all CodeQL warnings.
  • I have addressed all PR feedback received.

….5-8.5.7)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@marcschier
marcschier marked this pull request as ready for review July 19, 2026 11:30
Copilot AI review requested due to automatic review settings July 19, 2026 11:30
@marcschier
marcschier marked this pull request as draft July 19, 2026 11:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

This PR enforces OPC UA GDS KeyCredential authorization and ownership requirements by adding role/privilege checks, binding KeyCredential requests to the initiating SecureChannel client certificate, and persisting immutable application ownership identifiers.

Changes:

  • Added KeyCredentialAdmin role plus authorization helpers and updated tests/fixtures to cover admin/self-admin/app-admin behavior and certificate fingerprinting.
  • Extended KeyCredential request store APIs to support client-certificate binding and immutable application ownership, and updated the in-memory store accordingly.
  • Updated KeyCredential audit event reporting to redact exception details and added conformance tests for application URI resolution and pending-finish status codes.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/Opc.Ua.Gds.Tests/KeyCredentialRequestStoreTests.cs Adds tests for request binding, ownership, and legacy-store compatibility behavior.
tests/Opc.Ua.Gds.Tests/KeyCredentialConformanceTests.cs New conformance tests for app URI resolution and finish result status mapping.
tests/Opc.Ua.Gds.Tests/GdsTestFixture.cs Adds KeyCredentialAdmin role to the test server role list.
tests/Opc.Ua.Gds.Tests/AuthorizationHelperTests.cs Adds tests for new KeyCredential authorization and certificate fingerprint hashing.
tests/Opc.Ua.Gds.Tests/AuditEventsTests.cs Verifies KeyCredential audit failures redact exception details.
src/Opc.Ua.Gds.Server.Common/RoleBasedUserManagement/GdsRole.cs Introduces the KeyCredentialAdmin role definition.
src/Opc.Ua.Gds.Server.Common/RoleBasedUserManagement/AuthorizationHelper.cs Adds KeyCredential role sets and client certificate fingerprint extraction.
src/Opc.Ua.Gds.Server.Common/IKeyCredentialRequestStore.cs Extends store contracts for binding/ownership and updates in-memory store implementation.
src/Opc.Ua.Gds.Server.Common/Diagnostics/AuditEvents.cs Adds exception redaction support for KeyCredential audit events.
src/Opc.Ua.Gds.Server.Common/ApplicationsNodeManager.cs Enforces authorization/ownership/binding for KeyCredential Start/Finish/Revoke and adds resolution helpers.

Comment thread src/Opc.Ua.Gds.Server.Common/IKeyCredentialRequestStore.cs
Comment thread src/Opc.Ua.Gds.Server.Common/IKeyCredentialRequestStore.cs
@marcschier
marcschier marked this pull request as ready for review July 19, 2026 11:55
@marcschier
marcschier marked this pull request as draft July 19, 2026 12:45
@marcschier
marcschier marked this pull request as ready for review July 19, 2026 14:12
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@marcschier
marcschier marked this pull request as draft July 19, 2026 18:04
@marcschier
marcschier marked this pull request as ready for review July 19, 2026 18:08
@marcschier
marcschier requested a review from romanett July 19, 2026 18:37
@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 1.01351% with 293 lines in your changes missing coverage. Please review.
✅ Project coverage is 30.62%. Comparing base (70c74e7) to head (294d83a).
⚠️ Report is 12 commits behind head on master.

Files with missing lines Patch % Lines
...pc.Ua.Gds.Server.Common/ApplicationsNodeManager.cs 0.00% 131 Missing ⚠️
...Ua.Gds.Server.Common/IKeyCredentialRequestStore.cs 0.00% 126 Missing ⚠️
...mon/RoleBasedUserManagement/AuthorizationHelper.cs 0.00% 26 Missing ⚠️
...pc.Ua.Gds.Server.Common/Diagnostics/AuditEvents.cs 0.00% 10 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (70c74e7) and HEAD (294d83a). Click for more details.

HEAD has 53 uploads less than BASE
Flag BASE (70c74e7) HEAD (294d83a)
60 7
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #4054       +/-   ##
===========================================
- Coverage   73.55%   30.62%   -42.93%     
===========================================
  Files        1345     1063      -282     
  Lines      180036   143925    -36111     
  Branches    31677    26422     -5255     
===========================================
- Hits       132427    44083    -88344     
- Misses      36911    94144    +57233     
+ Partials    10698     5698     -5000     
Files with missing lines Coverage Δ
...s.Server.Common/RoleBasedUserManagement/GdsRole.cs 51.42% <100.00%> (-42.33%) ⬇️
...pc.Ua.Gds.Server.Common/Diagnostics/AuditEvents.cs 0.00% <0.00%> (-80.52%) ⬇️
...mon/RoleBasedUserManagement/AuthorizationHelper.cs 0.00% <0.00%> (-74.22%) ⬇️
...Ua.Gds.Server.Common/IKeyCredentialRequestStore.cs 0.00% <0.00%> (-97.23%) ⬇️
...pc.Ua.Gds.Server.Common/ApplicationsNodeManager.cs 0.00% <0.00%> (-71.65%) ⬇️

... and 1041 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants