-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix client returning BadUserAccessDenied when SecurityPolicy or SecurityMode not matching #3270
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Modified MatchEndpoints method to throw appropriate exceptions: - BadSecurityPolicyRejected when requested policy is not supported - BadSecurityModeRejected when requested mode is not supported - Maintains backward compatibility when no specific security settings requested Added comprehensive tests to verify the fix. Co-authored-by: romanett <[email protected]>
|
Applied code review feedback to extract parameter validation checks into separate boolean variables for better readability. Co-authored-by: romanett <[email protected]>
@copilot Please add an integration Test using the Clientfixture to connect to the testserver requesting a not supported SecurityPolicy |
Added ConnectWithUnsupportedSecurityPolicyAsync test that: - Uses ClientFixture to connect to test server - Requests a security policy not supported by server - Verifies BadSecurityPolicyRejected is thrown instead of BadUserAccessDenied - Tests run successfully on net8.0 and net9.0 targets Co-authored-by: romanett <[email protected]>
Added integration test |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3270 +/- ##
==========================================
+ Coverage 57.81% 57.88% +0.07%
==========================================
Files 365 365
Lines 79423 79441 +18
Branches 13870 13873 +3
==========================================
+ Hits 45920 45988 +68
+ Misses 29338 29287 -51
- Partials 4165 4166 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
matches.Add(description); | ||
} | ||
|
||
// no matches (security parameters may have changed). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
anyone has an Idea why this was put in?
(security parameters may have changed).
I am afraid we break some scenarios e.g. reconnect by chaning that area without careful consideration?
First tests show this indeed catches some cases were a not supported policy would be selected and sent to the server, however in cases with outdated EndpointDescriptions this could work (probably why it was put in inthe first place.
Problem
When connecting a client to a remote server where the server doesn't support the client's configured
SecurityPolicy
andSecurityMode
, the client'sSession.Create()
throwsStatusCodes.BadUserAccessDenied
instead of the more appropriateBadSecurityPolicyRejected
orBadSecurityModeRejected
.This makes it difficult for developers to diagnose configuration issues, as the error message suggests an authentication/authorization problem rather than a security configuration mismatch.
Root Cause
The issue was in the
MatchEndpoints
method inConfiguredEndpoints.cs
. When no endpoints matched the requested SecurityPolicy and SecurityMode, the method fell back to returning all available endpoints instead of throwing an appropriate error:This caused the client to attempt a connection with mismatched security settings, which later failed during session creation with a misleading
BadUserAccessDenied
error.Solution
Modified the
MatchEndpoints
method to check if specific security parameters were explicitly requested, and throw appropriate exceptions when no matches are found:BadSecurityPolicyRejected
when a specific security policy is requested but not foundBadSecurityModeRejected
when a specific security mode is requested but not foundBadSecurityPolicyRejected
with both policy and mode information when both are requested but not foundTesting
Added comprehensive unit tests in
ConfiguredEndpointTests.cs
to verify:Added integration test
ConnectWithUnsupportedSecurityPolicyAsync
inClientTest.cs
:BadSecurityPolicyRejected
is thrown instead ofBadUserAccessDenied
All existing tests continue to pass (24,768+ tests), confirming no regressions.
Impact
Clients will now receive clear, actionable error messages when connecting to servers with mismatched security configurations, making it significantly easier to diagnose and fix configuration issues:
Before:
BadUserAccessDenied: Endpoint does not support the user identity type provided.
After:
BadSecurityPolicyRejected: Server does not support the requested security policy 'http://opcfoundation.org/UA/SecurityPolicy#Aes256_Sha256_RsaPss'.
Fixes #3269
Original prompt
Fixes #3269
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.