- Notifications
You must be signed in to change notification settings - Fork311
Expose SspiAuthenticationParameters on SspiContextProvider#2454
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
4f6c051
to80e90cd
Comparecodecovbot commentedMay 8, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #2454 +/- ##==========================================- Coverage 67.03% 61.43% -5.61%========================================== Files 299 294 -5 Lines 65497 65207 -290 ==========================================- Hits 43907 40057 -3850- Misses 21590 25150 +3560
Flags with carried forward coverage won't be shown.Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
86512cb
toff9a983
Compare96b5fb5
to2f6ddef
Compare@mdaigle next step for sspi :) |
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/SSPIContextProvider.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
As part of this change, the SSPIContextProvider base class now iterates through all the server names similar to what NegotiateSSPIContextProvider did.
Hey@twsouthwick, I'll try to take a look at this today! |
The errors appear to also be on main and are an "Access denied" for something related to certificates. It doesn't appear to be caused by my change, but let me know if you believe it is. |
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.
I think this all makes sense to me. I'll take another look tomorrow and also see why the tests are failing.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/NegotiateSSPIContextProvider.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/SSPIContextProvider.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
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.
Testsshould be green now. We had a build agent issue that was resolved yesterday.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/SSPIContextProvider.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
{ | ||
var auth = new SqlAuthenticationParameters.Builder( | ||
authenticationMethod: connection.ConnectionOptions.Authentication, | ||
resource: null, |
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.
I'm trying to think of the best way to populate this. In other usage, I see serverSpn getting set as the resource and dataSource getting set as the serverName. I think it makes sense to continue that pattern here given that serverName and server SPN may be different.
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.
Line 2437 ine0fa87b
varauthParamsBuilder=newSqlAuthenticationParameters.Builder( |
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.
I'm also not sure how to feel about authority. In federated auth, it's assumed that authority will be present, but here we don't set a value.
Let me chat with the team to see if we'd prefer to create a new type to better represent this set of credentials.
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.
Sounds good. I refactored the builder so that the same pattern can be had wherever it's used if we want to use the existingSqlAuthenticationParameters
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/NativeSSPIContextProvider.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/NegotiateSSPIContextProvider.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlAuthenticationParameters.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/SSPIContextProvider.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
46b88bf
intodotnet:mainUh oh!
There was an error while loading.Please reload this page.
{ | ||
NegotiateAuthenticationStatusCode statusCode = NegotiateAuthenticationStatusCode.UnknownCredentials; | ||
_negotiateAuth ??= new(new NegotiateAuthenticationClientOptions { Package = "Negotiate", TargetName = authParams.Resource }); |
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.
I'm probably missing something, but from my understandingSspiContextProvider
attempts to generate sspi context by iterating over eachserverSpns
(which is passed viaauthParams.Resource
). But because_negotiateAuth
is cached here, if it fails for the first spn, it's also going to fail for the others since it's going to be the exact same spn.
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.
You're right, nice catch! The implementation used to reset _negotiateAuth to null if authentication failed, but that's missing now.@twsouthwick can we correct it in a follow up PR?
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.
Will do - good catch. I'll open a separate PR for that.
The looping was added independently of the work I've been doing, so I'm not fully aware of how it's expected to work.
It opens a question as to the design I've moved this to - if we have a list of SPNs, what's the expected outcome? I think things will be just fine if it's the first SPN that completes the SSPI handshake. However if it's the second, on subsequent attempts it will be trying the first again. I can update the SspiContextProvider base to keep track of what was the successfull SPN to only submit that on subsequent calls.
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.
Here's the PR to add that back:#3347
Uh oh!
There was an error while loading.Please reload this page.
This change updates the SSPI context provider to surface information to implementers via SqlAuthenticationParameters.
This also expands#2559 to loop through all SPNs in the base SSPIContextProvider rather than just a specific implementation.
Part of#2253